| Summary: | [META] Chrome video controls visual update | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Silvia Pfeiffer <silviapfeiffer1> |
| Component: | Media | Assignee: | Silvia Pfeiffer <silviapf> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | annacc, burg, dglazkov, eric.carlson, eric, feature-media-reviews, kenjibaheux, macpherson, menard, peter, qinmin, silviapf, syoichi, tkent, vcarbune, webkit.review.bot |
| Priority: | P2 | Keywords: | WebExposed |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | Unspecified | ||
| Bug Depends on: | 87683, 87835, 88297, 88623, 88724, 88743, 88818, 89050, 89093, 89284, 89344, 89416, 89508, 91368, 91802, 91811, 93222, 94395, 95395, 96339 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Silvia Pfeiffer
2012-04-23 23:52:10 PDT
Created attachment 138506 [details]
Patch
Attachment 138506 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/full..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 138510 [details]
removed No new tests line
Comment on attachment 138510 [details] removed No new tests line View in context: https://bugs.webkit.org/attachment.cgi?id=138510&action=review Publishing these comments now because I won't be able to look at this again today. > Source/WebCore/ChangeLog:17 > + (audio::-webkit-media-controls-panel, video::-webkit-media-controls-panel): > + (audio::-webkit-media-controls-play-button, video::-webkit-media-controls-play-button): > + (audio::-webkit-media-controls-timeline, video::-webkit-media-controls-timeline): > + (input[type="range"]::-webkit-slider-container): > + (input[type="range"]::-webkit-slider-thumb): > + (audio::-webkit-media-controls-time-remaining-display, video::-webkit-media-controls-time-remaining-display): I think it is really helpful, especially on a patch that is this big, to have comments on each line of the ChangeLog so a reviewer can see a summary of the changes in one place. > Source/WebCore/css/mediaControlsChromium.css:97 > +/* FIXME this might break other usage of input in shadow DOMs: however, this doesn't work: > +video::-webkit-media-controls input[type="range"]::-webkit-slider-container > +*/ Nit: FIXMEs should associated with a bug. > Source/WebCore/html/shadow/MediaControlElements.cpp:334 > +#if !PLATFORM(CHROMIUM) > element->hide(); > +#endif Instead of littering the code with #ifs, I would prefer if the "hide or not" behavior was defined by the theme - like we do for the position of the volume slider, the duration of the fade, etc. > Source/WebCore/html/shadow/MediaControlElements.cpp:353 > +#if !PLATFORM(CHROMIUM) > hide(); > +#endif Ditto. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:409 > -} > +} // namespace WebCore Nit: drive-by cleanups like this are generally frowned upon as part of a bigger patch. > Source/WebCore/html/shadow/SliderThumbElement.cpp:69 > +#if PLATFORM(CHROMIUM) > + return sliderStyle->appearance() == SliderVerticalPart; > +#else > return sliderStyle->appearance() == SliderVerticalPart || sliderStyle->appearance() == MediaVolumeSliderPart; > +#endif Can this logic also come from the theme instead of adding #ifs? > Source/WebCore/html/shadow/SliderThumbElement.cpp:120 > +#if PLATFORM(CHROMIUM) > + bool isVertical = style()->appearance() == SliderThumbVerticalPart; > +#else > bool isVertical = style()->appearance() == SliderThumbVerticalPart || style()->appearance() == MediaVolumeSliderThumbPart; > +#endif Ditto. Comment on attachment 138510 [details] removed No new tests line Attachment 138510 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12517358 New failing tests: fast/forms/range/slider-delete-while-dragging-thumb.html media/media-controls-invalid-url.html fast/dom/HTMLInputElement/input-slider-update.html fast/forms/range/slider-thumb-shared-style.html batterystatus/event-after-navigation.html accessibility/aria-disabled.html fast/frames/lots-of-objects.html media/track/track-cue-rendering-snap-to-lines-not-set.html fast/loader/text-document-wrapping.html media/video-controls-rendering-toggle-display-none.html fast/forms/range/input-appearance-range.html fast/forms/range/slider-mouse-events.html fast/canvas/webgl/shader-precision-format.html fast/forms/range/slider-padding.html fast/forms/input-appearance-height.html fast/forms/range/slider-onchange-event.html fast/dom/HTMLInputElement/input-slider-update-styled.html fast/forms/range/slider-thumb-stylability.html fullscreen/full-screen-stacking-context.html http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html fast/repaint/slider-thumb-drag-release.html fast/css/unknown-pseudo-element-matching.html fast/forms/box-shadow-override.html fast/repaint/slider-thumb-float.html fast/forms/range/range-thumb-height-percentage.html media/controls-layout-direction.html Created attachment 138644 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Thanks for your review (even though I committed explicitly with a no-review request ;-). At this stage, I only wanted to get some of the tests running on all platforms to see what the visual impact would be, which I seem to have achieved. Your input is much appreciated and I will address it, and also fix the remaining layout tests. (In reply to comment #4) > (From update of attachment 138510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138510&action=review > > Publishing these comments now because I won't be able to look at this again today. > > > Source/WebCore/ChangeLog:17 > > + (audio::-webkit-media-controls-panel, video::-webkit-media-controls-panel): > > + (audio::-webkit-media-controls-play-button, video::-webkit-media-controls-play-button): > > + (audio::-webkit-media-controls-timeline, video::-webkit-media-controls-timeline): > > + (input[type="range"]::-webkit-slider-container): > > + (input[type="range"]::-webkit-slider-thumb): > > + (audio::-webkit-media-controls-time-remaining-display, video::-webkit-media-controls-time-remaining-display): > > I think it is really helpful, especially on a patch that is this big, to have comments on each line of the ChangeLog so a reviewer can see a summary of the changes in one place. Will do. We've put just the most basic features into this patch that we want with the visual update of the video controls for https://code.google.com/p/chromium/issues/detail?id=88489 . Unfortunately even that is a fair amount. There will be more patches coming with further features. > > Source/WebCore/css/mediaControlsChromium.css:97 > > +/* FIXME this might break other usage of input in shadow DOMs: however, this doesn't work: > > +video::-webkit-media-controls input[type="range"]::-webkit-slider-container > > +*/ > > Nit: FIXMEs should associated with a bug. Added. > > Source/WebCore/html/shadow/MediaControlElements.cpp:334 > > +#if !PLATFORM(CHROMIUM) > > element->hide(); > > +#endif > > Instead of littering the code with #ifs, I would prefer if the "hide or not" behavior was defined by the theme - like we do for the position of the volume slider, the duration of the fade, etc. Done. > > Source/WebCore/html/shadow/MediaControlElements.cpp:353 > > +#if !PLATFORM(CHROMIUM) > > hide(); > > +#endif > > Ditto. Done. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:409 > > -} > > +} // namespace WebCore > > Nit: drive-by cleanups like this are generally frowned upon as part of a bigger patch. Removed. > > Source/WebCore/html/shadow/SliderThumbElement.cpp:69 > > +#if PLATFORM(CHROMIUM) > > + return sliderStyle->appearance() == SliderVerticalPart; > > +#else > > return sliderStyle->appearance() == SliderVerticalPart || sliderStyle->appearance() == MediaVolumeSliderPart; > > +#endif > > Can this logic also come from the theme instead of adding #ifs? Done. > > Source/WebCore/html/shadow/SliderThumbElement.cpp:120 > > +#if PLATFORM(CHROMIUM) > > + bool isVertical = style()->appearance() == SliderThumbVerticalPart; > > +#else > > bool isVertical = style()->appearance() == SliderThumbVerticalPart || style()->appearance() == MediaVolumeSliderThumbPart; > > +#endif > > Ditto. Done. Created attachment 140240 [details]
Patch - not to be committed before 7 May
Attachment 140240 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/full..." exit_code: 1
LayoutTests/ChangeLog:9: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:10: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:12: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:13: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:14: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:15: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:16: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:17: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:18: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:9: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:10: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:13: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:14: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:15: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:16: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:17: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:18: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 18 in 34 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 140240 [details] Patch - not to be committed before 7 May Attachment 140240 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12631193 New failing tests: fast/forms/range/slider-delete-while-dragging-thumb.html media/audio-repaint.html media/audio-controls-rendering.html fast/css/unknown-pseudo-element-matching.html http/tests/security/sandboxed-iframe-modify-self.html fast/forms/range/slider-thumb-shared-style.html accessibility/aria-disabled.html fast/frames/lots-of-objects.html fast/loader/text-document-wrapping.html fast/forms/range/input-appearance-range.html media/controls-after-reload.html fast/forms/range/slider-mouse-events.html fast/canvas/webgl/shader-precision-format.html media/controls-styling.html fast/forms/range/slider-padding.html fast/forms/range/slider-onchange-event.html fast/layers/video-layer.html media/controls-strict.html fast/dom/HTMLInputElement/input-slider-update-styled.html fast/forms/range/slider-thumb-stylability.html fullscreen/full-screen-stacking-context.html http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html fast/repaint/slider-thumb-drag-release.html fast/loader/javascript-url-in-embed.html fast/dom/HTMLInputElement/input-slider-update.html fast/repaint/slider-thumb-float.html fast/forms/range/range-thumb-height-percentage.html media/controls-layout-direction.html Created attachment 140256 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 140240 [details] Patch - not to be committed before 7 May View in context: https://bugs.webkit.org/attachment.cgi?id=140240&action=review > Source/WebCore/ChangeLog:1 > +2012-05-04 Silvia Pfeiffer <silviapf@chromium.org> I guess the part that changes the shadow pseudo ID of the slider when it is used in a media element shadow is not part of this patch yet? > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:62 > +// ---------------------------- I don’t think we need ASCII art like this. > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:103 > + // Show disabled play button when resource URL doesn't work. What does this mean? (In reply to comment #13) > (From update of attachment 140240 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140240&action=review > > > Source/WebCore/ChangeLog:1 > > +2012-05-04 Silvia Pfeiffer <silviapf@chromium.org> > > I guess the part that changes the shadow pseudo ID of the slider when it is used in a media element shadow is not part of this patch yet? No, I am preparing the patch right now. Please review again then. Thanks! > > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:62 > > +// ---------------------------- > > I don’t think we need ASCII art like this. The .cpp file uses that as a separator. But ok, I'll remove it. > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:103 > > + // Show disabled play button when resource URL doesn't work. > > What does this mean? A different play button resource will be shown when the resource cannot be loaded. I'll remove this comment, too. Created attachment 142379 [details]
Includes shadowPseudoId fix
Includes shadowPseudoId fix and feedback
Comment on attachment 142379 [details] Includes shadowPseudoId fix Attachment 142379 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12718400 New failing tests: media/media-document-audio-repaint.html media/video-no-audio.html media/video-display-toggle.html media/sources-fallback-codecs.html media/video-zoom-controls.html media/media-controls-clone.html media/video-empty-source.html media/video-playing-and-pause.html Created attachment 142385 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 142662 [details]
Fixed Linux tests, removed other tests
corrected test_expectations
Comment on attachment 142662 [details] Fixed Linux tests, removed other tests Attachment 142662 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12717813 New failing tests: media/media-document-audio-repaint.html fullscreen/full-screen-stacking-context.html media/video-no-audio.html media/controls-strict.html media/controls-styling.html media/video-display-toggle.html media/audio-repaint.html media/audio-controls-rendering.html media/video-zoom-controls.html media/controls-without-preload.html media/media-controls-clone.html fast/layers/video-layer.html media/video-empty-source.html media/video-playing-and-pause.html media/controls-layout-direction.html media/controls-after-reload.html Created attachment 142677 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 142862 [details] Flexbox changed in https://bugs.webkit.org/show_bug.cgi?id=86529 updated flexbox use Comment on attachment 142862 [details] Flexbox changed in https://bugs.webkit.org/show_bug.cgi?id=86529 Attachment 142862 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12737118 New failing tests: media/media-document-audio-repaint.html fullscreen/full-screen-stacking-context.html media/video-no-audio.html media/controls-strict.html media/controls-styling.html media/video-display-toggle.html media/audio-repaint.html media/audio-controls-rendering.html media/video-zoom-controls.html media/controls-without-preload.html media/media-controls-clone.html fast/layers/video-layer.html media/video-playing-and-pause.html media/controls-layout-direction.html media/controls-after-reload.html Created attachment 142868 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 142924 [details]
On Linux only pass image tests in test_expectations
Fail IMAGE+TEXT or TEXT only test expectations
Comment on attachment 142924 [details] On Linux only pass image tests in test_expectations View in context: https://bugs.webkit.org/attachment.cgi?id=142924&action=review > Source/WebCore/ChangeLog:20 > + - Replace images for play, pause, mute and volume buttons. > + - Add scalable buttons to fix https://crbug.com/110304 > + - Update CSS and use the new flexbox model. > + - Remove timeline and volume containers. > + - Add new enclosure div element with 30px high controls plus 5px padding. > + - Change volume slider from vertical to horizontal layout. > + - Set volume slider to 0 when media element is muted. > + - Introduce a duration display on top of current time display when video is not autoplay. > + - Remove complex code for coloring on playback and volume slider ranges in > + preparation for an upcoming patch that will do it all through CSS. > + - Introduce a special shadowPseudoId for styling of input ranges in controls. I know that you have been working on this for a while now, but this many changes really should be split into multiple patches/bugs. This makes it easier to review, but it also makes it easier to track down behavior changes and/or regressions later. It may be that some of these are interdependent, but at first glance it looks like at least the following can be split into separate patches: - Replace images for play, pause, mute and volume buttons. - Add scalable buttons to fix https://crbug.com/110304 - Change volume slider from vertical to horizontal layout. - Set volume slider to 0 when media element is muted. - Introduce a duration display on top of current time display when video is not autoplay. - Introduce a special shadowPseudoId for styling of input ranges in controls. > Source/WebCore/ChangeLog:45 > + * html/shadow/MediaControlElements.cpp: capture fade-out duration 0. > + (WebCore::MediaControlPanelElement::makeTransparent): > + * html/shadow/MediaControlRootElement.h: Add a duration element. > + (WebCore): > + * html/shadow/MediaControlRootElementChromium.cpp: The main visual update. > + (WebCore::MediaControlChromiumEnclosureElement::MediaControlChromiumEnclosureElement): > + (WebCore): > + (WebCore::MediaControlChromiumEnclosureElement::create): > + (WebCore::MediaControlChromiumEnclosureElement::shadowPseudoId): I find it *extremely* helpful to have per-function comments in a ChangeLog. This makes it much easier to review a patch, and it also makes it simpler for someone looking at just the ChangeLog later to see exactly what changed. > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:219 > + ExceptionCode ec; > + m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ec); Because you aren't using the exception code, this would be better as: m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION); > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376 > + insertBefore(textDisplayContainer.release(), m_enclosure, ec, true); Ditto. > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:82 > + if (!hasSource(mediaElement) || !mediaElement->hasAudio() || mediaElement->muted() || mediaElement->volume() <= 0.0f) "0.0f" should be simly "0". > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:85 > + if (mediaElement->volume() <= 0.33f) The "f" is unnecessary. > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:88 > + if (mediaElement->volume() <= 0.66f) Ditto. > Source/WebCore/rendering/RenderThemeChromiumMac.mm:192 > + // duration defines the format of how the time is rendered Comment should look like complete sentences by starting with a capital letter and ending with a period . (In reply to comment #25) > > I know that you have been working on this for a while now, but this many > changes really should be split into multiple patches/bugs. This makes it > easier to review, but it also makes it easier to track down behavior changes > and/or regressions later. All of these together make the newly styled video controls work in the new fashion. I have another 5 patches waiting which I want to roll out in steps on top of this. > It may be that some of these are interdependent, but at first glance it looks > like at least the following can be split into separate patches: > > - Replace images for play, pause, mute and volume buttons. I can't just replace the images for these - the controls would look hideous. The images and the CSS are a core requirement for the new layout. > - Add scalable buttons to fix https://crbug.com/110304 The scalability of the buttons is a side effect of the new CSS, not a separate fix. > - Change volume slider from vertical to horizontal layout. The layout won't work with a vertical slider. > - Set volume slider to 0 when media element is muted. This is part of the new look. > - Introduce a duration display on top of current time display when video is not autoplay. This is part of the new look. > - Introduce a special shadowPseudoId for styling of input ranges in controls. This is a requirement for the new CSS to work - I can't roll this out selectively. (In reply to comment #26) > (In reply to comment #25) > > > > I know that you have been working on this for a while now, but this many > > changes really should be split into multiple patches/bugs. This makes it > > easier to review, but it also makes it easier to track down behavior changes > > and/or regressions later. > > All of these together make the newly styled video controls work in the new fashion. I have another 5 patches waiting which I want to roll out in steps on top of this. > I wasn't suggesting that you try to land them separately, only that you consider breaking this into smaller pieces focusing on some of the changes you note it addresses. Once the separate patches have been reviewed, you can land them all simultaneously. I actually think you might want to consider getting your other pending patches into the review pipeline because it looks like there will be some unwanted UI changes if this lands by itself, eg. in this patch paintMediaSlider doesn't appear to do anything useful. (In reply to comment #27) > I wasn't suggesting that you try to land them separately, only that you > consider breaking this into smaller pieces focusing on some of the changes > you note it addresses. Once the separate patches have been reviewed, you can > land them all simultaneously. How do I do this? Create a new bug for each or attach them all to this bug? (In reply to comment #28) > > How do I do this? Create a new bug for each or attach them all to this bug? A new bug for each issue is preferable. [NOTE: patches are being delivered in linked bugs.] (In reply to comment #25) > > > Source/WebCore/ChangeLog:45 > > + * html/shadow/MediaControlElements.cpp: capture fade-out duration 0. > > + (WebCore::MediaControlPanelElement::makeTransparent): > > + * html/shadow/MediaControlRootElement.h: Add a duration element. > > + (WebCore): > > + * html/shadow/MediaControlRootElementChromium.cpp: The main visual update. > > + (WebCore::MediaControlChromiumEnclosureElement::MediaControlChromiumEnclosureElement): > > + (WebCore): > > + (WebCore::MediaControlChromiumEnclosureElement::create): > > + (WebCore::MediaControlChromiumEnclosureElement::shadowPseudoId): > > I find it *extremely* helpful to have per-function comments in a ChangeLog. This makes it much easier to review a patch, and it also makes it simpler for someone looking at just the ChangeLog later to see exactly what changed. Thanks, doing it now. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:219 > > + ExceptionCode ec; > > + m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ec); > > Because you aren't using the exception code, this would be better as: > > m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION); DONE, see bug 88724. > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376 > > + insertBefore(textDisplayContainer.release(), m_enclosure, ec, true); > > Ditto. I only did the rename from m_panel to m_enclosure. Want me to fix this drive-by? > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:82 > > + if (!hasSource(mediaElement) || !mediaElement->hasAudio() || mediaElement->muted() || mediaElement->volume() <= 0.0f) > > "0.0f" should be simly "0". DONE, see bug 88297. > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:85 > > + if (mediaElement->volume() <= 0.33f) > > The "f" is unnecessary. DONE, see bug 88297. > > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:88 > > + if (mediaElement->volume() <= 0.66f) > > Ditto. DONE, see bug 88297. > > Source/WebCore/rendering/RenderThemeChromiumMac.mm:192 > > + // duration defines the format of how the time is rendered > > Comment should look like complete sentences by starting with a capital letter and ending with a period . Thanks, doing it from now on. (In reply to comment #30) > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:219 > > > + ExceptionCode ec; > > > + m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ec); > > > > Because you aren't using the exception code, this would be better as: > > > > m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION); > > DONE, see bug 88724. > > > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376 > > > + insertBefore(textDisplayContainer.release(), m_enclosure, ec, true); > > > > Ditto. > > I only did the rename from m_panel to m_enclosure. Want me to fix this drive-by? > Yes, I think you may as well fix it while you change the name. (In reply to comment #31) > (In reply to comment #30) > > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376 > > > > + insertBefore(textDisplayContainer.release(), m_enclosure, ec, true); > > > > > > Ditto. > > > > I only did the rename from m_panel to m_enclosure. Want me to fix this drive-by? > > > Yes, I think you may as well fix it while you change the name. DONE - it's in the cq? patch for bug 87683 . |