Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783593002:
Redraw media volume slider when it is moving (Closed)
|
Created:
3 years, 8 months ago by sabbakumov Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, CJ, mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRedraw media volume slider when it is moving
Steps to reproduce:
1. Launch Chromium http://www.quirksmode.org/html5/tests/video.html
2. Try to change the video volume by dragging the volume slider while
holding the mouse key.
3. The volume slider isn't updated. It updates only when the mouse
button is released.
BUG=677903
Review-Url: https://codereview.chromium.org/2783593002
Cr-Commit-Position: refs/heads/master@{#462310}
Committed: https://chromium.googlesource.com/chromium/src/+/4d9872c0015abd0cac4cbaeeb36585109f1b13e8
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move volume slider invalidation to the slider event handler #Patch Set 3 : Test for volume slider invalidation on input while dragging #Patch Set 4 : Test for volume slider invalidation on input while dragging #
Total comments: 4
Patch Set 5 : Test the code modifications #
Messages
Total messages: 33 (19 generated)
Description was changed from ========== Redraw media volume slider when it is moving Steps to reproduce: 1. Launch Chromium http://www.quirksmode.org/html5/tests/video.html 2. Try to change the video volume by dragging the volume slider while holding the mouse key. 3. The volume slider isn't updated. It updates only when the mouse button is released. BUG=677903 ========== to ========== Redraw media volume slider when it is moving Steps to reproduce: 1. Launch Chromium http://www.quirksmode.org/html5/tests/video.html 2. Try to change the video volume by dragging the volume slider while holding the mouse key. 3. The volume slider isn't updated. It updates only when the mouse button is released. BUG=677903 ==========
sabbakumov@yandex-team.ru changed reviewers: + mlamouri@chromium.org
The CQ bit was checked by sabbakumov@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
+lethalantidote@ FYI I think you might want to move this into the input event handler for the slider.
lethalantidote@chromium.org changed reviewers: + lethalantidote@chromium.org
https://codereview.chromium.org/2783593002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2783593002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:809: invalidate(m_muteButton); Is this necessary?
I've moved the slider invalidation to the event handler.
sgtm but can you add a test for this? a test with an expected file where you have the slider on the left because the volume was set in JS and another one where the slider was moved using eventSender?
On 2017/03/30 at 19:28:02, mlamouri wrote: > sgtm but can you add a test for this? a test with an expected file where you have the slider on the left because the volume was set in JS and another one where the slider was moved using eventSender? If it is easier, you could instead write a unit test and check if the element is invalidated.
sabbakumov@yandex-team.ru changed reviewers: + skobes@chromium.org
Added a unit test for a fix.
https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp (right): https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:539: volumeSlider->setVolume(0.5); I'm a bit confused: setVolume() doesn't call the code path that you are changing. Am I missing something? Would it work if you set the value of the slider then call defaultEventHandler() with a fake input event? https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:1641: virtual void setShouldDoFullPaintInvalidation( Instead of overriding this method, can you look at `shouldDoFullPaintInvalidation()` or a similar public accessor?
https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp (right): https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:539: volumeSlider->setVolume(0.5); On 2017/03/31 11:20:16, mlamouri wrote: > I'm a bit confused: setVolume() doesn't call the code path that you are > changing. Am I missing something? > > Would it work if you set the value of the slider then call defaultEventHandler() > with a fake input event? Done. https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2783593002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:1641: virtual void setShouldDoFullPaintInvalidation( On 2017/03/31 11:20:16, mlamouri wrote: > Instead of overriding this method, can you look at > `shouldDoFullPaintInvalidation()` or a similar public accessor? I think that we need to test the interaction between modules, not the module state in this case. So, it's polymorphic here.
lgtm assuming core/layout/ OWNERS are happy with the added virtual.
The CQ bit was checked by sabbakumov@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sabbakumov@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
skobes@: Could you review the changes in LayoutObject.h please?
rs lgtm
The CQ bit was checked by sabbakumov@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491441707561080,
"parent_rev": "f9ddb72fe527216f17d7dccbc83ee805d4e98ff3", "commit_rev":
"4d9872c0015abd0cac4cbaeeb36585109f1b13e8"}
Message was sent while issue was closed.
Description was changed from ========== Redraw media volume slider when it is moving Steps to reproduce: 1. Launch Chromium http://www.quirksmode.org/html5/tests/video.html 2. Try to change the video volume by dragging the volume slider while holding the mouse key. 3. The volume slider isn't updated. It updates only when the mouse button is released. BUG=677903 ========== to ========== Redraw media volume slider when it is moving Steps to reproduce: 1. Launch Chromium http://www.quirksmode.org/html5/tests/video.html 2. Try to change the video volume by dragging the volume slider while holding the mouse key. 3. The volume slider isn't updated. It updates only when the mouse button is released. BUG=677903 Review-Url: https://codereview.chromium.org/2783593002 Cr-Commit-Position: refs/heads/master@{#462310} Committed: https://chromium.googlesource.com/chromium/src/+/4d9872c0015abd0cac4cbaeeb365... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4d9872c0015abd0cac4cbaeeb365... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
