Bug 54580

Created attachment 82683 [details] Patch

Attachment 82683 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7926374

cr-* failures due to chromium DEPS needing to be bumped. Will look into this.

Created attachment 82828 [details] Patch

New patch to kick ews bots.

+fishd (chromium public change)

Comment on attachment 82828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82828&action=review > Source/WebKit/chromium/public/WebMediaPlayer.h:132 > + virtual unsigned long decodedFrames() const = 0; nit: webkit style is to just use "unsigned" instead of "unsigned long" nit: are "decodedFrames" and "droppedFrames" meant to return counts of {decoded,dropped}Frames? if so, the names of these methods could be improved. as is, it sounds like the methods should be returning sets of frames--one set of decoded frames and one set of dropped frames, but clearly that's not what they do since they return integers. How about one of these names? {count,number}Of{Decoded,Dropped}Frames {decoded,dropped}FrameCount > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:453 > +unsigned long WebMediaPlayerClientImpl::decodedFrames() const looks like the WebCore interfaces have the same naming problem.

(In reply to comment #7) > (From update of attachment 82828 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82828&action=review > > > Source/WebKit/chromium/public/WebMediaPlayer.h:132 > > + virtual unsigned long decodedFrames() const = 0; > > nit: webkit style is to just use "unsigned" instead of "unsigned long" Will fix. > > nit: are "decodedFrames" and "droppedFrames" meant to return counts of {decoded,dropped}Frames? > if so, the names of these methods could be improved. as is, it sounds like the methods should > be returning sets of frames--one set of decoded frames and one set of dropped frames, but clearly > that's not what they do since they return integers. > > How about one of these names? > > {count,number}Of{Decoded,Dropped}Frames > {decoded,dropped}FrameCount I like the second ({decoded,dropped}FrameCount). Note that this is mirrored in the idl (HTML{Video,Media}Element.idl. WebCore/html idl style seems to be to use "unsigned long" rather than "unsigned" though, yes? And the implementation HTML*Element.{cpp,h} is to use "unsigned"? As I introduced this a couple of weeks ago, I think I should change the actual core APIs to match this (nothing depends on this as no port supports this yet). I.e. I will: 1) Change WebCore to reflect the API change. 2) Make the changes to chromium 3) Fix up this patch that wires 1 and 2 together. Sound like a plan? Will cc you on the changes. > > > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:453 > > +unsigned long WebMediaPlayerClientImpl::decodedFrames() const > > looks like the WebCore interfaces have the same naming problem. Yup - will fix as detailed above.

Sounds good, thanks!!

Created attachment 84082 [details] Patch

Latest patch includes new naming. Will turn on in chromium (build/features_override.gypi) after this...

Ping! :-)

Hey Darin - wondering if you could look at this?

Hey James - could you take a look at this?

Comment on attachment 84082 [details] Patch Clearing flags on attachment: 84082 Committed r80215: <http://trac.webkit.org/changeset/80215>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 84082 [details]: http/tests/media/video-load-twice.html bug 51304 (authors: eric.carlson@apple.com and jamesr@chromium.org) The commit-queue is continuing to process your patch.