Bug 70269

<rdar://problem/10298656>

Created attachment 111329 [details] Proposed patch

Attachment 111329 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource From git://git.webkit.org/WebKit 1bcef7b..c0d4f37 master -> origin/master M LayoutTests/platform/chromium-gpu-win/fast/canvas/canvas-composite-transformclip-expected.png M LayoutTests/platform/chromium-gpu-mac/fast/canvas/canvas-composite-transformclip-expected.png M LayoutTests/ChangeLog r97657 = c0d4f372e21680c78a2d8e16c2a90b5968f63fb3 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #3) > Attachment 111329 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 > > Updating OpenSource > From git://git.webkit.org/WebKit > 1bcef7b..c0d4f37 master -> origin/master > M LayoutTests/platform/chromium-gpu-win/fast/canvas/canvas-composite-transformclip-expected.png > M LayoutTests/platform/chromium-gpu-mac/fast/canvas/canvas-composite-transformclip-expected.png > M LayoutTests/ChangeLog > r97657 = c0d4f372e21680c78a2d8e16c2a90b5968f63fb3 (refs/remotes/trunk) > First, rewinding head to replay your work on top of it... > Fast-forwarded master to refs/remotes/trunk. > Updating chromium port dependencies using gclient... > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Re-trying 'depot_tools/gclient sync' > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Re-trying 'depot_tools/gclient sync' > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. > Re-trying 'depot_tools/gclient sync' > No such file or directory at Tools/Scripts/update-webkit line 104. > > > If any of these errors are false positives, please file a bug against check-webkit-style. Huh, none of these files are changed by this patch.

Comment on attachment 111329 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=111329&action=review > Source/WebCore/html/LoadableTextTrack.cpp:61 > + ASSERT_UNUSED(loader, m_cueLoader.get() == loader); Should not need the get() here. > Source/WebCore/html/LoadableTextTrack.cpp:68 > + ASSERT_UNUSED(loader, m_cueLoader.get() == loader); Nor here. > Source/WebCore/html/LoadableTextTrack.h:46 > + virtual void textTrackLoadingCompleted(LoadableTextTrack*, bool /* loadingFailed */) { } Should this be pure virtual instead of having an empty default implementation? > Source/WebCore/html/LoadableTextTrack.h:49 > +class LoadableTextTrack : public TextTrack, public CueLoaderClient { Does the CueLoaderClient inheritance need to be public? I’d like to see it be private. > Source/WebCore/html/LoadableTextTrack.h:64 > + // CueLoaderClient > + virtual bool shouldLoadCues(CueLoader*) { return true; } > + virtual void newCuesAvailable(CueLoader*); > + virtual void cueLoadingStarted(CueLoader*); > + virtual void cueLoadingCompleted(CueLoader*, bool loadingFailed); These should be private. > Source/WebCore/html/TextTrack.h:79 > + enum TextTrackType { BaseTextTrack, MutableTextTrack, LoadableTextTrack }; If this is a member of the TextTrack class it could just be named Type instead of TextTrackType. > Source/WebCore/html/track/WebVTTParser.cpp:47 > +static const int secondsPerHour = 3600; > +static const int secondsPerMinute = 60; > +static const double malformedTime = -1; Alexey told me we don’t need the static keyword to get internal linkage for these, at least the int ones. > Source/WebCore/html/track/WebVTTParser.cpp:49 > +static unsigned bomLength = 3; > +static unsigned fileIdentiferLength = 6; I think you forgot the const here. > Source/WebCore/html/track/WebVTTParser.cpp:76 > + if (line.length() > fileIdentiferLength && (line.substring(0, fileIdentiferLength) != "WEBVTT" > + || (line[fileIdentiferLength] != ' ' && line[fileIdentiferLength] != '\t'))) > return false; I think this would read better if it was written as a short inline helper function instead of a single long line. > Source/WebCore/loader/CueLoader.cpp:73 > + ASSERT(m_cachedCueData.get() == resource); Should not need get here. > Source/WebCore/loader/CueLoader.cpp:126 > + ASSERT(m_cachedCueData.get() == resource); Should not need get here. > Source/WebCore/loader/CueLoader.cpp:136 > + ASSERT(m_cachedCueData.get() == resource); Should not need get here. > Source/WebCore/loader/CueLoader.h:54 > +class CueLoader : public CachedResourceClient, public WebVTTParserClient { Does these client interfaces have to be public? Can we inherit from one or both of these base classes with private inheritance instead of public? > Source/WebCore/loader/CueLoader.h:72 > + // CachedResourceClient > + virtual void notifyFinished(CachedResource*); > + virtual void didReceiveData(CachedResource*); > + > + // WebVTTParserClient > + virtual void newCuesParsed(); Can these functions be private?

Comment on attachment 111329 [details] Proposed patch Attachment 111329 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10121061

Created attachment 111375 [details] Updated for review comments and missing HTMLTrackElement/HTMLMediaElement changes

Attachment 111375 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/html/track/WebVTTParser.h:40: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/html/HTMLMediaElement.cpp:880: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/html/HTMLMediaElement.cpp:885: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/html/TextTrack.h:50: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/html/TextTrack.h:51: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/html/HTMLMediaElement.h:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/html/HTMLMediaElement.h:358: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 7 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 111377 [details] Yet another patch. This time to appease my friend the style bot.

Comment on attachment 111377 [details] Yet another patch. This time to appease my friend the style bot. The patch looks fine, the comments are really about the existing code. Seems like lots of stuff that is currently called "Cue" should really be called "TextTrack". CueLoader -> TextTrackLoader, CachedCues -> CachedTextTrack, etc. Probably better to do those as a separate step. I'm confused by two level loader client interface. We have CueLoader which is a CachedResourceClient. That exposed client interface of its own, CueLoaderClient. I wonder if CueLoader and CachedCues (CachedTextTrack!) could merge and have just one level of clients.

*** Bug 62893 has been marked as a duplicate of this bug. ***