Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1078473002:
Remove WebViewImpl::setIsAcceleratedCompositingActive (Closed)
|
Created:
5 years, 8 months ago by enne (OOO) Modified:
5 years, 8 months ago Reviewers:
pdr. CC:
blink-reviews, danakj, jamesr Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRemove WebViewImpl::setIsAcceleratedCompositingActive
This function no longer makes sense now that WebViewImpl is either
always or never composited. The setup that is done when a new
root layer is set is now done whenever those settings are changed,
as the WebLayerTreeView is always there to store those values.
There's nothing pressing here to change this; it's just previous
cruft that never got removed when force compositing mode got turned
on. The new code seems a lot cleaner.
BUG=474818
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194415
Patch Set 1 #
Total comments: 12
Patch Set 2 : pdr review #Patch Set 3 : Remove is accelerated compositing setting #Patch Set 4 : Fix asserts differently #
Total comments: 1
Patch Set 5 : Early out during shutdown to fix crashes #Patch Set 6 : Fix issues #
Total comments: 3
Patch Set 7 : Fix Windows crash #
Messages
Total messages: 30 (11 generated)
enne@chromium.org changed reviewers: + pdr@chromium.org
I'm sorry for the nit-picky review. New to this code. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (left): https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4274: ASSERT(!active || m_layerTreeView); We lost this assert in setRootGraphicsLayer https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4315: page()->deprecatedLocalMainFrame()->view()->setClipsRepaints(!m_isAcceleratedCompositingActive); Don't we need to do this in setRootGraphicsLayer? https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:2726: return m_client && !m_client->allowsBrokenNullLayerTreeView(); I think you need a check for m_layerTreeView here to cover the shutdown flow. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4163: // commits until WebKit generates invalidations so we don't WebKit -> Blink. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4178: if (m_pageOverlays) Can you double-check this? It was contingent on layer before. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4384: if (page()) { Nit: no need for braces. Think of the bytes.
https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (left): https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4274: ASSERT(!active || m_layerTreeView); On 2015/04/09 21:30:46, pdr wrote: > We lost this assert in setRootGraphicsLayer Done. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4315: page()->deprecatedLocalMainFrame()->view()->setClipsRepaints(!m_isAcceleratedCompositingActive); On 2015/04/09 21:30:46, pdr wrote: > Don't we need to do this in setRootGraphicsLayer? Nope. I moved it above into setMainFrame. This code is awkward, at best. Basically it's trying to say that if you have a composited frame, you shouldn't clip repaints outside of its clip, since the compositor wants to know about them. Honestly, we could probably not clip repaints ever, but I didn't want to make that change here. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:2726: return m_client && !m_client->allowsBrokenNullLayerTreeView(); On 2015/04/09 21:30:46, pdr wrote: > I think you need a check for m_layerTreeView here to cover the shutdown flow. This is the check for m_layerTreeView. The ctor calls initializeLayerTreeView which asserts: ASSERT(m_layerTreeView || !m_client || m_client>allowsBrokenNullLayerTreeView()); Therefore, either you have a layer tree view, or you don't have a client, or your client allows a broken null layer tree view. I guess this function could just be if !m_layerTreeView, if you wanted. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4163: // commits until WebKit generates invalidations so we don't On 2015/04/09 21:30:46, pdr wrote: > WebKit -> Blink. I'm just moving this code!!!!!1111 Done. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4178: if (m_pageOverlays) On 2015/04/09 21:30:45, pdr wrote: > Can you double-check this? It was contingent on layer before. Good call. It looks like it's doing invalidation work. https://codereview.chromium.org/1078473002/diff/1/Source/web/WebViewImpl.cpp#... Source/web/WebViewImpl.cpp:4384: if (page()) { On 2015/04/09 21:30:46, pdr wrote: > Nit: no need for braces. Think of the bytes. Is this really in the Blink style guide? :P
The CQ bit was checked by pdr@chromium.org
LGTM
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078473002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/51278)
https://codereview.chromium.org/1078473002/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1078473002/diff/60001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:2730: return m_rootLayer; Reuploaded a new patch that fixes the asserts. This is the substantive change.
On 2015/04/15 at 21:03:42, enne wrote: > https://codereview.chromium.org/1078473002/diff/60001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/1078473002/diff/60001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:2730: return m_rootLayer; > Reuploaded a new patch that fixes the asserts. This is the substantive change. LGTM
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078473002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1078473002/#ps80001 (title: "Early out during shutdown to fix crashes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078473002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=193897
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1094453004/ by enne@chromium.org. The reason for reverting is: Causes blank pages on load sometimes.
PTAL at this patch again. I'd like to re-land it with some fixes. There are three changes here from the original that I've left comments about. https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... Source/web/WebViewImpl.cpp:2737: setRootGraphicsLayer(nullptr); This is the crash fix from https://codereview.chromium.org/1089783003 that I rolled into this patch. https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... Source/web/WebViewImpl.cpp:4156: updateRootLayerTransform(); I rolled https://codereview.chromium.org/1097183004 into this patch. It's just a cleanup. https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... Source/web/WebViewImpl.cpp:4168: bool visible = page()->visibilityState() == PageVisibilityStateVisible; This is the fix that made me revert the original patch. The LayerTreeView could get stuck in visible=false and the WebView would never be told that it was visible, causing no frames to ever get put up on screen.
On 2015/04/23 at 22:48:17, enne wrote: > PTAL at this patch again. I'd like to re-land it with some fixes. > > There are three changes here from the original that I've left comments about. > > https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... > Source/web/WebViewImpl.cpp:2737: setRootGraphicsLayer(nullptr); > This is the crash fix from https://codereview.chromium.org/1089783003 that I rolled into this patch. > > https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... > Source/web/WebViewImpl.cpp:4156: updateRootLayerTransform(); > I rolled https://codereview.chromium.org/1097183004 into this patch. It's just a cleanup. > > https://codereview.chromium.org/1078473002/diff/100001/Source/web/WebViewImpl... > Source/web/WebViewImpl.cpp:4168: bool visible = page()->visibilityState() == PageVisibilityStateVisible; > This is the fix that made me revert the original patch. The LayerTreeView could get stuck in visible=false and the WebView would never be told that it was visible, causing no frames to ever get put up on screen. LGTM
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078473002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/60615)
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1078473002/#ps120001 (title: "Fix Windows crash")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078473002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194415 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
