Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2462423002: Condition the use of ChromeBrowserMainExtraPartsViewsLinux to !use_ozone (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by tonikitoo
Modified:
4 months, 3 weeks ago
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 2

Patch Set 2 : addressed thomasanderson@ remarks #

Total comments: 8

Patch Set 3 : addressed more of thomasanderson@ remakrs #

Total comments: 1

Patch Set 4 : addressed thestig's review #

Patch Set 5 : addressed thestig's review (take 2) #

Patch Set 6 : fixed chromeos_linux build #

Patch Set 7 : same as patchset 6, w/ grouped includes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -37 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 2 chunks +12 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc View 1 2 3 4 5 6 2 chunks +37 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc View 1 2 3 3 chunks +0 lines, -33 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 56 (30 generated)
tonikitoo
PTAL
4 months, 3 weeks ago (2016-11-01 14:40:56 UTC) #2
Tom Anderson
I think we still want CBMEPVL for ozone builds. There some important stuff in there, ...
4 months, 3 weeks ago (2016-11-01 16:48:14 UTC) #8
fwang
4 months, 3 weeks ago (2016-11-01 17:43:31 UTC) #9
fwang
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#newcode1543 chrome/browser/ui/BUILD.gn:1543: # the use of this files in case Gtk ...
4 months, 3 weeks ago (2016-11-01 17:47:20 UTC) #11
tonikitoo
On 2016/11/01 16:48:14, Tom Anderson wrote: > I think we still want CBMEPVL for ozone ...
4 months, 3 weeks ago (2016-11-01 17:56:54 UTC) #12
Tom Anderson
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc 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_main_extra_parts_views_linux.cc#newcode35 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 ...
4 months, 3 weeks ago (2016-11-01 18:10:07 UTC) #14
Tom Anderson
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc 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_main_extra_parts_views_linux.cc#newcode35 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 ...
4 months, 3 weeks ago (2016-11-01 18:10:07 UTC) #15
tonikitoo
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc 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_main_extra_parts_views_linux.cc#newcode35 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: > ...
4 months, 3 weeks ago (2016-11-01 18:29:54 UTC) #16
Tom Anderson
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc 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_main_extra_parts_views_linux.cc#newcode142 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 ...
4 months, 3 weeks ago (2016-11-01 18:38:27 UTC) #17
Tom Anderson
lgtm Pinging thestig@. Not sure if you saw my earlier comment
4 months, 3 weeks ago (2016-11-01 19:21:55 UTC) #18
Lei Zhang
Can I just check and make sure my assumptions are correct? In the Ozone build, ...
4 months, 3 weeks ago (2016-11-01 21:13:32 UTC) #20
Lei Zhang
https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc 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_main_extra_parts_views_linux.cc#newcode142 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 ...
4 months, 3 weeks ago (2016-11-01 21:19:24 UTC) #21
tonikitoo
On 2016/11/01 21:13:32, Lei Zhang wrote: > Can I just check and make sure my ...
4 months, 3 weeks ago (2016-11-01 21:29:04 UTC) #22
tonikitoo
On 2016/11/01 21:19:24, Lei Zhang wrote: > https://codereview.chromium.org/2462423002/diff/20001/chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc > File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc > (right): > > ...
4 months, 3 weeks ago (2016-11-01 21:29:47 UTC) #23
Lei Zhang
Looking at this some more, it looks like the only thing shared by the ozone ...
4 months, 3 weeks ago (2016-11-01 21:36:26 UTC) #24
tonikitoo
On 2016/11/01 21:36:26, Lei Zhang wrote: > Looking at this some more, it looks like ...
4 months, 3 weeks ago (2016-11-01 21:40:21 UTC) #25
Tom Anderson
On 2016/11/01 21:40:21, tonikitoo wrote: > On 2016/11/01 21:36:26, Lei Zhang wrote: > > Looking ...
4 months, 3 weeks ago (2016-11-01 22:29:25 UTC) #26
Lei Zhang
On 2016/11/01 22:29:25, Tom Anderson wrote: > Ehh, PS3 still makes more sense in my ...
4 months, 3 weeks ago (2016-11-01 22:36:11 UTC) #27
Tom Anderson
On 2016/11/01 22:36:11, Lei Zhang wrote: > On 2016/11/01 22:29:25, Tom Anderson wrote: > > ...
4 months, 3 weeks ago (2016-11-01 22:52:58 UTC) #28
tonikitoo
On 2016/11/01 22:52:58, Tom Anderson wrote: > On 2016/11/01 22:36:11, Lei Zhang wrote: > > ...
4 months, 3 weeks ago (2016-11-01 23:06:35 UTC) #29
Lei Zhang
On 2016/11/01 23:06:35, tonikitoo wrote: > The big different to @thestig's suggestion to patchset #1 ...
4 months, 3 weeks ago (2016-11-01 23:34:52 UTC) #30
tonikitoo
On 2016/11/01 23:34:52, Lei Zhang wrote: > On 2016/11/01 23:06:35, tonikitoo wrote: > > The ...
4 months, 3 weeks ago (2016-11-02 14:37:30 UTC) #32
Lei Zhang
lgtm
4 months, 3 weeks ago (2016-11-02 19:25:20 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2462423002/120001
4 months, 3 weeks ago (2016-11-02 20:09:43 UTC) #50
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 months, 3 weeks ago (2016-11-02 20:44:07 UTC) #52
commit-bot: I haz the power
4 months, 3 weeks ago (2016-11-02 20:48:28 UTC) #54
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/da97a9ff8dbe3c463ff7d64fd765483401e2feb2
Cr-Commit-Position: refs/heads/master@{#429391}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62