Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2956023004:
Add a flag to update hover effect when a layout is changed (Closed)
|
Created:
3 years, 5 months ago by lanwei Modified:
3 years, 5 months ago CC:
chromium-reviews, dtapuska+blinkwatch_chromium.org, eae+blinkwatch, sof, jam, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, darin-cc_chromium.org, Navid Zolghadr, blink-reviews, blink-reviews-frames_chromium.org, kinuko+watch, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a flag to update hover effect when a layout is changed
We want to add a flag and implement updating hover effect when a layout
is changed behind this flag.
Bug=488886
Review-Url: https://codereview.chromium.org/2956023004
Cr-Commit-Position: refs/heads/master@{#486393}
Committed: https://chromium.googlesource.com/chromium/src/+/122b716f3278b62d6a2d837383bdaea6527f0d4c
Patch Set 1 : hover layout #
Total comments: 14
Patch Set 2 : add hover flag #
Total comments: 15
Patch Set 3 : add hover flag #
Total comments: 2
Patch Set 4 : Change layout test #
Total comments: 4
Patch Set 5 : Remove pause in test #Messages
Total messages: 62 (45 generated)
Description was changed from ========== hover layout Add a flag to update hover effect when a layout is changed We want to add a flag and implement updating hover effect when a layout is changed behind this flag. Bug:488886 Change-Id: Ic99d86b6517ddc6a1d5cf4b15d7a5f8fcbca85cb ========== to ========== Add a flag to update hover effect when a layout is changed We want to add a flag and implement updating hover effect when a layout is changed behind this flag. Bug=488886 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org
The CQ bit was checked by lanwei@chromium.org 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 checked by lanwei@chromium.org 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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dave, the previous patch is very messy, so I created a new one. Could you please take a look, thank you?
Lan here are some comments. Please have Navid look at it while I am out. And once Navid has looked at bokan@ can look at it. https://codereview.chromium.org/2956023004/diff/20001/content/child/runtime_f... File content/child/runtime_features.cc (right): https://codereview.chromium.org/2956023004/diff/20001/content/child/runtime_f... content/child/runtime_features.cc:82: WebRuntimeFeatures::EnableUpdateHoverPostLayout(true); Please remove. If you are making the feature as a stable flag set it as stable. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1887: if (!cursor_update_timer_.IsActive()) { No need to add braces here; code isn't changed. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (left): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:585: if (mouse_pressed_) So if mouse is pressed or cursor isn't known then we don't update the cursor. This is different than before. Do we need to take these into account? https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:63: const double kFakeMouseMoveIntervalPerFrame = 0.02; I believe constexpr is preferred https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.h (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.h:33: enum class DispatchInterval { kDuringScroll, kPerFrame }; Perhaps this can be scoped into the EventHandler class? As well the DispatchInterval name is a little odd since it isn't an Interval. Maybe something like FakeMouseMoveReason? https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.h:153: bool FakeMouseMovePending(); const? https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebRuntimeFeatures.h:163: BLINK_PLATFORM_EXPORT static void EnableUpdateHoverPostLayout(bool); Remove
The CQ bit was checked by lanwei@chromium.org 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 checked by lanwei@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
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 checked by lanwei@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
The CQ bit was checked by lanwei@chromium.org 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...
Patchset #2 (id:80001) has been deleted
Navid, could you please take a look, thanks? https://codereview.chromium.org/2956023004/diff/20001/content/child/runtime_f... File content/child/runtime_features.cc (right): https://codereview.chromium.org/2956023004/diff/20001/content/child/runtime_f... content/child/runtime_features.cc:82: WebRuntimeFeatures::EnableUpdateHoverPostLayout(true); On 2017/06/28 19:15:17, dtapuska wrote: > Please remove. If you are making the feature as a stable flag set it as stable. Done. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1887: if (!cursor_update_timer_.IsActive()) { On 2017/06/28 19:15:17, dtapuska wrote: > No need to add braces here; code isn't changed. Done. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (left): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:585: if (mouse_pressed_) On 2017/06/28 19:15:18, dtapuska wrote: > So if mouse is pressed or cursor isn't known then we don't update the cursor. > This is different than before. Do we need to take these into account? We should update hover states and mouse cursor when mouse is pressed, but we cannot update them correctly when the mouse position is unknown. I will fix it in another patch. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:63: const double kFakeMouseMoveIntervalPerFrame = 0.02; On 2017/06/28 19:15:17, dtapuska wrote: > I believe constexpr is preferred Done. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.h (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.h:33: enum class DispatchInterval { kDuringScroll, kPerFrame }; On 2017/06/28 19:15:18, dtapuska wrote: > Perhaps this can be scoped into the EventHandler class? As well the > DispatchInterval name is a little odd since it isn't an Interval. Maybe > something like FakeMouseMoveReason? Done. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.h:153: bool FakeMouseMovePending(); On 2017/06/28 19:15:18, dtapuska wrote: > const? Done. https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebRuntimeFeatures.h (right): https://codereview.chromium.org/2956023004/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebRuntimeFeatures.h:163: BLINK_PLATFORM_EXPORT static void EnableUpdateHoverPostLayout(bool); On 2017/06/28 19:15:18, dtapuska wrote: > Remove Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:21: var mouseover = false; Can we make this test a little more comprehensive? Say we add all the event listeners to the blue div and log them all in an array and only expect enter and over in the array and nothing else in that array instead of mouseover and mousemove flags? https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/resources/expect-cursor-update.js (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/resources/expect-cursor-update.js:31: if (internals.fakeMouseMovePending) { Is there any reason we don't wait for cursorUpdatePending as well? Does fakeMouseMovePending cover that? https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrameView.cpp (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrameView.cpp:2603: if (RuntimeEnabledFeatures::UpdateHoverPostLayoutEnabled()) { If we are dragging a draggable object on the page and the page modifies the layout say in drag event handler doesn't this cause mouse events being fired? Then I guess we are contradicting the drag spec as it says we should not send any input device events to the page. I believe when there is a drag operation going on we do not update the hover states either (also it was mentioned in your doc if I recall). https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.h:131: bool FakeMouseMovePending() const; I wonder if there is a macro when building the testing targets https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... that we can check and only expose this API with a #if instead of always exposing this API. https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseEventManager.cpp:328: if (!frame_->GetPage()->IsCursorVisible()) Does this apply for layout change causing fake event case as well? So it's like if the cursor is not visible and if the layout change we do not update the hover state? https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseEventManager.cpp:335: button = WebPointerProperties::Button::kLeft; Shouldn't this still be kNoButton since we are sending a move event? Did that cause any problem?
The CQ bit was checked by lanwei@chromium.org 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 checked by lanwei@chromium.org 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...
Patchset #3 (id:120001) has been deleted
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_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:21: var mouseover = false; On 2017/07/05 17:37:33, Navid Zolghadr wrote: > Can we make this test a little more comprehensive? Say we add all the event > listeners to the blue div and log them all in an array and only expect enter and > over in the array and nothing else in that array instead of mouseover and > mousemove flags? Done. https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/resources/expect-cursor-update.js (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/resources/expect-cursor-update.js:31: if (internals.fakeMouseMovePending) { On 2017/07/05 17:37:33, Navid Zolghadr wrote: > Is there any reason we don't wait for cursorUpdatePending as well? Does > fakeMouseMovePending cover that? Yes, they are different. When we sent fake mouse move events, and we have to use fakeMouseMovePending to decide if we have received the events. Using cursorUpdatePending will give the wrong result. https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrameView.cpp (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrameView.cpp:2603: if (RuntimeEnabledFeatures::UpdateHoverPostLayoutEnabled()) { On 2017/07/05 17:37:33, Navid Zolghadr wrote: > If we are dragging a draggable object on the page and the page modifies the > layout say in drag event handler doesn't this cause mouse events being fired? > Then I guess we are contradicting the drag spec as it says we should not send > any input device events to the page. I believe when there is a drag operation > going on we do not update the hover states either (also it was mentioned in your > doc if I recall). I tested that when we are starting the drag mode, this function does not get called. I think we won't send any mouse events. https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.h:131: bool FakeMouseMovePending() const; On 2017/07/05 17:37:33, Navid Zolghadr wrote: > I wonder if there is a macro when building the testing targets > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... > > that we can check and only expose this API with a #if instead of always exposing > this API. Sorry, I did not find a macro for testing. https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseEventManager.cpp:328: if (!frame_->GetPage()->IsCursorVisible()) On 2017/07/05 17:37:33, Navid Zolghadr wrote: > Does this apply for layout change causing fake event case as well? So it's like > if the cursor is not visible and if the layout change we do not update the hover > state? I think we should not send the fake mouse move when the mouse cursor is not visible when layout changes neither, because we should send any boundary events or update hover states when the mouse is not visible. https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseEventManager.cpp:335: button = WebPointerProperties::Button::kLeft; On 2017/07/05 17:37:33, Navid Zolghadr wrote: > Shouldn't this still be kNoButton since we are sending a move event? Did that > cause any problem? Yes, I tested when we pressed a button and moved the mouse, we got the button of WebPointerProperties::Button::kLeft, and also here, if the button is not Button::kLeft, it will reset the mouse_pressed_ to false. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Mou...
https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.h:131: bool FakeMouseMovePending() const; On 2017/07/06 03:04:34, lanwei wrote: > On 2017/07/05 17:37:33, Navid Zolghadr wrote: > > I wonder if there is a macro when building the testing targets > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... > > > > that we can check and only expose this API with a #if instead of always > exposing > > this API. > > Sorry, I did not find a macro for testing. I couldn't find any existing one either. Dave, do you think it is worth defining a FOR_TESTING_ONLY or something like that in the corresponding testing target in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... ?
https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.h:131: bool FakeMouseMovePending() const; On 2017/07/06 15:06:47, Navid Zolghadr wrote: > On 2017/07/06 03:04:34, lanwei wrote: > > On 2017/07/05 17:37:33, Navid Zolghadr wrote: > > > I wonder if there is a macro when building the testing targets > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... > > > > > > that we can check and only expose this API with a #if instead of always > > exposing > > > this API. > > > > Sorry, I did not find a macro for testing. > > I couldn't find any existing one either. Dave, do you think it is worth defining > a FOR_TESTING_ONLY or something like that in the corresponding testing target in > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... > ? Nevermind. There are already other testing only APIs in this class anyway. It is better to move them all around later together. https://codereview.chromium.org/2956023004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html (right): https://codereview.chromium.org/2956023004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:49: assert_true(result == 'mouseover,mouseenter' || result == 'mouseenter,mouseover'); Why do we check for both ordering? Aren't we always sending over first and then enter events? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Bou...
https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2956023004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.h:131: bool FakeMouseMovePending() const; On 2017/07/06 15:06:47, Navid Zolghadr wrote: > On 2017/07/06 03:04:34, lanwei wrote: > > On 2017/07/05 17:37:33, Navid Zolghadr wrote: > > > I wonder if there is a macro when building the testing targets > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... > > > > > > that we can check and only expose this API with a #if instead of always > > exposing > > > this API. > > > > Sorry, I did not find a macro for testing. > > I couldn't find any existing one either. Dave, do you think it is worth defining > a FOR_TESTING_ONLY or something like that in the corresponding testing target in > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/BUILD.gn?... > ? No we generally don't care for ifdef's.. Just add the functions and the name is fine because the CursorUpdatePending is already there.interped
The CQ bit was checked by lanwei@chromium.org 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2956023004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html (right): https://codereview.chromium.org/2956023004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:49: assert_true(result == 'mouseover,mouseenter' || result == 'mouseenter,mouseover'); On 2017/07/06 15:26:44, Navid Zolghadr wrote: > Why do we check for both ordering? Aren't we always sending over first and then > enter events? > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Bou... Acknowledged.
lgtm
https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html (right): https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:61: { name: 'pause', duration: 0.1 }]}]; This duration is kind of funky. Could this lead to any flakiness? https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseEventManager.cpp:600: // We will fix it soon by keeping the last mouse position somehwhere in This should probably be a TODO with a crbug. (I think 714378 is the relevant bug)
The CQ bit was checked by lanwei@chromium.org 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...
https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html (right): https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:61: { name: 'pause', duration: 0.1 }]}]; On 2017/07/12 17:20:19, dtapuska wrote: > This duration is kind of funky. Could this lead to any flakiness? Done. https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseEventManager.cpp:600: // We will fix it soon by keeping the last mouse position somehwhere in On 2017/07/12 17:20:19, dtapuska wrote: > This should probably be a TODO with a crbug. (I think 714378 is the relevant > bug) Done.
On 2017/07/12 19:00:23, lanwei wrote: > https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html > (right): > > https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/layout-change-should-fire-mouseover.html:61: > { name: 'pause', duration: 0.1 }]}]; > On 2017/07/12 17:20:19, dtapuska wrote: > > This duration is kind of funky. Could this lead to any flakiness? > > Done. > > https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): > > https://codereview.chromium.org/2956023004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/MouseEventManager.cpp:600: // We will fix > it soon by keeping the last mouse position somehwhere in > On 2017/07/12 17:20:19, dtapuska wrote: > > This should probably be a TODO with a crbug. (I think 714378 is the relevant > > bug) > > Done. lgtm
lanwei@chromium.org changed reviewers: + vollick@chromium.org
vollick@chromium.org: Please review changes in third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5, thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/12 19:08:20, lanwei wrote: > mailto:vollick@chromium.org: Please review changes in > > third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5, thank you. lgtm.
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2956023004/#ps180001 (title: "Remove pause in test")
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": 180001, "attempt_start_ts": 1499960880652290,
"parent_rev": "dee0b4dc2f1538b274616ab361a4e03781dd28db", "commit_rev":
"122b716f3278b62d6a2d837383bdaea6527f0d4c"}
Message was sent while issue was closed.
Description was changed from ========== Add a flag to update hover effect when a layout is changed We want to add a flag and implement updating hover effect when a layout is changed behind this flag. Bug=488886 ========== to ========== Add a flag to update hover effect when a layout is changed We want to add a flag and implement updating hover effect when a layout is changed behind this flag. Bug=488886 Review-Url: https://codereview.chromium.org/2956023004 Cr-Commit-Position: refs/heads/master@{#486393} Committed: https://chromium.googlesource.com/chromium/src/+/122b716f3278b62d6a2d837383bd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/122b716f3278b62d6a2d837383bd... |
