Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1459023002:
Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp (Closed)
|
Created:
5 years, 1 month ago by loyso (OOO) Modified:
5 years ago CC:
blink-layers+watch_chromium.org, blink-reviews, chromium-reviews, kenneth.christiansen, mlamouri+watch-blink_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCompositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp
We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here:
https://codereview.chromium.org/1308053006/
So we need to notify Page and ScrollingCoordinator on main frame swapping.
Otherwise, the following webkit_unit_tests fail in new mode:
WebFrameTest.SwapMainFrameWhileLoading
WebFrameSwapTest.FramesOfRemoteParentAreIndexable
WebFrameSwapTest.FrameElementInFramesWithRemoteParent
Note that this code works only in --site-per-process mode.
BUG=394777
Committed: https://crrev.com/c65cc1e198dbc38c38aa0c27773802947c1c33ff
Cr-Commit-Position: refs/heads/master@{#363128}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Plumb it from detach method instead. #
Total comments: 7
Patch Set 3 : Make ProgrammaticScrollAnimatorTimeline to match LayerTreeView lifetime scope. #Patch Set 4 : Simplify m_programmaticScrollAnimatorTimeline attachment. #Patch Set 5 : Fixes. #Patch Set 6 : Plumb it for WebPagePopup. #
Total comments: 5
Patch Set 7 : Remove m_page nullptr checks. #Patch Set 8 : Check if m_page is null in WebPagePopupImpl. #
Messages
Total messages: 41 (11 generated)
Description was changed from ========== Fix swap in WebFrameTest BUG= ========== to ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: All/ParameterizedWebFrameTest.CompositedSelectionBoundsCleared WebFrameSwapTest.FrameElementInFramesWithRemoteParent WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameTest.SwapMainFrameWhileLoading BUG=394777 ==========
loyso@chromium.org changed reviewers: + aelias@chromium.org, dcheng@chromium.org, vollick@chromium.org
aelias@chromium.org: Please review changes in vollick@chromium.org: Please review changes in dcheng@chromium.org: Please review changes in
Description was changed from ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: All/ParameterizedWebFrameTest.CompositedSelectionBoundsCleared WebFrameSwapTest.FrameElementInFramesWithRemoteParent WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameTest.SwapMainFrameWhileLoading BUG=394777 ========== to ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: All/ParameterizedWebFrameTest.CompositedSelectionBoundsCleared WebFrameSwapTest.FrameElementInFramesWithRemoteParent WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameTest.SwapMainFrameWhileLoading Note that this code works only in --site-per-process mode. BUG=394777 ==========
https://codereview.chromium.org/1459023002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/1459023002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrame.cpp:98: oldFrame->detach(FrameDetachType::Swap); Please plumb it from this detach() method instead. It already knows it's a swap-type detach from the enum passed in here, and you're not using the argument you passed in for the new frame.
https://codereview.chromium.org/1459023002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/1459023002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrame.cpp:98: oldFrame->detach(FrameDetachType::Swap); On 2015/11/19 01:19:58, aelias wrote: > Please plumb it from this detach() method instead. It already knows it's a > swap-type detach from the enum passed in here, and you're not using the argument > you passed in for the new frame. Done.
https://codereview.chromium.org/1459023002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrame.cpp (right): https://codereview.chromium.org/1459023002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrame.cpp:98: oldFrame->detach(FrameDetachType::Swap); On 2015/11/19 01:19:58, aelias wrote: > Please plumb it from this detach() method instead. It already knows it's a > swap-type detach from the enum passed in here, and you're not using the argument > you passed in for the new frame. p.s. WebFrame::detach is already called from Page::willBeDestroyed. So, calling something back is strange in terms of ownership.
https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:96: if (type == FrameDetachType::Swap && isMainFrame() && page()) I don't think page() can ever be null here. An invariant that Frame and its subclasses try to maintain is that !!page() == !!client() == !!host(). https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:724: createProgrammaticScrollAnimatorTimeline(); It feels like this should be tied to initializing the layer tree view, perhaps? https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:741: destroyProgrammaticScrollAnimatorTimeline(); See previous comment: I'm a bit confused. Why is this called in both willBeDestroyed() and willCloseLayerTreeView()? They're both called on page shutdown, and it seems like if this code was tied to layer tree creation/cleanup, then we would minimize the amount of code that shows up in other layers.
kenrb@chromium.org changed reviewers: + kenrb@chromium.org
Would it be possible to fix this problem just by having the main frame RenderWidget close its LayerTreeView when the main frame gets swapped out? The main frame's compositor is not needed at all.
On 2015/11/19 19:16:59, kenrb wrote: > Would it be possible to fix this problem just by having the main frame > RenderWidget close its LayerTreeView when the main frame gets swapped out? The > main frame's compositor is not needed at all. We call WebFrame::swap from RenderFrameImpl::OnSwapOut and I could call RenderWidget::WillCloseLayerTreeView there (note that WebFrameTest.cpp does nothing like that, it's too low level) But we also call WebFrame::swap from RenderFrameImpl::didCommitProvisionalLoad. What should I do there?
On 2015/11/20 03:58:42, loyso wrote: > On 2015/11/19 19:16:59, kenrb wrote: > > Would it be possible to fix this problem just by having the main frame > > RenderWidget close its LayerTreeView when the main frame gets swapped out? The > > main frame's compositor is not needed at all. > > We call WebFrame::swap from RenderFrameImpl::OnSwapOut and I could call > RenderWidget::WillCloseLayerTreeView there (note that WebFrameTest.cpp does > nothing like that, it's too low level) > > But we also call WebFrame::swap from RenderFrameImpl::didCommitProvisionalLoad. > What should I do there? kenrb@, any suggestions/details? ping!
Description was changed from ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: All/ParameterizedWebFrameTest.CompositedSelectionBoundsCleared WebFrameSwapTest.FrameElementInFramesWithRemoteParent WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameTest.SwapMainFrameWhileLoading Note that this code works only in --site-per-process mode. BUG=394777 ========== to ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: WebFrameTest.SwapMainFrameWhileLoading WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameSwapTest.FrameElementInFramesWithRemoteParent Note that this code works only in --site-per-process mode. BUG=394777 ==========
On 2015/11/20 03:58:42, loyso wrote: > On 2015/11/19 19:16:59, kenrb wrote: > > Would it be possible to fix this problem just by having the main frame > > RenderWidget close its LayerTreeView when the main frame gets swapped out? The > > main frame's compositor is not needed at all. > > We call WebFrame::swap from RenderFrameImpl::OnSwapOut and I could call > RenderWidget::WillCloseLayerTreeView there (note that WebFrameTest.cpp does > nothing like that, it's too low level) > > But we also call WebFrame::swap from RenderFrameImpl::didCommitProvisionalLoad. > What should I do there? I'm not certain, but I don't think you have to worry about that. I don't think a remote main frame should ever be swapped to a local main frame with --site-per-process. Without the flag, swapped out RenderViews can be swapped in again, but in that case we shouldn't be changing anything about the RenderWidget. I don't know for sure that calling WillCloseLayerTreeView when the main frame gets swapped out will work, since that is normally only called when RenderWidget is about to get deleted, but it is worth trying since the RenderWidget should be unused after that point.
On 2015/11/24 at 15:09:07, kenrb wrote: > On 2015/11/20 03:58:42, loyso wrote: > > On 2015/11/19 19:16:59, kenrb wrote: > > > Would it be possible to fix this problem just by having the main frame > > > RenderWidget close its LayerTreeView when the main frame gets swapped out? The > > > main frame's compositor is not needed at all. > > > > We call WebFrame::swap from RenderFrameImpl::OnSwapOut and I could call > > RenderWidget::WillCloseLayerTreeView there (note that WebFrameTest.cpp does > > nothing like that, it's too low level) > > > > But we also call WebFrame::swap from RenderFrameImpl::didCommitProvisionalLoad. > > What should I do there? > > I'm not certain, but I don't think you have to worry about that. I don't think a remote main frame should ever be swapped to a local main frame with --site-per-process. Err, why not? I would expect this to happen at least some of the time. > > Without the flag, swapped out RenderViews can be swapped in again, but in that case we shouldn't be changing anything about the RenderWidget. > > I don't know for sure that calling WillCloseLayerTreeView when the main frame gets swapped out will work, since that is normally only called when RenderWidget is about to get deleted, but it is worth trying since the RenderWidget should be unused after that point. In an ideal world where OOPI is done, swapping the main frame to/from local would create/close a corresponding RenderWidget/WebWidget. If we tied this to layer tree creation/destruction, it seems like this would Just Work (tm). Unfortunately, we're not there yet, but avi@ has been working on separating the dependencies on the browser side, and we've made progress on the renderer side as well. Even without the finished RenderWidget/WebWidget plumbing, I think we could still update the plumbing to create/destroy the layer tree view on main frame swap.
(Also, sending my draft comment) https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:741: destroyProgrammaticScrollAnimatorTimeline(); On 2015/11/19 at 05:09:56, dcheng wrote: > See previous comment: I'm a bit confused. Why is this called in both willBeDestroyed() and willCloseLayerTreeView()? They're both called on page shutdown, and it seems like if this code was tied to layer tree creation/cleanup, then we would minimize the amount of code that shows up in other layers. Ping? I'm still curious about an answer to this, since this affects what we might choose to do here.
On 2015/11/25 06:09:33, dcheng wrote: > On 2015/11/24 at 15:09:07, kenrb wrote: > > On 2015/11/20 03:58:42, loyso wrote: > > > On 2015/11/19 19:16:59, kenrb wrote: > > > > Would it be possible to fix this problem just by having the main frame > > > > RenderWidget close its LayerTreeView when the main frame gets swapped out? > The > > > > main frame's compositor is not needed at all. > > > > > > We call WebFrame::swap from RenderFrameImpl::OnSwapOut and I could call > > > RenderWidget::WillCloseLayerTreeView there (note that WebFrameTest.cpp does > > > nothing like that, it's too low level) > > > > > > But we also call WebFrame::swap from > RenderFrameImpl::didCommitProvisionalLoad. > > > What should I do there? > > > > I'm not certain, but I don't think you have to worry about that. I don't think > a remote main frame should ever be swapped to a local main frame with > --site-per-process. > > Err, why not? I would expect this to happen at least some of the time. > I had thought that a top-level navigation would cause the frame tree to be recreated, and main frames being swapped back in would only occur when there are non-site-per-process swapped out RenderViews, but I believe you if you say that is not the case. If top-level navigation can cause a remote frame to be swapped back to a local frame in site-per-process, then we need to be able to restore the top-level compositor to a useful state. > > > > Without the flag, swapped out RenderViews can be swapped in again, but in that > case we shouldn't be changing anything about the RenderWidget. > > > > I don't know for sure that calling WillCloseLayerTreeView when the main frame > gets swapped out will work, since that is normally only called when RenderWidget > is about to get deleted, but it is worth trying since the RenderWidget should be > unused after that point. > > In an ideal world where OOPI is done, swapping the main frame to/from local > would create/close a corresponding RenderWidget/WebWidget. If we tied this to > layer tree creation/destruction, it seems like this would Just Work (tm). > Unfortunately, we're not there yet, but avi@ has been working on separating the > dependencies on the browser side, and we've made progress on the renderer side > as well. Even without the finished RenderWidget/WebWidget plumbing, I think we > could still update the plumbing to create/destroy the layer tree view on main > frame swap. Thanks for the update on that. This is what I am hoping for.
This CL is the only blocker for this: https://codereview.chromium.org/1308053006/ (targeted M48) Can we land it as in PatchSet#3? I could come up with a proper-designed solution in a separate dependent CL. Still not sure that we need to recreate LayerTreeView. It looks that we could go with something like WebWidget::attachToLayerTreeView / WebWidget::detachFromLayerTreeView to be called from RenderFrameImpl. https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Frame.cpp:96: if (type == FrameDetachType::Swap && isMainFrame() && page()) On 2015/11/19 05:09:56, dcheng wrote: > I don't think page() can ever be null here. An invariant that Frame and its > subclasses try to maintain is that !!page() == !!client() == !!host(). Acknowledged. https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:724: createProgrammaticScrollAnimatorTimeline(); On 2015/11/19 05:09:56, dcheng wrote: > It feels like this should be tied to initializing the layer tree view, perhaps? Done. https://codereview.chromium.org/1459023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:741: destroyProgrammaticScrollAnimatorTimeline(); On 2015/11/25 06:09:44, dcheng wrote: > On 2015/11/19 at 05:09:56, dcheng wrote: > > See previous comment: I'm a bit confused. Why is this called in both > willBeDestroyed() and willCloseLayerTreeView()? They're both called on page > shutdown, and it seems like if this code was tied to layer tree > creation/cleanup, then we would minimize the amount of code that shows up in > other layers. > > Ping? I'm still curious about an answer to this, since this affects what we > might choose to do here. We have many situations when we call Page::willBeDestroyed and we don't call Page::willCloseLayerTreeView. I was afraid that we don't destroy it properly in all configurations. Now I agree, that if we bind ProgrammaticScrollAnimatorTimeline existence and lifetime scope to WebViewImpl::initializeLayerTreeView/WebViewImpl::willCloseLayerTreeView then it will be just fine.
PTAL, everyone! It's very simple now.
Anyone?
On 2015/11/29 23:22:15, loyso wrote: > Anyone? The changes in core/ lgtm, but kenrb or dcheng will need to comment on the stuff in web/.
https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:607: if (page() && m_layerTreeView) Out of curiosity... how can we have a null page here? Can this get called on an already detached frame? https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right): https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebPagePopupImpl.cpp:346: if (m_page) Is the m_page test here necessary? It looks asymmetrical with the other web/ changes.
https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:607: if (page() && m_layerTreeView) On 2015/12/01 01:16:30, dcheng wrote: > Out of curiosity... how can we have a null page here? Can this get called on an > already detached frame? There is a comment: // Returns the page object associated with this widget. This may be null when // the page is shutting down, but will be valid at all other times. Page* page() const { return view()->page(); } It's not completely clear, what the "shutting down" scope means. But anyway, the lifetime scope for m_layerTreeView must be within the case when frame is attached. Fixed. https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebPagePopupImpl.cpp (right): https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebPagePopupImpl.cpp:346: if (m_page) On 2015/12/01 01:16:30, dcheng wrote: > Is the m_page test here necessary? It looks asymmetrical with the other web/ > changes. WebPagePopupImpl tests m_page for nullptr almost everywhere (same for WebViewImpl). But I agree, there shouldn't be any checks for the sake of symmetry. Now I remember that invariant !!page() == !!client() == !!host() :) Fixed.
https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/1459023002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:607: if (page() && m_layerTreeView) On 2015/12/01 01:16:30, dcheng wrote: > Out of curiosity... how can we have a null page here? Can this get called on an > already detached frame? Well, we have it null here in fast/forms/ layout tests (see failed webkit_tests). It's called from RenderWidget::DoDeferredClose.
kenrb@ or dcheng@, PTAL?
lgtm but mind explaining to me why you ended up needing to check m_page in WebPagePopupImpl::willCloseLayerTreeView()? I'm wondering if this is something we can cleanup.
On 2015/12/03 07:06:50, dcheng wrote: > lgtm > > but mind explaining to me why you ended up needing to check m_page in > WebPagePopupImpl::willCloseLayerTreeView()? I'm wondering if this is something > we can cleanup. The following layout tests fail on nullptr call for m_page: fast/forms/calendar-picker/calendar-picker-should-not-change-datetimelocal-time.html fast/forms/select/popup-menu-touch-operations.html fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance.html fast/forms/calendar-picker/month-picker-choose-default-value-after-set-value.html fast/forms/suggestion-picker/date-suggestion-picker-appearance-zoom200.html fast/forms/suggestion-picker/datetimelocal-suggestion-picker-key-operations.html fast/forms/suggestion-picker/week-suggestion-picker-mouse-operations.html fast/forms/select/optgroup-disabled.html fast/forms/suggestion-picker/week-suggestion-picker-key-operations.html fast/forms/select/popup-menu-position.html fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html accessibility/menu-list-optgroup.html fast/forms/suggestion-picker/time-suggestion-picker-step-attribute.html fast/forms/calendar-picker/calendar-picker-appearance-minimum-date.html fast/forms/select/select-disabled.html fast/forms/calendar-picker/calendar-picker-mouse-operations.html fast/forms/suggestion-picker/month-suggestion-picker-key-operations.html fast/forms/calendar-picker/calendar-picker-appearance-step.html fast/forms/implicit-submission.html fast/forms/label/label-contains-other-interactive-content.html fast/events/input-image-scrolled-x-y.html fast/forms/calendar-picker/month-picker-appearance-step.html fast/forms/select/popup-menu-appearance-many.html fast/forms/calendar-picker/month-picker-touch-operations.html fast/forms/calendar-picker/date-picker-ax.html fast/forms/suggestion-picker/datetimelocal-suggestion-picker-reset-value-after-reload.html fast/forms/select/popup-with-display-none-optgroup.html fast/forms/color/color-type-change-on-close.html fast/forms/suggestion-picker/time-suggestion-picker-appearance.html fast/forms/calendar-picker/week-picker-appearance-step.html fast/forms/select/multiselect-in-listbox-mouse-release-outside.html fast/forms/select/popup-menu-mouse-operations.html fast/forms/calendar-picker/week-picker-choose-default-value-after-set-value.html http/tests/misc/client-hints-picture.html fast/forms/calendar-picker/calendar-picker-appearance-zoom200.html fast/forms/select/popup-menu-nested-style.html fast/forms/calendar-picker/datetimelocal-picker-choose-default-value-after-set-value.html fast/forms/select/popup-menu-crash-on-style-update.html fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-rtl.html fast/forms/select/popup-menu-appearance-fractional-width.html fast/forms/select/menulist-remove-option-onchange.html fast/forms/calendar-picker/week-picker-touch-operations.html fast/forms/suggestion-picker/month-suggestion-picker-step-attribute.html fast/forms/calendar-picker/month-picker-key-operations.html fast/forms/select/popup-menu-key-operations.html fast/forms/calendar-picker/week-picker-ax.html http/tests/misc/client-hints-picture-source-removal.html http/tests/webfont/same-origin-credentials.html fast/forms/calendar-picker/date-picker-events.html fast/forms/suggestion-picker/month-suggestion-picker-reset-value-after-reload.html fast/forms/suggestion-picker/time-suggestion-picker-appearance-rtl.html fast/forms/calendar-picker/month-picker-ax.html fast/forms/select/popup-menu-appearance.html fast/forms/calendar-picker/calendar-picker-type-change-onchange.html editing/selection/move-by-word-visually-mac.html virtual/pointerevent/fast/events/focus-event-source-device-from-mouse.html fast/forms/color/color-suggestion-picker-two-row-appearance.html editing/spelling/spellcheck-editable-on-focus.html fast/forms/suggestion-picker/date-suggestion-picker-step-attribute.html fast/forms/suggestion-picker/date-suggestion-picker-reset-value-after-reload.html fast/forms/calendar-picker/week-picker-key-operations.html fast/forms/suggestion-picker/date-suggestion-picker-key-operations.html fast/forms/calendar-picker/calendar-picker-pre-100-year.html fast/forms/suggestion-picker/datetimelocal-suggestion-picker-step-attribute.html fast/forms/suggestion-picker/date-suggestion-picker-mouse-operations.html fast/forms/select/popup-menu-crash-on-select.html virtual/pointerevent/fast/events/input-image-scrolled-x-y.html fast/forms/calendar-picker/calendar-picker-appearance-required.html virtual/prefer_compositing_to_lcd_text/compositing/overflow/updating-scrolling-container-and-content.html fast/forms/select/popup-menu-crash-on-cancel.html fast/forms/select/popup-menu-appearance-tall.html fast/forms/calendar-picker/calendar-picker-with-step.html fast/forms/suggestion-picker/datetimelocal-suggestion-picker-mouse-operations.html fast/forms/suggestion-picker/time-suggestion-picker-mouse-operations.html virtual/trustedeventsdefaultaction/fast/events/input-image-scrolled-x-y.html fast/forms/calendar-picker/calendar-picker-date-types.html fast/forms/radio/ValidityState-valueMissing-radio.html fast/forms/select/menulist-popup-open-hide-using-keyboard.html fast/forms/calendar-picker/date-picker-choose-default-value-after-set-value.html fast/forms/select/popup-menu-appearance-single-option.html fast/forms/select/menulist-popup-crash.html compositing/overflow/updating-scrolling-container-and-content.html fast/forms/calendar-picker/calendar-picker-key-operations.html fast/forms/calendar-picker/month-picker-with-step.html fast/forms/select/popup-menu-appearance-transform.html fast/forms/select/menulist-popup-item-style.html fast/events/focus-event-source-device-from-mouse.html fast/forms/suggestion-picker/time-suggestion-picker-key-operations.html virtual/trustedeventsdefaultaction/fast/events/focus-event-source-device-from-mouse.html fast/forms/select/popup-closes-on-blur.html fast/forms/calendar-picker/calendar-picker-datetimelocal.html fast/forms/suggestion-picker/month-suggestion-picker-mouse-operations.html fast/forms/calendar-picker/datetimelocal-picker-events.html fast/forms/calendar-picker/datetimelocal-change-type-on-input-crash.html fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html fast/forms/color/color-suggestion-picker-appearance-zoom200.html http/tests/xmlhttprequest/access-control-sandboxed-iframe-allow.html fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html virtual/threaded/inspector/tracing/compile-script.html fast/forms/color/color-suggestion-picker-crash-on-set-value.html virtual/spv2/paint/invalidation/spv2/background-image-paint-invalidation.html
The CQ bit was checked by loyso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org Link to the patchset: https://codereview.chromium.org/1459023002/#ps140001 (title: "Check if m_page is null in WebPagePopupImpl.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459023002/140001
On 2015/12/03 at 22:57:31, loyso wrote: > On 2015/12/03 07:06:50, dcheng wrote: > > lgtm > > > > but mind explaining to me why you ended up needing to check m_page in > > WebPagePopupImpl::willCloseLayerTreeView()? I'm wondering if this is something > > we can cleanup. > > The following layout tests fail on nullptr call for m_page: > > fast/forms/calendar-picker/calendar-picker-should-not-change-datetimelocal-time.html > fast/forms/select/popup-menu-touch-operations.html > fast/forms/calendar-picker/calendar-picker-datetimelocal-with-step.html > fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance.html > fast/forms/calendar-picker/month-picker-choose-default-value-after-set-value.html > fast/forms/suggestion-picker/date-suggestion-picker-appearance-zoom200.html > fast/forms/suggestion-picker/datetimelocal-suggestion-picker-key-operations.html > fast/forms/suggestion-picker/week-suggestion-picker-mouse-operations.html > fast/forms/select/optgroup-disabled.html > fast/forms/suggestion-picker/week-suggestion-picker-key-operations.html > fast/forms/select/popup-menu-position.html > fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html > accessibility/menu-list-optgroup.html > fast/forms/suggestion-picker/time-suggestion-picker-step-attribute.html > fast/forms/calendar-picker/calendar-picker-appearance-minimum-date.html > fast/forms/select/select-disabled.html > fast/forms/calendar-picker/calendar-picker-mouse-operations.html > fast/forms/suggestion-picker/month-suggestion-picker-key-operations.html > fast/forms/calendar-picker/calendar-picker-appearance-step.html > fast/forms/implicit-submission.html > fast/forms/label/label-contains-other-interactive-content.html > fast/events/input-image-scrolled-x-y.html > fast/forms/calendar-picker/month-picker-appearance-step.html > fast/forms/select/popup-menu-appearance-many.html > fast/forms/calendar-picker/month-picker-touch-operations.html > fast/forms/calendar-picker/date-picker-ax.html > fast/forms/suggestion-picker/datetimelocal-suggestion-picker-reset-value-after-reload.html > fast/forms/select/popup-with-display-none-optgroup.html > fast/forms/color/color-type-change-on-close.html > fast/forms/suggestion-picker/time-suggestion-picker-appearance.html > fast/forms/calendar-picker/week-picker-appearance-step.html > fast/forms/select/multiselect-in-listbox-mouse-release-outside.html > fast/forms/select/popup-menu-mouse-operations.html > fast/forms/calendar-picker/week-picker-choose-default-value-after-set-value.html > http/tests/misc/client-hints-picture.html > fast/forms/calendar-picker/calendar-picker-appearance-zoom200.html > fast/forms/select/popup-menu-nested-style.html > fast/forms/calendar-picker/datetimelocal-picker-choose-default-value-after-set-value.html > fast/forms/select/popup-menu-crash-on-style-update.html > fast/forms/suggestion-picker/datetimelocal-suggestion-picker-appearance-rtl.html > fast/forms/select/popup-menu-appearance-fractional-width.html > fast/forms/select/menulist-remove-option-onchange.html > fast/forms/calendar-picker/week-picker-touch-operations.html > fast/forms/suggestion-picker/month-suggestion-picker-step-attribute.html > fast/forms/calendar-picker/month-picker-key-operations.html > fast/forms/select/popup-menu-key-operations.html > fast/forms/calendar-picker/week-picker-ax.html > http/tests/misc/client-hints-picture-source-removal.html > http/tests/webfont/same-origin-credentials.html > fast/forms/calendar-picker/date-picker-events.html > fast/forms/suggestion-picker/month-suggestion-picker-reset-value-after-reload.html > fast/forms/suggestion-picker/time-suggestion-picker-appearance-rtl.html > fast/forms/calendar-picker/month-picker-ax.html > fast/forms/select/popup-menu-appearance.html > fast/forms/calendar-picker/calendar-picker-type-change-onchange.html > editing/selection/move-by-word-visually-mac.html > virtual/pointerevent/fast/events/focus-event-source-device-from-mouse.html > fast/forms/color/color-suggestion-picker-two-row-appearance.html > editing/spelling/spellcheck-editable-on-focus.html > fast/forms/suggestion-picker/date-suggestion-picker-step-attribute.html > fast/forms/suggestion-picker/date-suggestion-picker-reset-value-after-reload.html > fast/forms/calendar-picker/week-picker-key-operations.html > fast/forms/suggestion-picker/date-suggestion-picker-key-operations.html > fast/forms/calendar-picker/calendar-picker-pre-100-year.html > fast/forms/suggestion-picker/datetimelocal-suggestion-picker-step-attribute.html > fast/forms/suggestion-picker/date-suggestion-picker-mouse-operations.html > fast/forms/select/popup-menu-crash-on-select.html > virtual/pointerevent/fast/events/input-image-scrolled-x-y.html > fast/forms/calendar-picker/calendar-picker-appearance-required.html > virtual/prefer_compositing_to_lcd_text/compositing/overflow/updating-scrolling-container-and-content.html > fast/forms/select/popup-menu-crash-on-cancel.html > fast/forms/select/popup-menu-appearance-tall.html > fast/forms/calendar-picker/calendar-picker-with-step.html > fast/forms/suggestion-picker/datetimelocal-suggestion-picker-mouse-operations.html > fast/forms/suggestion-picker/time-suggestion-picker-mouse-operations.html > virtual/trustedeventsdefaultaction/fast/events/input-image-scrolled-x-y.html > fast/forms/calendar-picker/calendar-picker-date-types.html > fast/forms/radio/ValidityState-valueMissing-radio.html > fast/forms/select/menulist-popup-open-hide-using-keyboard.html > fast/forms/calendar-picker/date-picker-choose-default-value-after-set-value.html > fast/forms/select/popup-menu-appearance-single-option.html > fast/forms/select/menulist-popup-crash.html > compositing/overflow/updating-scrolling-container-and-content.html > fast/forms/calendar-picker/calendar-picker-key-operations.html > fast/forms/calendar-picker/month-picker-with-step.html > fast/forms/select/popup-menu-appearance-transform.html > fast/forms/select/menulist-popup-item-style.html > fast/events/focus-event-source-device-from-mouse.html > fast/forms/suggestion-picker/time-suggestion-picker-key-operations.html > virtual/trustedeventsdefaultaction/fast/events/focus-event-source-device-from-mouse.html > fast/forms/select/popup-closes-on-blur.html > fast/forms/calendar-picker/calendar-picker-datetimelocal.html > fast/forms/suggestion-picker/month-suggestion-picker-mouse-operations.html > fast/forms/calendar-picker/datetimelocal-picker-events.html > fast/forms/calendar-picker/datetimelocal-change-type-on-input-crash.html > fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html > fast/forms/color/color-suggestion-picker-appearance-zoom200.html > http/tests/xmlhttprequest/access-control-sandboxed-iframe-allow.html > fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html > virtual/threaded/inspector/tracing/compile-script.html > fast/forms/color/color-suggestion-picker-crash-on-set-value.html > virtual/spv2/paint/invalidation/spv2/background-image-paint-invalidation.html My question is more about 'why' though: is it because we're calling it multiple times? During shutdown? Some other time? etc.
On 2015/12/03 07:06:50, dcheng wrote: > but mind explaining to me why you ended up needing to check m_page in > WebPagePopupImpl::willCloseLayerTreeView()? I'm wondering if this is something > we can cleanup. Have set a bug: https://code.google.com/p/chromium/issues/detail?id=565629
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by loyso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1459023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1459023002/140001
Message was sent while issue was closed.
Description was changed from ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: WebFrameTest.SwapMainFrameWhileLoading WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameSwapTest.FrameElementInFramesWithRemoteParent Note that this code works only in --site-per-process mode. BUG=394777 ========== to ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: WebFrameTest.SwapMainFrameWhileLoading WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameSwapTest.FrameElementInFramesWithRemoteParent Note that this code works only in --site-per-process mode. BUG=394777 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: WebFrameTest.SwapMainFrameWhileLoading WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameSwapTest.FrameElementInFramesWithRemoteParent Note that this code works only in --site-per-process mode. BUG=394777 ========== to ========== Compositor Animation Timelines: Fix frame swapping tests in WebFrameTest.cpp We want to enable compositor animation timelines (external cc::AnimationHost) mode by default here: https://codereview.chromium.org/1308053006/ So we need to notify Page and ScrollingCoordinator on main frame swapping. Otherwise, the following webkit_unit_tests fail in new mode: WebFrameTest.SwapMainFrameWhileLoading WebFrameSwapTest.FramesOfRemoteParentAreIndexable WebFrameSwapTest.FrameElementInFramesWithRemoteParent Note that this code works only in --site-per-process mode. BUG=394777 Committed: https://crrev.com/c65cc1e198dbc38c38aa0c27773802947c1c33ff Cr-Commit-Position: refs/heads/master@{#363128} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c65cc1e198dbc38c38aa0c27773802947c1c33ff Cr-Commit-Position: refs/heads/master@{#363128} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
