Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089
Committed: https://crrev.com/da97a9ff8dbe3c463ff7d64fd765483401e2feb2
Cr-Commit-Position: refs/heads/master@{#429391}

[tonikitoo, 2016-11-01 14:40:56]

tonikitoo@igalia.com changed reviewers:
+ thestig@chromium.org, thomasanderson@google.com

[tonikitoo, 2016-11-01 14:40:56]

PTAL

[tonikitoo, 2016-11-01 14:42:13]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

BUG=295089
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

BUG=295089
==========

[not to use - tonikitoo, 2016-11-01 14:56:08]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 14:56:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 15:59:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 15:59:05]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-11-01 16:48:14]

I think we still want CBMEPVL for ozone builds.  There some important stuff in
there, like a check that the user is not running as root.

Maybe a better approach would be to surround the X11DesktopHandler stuff with
#if defined(USE_X11).
And for now, surround the gtk stuff with #if !defined(USE_OZONE).

[fwang, 2016-11-01 17:47:19]

fwang@igalia.com changed reviewers:
+ fwang@igalia.com

[fwang, 2016-11-01 17:47:20]

https://codereview.chromium.org/2462423002/diff/1/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2462423002/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1543: # the use of this files in case Gtk is
available with its Wayland
*these* files

https://codereview.chromium.org/2462423002/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1544: # backend. An extra GN vaiable to control it
would be safer.
*variable*

[tonikitoo, 2016-11-01 17:56:54]

On 2016/11/01 16:48:14, Tom Anderson wrote:
> I think we still want CBMEPVL for ozone builds.  There some important stuff in
> there, like a check that the user is not running as root.
> 
> Maybe a better approach would be to surround the X11DesktopHandler stuff with
> #if defined(USE_X11).
> And for now, surround the gtk stuff with #if !defined(USE_OZONE).

Done.

[tonikitoo, 2016-11-01 18:01:25]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

BUG=295089
==========

to

==========
Guard the Gtk+ entry points within ChromeBrowserMainExtraPartsViewsLinux with
!use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/use of
views::LinuxUI with ::BuildGtk2UI in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  #if !defined(USE_OZONE) || defined(USE_GTK_WAYLAND_BACKEND)
    views::LinuxUI* gtk2_ui = BuildGtk2UI();
    ..  
  #endif

BUG=295089
==========

[Tom (Use chromium acct), 2016-11-01 18:10:07]

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:35: #if
defined(USE_X11)
nit: Please move the gtk_ui.h include and the x11_desktop_handler.h include
below all of the normal includes

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:79: //
X11DesktopHandler is destructed at this point, so we don't need to remove
nit: Surround this comment and DCHECK with #if defined(USE_X11)

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:101: //
Intentionally do not call the parent class' PreCreateThreads
Add the check for !defined(USE_OZONE) in
ChromeBrowserMainExtraPartsViews::PreCreateThreads instead, and unconditionally
call ChromeBrowserMainExtraPartsViews::PreCreateThreads from here.

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:142: void
ChromeBrowserMainExtraPartsViewsLinux::OnWorkspaceChanged(
I'm surprised this doesn't need to be surrounded with #if defined(USE_X11) as
well, since OnWorkspaceChanged is an override from X11DesktopHandlerObserver.

I guess the "right" thing to do would be to move x11_desktop_handler_observer.*
into the use_x11 portion of BUILD.gn and add the USE_X11 checks here

[Tom (Use chromium acct), 2016-11-01 18:10:07]

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:35: #if
defined(USE_X11)
nit: Please move the gtk_ui.h include and the x11_desktop_handler.h include
below all of the normal includes

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:79: //
X11DesktopHandler is destructed at this point, so we don't need to remove
nit: Surround this comment and DCHECK with #if defined(USE_X11)

[tonikitoo, 2016-11-01 18:29:54]

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:35: #if
defined(USE_X11)
On 2016/11/01 18:10:07, Tom Anderson wrote:
> nit: Please move the gtk_ui.h include and the x11_desktop_handler.h include
> below all of the normal includes

Done.

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:79: //
X11DesktopHandler is destructed at this point, so we don't need to remove
On 2016/11/01 18:10:07, Tom Anderson wrote:
> nit: Surround this comment and DCHECK with #if defined(USE_X11)

Done.

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:101: //
Intentionally do not call the parent class' PreCreateThreads
On 2016/11/01 18:10:06, Tom Anderson wrote:
> Add the check for !defined(USE_OZONE) in
> ChromeBrowserMainExtraPartsViews::PreCreateThreads instead, and
unconditionally
> call ChromeBrowserMainExtraPartsViews::PreCreateThreads from here.

Done.

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:142: void
ChromeBrowserMainExtraPartsViewsLinux::OnWorkspaceChanged(
On 2016/11/01 18:10:06, Tom Anderson wrote:
> I'm surprised this doesn't need to be surrounded with #if defined(USE_X11) as
> well, since OnWorkspaceChanged is an override from X11DesktopHandlerObserver.
> 
> I guess the "right" thing to do would be to move
x11_desktop_handler_observer.*
> into the use_x11 portion of BUILD.gn and add the USE_X11 checks here

I believe the reason it "works" is because X11DesktopHandlerObserver itself has
nothing X11-specific, although its named is prefixed with X11.

Its method ::OnWorkspaceChanged is only called from X11DesktopHandler thought,
so it ends up being an dead code in case of !USE_X11 (or USE_OZONE).

If you think cleaning it up this dependency is worth it, I could give it a shot.

[Tom (Use chromium acct), 2016-11-01 18:38:27]

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:142: void
ChromeBrowserMainExtraPartsViewsLinux::OnWorkspaceChanged(
On 2016/11/01 18:29:54, tonikitoo wrote:
> On 2016/11/01 18:10:06, Tom Anderson wrote:
> > I'm surprised this doesn't need to be surrounded with #if defined(USE_X11)
as
> > well, since OnWorkspaceChanged is an override from
X11DesktopHandlerObserver.
> > 
> > I guess the "right" thing to do would be to move
> x11_desktop_handler_observer.*
> > into the use_x11 portion of BUILD.gn and add the USE_X11 checks here
> 
> I believe the reason it "works" is because X11DesktopHandlerObserver itself
has
> nothing X11-specific, although its named is prefixed with X11.
> 
> Its method ::OnWorkspaceChanged is only called from X11DesktopHandler thought,
> so it ends up being an dead code in case of !USE_X11 (or USE_OZONE).
> 
> If you think cleaning it up this dependency is worth it, I could give it a
shot.

I'll let thestig@ decide this one

[Tom (Use chromium acct), 2016-11-01 19:21:55]

lgtm

Pinging thestig@.  Not sure if you saw my earlier comment

[Lei Zhang, 2016-11-01 21:02:47]

Description was changed from

==========
Guard the Gtk+ entry points within ChromeBrowserMainExtraPartsViewsLinux with
!use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/use of
views::LinuxUI with ::BuildGtk2UI in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  #if !defined(USE_OZONE) || defined(USE_GTK_WAYLAND_BACKEND)
    views::LinuxUI* gtk2_ui = BuildGtk2UI();
    ..  
  #endif

BUG=295089
==========

to

==========
Guard the Gtk+ entry points within ChromeBrowserMainExtraPartsViewsLinux with
!use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/use of
views::LinuxUI with ::BuildGtk2UI in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  #if !defined(USE_OZONE) || defined(USE_GTK_WAYLAND_BACKEND)
    views::LinuxUI* gtk2_ui = BuildGtk2UI();
    ..  
  #endif

BUG=295089
==========

[Lei Zhang, 2016-11-01 21:13:32]

Can I just check and make sure my assumptions are correct? In the Ozone build,
we have:

use_aura = true
use_ozone = true
use_x11 = false

[Lei Zhang, 2016-11-01 21:19:24]

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:142: void
ChromeBrowserMainExtraPartsViewsLinux::OnWorkspaceChanged(
On 2016/11/01 18:29:54, tonikitoo wrote:
> If you think cleaning it up this dependency is worth it, I could give it a
shot.

Will OnWorkspaceChanged() ever be useful in an Ozone build? If you are not sure,
I think it's ok to leave it for now and have a few unused bits in the Ozone
build. You can clean it up later.

[tonikitoo, 2016-11-01 21:29:04]

On 2016/11/01 21:13:32, Lei Zhang wrote:
> Can I just check and make sure my assumptions are correct? In the Ozone build,
> we have:
> 
> use_aura = true
> use_ozone = true
> use_x11 = false

This is correct.

[tonikitoo, 2016-11-01 21:29:47]

On 2016/11/01 21:19:24, Lei Zhang wrote:
>
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
> (right):
> 
>
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:142:
void
> ChromeBrowserMainExtraPartsViewsLinux::OnWorkspaceChanged(
> On 2016/11/01 18:29:54, tonikitoo wrote:
> > If you think cleaning it up this dependency is worth it, I could give it a
> shot.
> 
> Will OnWorkspaceChanged() ever be useful in an Ozone build? If you are not
sure,
> I think it's ok to leave it for now and have a few unused bits in the Ozone
> build. You can clean it up later.

OnWorkspaceChanged is not going to be used as-is by an Ozone build.

[Lei Zhang, 2016-11-01 21:36:26]

Looking at this some more, it looks like the only thing shared by the ozone
build and the standard Linux build is
ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit(). Everything else looks
commented out. Given that, an alternative is to move
ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit() into
ChromeBrowserMainExtraPartsViews::PreProfileInit(), ifdef'd. Then for Ozone,
instantiate ChromeBrowserMainExtraPartsViews instead of
ChromeBrowserMainExtraPartsViewsLinux.

https://codereview.chromium.org/2462423002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/2462423002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:44:
ui::NativeTheme* GetNativeThemeForWindow(aura::Window* window) {
Is the Ozone build going to complain that this is dead code?

[tonikitoo, 2016-11-01 21:40:21]

On 2016/11/01 21:36:26, Lei Zhang wrote:
> Looking at this some more, it looks like the only thing shared by the ozone
> build and the standard Linux build is
> ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit(). Everything else looks
> commented out. Given that, an alternative is to move
> ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit() into
> ChromeBrowserMainExtraPartsViews::PreProfileInit(), ifdef'd. Then for Ozone,
> instantiate ChromeBrowserMainExtraPartsViews instead of
> ChromeBrowserMainExtraPartsViewsLinux.

Yes, this is closer to what patchset #1 did. thomasanderson@: Are you ok with
it?

>
https://codereview.chromium.org/2462423002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
> (right):
> 
>
https://codereview.chromium.org/2462423002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:44:
> ui::NativeTheme* GetNativeThemeForWindow(aura::Window* window) {
> Is the Ozone build going to complain that this is dead code?

It does not complain locally (release / debug). I will watch it.

[Tom (Use chromium acct), 2016-11-01 22:29:25]

On 2016/11/01 21:40:21, tonikitoo wrote:
> On 2016/11/01 21:36:26, Lei Zhang wrote:
> > Looking at this some more, it looks like the only thing shared by the ozone
> > build and the standard Linux build is
> > ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit(). Everything else
looks
> > commented out. Given that, an alternative is to move
> > ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit() into
> > ChromeBrowserMainExtraPartsViews::PreProfileInit(), ifdef'd. Then for Ozone,
> > instantiate ChromeBrowserMainExtraPartsViews instead of
> > ChromeBrowserMainExtraPartsViewsLinux.
> 
> Yes, this is closer to what patchset #1 did. thomasanderson@: Are you ok with
> it?
> 

Ehh, PS3 still makes more sense in my mind than PS1, but thestig@'s review takes
precedence here

> >
>
https://codereview.chromium.org/2462423002/diff/40001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2462423002/diff/40001/chrome/browser/ui/views...
> > chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:44:
> > ui::NativeTheme* GetNativeThemeForWindow(aura::Window* window) {
> > Is the Ozone build going to complain that this is dead code?
> 
> It does not complain locally (release / debug). I will watch it.

[Lei Zhang, 2016-11-01 22:36:11]

On 2016/11/01 22:29:25, Tom Anderson wrote:
> Ehh, PS3 still makes more sense in my mind than PS1, but thestig@'s review
takes
> precedence here

My thoughts are:
- Too many things in ChromeBrowserMainExtraPartsViewsLinux are ifdef'd out in
PS3.
- ChromeBrowserMainExtraPartsViews already has some ifdef conditions internally,
and would only need 1 more.

[Tom (Use chromium acct), 2016-11-01 22:52:58]

On 2016/11/01 22:36:11, Lei Zhang wrote:
> On 2016/11/01 22:29:25, Tom Anderson wrote:
> > Ehh, PS3 still makes more sense in my mind than PS1, but thestig@'s review
> takes
> > precedence here
> 
> My thoughts are:
> - Too many things in ChromeBrowserMainExtraPartsViewsLinux are ifdef'd out in
> PS3.
> - ChromeBrowserMainExtraPartsViews already has some ifdef conditions
internally,
> and would only need 1 more.

We're going to want to use gtk on ozone eventually for print dialogs and such,
and when that happens, we'll need to move the gtk code in CBMEPVL into CBMEPV. 
Also, I don't think the X11DesktopHandler code belongs in CBMEPVL, as we have a
ChromeBrowserMainExtraPartsX11.  It's on my TODO list to refactor this at some
point, but I think your/tonikitoo@'s solution  is fine for now.

[tonikitoo, 2016-11-01 23:06:35]

On 2016/11/01 22:52:58, Tom Anderson wrote:
> On 2016/11/01 22:36:11, Lei Zhang wrote:
> > On 2016/11/01 22:29:25, Tom Anderson wrote:
> > > Ehh, PS3 still makes more sense in my mind than PS1, but thestig@'s review
> > takes
> > > precedence here
> > 
> > My thoughts are:
> > - Too many things in ChromeBrowserMainExtraPartsViewsLinux are ifdef'd out
in
> > PS3.
> > - ChromeBrowserMainExtraPartsViews already has some ifdef conditions
> internally,
> > and would only need 1 more.
> 
> We're going to want to use gtk on ozone eventually for print dialogs and such,
> and when that happens, we'll need to move the gtk code in CBMEPVL into CBMEPV.

> Also, I don't think the X11DesktopHandler code belongs in CBMEPVL, as we have
a
> ChromeBrowserMainExtraPartsX11.  It's on my TODO list to refactor this at some
> point, but I think your/tonikitoo@'s solution  is fine for now.

It think even if we instantiate CBMEPV (rather than CBMEPVL) for OZONE, and
properly add a comment, it should not be a problem to change Ozone to use
CBMEPVL in the future (e.g. once gtk-wayland) is available.

My question now is:

the instantiation code would change like this:

 #if defined(TOOLKIT_VIEWS)
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS) && !defined(USE_OZONE)
   main_parts->AddParts(new ChromeBrowserMainExtraPartsViewsLinux());
 #else
   main_parts->AddParts(new ChromeBrowserMainExtraPartsViews());
 #endif
 #endif

...which should be fine per si (this is mostly patchset #1).

The big different to @thestig's suggestion to patchset #1 is that if we would
move CBMEPVL::PreProfileInit to CBMEPV, we would change current CHROMEOS
behavior (to not allow launch as root).

what do you guys think?

[Lei Zhang, 2016-11-01 23:34:52]

On 2016/11/01 23:06:35, tonikitoo wrote:
> The big different to @thestig's suggestion to patchset #1 is that if we would
> move CBMEPVL::PreProfileInit to CBMEPV, we would change current CHROMEOS
> behavior (to not allow launch as root).

Can't you keep the existing behavior by moving
ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit() into
ChromeBrowserMainExtraPartsViews::PreProfileInit(), but ifdefing it to
defined(OS_LINUX) && !defined(OS_CHROMEOS) ?

[tonikitoo, 2016-11-02 14:34:40]

Description was changed from

==========
Guard the Gtk+ entry points within ChromeBrowserMainExtraPartsViewsLinux with
!use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/use of
views::LinuxUI with ::BuildGtk2UI in ozone builds.

It is still possible to optionally opt-in it back in the future,
but it would be safer to condition it to an extra GN variable, where
one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  #if !defined(USE_OZONE) || defined(USE_GTK_WAYLAND_BACKEND)
    views::LinuxUI* gtk2_ui = BuildGtk2UI();
    ..  
  #endif

BUG=295089
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, so

BUG=295089
==========

[tonikitoo, 2016-11-02 14:37:30]

On 2016/11/01 23:34:52, Lei Zhang wrote:
> On 2016/11/01 23:06:35, tonikitoo wrote:
> > The big different to @thestig's suggestion to patchset #1 is that if we
would
> > move CBMEPVL::PreProfileInit to CBMEPV, we would change current CHROMEOS
> > behavior (to not allow launch as root).
> 
> Can't you keep the existing behavior by moving
> ChromeBrowserMainExtraPartsViewsLinux::PreProfileInit() into
> ChromeBrowserMainExtraPartsViews::PreProfileInit(), but ifdefing it to
> defined(OS_LINUX) && !defined(OS_CHROMEOS) ?

Done. PTAL

[not to use - tonikitoo, 2016-11-02 14:38:25]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 14:38:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2016-11-02 14:50:16]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly speficy that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, so

BUG=295089
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, so

BUG=295089
==========

[tonikitoo, 2016-11-02 14:51:47]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, so

BUG=295089
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089
==========

[commit-bot: I haz the power, 2016-11-02 15:34:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 15:34:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[not to use - tonikitoo, 2016-11-02 15:52:29]

The CQ bit was checked by tonikitoo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 15:52:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 17:20:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 17:20:09]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-11-02 19:23:59]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 19:24:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-11-02 19:25:20]

lgtm

[commit-bot: I haz the power, 2016-11-02 19:48:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 19:48:13]

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...)

[tonikitoo, 2016-11-02 20:09:10]

The CQ bit was checked by tonikitoo@igalia.com

[tonikitoo, 2016-11-02 20:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
thomasanderson@google.com
Link to the patchset: https://codereview.chromium.org/2462423002/#ps120001
(title: "same as patchset 6, w/ grouped includes")

[commit-bot: I haz the power, 2016-11-02 20:09:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 20:44:05]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089
==========

[commit-bot: I haz the power, 2016-11-02 20:44:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-02 20:48:27]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]   
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]   
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089

Committed: https://crrev.com/da97a9ff8dbe3c463ff7d64fd765483401e2feb2
Cr-Commit-Position: refs/heads/master@{#429391}
==========

[commit-bot: I haz the power, 2016-11-02 20:48:28]

Patchset 7 (id:??) landed as
https://crrev.com/da97a9ff8dbe3c463ff7d64fd765483401e2feb2
Cr-Commit-Position: refs/heads/master@{#429391}

[tonikitoo, 2016-11-02 22:13:20]

Description was changed from

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089

Committed: https://crrev.com/da97a9ff8dbe3c463ff7d64fd765483401e2feb2
Cr-Commit-Position: refs/heads/master@{#429391}
==========

to

==========
Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone

In order to bring up a Chrome/Chromium Ozone/Wayland build,
it is needed to opt out the instantiation/inclusion of
chrome_browser_main_extra_parts_views_linux.cc/h in Ozone builds.

It should still possible to opt-in the use of CBMEPVL for Ozone builds
in the future, but it would be safer to condition it to an extra GN variable,
where one could explicitly specify that the Gtk wayland backend is to be used.
E.g.:

  if (!use_ozone || use_gtk_wayland_backend) {
    sources += [
      "views/chrome_browser_main_extra_parts_views_linux.cc",
      "views/chrome_browser_main_extra_parts_views_linux.h"
    ]
  } else # regular ozone case {
    sources += [
      "views/chrome_browser_main_extra_parts_views.cc",
      "views/chrome_browser_main_extra_parts_views.h"
    ]
  }

CL also moves the PreProfileInit override from CBMEPVL to CBMEPV,
if-def'ing it out for CHROMEOS, in order to keep the behavior to
desktop linux builds (x11/ozone).

BUG=295089

Committed: https://crrev.com/da97a9ff8dbe3c463ff7d64fd765483401e2feb2
Cr-Commit-Position: refs/heads/master@{#429391}
==========

[tonikitoo, 2016-11-02 22:13:20]

tonikitoo@igalia.com changed reviewers:
+ rjkroege@chromium.org
- tonikitoo@chromium.org