Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1808313002:
Register surface namespace in BlimpUiContextFactory (Closed)
|
Created:
4 years, 9 months ago by enne (OOO) Modified:
4 years, 9 months ago CC:
anandc+watch-blimp_chromium.org, chromium-reviews, dtrainor+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRegister surface namespace in BlimpUiContextFactory
The BlimpUiContextFactory handles creating surface id allocators, so
also needs to register them as being valid surfaces with the surface
manager like GpuProcessTransportFactory does.
In order to do this, the surface manager and surface id allocator
logic from content/browser/compositor/surface_utils is hoisted up
into content/public/browser so that BlimpUiContextFactory can get
at the global SurfaceManager.
This is a band-aid fix, as it's possible for existing code to create
a surface using the GpuProcessTransportFactory::GetContextFactory
(which is the GpuProcessTransportFactory itself) instead of the
ui::aura::Env::GetInstance()->GetContextFactory() which is overridden
by Blimp and is the BlimpUiContextmanager. This needs to be cleaned
up separately.
BUG=595497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c9295554c6764463c506aca4b81c0ff95a851f86
Cr-Commit-Position: refs/heads/master@{#383096}
Patch Set 1 #Patch Set 2 : Host surface_utils into chrome/public #
Total comments: 1
Patch Set 3 : Rebase;2016 #Patch Set 4 : Move SurfaceManager to ContextFactory #
Total comments: 1
Patch Set 5 : Fix compilation errors; remove GetSurfaceManager from ITP #Patch Set 6 : Add missing include #Patch Set 7 : Add missing include #Patch Set 8 : Fix Blimp #Patch Set 9 : Fix views_examples too #Patch Set 10 : Use the aura context factory in DelegatedFrameHost #Patch Set 11 : Fix RWHV missing context factories #Patch Set 12 : Add aura as content browser dep #Patch Set 13 : Back to patch #1 T_T #
Messages
Total messages: 36 (18 generated)
Description was changed from ========== Register surface namespace in blimp ui context factory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. BUG=595497 ========== to ========== Register surface namespace in blimp ui context factory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Register surface namespace in blimp ui context factory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Register surface namespace in blimp ui context factory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
enne@chromium.org changed reviewers: + dtrainor@chromium.org, sievers@chromium.org
Description was changed from ========== Register surface namespace in blimp ui context factory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Register surface namespace in BlimpUiContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lgtm https://codereview.chromium.org/1808313002/diff/20001/content/public/browser/... File content/public/browser/surface_utils.h (right): https://codereview.chromium.org/1808313002/diff/20001/content/public/browser/... content/public/browser/surface_utils.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
Can the call sites to CreateSurfaceIdAllocator() maybe just register the namespace instead, so we don't have to expose this? Also, blimp doesn't really need or use Surfaces on the server does it?
On 2016/03/17 at 23:44:38, sievers wrote: > Can the call sites to CreateSurfaceIdAllocator() maybe just register the namespace instead, so we don't have to expose this? I thought about that, but I didn't know what to do about ui::Compositor. > Also, blimp doesn't really need or use Surfaces on the server does it? No, it doesn't. However, I don't know how to make blimp less real here. Blimp is clearly still creating a compositor with surfaces, etc, so I think it seems reasonable that it does the same thing with them on the server that it does on the client.
On 2016/03/18 00:33:19, enne wrote: > On 2016/03/17 at 23:44:38, sievers wrote: > > Can the call sites to CreateSurfaceIdAllocator() maybe just register the > namespace instead, so we don't have to expose this? > > I thought about that, but I didn't know what to do about ui::Compositor. > Maybe we can move the ownership of SurfaceManager from ImageTransportFactory to ContextFactory? (And ITF always has a GetContextFactory() interface.)
Description was changed from ========== Register surface namespace in BlimpUiContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Move SurfaceManager to ContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. To fix this, move SurfaceManager into ContextFactory so that all ContextFactories that are allocating surface id allocators can also register them with the global SurfaceManager. As part of fixing this up for InProcessContextFactory, I just made all compositor layer tests also use surfaces (since previously the SurfaceManager was elsewhere and optional). BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
enne@chromium.org changed reviewers: + danakj@chromium.org, oshima@chromium.org
On 2016/03/18 at 01:40:12, sievers wrote: > On 2016/03/18 00:33:19, enne wrote: > > On 2016/03/17 at 23:44:38, sievers wrote: > > > Can the call sites to CreateSurfaceIdAllocator() maybe just register the > > namespace instead, so we don't have to expose this? > > > > I thought about that, but I didn't know what to do about ui::Compositor. > > > > Maybe we can move the ownership of SurfaceManager from ImageTransportFactory to ContextFactory? > (And ITF always has a GetContextFactory() interface.) Done. Thanks for the suggestion. https://codereview.chromium.org/1808313002/diff/60001/ui/compositor/test/in_p... File ui/compositor/test/in_process_context_factory.cc (right): https://codereview.chromium.org/1808313002/diff/60001/ui/compositor/test/in_p... ui/compositor/test/in_process_context_factory.cc:172: ResizeDisplay(compositor.get(), compositor->size()); This was needed because when switching to surfaces, one test was failed because the display was sized to 0x0 and so clipped a bunch of the content via partial swap.
lgtm
oshima@chromium.org changed reviewers: + sky@chromium.org
-> sky for mus change
LGTM
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/1808313002/#ps60001 (title: "Move SurfaceManager to ContextFactory")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808313002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808313002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, sievers@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1808313002/#ps80001 (title: "Fix compilation errors; remove GetSurfaceManager from ITP")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808313002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808313002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Move SurfaceManager to ContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. To fix this, move SurfaceManager into ContextFactory so that all ContextFactories that are allocating surface id allocators can also register them with the global SurfaceManager. As part of fixing this up for InProcessContextFactory, I just made all compositor layer tests also use surfaces (since previously the SurfaceManager was elsewhere and optional). BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. This is a band-aid fix, as it's possible for existing code to create a surface using the GpuProcessTransportFactory::GetContextFactory (which is the GpuProcessTransportFactory itself) instead of the ui::aura::Env::GetInstance()->GetContextFactory() which is overridden by Blimp and is the BlimpUiContextmanager. This needs to be cleaned up separately. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Ok, looks like patch #3 to move SurfaceManager into ContextFactory can't work because I can't link aura into content_browser, and so there's no way to get to ui/aura/env.h from content/browser/surface_utils.h. So, back to patch #1. <_< As I put in the third paragraph of the description, I think there are some larger bugs at play here that started with the addition of ui::aura::Env::set_context_factory. The fact that ImageTransportFactory::GetContextFactory doesn't return the same thing means that there can always be two context factories. This fix is just a workaround for existing bad behavior that now gets DCHECK'd in the SurfaceManager. I think there needs to be some larger fix, but it'd be good to not DCHECK in the short term.
I'm going to look into a better fix, but after talking to sievers I'm going to land patchset #1 as a quick fix.
Description was changed from ========== The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. This is a band-aid fix, as it's possible for existing code to create a surface using the GpuProcessTransportFactory::GetContextFactory (which is the GpuProcessTransportFactory itself) instead of the ui::aura::Env::GetInstance()->GetContextFactory() which is overridden by Blimp and is the BlimpUiContextmanager. This needs to be cleaned up separately. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Register surface namespace in BlimpUiContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. This is a band-aid fix, as it's possible for existing code to create a surface using the GpuProcessTransportFactory::GetContextFactory (which is the GpuProcessTransportFactory itself) instead of the ui::aura::Env::GetInstance()->GetContextFactory() which is overridden by Blimp and is the BlimpUiContextmanager. This needs to be cleaned up separately. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, sievers@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1808313002/#ps240001 (title: "Back to patch #1 T_T")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808313002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808313002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Register surface namespace in BlimpUiContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. This is a band-aid fix, as it's possible for existing code to create a surface using the GpuProcessTransportFactory::GetContextFactory (which is the GpuProcessTransportFactory itself) instead of the ui::aura::Env::GetInstance()->GetContextFactory() which is overridden by Blimp and is the BlimpUiContextmanager. This needs to be cleaned up separately. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Register surface namespace in BlimpUiContextFactory The BlimpUiContextFactory handles creating surface id allocators, so also needs to register them as being valid surfaces with the surface manager like GpuProcessTransportFactory does. In order to do this, the surface manager and surface id allocator logic from content/browser/compositor/surface_utils is hoisted up into content/public/browser so that BlimpUiContextFactory can get at the global SurfaceManager. This is a band-aid fix, as it's possible for existing code to create a surface using the GpuProcessTransportFactory::GetContextFactory (which is the GpuProcessTransportFactory itself) instead of the ui::aura::Env::GetInstance()->GetContextFactory() which is overridden by Blimp and is the BlimpUiContextmanager. This needs to be cleaned up separately. BUG=595497 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c9295554c6764463c506aca4b81c0ff95a851f86 Cr-Commit-Position: refs/heads/master@{#383096} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/c9295554c6764463c506aca4b81c0ff95a851f86 Cr-Commit-Position: refs/heads/master@{#383096} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
