Summary: | [chromium] Enable media elements statistics support in chromium | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Steve Lacey <sjl> | ||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, fishd, jamesr, scherkus, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Steve Lacey
2011-02-16 13:11:56 PST
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. |