Bug 122001

Actually, I mixed up -bad and -ugly. I don't think there's any particular reason to avoid linking against -bad, except that it introduce another dependency. Since we already require -base, I'm not sure if it really matters.

Is there a better way to check if a GstMessage has MPEG-TS section data than to try to parse it? I'm currently doing this: GstMpegTsSection* section = gst_message_parse_mpegts_section(message); if (section) { processMpegTsSections(section); gst_mpegts_section_unref(section); }

Check the message type perhaps?

(In reply to comment #3) > Check the message type perhaps? As far as I can tell, there's no MPEG-related message types: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstMessage.html#GstMessageType http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html I'm guessing it's GST_MESSAGE_TYPE_UNKNOWN, right?

Or maybe it's a GST_MESSAGE_ELEMENT?

Make sure with the GST_MESSAGE_TYPE_NAME macro :)

I'm also having trouble getting it to link: gcc $(gst-git pkg-config --cflags --libs gstreamer-plugins-bad-1.0) test.c /tmp/ccyU76G0.o:test.c:function main: error: undefined reference to 'gst_message_parse_mpegts_section' collect2: error: ld returned 1 exit status

Created this GStreamer bug for the linking issue: https://bugzilla.gnome.org/show_bug.cgi?id=709145

Created attachment 213128 [details] Initial patch, needs tests Here's an initial patch. I need to make it not break the build when gstreamer-mpegts-1.0 isn't available, and it needs some tests (the file I've been using is much too large to check in). I have a question for people with more experience with WebKit: Is there a better way to generate JSON? JSONObject appears to be designed for JavaScriptCore, and I didn't see anything else JSON related, but WebKit is a big place so I might have missed it.

Comment on attachment 213128 [details] Initial patch, needs tests Attachment 213128 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2893207

Comment on attachment 213128 [details] Initial patch, needs tests Attachment 213128 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/2902193

Comment on attachment 213128 [details] Initial patch, needs tests Attachment 213128 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/2891205

Comment on attachment 213128 [details] Initial patch, needs tests Attachment 213128 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/2907202

For making this compile on older versions, it's possible to check GST_CHECK_VERSION, but I'm not sure how to remove that requirement from pkg-config: PKGCONFIG += glib-2.0 gio-2.0 gstreamer-1.0 gstreamer-app-1.0 gstreamer-base-1.0 gstreamer-mpegts-1.0 gstreamer-pbutils-1.0 gstreamer-plugins-base-1.0 gstreamer-video-1.0 gstreamer-audio-1.0 I'll look into this more tomorrow, but if anyone knows off the top of their head, it would be helpful.

Comment on attachment 213128 [details] Initial patch, needs tests Attachment 213128 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/2857230

Created attachment 213170 [details] Make gstreamer-mpegts optional. I'm not really sure how to handle this in WebKitGTK or EFLWebKit. I think I have an idea of how to do the GTK version (another PKG_CHECK_MODULES check with AC_DEFINE([WTF_USE_GSTREAMER_MPEGTS]), and somehow concatenate GSTREAMER_MPEGTS_LIBS and GSTREAMER_LIBS, etc. I need to get WebKitGTK building so I can test that. For CMake, I'm not even sure where the libraries are being found yet.

(In reply to comment #0) > The information is available in a GstMessage: > > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html > > There is a spec from CableLabs (my company): > > http://html5.cablelabs.com/tracks/media-container-mapping.html > > The interesting part of this is going to be that the MPEG-TS metadata is only available if we link against gst-plugins-bad. I'm guessing WebKitGTK won't want that as a dependency, so it will probably need to be conditional. I don't think it makes sense to do this until we update our TextTrack implementation to the current spec (bug 122218), which (among other things) has added a DataCue interface specifically for in-band metadata tracks.

(In reply to comment #17) > I don't think it makes sense to do this until we update our TextTrack implementation to the current spec (bug 122218), which (among other things) has added a DataCue interface specifically for in-band metadata tracks. I'm not sure we want to present this as binary though. For one thing, it would be significantly harder for JS developers to use. For another, GStreamer isn't really designed to give out binary data. I guess I should see what the W3C says about this spec..

(In reply to comment #18) > (In reply to comment #17) > > I don't think it makes sense to do this until we update our TextTrack implementation to the current spec (bug 122218), which (among other things) has added a DataCue interface specifically for in-band metadata tracks. > > I'm not sure we want to present this as binary though. For one thing, it would be significantly harder for JS developers to use. For another, GStreamer isn't really designed to give out binary data. I guess I should see what the W3C says about this spec.. I don't think DataCue is just for binary data, the spec says "if the browser is unable to identify a TextTrackCue interface that is more appropriate to expose the data in the cues of a media-resource-specific text track, the DataCue object is used." Or does "MPEG-TS metadata" actually mean *captions* in the MPEG transport stream? [1] http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#text-tracks-exposing-in-band-metadata

(In reply to comment #19) > I don't think DataCue is just for binary data, the spec says "if the browser is unable to identify a TextTrackCue interface that is more appropriate to expose the data in the cues of a media-resource-specific text track, the DataCue object is used." What's strange about using a DataCue is that the .data attribute doesn't make much sense in this case, since we can either expose exactly the same data as .text, but as binary, or we can expose the binary PMT, which would be relatively difficult and not very useful. > Or does "MPEG-TS metadata" actually mean *captions* in the MPEG transport stream? No, captions will be handled by the existing in-band track code (once our decoder gets into GStreamer).

(In reply to comment #0) > The information is available in a GstMessage: > > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad-libs/html/gst-plugins-bad-libs-Base-MPEG-TS-sections.html > > There is a spec from CableLabs (my company): > > http://html5.cablelabs.com/tracks/media-container-mapping.html > > The interesting part of this is going to be that the MPEG-TS metadata is only available if we link against gst-plugins-bad. I'm guessing WebKitGTK won't want that as a dependency, so it will probably need to be conditional. I think it better to expose via the binary, ArrayBuffer using the data attribute. The only reason the MPEG-2 mapping spec used the text attribute (to expose binary data as BASE64) is because there was no attribute to expose binary data. The correct solution would be to update the MPEG-2 mapping spec to use the new DataCue interface and expose as binary directly. However, for compatibility with the previous mapping, one might *also* expose it as BASE64 via the text attribute.

So, does this patch needs a review or does it need to be reworked addressing Eric's and Glenn's doubts? It'd be nice to get it working in the GTK+ port too at least, now that Qt was removed from trunk.

The patch will work in GTK as soon as I can figure out how to add an optional pkgconfig dependency. I was planning to get back to that after refactoring the cue interfaces to match the new spec. I'm starting to wonder if binary is a useful format for this though, since at least in GStreamer, it's much easier to get structured data than the original binary.

Comment on attachment 213170 [details] Make gstreamer-mpegts optional. Ok so I r- this for now :)

Created attachment 224018 [details] Patch

This latest patch is a work-in-progress using DataCue. I think the only remaining issue is that I'm using GstMpegTsSection->data and GstMpegTsSection->section_length, which are private API. I need to either convince GStreamer to make those public, or rebuild the binary PMT by hand, which doesn't sound very fun. Do we have any WebKit-y way to write bitstreams?

I'm attempting to add a function to GStreamer to get at this data (gst_mpegts_section_get_data()): https://bugzilla.gnome.org/show_bug.cgi?id=724255

I'll also need to add a version check once this is finished, so we can make sure GStreamer provides the function.

Comment on attachment 224018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224018&action=review Don't we also need inBandMetadataTrackDispatchType? > Source/WebCore/html/track/InbandDataTextTrack.h:53 > + virtual void removeCue(TextTrackCue*, ExceptionCode&) override { ASSERT_NOT_REACHED(); } Why should this never be called, can data cues not be removed? > Source/WebCore/html/track/InbandWebVTTTextTrack.h:56 > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const char*, unsigned) override { ASSERT_NOT_REACHED(); } > virtual void addGenericCue(InbandTextTrackPrivate*, PassRefPtr<GenericCueData>) override { ASSERT_NOT_REACHED(); } > virtual void updateGenericCue(InbandTextTrackPrivate*, GenericCueData*) override { ASSERT_NOT_REACHED(); } > virtual void removeGenericCue(InbandTextTrackPrivate*, GenericCueData*) override { ASSERT_NOT_REACHED(); } Instead of requiring each derived class to implement ASSERTing versions of the methods they don't need, why not make the base class versions ASSERT instead of being pure virtual?

Created attachment 224075 [details] Patch Moved InbandTextTrackPrivateClient ASSERTing functions to InbandTextTrack, and added InbandDataTextTrack to the Mac build to hopefully fix that linking error. For the WebKitGTK error, I'll probably change the USE(GSTREAMER_MPEG) check to check the GStreamer version, instead of blindly relying on gstreamer-mpegts-1.0 existing.

I noticed after uploading that InbandDataTextTrack.cpp has a bunch of unnecessary #includes. The next patch I upload will have those removed.

Created attachment 224079 [details] Patch Removed unused parameters (caused Mac build errors) and #includes.

Comment on attachment 224079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224079&action=review Don't we also need textTrack.inBandMetadataTrackDispatchType? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1124 > + m_trackDescriptionTrack = InbandMetadataTextTrackPrivateGStreamer::create(InbandTextTrackPrivate::Metadata, > + InbandTextTrackPrivate::Data, "video/mp2t track-description"); Do you really want this hardcoded label on all metadata tracks? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1140 > + m_chaptersTrack = InbandMetadataTextTrackPrivateGStreamer::create(InbandTextTrackPrivate::Chapters, InbandTextTrackPrivate::Generic); Are chapter tracks never labeled?

(In reply to comment #33) > (From update of attachment 224079 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224079&action=review > > Don't we also need textTrack.inBandMetadataTrackDispatchType? I just looked at the HTML5 spec for this, and it sounds like this was written before DataCue existed, since it wants us to encode binary as base64: http://www.w3.org/html/wg/drafts/html/CR/embedded-content-0.html#steps-to-expose-a-media-resource-specific-text-track > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1124 > > + m_trackDescriptionTrack = InbandMetadataTextTrackPrivateGStreamer::create(InbandTextTrackPrivate::Metadata, > > + InbandTextTrackPrivate::Data, "video/mp2t track-description"); > > Do you really want this hardcoded label on all metadata tracks? Only for MPEG-TS PMT tracks. I just looked at our spec again, and it's actually supposed to be the "id" though. > Are chapter tracks never labeled? I don't know what I would use for the label.

Created attachment 224797 [details] Patch

This now relies on public API (to be released in GStreamer 1.3.1). I also switched the 'video/mp2t track-description' to be the id, not the label.

Comment on attachment 224797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224797&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:58 > +extern "C" { > +#include <gst/mpegts/mpegts.h> > +} Having to use explicit extern "C" here is odd. Have you filed a gstreamer bug for this? > Source/autotools/FindDependencies.m4:421 > + PKG_CHECK_MODULES([GSTREAMER_MPEGTS], [gstreamer-mpegts-1.0 >= 1.3.0], I think the cmake build needs a similar change

Created attachment 224935 [details] Patch It looks like the extern C{} was unnecessary. I looked at CMake build support, but I wasn't able to get it working. I'm about to be out of the office for two weeks, but I'll look into the CMake build more when I get back.

It sounds like the working group is planning to use the inbandMetadataTrackDispatch type for this instead: http://www.w3.org/TR/html5/embedded-content-0.html#steps-to-expose-a-media-resource-specific-text-track I'll rework my patch to use that.

I figured out why my patch wasn't working. The CMake build wasn't finding GStreamer libraries when built from gst-uninstalled: https://bugs.webkit.org/show_bug.cgi?id=130418 I'm waiting on the W3C to finalize some things before moving forward with a new inbandMetadataWhatever patch though.

(In reply to comment #40) > > I'm waiting on the W3C to finalize some things before moving forward with a new inbandMetadataWhatever patch though. What issues are you waiting on? If they aren't *too* disruptive, I think don't think it would hurt to go ahead and get this in now and get some experience with it. We can update once the issue(s) have been settled without worrying about existing content.

(In reply to comment #41) > (In reply to comment #40) > > > > I'm waiting on the W3C to finalize some things before moving forward with a new inbandMetadataWhatever patch though. > > What issues are you waiting on? > > If they aren't *too* disruptive, I think don't think it would hurt to go ahead and get this in now and get some experience with it. We can update once the issue(s) have been settled without worrying about existing content. There were a lot of proposals flying around for metadata recently: * Making inbandMetadataBlahBlah available for all tracks. * Splitting that attribute with the unreasonable long name into two attributes, one ArrayBuffer with the binary metadata and one DOMString to tell you what format it is. * Using DataCues for the PMT like I was originally planning to implement. It seems likely that inbandMetadata[...] whatever will likely be the final version, so I can implement that now if you're interested. I just didn't want to waste your time by applying a patch that might be need to be removed or substantially changed.

(In reply to comment #42) > > There were a lot of proposals flying around for metadata recently: > > * Making inbandMetadataBlahBlah available for all tracks. > * Splitting that attribute with the unreasonable long name into two attributes, one ArrayBuffer with the binary metadata and one DOMString to tell you what format it is. > * Using DataCues for the PMT like I was originally planning to implement. > > It seems likely that inbandMetadata[...] whatever will likely be the final version, so I can implement that now if you're interested. I just didn't want to waste your time by applying a patch that might be need to be removed or substantially changed. I think we should go ahead. As you say, we can change it when/if necessary.

I've had a bunch going on but I'm getting back to this. Should inBandMetadataTrackDispatchType be an AtomicString or String? It seems unlikely that people would be checking "if (track.inBandMetadataTrackDispatchType == ...)", and internally it doesn't help us much since generating the value is 90% of the work. Everything else is an AtomicString though.

(In reply to comment #44) > I've had a bunch going on but I'm getting back to this. > > Should inBandMetadataTrackDispatchType be an AtomicString or String? It seems unlikely that people would be checking "if (track.inBandMetadataTrackDispatchType == ...)", and internally it doesn't help us much since generating the value is 90% of the work. Everything else is an AtomicString though. I don't think it will matter a great either way.

Created attachment 228446 [details] Patch

I think this is correct. I need to check with someone to make sure the inbandMetadataTrackDispatchType is correct (I'm pretty sure I'm doing exactly what the spec says to do, but I need to do a sanity test on the output, which requires figuring out what these values actually mean).

Comment on attachment 228446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228446&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1072 > + track->client()->addDataCue(track.get(), currentTimeDouble(), currentTimeDouble(), bytes, size); Nit: it would be cleaner to have track->addDataCue() so you don't have to call the client interface directly.

Comment on attachment 228446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228446&action=review This is my an informal review as I am not a reviewer. Feel free to cherry pick what you think is best, considering that eric has already r+ it. > Source/WebCore/PlatformGTK.cmake:499 > + > Modules/quota/StorageInfo.idl > Modules/quota/StorageQuota.idl > + Nit: why are you adding these two lines? > Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:138 > + Nit: not necessary line? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:54 > +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS) This is only a though coming to my mind rather than a recommendation: When I see these #if statements I always wonder if it is better to just ensure that that GSTREAMER_MPEGTS is only enabled with VIDEO_TRACK is also enabled, then you could avoid asking only for GSTREAMER_MPEGTS. I can see reasons to keep it like this so feel free to completely ignore this. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:989 > +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS) > + else if ((section = gst_message_parse_mpegts_section(message))) { > + processMpegTsSection(section); > + gst_mpegts_section_unref(section); > + } > +#endif Personal preference: if I were you, I'd open the else, declare the variable inside while retrieving the value (this way you can remove the declaration from line 879) and then check for the content, operate and free it. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1032 > + for (guint i = 0; i < pmt->streams->len; ++i) { unsigned instead of guint? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1034 > + if (stream->stream_type == 0x05 || stream->stream_type >= 0x80) { Personal preference: what about adding constants for these values? I guess retrieving them from any other header file is impossible or very painful, right? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1051 > + for (guint j = 0; j < stream->descriptors->len; ++j) { Ditto. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1053 > + for (guint k = 0; k < descriptor->length; ++k) Ditto.

(In reply to comment #49) > > Source/WebCore/PlatformGTK.cmake:499 > > + > > Modules/quota/StorageInfo.idl > > Modules/quota/StorageQuota.idl > > + > > Nit: why are you adding these two lines? The style checker was complaining about them (even though they're not lines I changed?), so I just added the two newlines so it would pass the style check 100%. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:54 > > +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS) > > This is only a though coming to my mind rather than a recommendation: When I see these #if statements I always wonder if it is better to just ensure that that GSTREAMER_MPEGTS is only enabled with VIDEO_TRACK is also enabled, then you could avoid asking only for GSTREAMER_MPEGTS. I can see reasons to keep it like this so feel free to completely ignore this. At some point I expect this to solve itself, when ENABLE_VIDEO_TRACK is removed (I think all ports turn it on right now). Personally, I prefer to have both checks, but I don't have strong feelings either way. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:989 > > +#if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS) > > + else if ((section = gst_message_parse_mpegts_section(message))) { > > + processMpegTsSection(section); > > + gst_mpegts_section_unref(section); > > + } > > +#endif > > Personal preference: if I were you, I'd open the else, declare the variable inside while retrieving the value (this way you can remove the declaration from line 879) and then check for the content, operate and free it. That makes sense. I'll make this change. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1032 > > + for (guint i = 0; i < pmt->streams->len; ++i) { > > unsigned instead of guint? streams is a GPtrArray and len is a guint. unsigned in probably the same type, but it seemed better to be exact. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1034 > > + if (stream->stream_type == 0x05 || stream->stream_type >= 0x80) { > > Personal preference: what about adding constants for these values? I guess retrieving them from any other header file is impossible or very painful, right? The spec I was following gives these as hex values: http://www.cablelabs.com/wp-content/uploads/specdocs/CL-SP-HTML5-MAP-I03-140207.pdf I could replace 0x05 with a constant, but for the '>=' comparison it seems like using a named constant would just be confusing. I'm actually checking with someone internally to make sure these values are correct, since 0x80 - 0x87 are apparently audio :\ http://en.wikipedia.org/wiki/Program_Specific_Information#Elementary_stream_types

(In reply to comment #50) > (In reply to comment #49) > > > Source/WebCore/PlatformGTK.cmake:499 > > > + > > > Modules/quota/StorageInfo.idl > > > Modules/quota/StorageQuota.idl > > > + > > > > Nit: why are you adding these two lines? > > The style checker was complaining about them (even though they're not lines I changed?), so I just added the two newlines so it would pass the style check 100%. For me it looks like a bug in the style checker, specially if you haven't changed the lines. I'd go without the lines and file a bug against the style checker. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1032 > > > + for (guint i = 0; i < pmt->streams->len; ++i) { > > > > unsigned instead of guint? > > streams is a GPtrArray and len is a guint. unsigned in probably the same type, but it seemed better to be exact. In out code you can find inconsistencies like this for both sides so maybe Phil has an opinion about what to do for newly written code. If I pointed this out is because I remember some comments in this direction even when mixing WebKit and GStreamer code.

Created attachment 228516 [details] Patch

I made some changes: * Added addDataCue() and addGenericCue() to InbandMetadataTextTrackPrivateGStreamer. * Moved the GstMpegTsSection* declaration in MediaPlayerPrivateGStreamer::handleMessage() closer to where it's used. * Fixed the gstreamer-mpegts check to require >=1.3.0 to hopefully fix the gtk-wk2 build. * Added the bits to make this work in the Efl build too (OptionsEfl.cmake). I also checked with people to make sure that the 0x05 and 0x80 values are correct. I don't think it's any less confusing with named constants though :\ I'm planning to commit this if it builds properly, assuming I get another r+ (so the build bot won't complain about the "Oops no reviewer" line).

Comment on attachment 228516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228516&action=review > Source/WebCore/html/track/InbandDataTextTrack.h:48 > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override; Nit: please name the "start" and "end" variables. > Source/WebCore/html/track/InbandTextTrack.h:65 > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override { ASSERT_NOT_REACHED(); } Ditto. > Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:137 > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) = 0; Ditto. > Source/WebCore/platform/graphics/gstreamer/InbandMetadataTextTrackPrivateGStreamer.h:50 > + void addDataCue(double start, double stop, const void* data, unsigned length) "stop" - do you mean "end"?

Comment on attachment 228516 [details] Patch Lets hold off on this for a few days.

Comment on attachment 228516 [details] Patch (In reply to comment #55) > (From update of attachment 228516 [details]) > Lets hold off on this for a few days. Actually, lets go ahead and get this checked in. We talked about a potential change to DataCue at the face-to-face today and having this in will make it simpler to experiment with the change.

(In reply to comment #54) > (From update of attachment 228516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228516&action=review > > > Source/WebCore/html/track/InbandDataTextTrack.h:48 > > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override; > > Nit: please name the "start" and "end" variables. > > > Source/WebCore/html/track/InbandTextTrack.h:65 > > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) override { ASSERT_NOT_REACHED(); } > > Ditto. > > > Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:137 > > + virtual void addDataCue(InbandTextTrackPrivate*, double, double, const void*, unsigned) = 0; > > Ditto. Should I name the data and length parameters too?

Created attachment 228966 [details] Patch I added parameter names where I could (in InbandTextTrack.h, it complains about unused parameters if we name them).

Attachment 228966 [details] did not pass style-queue: ERROR: Source/WebCore/PlatformGTK.cmake:346: There should be exactly one empty line instead of 0 between "Modules/gamepad/GamepadList.idl" and "Modules/geolocation/Geolocation.idl". [list/emptyline] [5] ERROR: Source/WebCore/PlatformGTK.cmake:347: There should be exactly one empty line instead of 0 between "Modules/geolocation/Geolocation.idl" and "Modules/mediasource/VideoPlaybackQuality.idl". [list/emptyline] [5] Total errors found: 2 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #57) > > Should I name the data and length parameters too? I don't think that is necessary. I only suggested naming start and end because both are doubles.

Created attachment 228967 [details] Patch More style fixes. I don't know why it complains about the entire .cmake file when you change one line, but might as well fix these while I'm here. I also removed the data and length parameter names in function definitions.

Comment on attachment 228967 [details] Patch Clearing flags on attachment: 228967 Committed r167025: <http://trac.webkit.org/changeset/167025>

All reviewed patches have been landed. Closing bug.