Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1840113005:
Move viewport actions into an ApplyScroll callback. (Closed)
|
Created:
4 years, 8 months ago by bokan Modified:
4 years, 8 months ago Reviewers:
tdresser CC:
chromium-reviews, kenneth.christiansen, blink-reviews-html_chromium.org, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, sof, eae+blinkwatch, leviw+renderwatch, blink-reviews-dom_chromium.org, dglazkov+blink, dtapuska+blinkwatch_chromium.org, jchaffraix+rendering, pdr+renderingwatchlist_chromium.org, blink-reviews, rwlbuis, dtapuska Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove viewport actions into an ApplyScroll callback.
This CL moves viewport related actions like top controls movement
and overscroll handling into an applyScroll callback. This callback
is installed on the root document's scrollingElement, or
the documentElement if scrollingElement is null.
Since scroll customization isn't enabled by default, this requires
enabling some of its machinery to run without enabling scroll
customization for web content. In addition, we're still scrolling
on non-scroll customization paths in EventHandler so this patch
explicitly checks if an element has an apply scroll callback and
branches into the scroll customization path in that case.
The rationale for this change is to bundle up all the viewport
actions in one place as a prerequisite for the non-document root
scrollers proposal. The followup to this will be to allow content
to somehow move the callback onto elements other than the
scrollingElement which will cause them to become the de factor
"root scroller".
BUG=505516
Committed: https://crrev.com/3bd3454e3882716955c7db2b0b6ed8a2f1454107
Cr-Commit-Position: refs/heads/master@{#387224}
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Rebase #Patch Set 3 : Addressing Tim's feedback #Patch Set 4 : Removed TODO re: start_position #Patch Set 5 : Rebase and Disable track-word-breaking.html test #Patch Set 6 : Rebase #Patch Set 7 : Nit: removed unused headers #Patch Set 8 : Disabled track-word-breaking.html on release too and remove position in defaultWheelEventHandler #
Total comments: 7
Patch Set 9 : Addressing Tim's feedback #Patch Set 10 : Rebase #Patch Set 11 : Add timeout expectation on Crashing test due to problem on Mac #Patch Set 12 : Rebase #Patch Set 13 : Fixed start_position -> position #Patch Set 14 : Rebase over my own changes #Messages
Total messages: 83 (45 generated)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
bokan@chromium.org changed reviewers: + tdresser@chromium.org
Hey Tim, PTAL, I've called out a few issues I'd like your input on. I've got one problem left to fix (the place where I'm calling updateViewportApplyScroll in Document is causing an ASSERT so I'm searching for a better place) but I think you should be able to start taking a look. Thanks! https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:2022: updateViewportApplyScroll(); This is the wrong place for this. Because scrollingElement calls updateLayoutTree and we might needStyleRecalc when we get here which makes that call invalid. I'm looking for a better place to hook this in. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:659: // TODO(bokan): This is abusing start_position. How should we store current I obviously shouldn't be doing this but I'd like your input on how to pass the current position through. Should we add another field? https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:663: // TODO(bokan): This also feels wrong. Ditto here. delta_garnularity is a double so I think you mean to pass the number of pixels per unit of delta. I cast the enum here but that's a hack to make everything work. Thoughts? https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1990: // hacky, there should be a more explicit way to do this. Wheel events shouldn't report overscroll unless this setting is on. OTOH, I think wheel gestures provides overscroll without checking this flag so I think we could maybe just remove that check and then we wouldn't need a way to designate this as a wheel scroll. WDYT?
+dtapuska, for wheel overscroll question. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:726: if (m_oldScrollingElement) { Ah, is this why we need m_oldScrollingElement to be a Member? So that it doesn't get deleted until we've copied off its applyScrollHandler? https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:733: applyScroll = new ViewportScrollCallback(*this, *frameHost()); This is going to end up looking pretty different once we change the scroll customization API to allow for reasonable composition. I think this is reasonable for now though... https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:737: "disable-native-scroll"); Add a comment indicating why we disable-native-scroll. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1277: Member<Element> m_oldScrollingElement; Add a comment indicating why this can't be a WeakMember. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:133: void computeScrollChainForOne(Node& node, std::deque<int>& scrollChain) Let's be a bit more verbose, and go with "computeScrollChainForSingleNode", or something like that. Should we add a TODO that this method should cease existing once we've migrated everything over to the scroll customization path? https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:659: // TODO(bokan): This is abusing start_position. How should we store current On 2016/04/04 21:44:38, bokan wrote: > I obviously shouldn't be doing this but I'd like your input on how to pass the > current position through. Should we add another field? Yeah, add another field. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:663: // TODO(bokan): This also feels wrong. On 2016/04/04 21:44:38, bokan wrote: > Ditto here. delta_garnularity is a double so I think you mean to pass the number > of pixels per unit of delta. I cast the enum here but that's a hack to make > everything work. Thoughts? Eventually granularity will be web exposed, so there will be spec debate here. I don't feel particularly strongly about what we assign granularity to here for now, we can likely just leave a TODO. Though it wouldn't be too much effort to add a method which mapped from granularity to something at least plausible. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1990: // hacky, there should be a more explicit way to do this. On 2016/04/04 21:44:38, bokan wrote: > Wheel events shouldn't report overscroll unless this setting is on. OTOH, I > think wheel gestures provides overscroll without checking this flag so I think > we could maybe just remove that check and then we wouldn't need a way to > designate this as a wheel scroll. WDYT? There have been a bunch of bugs recently with wheel overscroll. dtapuska@, do you know if we should be respecting this flag? https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:42: #if !ENABLE(OILPAN) While you're here, can you remove everything with !ENABLE(OILPAN)?
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1990: // hacky, there should be a more explicit way to do this. On 2016/04/05 19:31:12, tdresser wrote: > On 2016/04/04 21:44:38, bokan wrote: > > Wheel events shouldn't report overscroll unless this setting is on. OTOH, I > > think wheel gestures provides overscroll without checking this flag so I think > > we could maybe just remove that check and then we wouldn't need a way to > > designate this as a wheel scroll. WDYT? > > There have been a bunch of bugs recently with wheel overscroll. dtapuska@, do > you know if we should be respecting this flag? The setting is kind of useless; my guess is that it was to reduce the IPCs sent. handleOverscroll was always sent on gestures; which causes an IPC up to the render_host. Only Android and MacOSX do overscroll handling. The reportWheelOverscroll is pretty much always set on Mac OSX, there is an option that users can adjust but it was more useful when we supported older Mac OS versions that didn't do rubberbanding. It was never set for Android; ultimately I do believe this code can be removed.
https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:726: if (m_oldScrollingElement) { On 2016/04/05 19:31:12, tdresser wrote: > Ah, is this why we need m_oldScrollingElement to be a Member? So that it doesn't > get deleted until we've copied off its applyScrollHandler? Ah, TBH I did that out of habit and hadn't thought about this. But yes, we do want to keep it around for that reason if the element is removed. I did just realize that I'm (potentially) not handling the case where the Document itself is detached/reattached. I'm not perfectly clear yet on the the mechanics of that so let me investigate that. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:733: applyScroll = new ViewportScrollCallback(*this, *frameHost()); On 2016/04/05 19:31:12, tdresser wrote: > This is going to end up looking pretty different once we change the scroll > customization API to allow for reasonable composition. > > I think this is reasonable for now though... Acknowledged. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:737: "disable-native-scroll"); On 2016/04/05 19:31:12, tdresser wrote: > Add a comment indicating why we disable-native-scroll. Done. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1277: Member<Element> m_oldScrollingElement; On 2016/04/05 19:31:12, tdresser wrote: > Add a comment indicating why this can't be a WeakMember. Actually, I think it can and should. Changed to WeakMember. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:133: void computeScrollChainForOne(Node& node, std::deque<int>& scrollChain) On 2016/04/05 19:31:12, tdresser wrote: > Let's be a bit more verbose, and go with "computeScrollChainForSingleNode", or > something like that. > > Should we add a TODO that this method should cease existing once we've migrated > everything over to the scroll customization path? Both done. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:659: // TODO(bokan): This is abusing start_position. How should we store current On 2016/04/05 19:31:12, tdresser wrote: > On 2016/04/04 21:44:38, bokan wrote: > > I obviously shouldn't be doing this but I'd like your input on how to pass the > > current position through. Should we add another field? > > Yeah, add another field. Ok, I'll add another field in a separate CL before landing this one (since it'll add lots of boilerplate to this already large CL). https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:663: // TODO(bokan): This also feels wrong. On 2016/04/05 19:31:12, tdresser wrote: > On 2016/04/04 21:44:38, bokan wrote: > > Ditto here. delta_garnularity is a double so I think you mean to pass the > number > > of pixels per unit of delta. I cast the enum here but that's a hack to make > > everything work. Thoughts? > > Eventually granularity will be web exposed, so there will be spec debate here. I > don't feel particularly strongly about what we assign granularity to here for > now, we can likely just leave a TODO. sgtm, TODO added. > Though it wouldn't be too much effort to add a method which mapped from > granularity to something at least plausible. The problem with mapping granularity to pixel deltas here is that it'll depend on the scrollable area it ends up scrolling. e.g. If we set page granularity, the amount to scroll will increase as the area is larger. I suppose we could do it here (it would require exposing bits of ScrollableArea I'd rather not) since we know the node we're going to scrolling but this won't work in the general ScrollCustomization case. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1990: // hacky, there should be a more explicit way to do this. On 2016/04/05 19:58:58, dtapuska wrote: > On 2016/04/05 19:31:12, tdresser wrote: > > On 2016/04/04 21:44:38, bokan wrote: > > > Wheel events shouldn't report overscroll unless this setting is on. OTOH, I > > > think wheel gestures provides overscroll without checking this flag so I > think > > > we could maybe just remove that check and then we wouldn't need a way to > > > designate this as a wheel scroll. WDYT? > > > > There have been a bunch of bugs recently with wheel overscroll. dtapuska@, do > > you know if we should be respecting this flag? > > The setting is kind of useless; my guess is that it was to reduce the IPCs sent. > handleOverscroll was always sent on gestures; which causes an IPC up to the > render_host. Only Android and MacOSX do overscroll handling. > > The reportWheelOverscroll is pretty much always set on Mac OSX, there is an > option that users can adjust but it was more useful when we supported older Mac > OS versions that didn't do rubberbanding. It was never set for Android; > ultimately I do believe this code can be removed. Ok, I've removed the setting from Blink in a separate CL (https://codereview.chromium.org/1861343002). It can still be disabled on Mac by setting a value for the NSScrollViewRubberbanding system setting so drop the overscroll based on that flag before sending it to the browser process on Mac. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:42: #if !ENABLE(OILPAN) On 2016/04/05 19:31:12, tdresser wrote: > While you're here, can you remove everything with !ENABLE(OILPAN)? Done.
https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1277: Member<Element> m_oldScrollingElement; On 2016/04/06 15:21:01, bokan wrote: > On 2016/04/05 19:31:12, tdresser wrote: > > Add a comment indicating why this can't be a WeakMember. > > Actually, I think it can and should. Changed to WeakMember. Forgot to remove this comment after pondering the implications in Document::UpdateViewportScrollCallback. Comment added.
bokan@chromium.org changed reviewers: + eae@chromium.org
+eae@ for layout issue/question: In Document.cpp, I added updateViewportApplyScroll to check if the scrollingElement() has changed. Because scrollingElement depends on layout (in quirksmode and scrollTopLeftInterop, it depends on whether body has overflow) I want to call this method at the end of layout. I tried putting it in Document::LayoutUpdated but that's called while we're in "AfterPerformLayout" and scrollingElement() calls updateLayoutTree which then tries to go to InStyleRecalc which trips the ASSERTs in advanceTo() (LayoutClean->InStyleRecalc is fine and that happens right after LayoutUpdated). Do you know where the appropriate place to put this is? Or should scrollingElement be calling something other than updateLayoutTree()? Thanks. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:726: if (m_oldScrollingElement) { On 2016/04/06 15:21:01, bokan wrote: > On 2016/04/05 19:31:12, tdresser wrote: > > Ah, is this why we need m_oldScrollingElement to be a Member? So that it > doesn't > > get deleted until we've copied off its applyScrollHandler? > > Ah, TBH I did that out of habit and hadn't thought about this. But yes, we do > want to keep it around for that reason if the element is removed. > > I did just realize that I'm (potentially) not handling the case where the > Document itself is detached/reattached. I'm not perfectly clear yet on the the > mechanics of that so let me investigate that. Ok, as far as I can tell there's no way to detach the #document itself from the window. The documentElement can be replaced but this mechanism covers that case so I think we're good. https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:659: // TODO(bokan): This is abusing start_position. How should we store current On 2016/04/06 15:21:01, bokan wrote: > On 2016/04/05 19:31:12, tdresser wrote: > > On 2016/04/04 21:44:38, bokan wrote: > > > I obviously shouldn't be doing this but I'd like your input on how to pass > the > > > current position through. Should we add another field? > > > > Yeah, add another field. > > Ok, I'll add another field in a separate CL before landing this one (since it'll > add lots of boilerplate to this already large CL). On second look, I think start_position has the semantics I'm looking for and I got confused by the name. I sent you an email to clarify but I think this is doing the same thing as CC does so I removed the TODO.
bokan@chromium.org changed reviewers: + leviw@chromium.org - eae@chromium.org
Seems eae@ is OOO, +Levi do you know the answer to my question above? Or could loop in someone who might?
bokan@chromium.org changed reviewers: + leviw@chromium.org - eae@chromium.org
Seems eae@ is OOO, +Levi do you know the answer to my question above? Or could loop in someone who might?
On 2016/04/07 at 01:27:15, bokan wrote: > +eae@ for layout issue/question: In Document.cpp, I added updateViewportApplyScroll to check if the scrollingElement() has changed. Because scrollingElement depends on layout (in quirksmode and scrollTopLeftInterop, it depends on whether body has overflow) I want to call this method at the end of layout. I tried putting it in Document::LayoutUpdated but that's called while we're in "AfterPerformLayout" and scrollingElement() calls updateLayoutTree which then tries to go to InStyleRecalc which trips the ASSERTs in advanceTo() (LayoutClean->InStyleRecalc is fine and that happens right after LayoutUpdated). Do you know where the appropriate place to put this is? Or should scrollingElement be calling something other than updateLayoutTree()? So generally speaking, scroll is always handled after layout (as it always depends on layout), so why not do this the same time we do the other scroll update work? Maybe in updateLayerPositions? My other question is: why are we trying to rewind to InStyleRecalc when we call updateLayoutTree? Looking at that code, we should only do that if the layout tree isn't already clean (it should be, no?) and lifecycle().state() < DocumentLifecycle::StyleClean (it also shouldn't be). Otherwise, it looks like it just early returns, no? dglazkov@ or esprehn@ are ultimately your lifecycle gurus, but this doesn't smell right to me already. > > Thanks. > > https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Document.cpp:726: if (m_oldScrollingElement) { > On 2016/04/06 15:21:01, bokan wrote: > > On 2016/04/05 19:31:12, tdresser wrote: > > > Ah, is this why we need m_oldScrollingElement to be a Member? So that it > > doesn't > > > get deleted until we've copied off its applyScrollHandler? > > > > Ah, TBH I did that out of habit and hadn't thought about this. But yes, we do > > want to keep it around for that reason if the element is removed. > > > > I did just realize that I'm (potentially) not handling the case where the > > Document itself is detached/reattached. I'm not perfectly clear yet on the the > > mechanics of that so let me investigate that. > > Ok, as far as I can tell there's no way to detach the #document itself from the window. The documentElement can be replaced but this mechanism covers that case so I think we're good. > > https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/EventHandler.cpp (right): > > https://codereview.chromium.org/1840113005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/EventHandler.cpp:659: // TODO(bokan): This is abusing start_position. How should we store current > On 2016/04/06 15:21:01, bokan wrote: > > On 2016/04/05 19:31:12, tdresser wrote: > > > On 2016/04/04 21:44:38, bokan wrote: > > > > I obviously shouldn't be doing this but I'd like your input on how to pass > > the > > > > current position through. Should we add another field? > > > > > > Yeah, add another field. > > > > Ok, I'll add another field in a separate CL before landing this one (since it'll > > add lots of boilerplate to this already large CL). > > On second look, I think start_position has the semantics I'm looking for and I got confused by the name. I sent you an email to clarify but I think this is doing the same thing as CC does so I removed the TODO.
On 2016/04/07 23:50:37, leviw wrote: > On 2016/04/07 at 01:27:15, bokan wrote: > > +eae@ for layout issue/question: In Document.cpp, I added > updateViewportApplyScroll to check if the scrollingElement() has changed. > Because scrollingElement depends on layout (in quirksmode and > scrollTopLeftInterop, it depends on whether body has overflow) I want to call > this method at the end of layout. I tried putting it in Document::LayoutUpdated > but that's called while we're in "AfterPerformLayout" and scrollingElement() > calls updateLayoutTree which then tries to go to InStyleRecalc which trips the > ASSERTs in advanceTo() (LayoutClean->InStyleRecalc is fine and that happens > right after LayoutUpdated). Do you know where the appropriate place to put this > is? Or should scrollingElement be calling something other than > updateLayoutTree()? > > So generally speaking, scroll is always handled after layout (as it always > depends on layout), so why not do this the same time we do the other scroll > update work? Maybe in updateLayerPositions? > > My other question is: why are we trying to rewind to InStyleRecalc when we call > updateLayoutTree? Looking at that code, we should only do that if the layout > tree isn't already clean (it should be, no?) and lifecycle().state() < > DocumentLifecycle::StyleClean (it also shouldn't be). Otherwise, it looks like > it just early returns, no? > > dglazkov@ or esprehn@ are ultimately your lifecycle gurus, but this doesn't > smell right to me already. > So looking at the test (media/track/track-word-breaking.html) I think calling my method from updateLayoutTree is valid but I'm being bitten by http://crbug.com/372245. I'll continue the discussion there. Thanks!
Description was changed from ========== Move viewport actions into an ApplyScroll callback. BUG=505516 ========== to ========== Move viewport actions into an ApplyScroll callback. This CL moves viewport related actions like top controls movement and overscroll handling into an applyScroll callback. This callback is installed on the root document's scrollingElement, or the documentElement if scrollingElement is null. Since scroll customization isn't enabled by default, this requires enabling some of its machinery to run without enabling scroll customization for web content. In addition, we're still scrolling on non-scroll customization paths in EventHandler so this patch explicitly checks if an element has an apply scroll callback and branches into the scroll customization path in that case. The rationale for this change is to bundle up all the viewport actions in one place as a prerequisite for the non-document root scrollers proposal. The followup to this will be to allow content to somehow move the callback onto elements other than the scrollingElement which will cause them to become the de factor "root scroller". BUG=505516 ==========
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #8 (id:300001) has been deleted
Tim, ptal. I've addressed all the feedback. The issue with the placement of updateViewportApplyScroll in updateLayoutTree turns out to be an existing bug elsewhere that this patch tickles in one layout test so I've disabled that test.
LGTM https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1288: // it around until we move its ApplyScroll onto the new scrollingElement. Comment wording is confusing. I think you just need to remove "if we want to remove the scrolling element". https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:638: // TODO(bokan): If the applyScroll is installed on the body, we can still "the applyScroll" doesn't necessarily refer to a unique callback. https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.h (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.h:34: Member<Document> m_document; Should these be Members or WeakMembers?
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:56: crbug.com/372245 media/track/track-word-breaking.html [ Failure ] Changed this to a Crash expectation since it's still causing redness on the bots. https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:1288: // it around until we move its ApplyScroll onto the new scrollingElement. On 2016/04/12 13:59:15, tdresser wrote: > Comment wording is confusing. > > I think you just need to remove "if we want to remove the scrolling element". Done. https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:638: // TODO(bokan): If the applyScroll is installed on the body, we can still On 2016/04/12 13:59:15, tdresser wrote: > "the applyScroll" doesn't necessarily refer to a unique callback. Changed "applyScroll" to "ViewportScrollCallback" which is more accurate. https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.h (right): https://codereview.chromium.org/1840113005/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ViewportScrollCallback.h:34: Member<Document> m_document; On 2016/04/12 13:59:15, tdresser wrote: > Should these be Members or WeakMembers? WeakMembers. Changed and added corresponding null-checks out in handleEvent.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/360001
bokan@chromium.org changed reviewers: - dtapuska@chromium.org, leviw@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1840113005/#ps400001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1840113005/#ps420001 (title: "Fixed start_position -> position")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for third_party/WebKit/Source/core/input/EventHandler.cpp:
While running git apply --index -3 -p1;
error: patch failed:
third_party/WebKit/Source/core/input/EventHandler.cpp:2457
Falling back to three-way merge...
Applied patch to 'third_party/WebKit/Source/core/input/EventHandler.cpp' with
conflicts.
U third_party/WebKit/Source/core/input/EventHandler.cpp
Patch: third_party/WebKit/Source/core/input/EventHandler.cpp
Index: third_party/WebKit/Source/core/input/EventHandler.cpp
diff --git a/third_party/WebKit/Source/core/input/EventHandler.cpp
b/third_party/WebKit/Source/core/input/EventHandler.cpp
index
e54b78d72edbaf4f11f619e3a84ca813e6dae23e..092195dbc2491a721026d6caadde2e72a3a5f18c
100644
--- a/third_party/WebKit/Source/core/input/EventHandler.cpp
+++ b/third_party/WebKit/Source/core/input/EventHandler.cpp
@@ -164,6 +164,47 @@ bool shouldRefetchEventTarget(const
MouseEventWithHitTestResults& mev)
return targetNode->isShadowRoot() &&
isHTMLInputElement(*toShadowRoot(targetNode)->host());
}
+// TODO(bokan): This method can go away once all scrolls happen through the
+// scroll customization path.
+void computeScrollChainForSingleNode(Node& node, std::deque<int>& scrollChain)
+{
+ scrollChain.clear();
+
+ ASSERT(node.layoutObject());
+ Element* element = toElement(&node);
+
+ scrollChain.push_front(DOMNodeIds::idForNode(element));
+}
+
+void recomputeScrollChain(const LocalFrame& frame, const Node& startNode,
+ std::deque<int>& scrollChain)
+{
+ scrollChain.clear();
+
+ ASSERT(startNode.layoutObject());
+ LayoutBox* curBox = startNode.layoutObject()->enclosingBox();
+
+ // Scrolling propagates along the containing block chain.
+ while (curBox && !curBox->isLayoutView()) {
+ Node* curNode = curBox->node();
+ // FIXME: this should reject more elements, as part of
crbug.com/410974.
+ if (curNode && curNode->isElementNode()) {
+ Element* curElement = toElement(curNode);
+ if (curElement == frame.document()->scrollingElement())
+ break;
+ scrollChain.push_front(DOMNodeIds::idForNode(curElement));
+ }
+ curBox = curBox->containingBlock();
+ }
+ // TODO(tdresser): this should sometimes be excluded, as part of
crbug.com/410974.
+ // We need to ensure that the scrollingElement is always part of
+ // the scroll chain. In quirks mode, when the scrollingElement is
+ // the body, some elements may use the documentElement as their
+ // containingBlock, so we ensure the scrollingElement is added
+ // here.
+
scrollChain.push_front(DOMNodeIds::idForNode(frame.document()->scrollingElement()));
+}
+
} // namespace
using namespace HTMLNames;
@@ -222,35 +263,6 @@ private:
Cursor m_cursor;
};
-void recomputeScrollChain(const LocalFrame& frame, const Node& startNode,
- std::deque<int>& scrollChain)
-{
- scrollChain.clear();
-
- ASSERT(startNode.layoutObject());
- LayoutBox* curBox = startNode.layoutObject()->enclosingBox();
-
- // Scrolling propagates along the containing block chain.
- while (curBox && !curBox->isLayoutView()) {
- Node* curNode = curBox->node();
- // FIXME: this should reject more elements, as part of
crbug.com/410974.
- if (curNode && curNode->isElementNode()) {
- Element* curElement = toElement(curNode);
- if (curElement == frame.document()->scrollingElement())
- break;
- scrollChain.push_front(DOMNodeIds::idForNode(curElement));
- }
- curBox = curBox->containingBlock();
- }
- // TODO(tdresser): this should sometimes be excluded, as part of
crbug.com/410974.
- // We need to ensure that the scrollingElement is always part of
- // the scroll chain. In quirks mode, when the scrollingElement is
- // the body, some elements may use the documentElement as their
- // containingBlock, so we ensure the scrollingElement is added
- // here.
-
scrollChain.push_front(DOMNodeIds::idForNode(frame.document()->scrollingElement()));
-}
-
EventHandler::EventHandler(LocalFrame* frame)
: m_frame(frame)
, m_mousePressed(false)
@@ -611,7 +623,83 @@ void EventHandler::stopAutoscroll()
controller->stopAutoscroll();
}
-ScrollResult EventHandler::physicalScroll(ScrollGranularity granularity, const
FloatSize& delta, Node* startNode, Node** stopNode, bool* consumed)
+ScrollResult EventHandler::scrollBox(LayoutBox* box,
+ ScrollGranularity granularity, const FloatSize& delta,
+ const FloatPoint& position, const FloatSize& velocity,
+ bool* wasRootScroller)
+{
+ ASSERT(box);
+ Node* node = box->node();
+ Document* document = m_frame->document();
+ Element* scrollingElement = document->scrollingElement();
+
+ bool isRootFrame = !document->ownerElement();
+
+ // TODO(bokan): If the ViewportScrollCallback is installed on the body, we
+ // can still hit the HTML element for scrolling in which case it'll bubble
+ // up to the document node and try to scroll the LayoutView directly. Make
+ // sure we never scroll the LayoutView like this by manually resetting the
+ // scroll to happen on the scrolling element. This can also happen in
+ // QuirksMode when the body is scrollable and scrollingElement == nullptr.
+ if (node && node->isDocumentNode() && isRootFrame) {
+ node = scrollingElement
+ ? scrollingElement
+ : document->documentElement();
+ }
+
+ // If there's no ApplyScroll callback on the element, scroll as usuall in
+ // the non-scroll-customization case.
+ if (!node || !node->isElementNode() || !toElement(node)->getApplyScroll())
{
+ ASSERT(!isRootFrame
+ || node != scrollingElement
+ || (!scrollingElement && node != document->documentElement()));
+ *wasRootScroller = false;
+ return box->scroll(granularity, delta);
+ }
+
+ ASSERT(isRootFrame);
+
+ // If there is an ApplyScroll callback, its because we placed one on the
+ // root scroller to control top controls and overscroll. Invoke a scroll
+ // using parts of the scroll customization framework on just this element.
+ computeScrollChainForSingleNode(*node, m_currentScrollChain);
+
+ OwnPtr<ScrollStateData> scrollStateData = adoptPtr(new ScrollStateData());
+ scrollStateData->delta_x = delta.width();
+ scrollStateData->delta_y = delta.height();
+ scrollStateData->position_x = position.x();
+ scrollStateData->position_y = position.y();
+ // TODO(bokan): delta_granularity is meant to be the number of pixels per
+ // unit of delta but we can't determine that until we get to the area we'll
+ // scroll. This is a hack, we stuff the enum into the double value for
+ // now.
+ scrollStateData->delta_granularity = static_cast<double>(granularity);
+ scrollStateData->velocity_x = velocity.width();
+ scrollStateData->velocity_y = velocity.height();
+ scrollStateData->should_propagate = false;
+ scrollStateData->is_in_inertial_phase = false;
+ scrollStateData->from_user_input = true;
+ scrollStateData->delta_consumed_for_scroll_sequence = false;
+ ScrollState* scrollState =
+ ScrollState::create(scrollStateData.release());
+
+ customizedScroll(*node, *scrollState);
+
+ ScrollResult result(
+ scrollState->deltaX() != delta.width(),
+ scrollState->deltaY() != delta.height(),
+ scrollState->deltaX(),
+ scrollState->deltaY());
+
+ *wasRootScroller = true;
+ m_currentScrollChain.clear();
+
+ return result;
+}
+
+ScrollResult EventHandler::physicalScroll(ScrollGranularity granularity,
+ const FloatSize& delta, const FloatPoint& position,
+ const FloatSize& velocity, Node* startNode, Node** stopNode, bool*
consumed)
{
if (consumed)
*consumed = false;
@@ -631,7 +719,15 @@ ScrollResult EventHandler::physicalScroll(ScrollGranularity
granularity, const F
// chain past it.
bool shouldStopChaining =
stopNode && *stopNode && curBox->node() == *stopNode;
- result = curBox->scroll(granularity, delta);
+ bool wasRootScroller = false;
+
+ result = scrollBox(
+ curBox,
+ granularity,
+ delta,
+ position,
+ velocity,
+ &wasRootScroller);
if (result.didScroll() && stopNode)
*stopNode = curBox->node();
@@ -641,6 +737,10 @@ ScrollResult EventHandler::physicalScroll(ScrollGranularity
granularity, const F
if (consumed)
*consumed = true;
return result;
+ } else if (wasRootScroller) {
+ // Don't try to chain past the root scroller, even if there's
+ // eligible ancestors.
+ break;
}
curBox = curBox->containingBlock();
@@ -1894,13 +1994,18 @@ void EventHandler::defaultWheelEventHandler(Node*
startNode, WheelEvent* wheelEv
// FIXME: enable scroll customization in this case. See crbug.com/410974.
bool consumed = false;
- ScrollResult result = physicalScroll(granularity, delta, startNode, &node,
&consumed);
+
+ physicalScroll(
+ granularity,
+ delta,
+ FloatPoint(),
+ FloatSize(),
+ startNode,
+ &node,
+ &consumed);
if (consumed)
wheelEvent->setDefaultHandled();
-
- if (m_frame->isMainFrame())
- handleOverscroll(result);
}
WebInputEventResult EventHandler::handleGestureShowPress()
@@ -2386,6 +2491,20 @@ void EventHandler::handleOverscroll(const ScrollResult&
scrollResult, const Floa
}
}
+bool EventHandler::isRootScroller(const Node& node) const
+{
+ // The root scroller is the one Element on the page designated to perform
+ // "viewport actions" like top controls movement and overscroll glow.
+
+ if (!node.isElementNode() || node.document().ownerElement())
+ …
(message too large)
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1840113005/#ps440001 (title: "Rebase over my own changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840113005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840113005/440001
Message was sent while issue was closed.
Description was changed from ========== Move viewport actions into an ApplyScroll callback. This CL moves viewport related actions like top controls movement and overscroll handling into an applyScroll callback. This callback is installed on the root document's scrollingElement, or the documentElement if scrollingElement is null. Since scroll customization isn't enabled by default, this requires enabling some of its machinery to run without enabling scroll customization for web content. In addition, we're still scrolling on non-scroll customization paths in EventHandler so this patch explicitly checks if an element has an apply scroll callback and branches into the scroll customization path in that case. The rationale for this change is to bundle up all the viewport actions in one place as a prerequisite for the non-document root scrollers proposal. The followup to this will be to allow content to somehow move the callback onto elements other than the scrollingElement which will cause them to become the de factor "root scroller". BUG=505516 ========== to ========== Move viewport actions into an ApplyScroll callback. This CL moves viewport related actions like top controls movement and overscroll handling into an applyScroll callback. This callback is installed on the root document's scrollingElement, or the documentElement if scrollingElement is null. Since scroll customization isn't enabled by default, this requires enabling some of its machinery to run without enabling scroll customization for web content. In addition, we're still scrolling on non-scroll customization paths in EventHandler so this patch explicitly checks if an element has an apply scroll callback and branches into the scroll customization path in that case. The rationale for this change is to bundle up all the viewport actions in one place as a prerequisite for the non-document root scrollers proposal. The followup to this will be to allow content to somehow move the callback onto elements other than the scrollingElement which will cause them to become the de factor "root scroller". BUG=505516 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Move viewport actions into an ApplyScroll callback. This CL moves viewport related actions like top controls movement and overscroll handling into an applyScroll callback. This callback is installed on the root document's scrollingElement, or the documentElement if scrollingElement is null. Since scroll customization isn't enabled by default, this requires enabling some of its machinery to run without enabling scroll customization for web content. In addition, we're still scrolling on non-scroll customization paths in EventHandler so this patch explicitly checks if an element has an apply scroll callback and branches into the scroll customization path in that case. The rationale for this change is to bundle up all the viewport actions in one place as a prerequisite for the non-document root scrollers proposal. The followup to this will be to allow content to somehow move the callback onto elements other than the scrollingElement which will cause them to become the de factor "root scroller". BUG=505516 ========== to ========== Move viewport actions into an ApplyScroll callback. This CL moves viewport related actions like top controls movement and overscroll handling into an applyScroll callback. This callback is installed on the root document's scrollingElement, or the documentElement if scrollingElement is null. Since scroll customization isn't enabled by default, this requires enabling some of its machinery to run without enabling scroll customization for web content. In addition, we're still scrolling on non-scroll customization paths in EventHandler so this patch explicitly checks if an element has an apply scroll callback and branches into the scroll customization path in that case. The rationale for this change is to bundle up all the viewport actions in one place as a prerequisite for the non-document root scrollers proposal. The followup to this will be to allow content to somehow move the callback onto elements other than the scrollingElement which will cause them to become the de factor "root scroller". BUG=505516 Committed: https://crrev.com/3bd3454e3882716955c7db2b0b6ed8a2f1454107 Cr-Commit-Position: refs/heads/master@{#387224} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3bd3454e3882716955c7db2b0b6ed8a2f1454107 Cr-Commit-Position: refs/heads/master@{#387224} |
