Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629983002:
Fix the MessageLoop type in case more than one ozone platform is built (Closed)
DescriptionFix the MessageLoop type in case more than one ozone platform is built
One of the premises of Ozone is that one can trigger
a backend at runtime. For example, if one has
ozone_platform_x11=true
ozone_platform_wayland=true
.. in the GN args, it should be able to launch either backend
by passing --ozone-platform=x11|wayland. This enforces no compile
time branches where possible.
In [1], a OZONE_X11 define was added, and slightly breaks that
premise.
This CL fixes it by:
- making OzonePlatform::CreateInstance public (and renaming it to
::EnsureInstance). This allows the creation of the OzonePlatform
instance without doing any actual initialization.
- Adding a OzonePlatform::GetMessageLoopTypeForGpu virtual method,
which returns the MessageLoop type required for the GPU depending
on the Ozone platform selected at runtime.
[1] https://codereview.chromium.org/1723303002
BUG=640613, 686092
Review-Url: https://codereview.chromium.org/2629983002
Cr-Commit-Position: refs/heads/master@{#449068}
Committed: https://chromium.googlesource.com/chromium/src/+/fb807b102e67eb2dfb2bc2318a5c913a021350b8
Patch Set 1 #Patch Set 2 : Fix the MessageLoop type in case more than one ozone platform it built #Patch Set 3 : Fix the MessageLoop type in case more than one ozone platform it built #
Total comments: 2
Patch Set 4 : added a comment explain the static method #
Total comments: 6
Patch Set 5 : addressed moar sadrul/kylechar's feedback #
Total comments: 12
Patch Set 6 : Fixed moar feedback from kylechar/fwang #
Total comments: 6
Patch Set 7 : addressed sadrul's remarks. #
Total comments: 2
Patch Set 8 : addressed kbr's feedback #
Messages
Total messages: 71 (38 generated)
Description was changed from ========== Fix the MessageLoop type in case more than one ozone platform it built One of the premisses of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. on the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforce no build time code branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premisse. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613 ========== to ========== Fix the MessageLoop type in case more than one ozone platform it built One of the premisses of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. on the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforce no build time code branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premisse. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613 ==========
tonikitoo@igalia.com changed reviewers: + kylechar@chromium.org
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
Description was changed from ========== Fix the MessageLoop type in case more than one ozone platform it built One of the premisses of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. on the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforce no build time code branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premisse. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613 ========== to ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613 ==========
On 2017/01/13 23:38:00, tonikitoo wrote:
> PTAL
Is MessageLoop::TYPE_UI causing problems for wayland? I actually had a CL that
did basically this but received push back. If TYPE_UI is causing problems with
wayland, then absolutely this is something worth doing. It's probably worth
doing anyways.
I don't think gpu_main.cc should know anything about the Ozone implementation
though. That is a layering violation. However, you could add a static function
on OzonePlatform to tell you the MessageLoop type. It just needs a comment
explaining why that method can't be part of OzonePlatform, because it has to
happen before OzonePlatform is initialized.
// Returns the message loop type required for OzonePlatform instance that will
be initialized.
static base::MessageLoop OzonePlatform::GetMessageLoopType() { ... }
Thanks for the the feedback, Kyle.
On 2017/01/14 15:56:31, kylechar wrote:
> On 2017/01/13 23:38:00, tonikitoo wrote:
> > PTAL
>
> Is MessageLoop::TYPE_UI causing problems for wayland? I actually had a CL that
> did basically this but received push back. If TYPE_UI is causing problems with
> wayland, then absolutely this is something worth doing. It's probably worth
> doing anyways.
Actually, what is happening is this:
if one builds with both ozone_platform_x11=true AND ozone_platform_wayland=true
in the GN args, then OZONE_X11 define will be defined (because of
ozone_platform_x11). In this case, MessageLoop::TYPE_UI will be always used,
regardless of the backend chosen at runtime (i.e. the value passed to
--ozone-platform at command line).
On the other hand, in case only ozone_platform_wayland=true is defined in GN
args, then OZONE_X11 is not defined and the message type of
--ozone-platform=wayland is going to be TYPE_DEFAULT.
This is inconsistent, at least, and is the original problem I wanted to fix.
> I don't think gpu_main.cc should know anything about the Ozone implementation
> though. That is a layering violation. However, you could add a static function
> on OzonePlatform to tell you the MessageLoop type. It just needs a comment
> explaining why that method can't be part of OzonePlatform, because it has to
> happen before OzonePlatform is initialized.
>
> // Returns the message loop type required for OzonePlatform instance that will
> be initialized.
> static base::MessageLoop OzonePlatform::GetMessageLoopType() { ... }
I thought of this too yesterday, and it is a better idea, indeed. will reupload.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/14 15:56:31, kylechar wrote:
> On 2017/01/13 23:38:00, tonikitoo wrote:
> > PTAL
>
> Is MessageLoop::TYPE_UI causing problems for wayland? I actually had a CL that
> did basically this but received push back. If TYPE_UI is causing problems with
> wayland, then absolutely this is something worth doing. It's probably worth
> doing anyways.
>
> I don't think gpu_main.cc should know anything about the Ozone implementation
> though. That is a layering violation. However, you could add a static function
> on OzonePlatform to tell you the MessageLoop type. It just needs a comment
> explaining why that method can't be part of OzonePlatform, because it has to
> happen before OzonePlatform is initialized.
>
> // Returns the message loop type required for OzonePlatform instance that will
> be initialized.
> static base::MessageLoop OzonePlatform::GetMessageLoopType() { ... }
Done in ps3.
tonikitoo@igalia.com changed reviewers: + rjkroege@chromium.org
fwang@igalia.com changed reviewers: + fwang@igalia.com
https://codereview.chromium.org/2629983002/diff/40001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/2629983002/diff/40001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:108: // If we are be running Ozone X11 we need a UI loop to grab Expose events. are be => are
https://codereview.chromium.org/2629983002/diff/40001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/40001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.h:96: static base::MessageLoop::Type GetMessageLoopType(); Add a comment to why this is here.
On 2017/01/17 21:54:53, kylechar wrote: > https://codereview.chromium.org/2629983002/diff/40001/ui/ozone/public/ozone_p... > File ui/ozone/public/ozone_platform.h (right): > > https://codereview.chromium.org/2629983002/diff/40001/ui/ozone/public/ozone_p... > ui/ozone/public/ozone_platform.h:96: static base::MessageLoop::Type > GetMessageLoopType(); > Add a comment to why this is here. Done
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks!
tonikitoo@igalia.com changed reviewers: + spang@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:110: return (!strcmp(GetOzonePlatformName(), "x11")) Don't use the platform name. Use a virtual function.
https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:110: return (!strcmp(GetOzonePlatformName(), "x11")) On 2017/01/19 16:31:55, spang wrote: > Don't use the platform name. Use a virtual function. Right, that's a better idea. We can't run OzonePlatform::InitializeForGPU() before the main MessageLoop is created. Something will try and use the MessageLoop then crash. However, we can create the OzonePlatform instance without doing any initialization if OzonePlatform::CreateInstance() is made public. It will get still get initialized later and GetMessageLoopType() can be virtual.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc... content/gpu/gpu_main.cc:223: new base::MessageLoop(ui::OzonePlatform::GetMessageLoopType())); Is there a reason to not always use TYPE_UI here?
On 2017/01/31 17:12:37, sadrul wrote: > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc > File content/gpu/gpu_main.cc (right): > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc... > content/gpu/gpu_main.cc:223: new > base::MessageLoop(ui::OzonePlatform::GetMessageLoopType())); > Is there a reason to not always use TYPE_UI here? It could have a performance impact on Ozone DRM where it's not needed.
On 2017/01/31 17:23:47, kylechar wrote: > On 2017/01/31 17:12:37, sadrul wrote: > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc > > File content/gpu/gpu_main.cc (right): > > > > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc... > > content/gpu/gpu_main.cc:223: new > > base::MessageLoop(ui::OzonePlatform::GetMessageLoopType())); > > Is there a reason to not always use TYPE_UI here? > > It could have a performance impact on Ozone DRM where it's not needed. Can you explain what kind of perf impact (or why)?
On 2017/01/31 17:27:36, sadrul wrote: > On 2017/01/31 17:23:47, kylechar wrote: > > On 2017/01/31 17:12:37, sadrul wrote: > > > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc > > > File content/gpu/gpu_main.cc (right): > > > > > > > > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc... > > > content/gpu/gpu_main.cc:223: new > > > base::MessageLoop(ui::OzonePlatform::GetMessageLoopType())); > > > Is there a reason to not always use TYPE_UI here? > > > > It could have a performance impact on Ozone DRM where it's not needed. > > Can you explain what kind of perf impact (or why)? Look at pimans comment on gpu_main.cc in https://codereview.chromium.org/1723303002. That was the review where I added it originally.
On 2017/01/31 17:30:06, kylechar wrote: > On 2017/01/31 17:27:36, sadrul wrote: > > On 2017/01/31 17:23:47, kylechar wrote: > > > On 2017/01/31 17:12:37, sadrul wrote: > > > > > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc > > > > File content/gpu/gpu_main.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2629983002/diff/60001/content/gpu/gpu_main.cc... > > > > content/gpu/gpu_main.cc:223: new > > > > base::MessageLoop(ui::OzonePlatform::GetMessageLoopType())); > > > > Is there a reason to not always use TYPE_UI here? > > > > > > It could have a performance impact on Ozone DRM where it's not needed. > > > > Can you explain what kind of perf impact (or why)? > > Look at pimans comment on gpu_main.cc in > https://codereview.chromium.org/1723303002. That was the review where I added it > originally. OK. tonikitoo@: Can you please update mus-gpu too? (https://cs.chromium.org/chromium/src/services/ui/gpu/gpu_main.cc?type=cs&q=Gp...)
https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.h:98: static base::MessageLoop::Type GetMessageLoopType(); Also, change this to GetMessageLoopTypeForGpu()?
On 2017/01/31 17:40:09, sadrul wrote: > https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... > File ui/ozone/public/ozone_platform.h (right): > > https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... > ui/ozone/public/ozone_platform.h:98: static base::MessageLoop::Type > GetMessageLoopType(); > Also, change this to GetMessageLoopTypeForGpu()? Thanks for the feedback, guys. Sure, I will be updating the CL (expect it for the EOD today since I am on blinkon7).
patchset 5 should fix the review comments. https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:110: return (!strcmp(GetOzonePlatformName(), "x11")) On 2017/01/19 16:59:50, kylechar wrote: > On 2017/01/19 16:31:55, spang wrote: > > Don't use the platform name. Use a virtual function. > > Right, that's a better idea. We can't run OzonePlatform::InitializeForGPU() > before the main MessageLoop is created. Something will try and use the > MessageLoop then crash. > > However, we can create the OzonePlatform instance without doing any > initialization if OzonePlatform::CreateInstance() is made public. It will get > still get initialized later and GetMessageLoopType() can be virtual. Done. https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.h:98: static base::MessageLoop::Type GetMessageLoopType(); On 2017/01/31 17:40:09, sadrul wrote: > Also, change this to GetMessageLoopTypeForGpu()? Done.
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
informal l g t m https://codereview.chromium.org/2629983002/diff/80001/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/80001/content/gpu/gpu_main.cc... content/gpu/gpu_main.cc:227: // This is needed so that we can inquery a MessageLoopType, which varies inquire? https://codereview.chromium.org/2629983002/diff/80001/services/ui/gpu/gpu_mai... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/80001/services/ui/gpu/gpu_mai... services/ui/gpu/gpu_main.cc:59: // This is needed so that we can inquery a MessageLoopType, which varies inquire? https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/platform/x11/o... File ui/ozone/platform/x11/ozone_platform_x11.cc (right): https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/platform/x11/o... ui/ozone/platform/x11/ozone_platform_x11.cc:109: // See GLSurfaceGLX and https://crbug.com/326995. "we are running" grab or Expose?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks for doing this! I've noted a few minor things. https://codereview.chromium.org/2629983002/diff/80001/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/80001/content/gpu/gpu_main.cc... content/gpu/gpu_main.cc:227: // This is needed so that we can inquery a MessageLoopType, which varies Maybe replace the last sentence with something like: The MessageLoop type required depends on the Ozone platform selected at runtime. https://codereview.chromium.org/2629983002/diff/80001/services/ui/gpu/gpu_mai... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/80001/services/ui/gpu/gpu_mai... services/ui/gpu/gpu_main.cc:64: thread_options.message_loop_type = base::MessageLoop::TYPE_UI; I think this line was supposed to be deleted? https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:35: base::MessageLoop::Type OzonePlatform::GetMessageLoopTypeForGpu() { This should be ordered the same as the ozone_platform.h, so just above OzonePlatform::AddInterfaces. https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:79: void OzonePlatform::CreateInstance() { Change order to match .h. https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.h:126: static void CreateInstance(); Please put this with the static functions, above "static void InitializeForUI()".
https://codereview.chromium.org/2629983002/diff/80001/content/gpu/gpu_main.cc File content/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/80001/content/gpu/gpu_main.cc... content/gpu/gpu_main.cc:227: // This is needed so that we can inquery a MessageLoopType, which varies On 2017/02/07 17:41:46, kylechar wrote: > Maybe replace the last sentence with something like: > > The MessageLoop type required depends on the Ozone platform selected at runtime. Done. https://codereview.chromium.org/2629983002/diff/80001/services/ui/gpu/gpu_mai... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/80001/services/ui/gpu/gpu_mai... services/ui/gpu/gpu_main.cc:64: thread_options.message_loop_type = base::MessageLoop::TYPE_UI; On 2017/02/07 17:41:47, kylechar wrote: > I think this line was supposed to be deleted? my fault. let it pass doing 'git add -p'. Fixed. https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.cc (right): https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.cc:35: base::MessageLoop::Type OzonePlatform::GetMessageLoopTypeForGpu() { On 2017/02/07 17:41:47, kylechar wrote: > This should be ordered the same as the ozone_platform.h, so just above > OzonePlatform::AddInterfaces. Done. https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/80001/ui/ozone/public/ozone_p... ui/ozone/public/ozone_platform.h:126: static void CreateInstance(); On 2017/02/07 17:41:47, kylechar wrote: > Please put this with the static functions, above "static void > InitializeForUI()". Done.
Description was changed from ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613 ========== to ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613,686092 ==========
The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run
lgtm.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tonikitoo@igalia.com changed reviewers: + kbr@chromium.org
kbr@chromium.org: Please review changes in content/gpu/.
https://codereview.chromium.org/2629983002/diff/100001/services/ui/gpu/gpu_ma... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/100001/services/ui/gpu/gpu_ma... services/ui/gpu/gpu_main.cc:61: ui::OzonePlatform::CreateInstance(); We should not have this here. https://codereview.chromium.org/2629983002/diff/100001/ui/ozone/public/ozone_... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/100001/ui/ozone/public/ozone_... ui/ozone/public/ozone_platform.h:75: static void CreateInstance(); Why this change? https://codereview.chromium.org/2629983002/diff/100001/ui/ozone/public/ozone_... ui/ozone/public/ozone_platform.h:116: virtual base::MessageLoop::Type GetMessageLoopTypeForGpu(); Mention in the comment that this is for the gpu process only.
https://codereview.chromium.org/2629983002/diff/100001/services/ui/gpu/gpu_ma... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2629983002/diff/100001/services/ui/gpu/gpu_ma... services/ui/gpu/gpu_main.cc:61: ui::OzonePlatform::CreateInstance(); On 2017/02/08 01:49:26, sadrul wrote: > We should not have this here. It was originally suggested by kyle in https://codereview.chromium.org/2629983002/diff/60001/ui/ozone/public/ozone_p..., and allows us to create a OzonePlatform instance without doing any initialization, so that we can make method calls, like ::GetMessageLoopTypeForGpu Quoting kyle, "We can't run OzonePlatform::InitializeForGPU() before the main MessageLoop is created. Something will try and use the MessageLoop then crash." However, for mus today still we can call ::GetMessageLoopTypeForGpu without calling ::CreateInstance. My understanding is that it only works because mus-ui/mus-gpu process split has not happened yet. I will leave up a comment with a FIXME, but the previous call to ::CreateInstance would be no-op here. https://codereview.chromium.org/2629983002/diff/100001/ui/ozone/public/ozone_... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/100001/ui/ozone/public/ozone_... ui/ozone/public/ozone_platform.h:75: static void CreateInstance(); On 2017/02/08 01:49:26, sadrul wrote: > Why this change? It creates the OzonePlatform instance without doing any initialization. I added a comment explaining too. Basically, this allow us to call OzonePlatform::GetInstance()->Bleh(), being "bleh" a virtual/overriden method. In practice, it allows us to call the proper virtual ::GetMessageLoopTypeForGpu, which depends on the selected ozone platform at runtime. https://codereview.chromium.org/2629983002/diff/100001/ui/ozone/public/ozone_... ui/ozone/public/ozone_platform.h:116: virtual base::MessageLoop::Type GetMessageLoopTypeForGpu(); On 2017/02/08 01:49:26, sadrul wrote: > Mention in the comment that this is for the gpu process only. Done.
LGTM with comment https://codereview.chromium.org/2629983002/diff/120001/ui/ozone/public/ozone_... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/120001/ui/ozone/public/ozone_... ui/ozone/public/ozone_platform.h:77: // platform selected at runtime, e.g. ::GetMessageLoopTypeForGpu, Document that this is idempotent, and has no effect if the platform's already been instantiated and initialized. At least, I hope those are the semantics.
Description was changed from ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL tries to fix it by checking whether the Ozone backend launched is X11 at runtime, and sets the proper MessageLoop type accordingly. [1] https://codereview.chromium.org/1723303002 BUG=640613,686092 ========== to ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL fixes it by: - making OzonePlatform::CreateInstance public (and renaming it to ::EnsureInstance). This allows the creation of the OzonePlatform instance without doing any actual initialization. - Adding a OzonePlatform::GetMessageLoopTypeForGpu virtual method, which returns the MessageLoop type required for the GPU depending on the Ozone platform selected at runtime. [1] https://codereview.chromium.org/1723303002 BUG=640613,686092 ==========
https://codereview.chromium.org/2629983002/diff/120001/ui/ozone/public/ozone_... File ui/ozone/public/ozone_platform.h (right): https://codereview.chromium.org/2629983002/diff/120001/ui/ozone/public/ozone_... ui/ozone/public/ozone_platform.h:77: // platform selected at runtime, e.g. ::GetMessageLoopTypeForGpu, On 2017/02/08 04:57:45, Ken Russell wrote: > Document that this is idempotent, and has no effect if the platform's already > been instantiated and initialized. At least, I hope those are the semantics. Done.
OK. Thanks for explaining. lgtm
lgtm
The CQ bit was checked by tonikitoo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from kylechar@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2629983002/#ps140001 (title: "addressed kbr's feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/08 18:10:56, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... lgtm in the interests of completeness
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486577416620440,
"parent_rev": "89d59827bf8706769513e1b6b0ad1a113bbfa65d", "commit_rev":
"fb807b102e67eb2dfb2bc2318a5c913a021350b8"}
Message was sent while issue was closed.
Description was changed from ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL fixes it by: - making OzonePlatform::CreateInstance public (and renaming it to ::EnsureInstance). This allows the creation of the OzonePlatform instance without doing any actual initialization. - Adding a OzonePlatform::GetMessageLoopTypeForGpu virtual method, which returns the MessageLoop type required for the GPU depending on the Ozone platform selected at runtime. [1] https://codereview.chromium.org/1723303002 BUG=640613,686092 ========== to ========== Fix the MessageLoop type in case more than one ozone platform is built One of the premises of Ozone is that one can trigger a backend at runtime. For example, if one has ozone_platform_x11=true ozone_platform_wayland=true .. in the GN args, it should be able to launch either backend by passing --ozone-platform=x11|wayland. This enforces no compile time branches where possible. In [1], a OZONE_X11 define was added, and slightly breaks that premise. This CL fixes it by: - making OzonePlatform::CreateInstance public (and renaming it to ::EnsureInstance). This allows the creation of the OzonePlatform instance without doing any actual initialization. - Adding a OzonePlatform::GetMessageLoopTypeForGpu virtual method, which returns the MessageLoop type required for the GPU depending on the Ozone platform selected at runtime. [1] https://codereview.chromium.org/1723303002 BUG=640613,686092 Review-Url: https://codereview.chromium.org/2629983002 Cr-Commit-Position: refs/heads/master@{#449068} Committed: https://chromium.googlesource.com/chromium/src/+/fb807b102e67eb2dfb2bc2318a5c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/fb807b102e67eb2dfb2bc2318a5c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
