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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by tonikitoo
Modified:
5 months, 4 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
6 months 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, ...
6 months ago (2016-11-01 16:48:14 UTC) #8
fwang
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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: > ...
6 months 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 ...
6 months ago (2016-11-01 18:38:27 UTC) #17
Tom Anderson
lgtm Pinging thestig@. Not sure if you saw my earlier comment
6 months 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, ...
6 months 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 ...
6 months 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 ...
6 months 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): > > ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
6 months 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 ...
5 months, 4 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: > > ...
5 months, 4 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: > > ...
5 months, 4 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 ...
5 months, 4 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 ...
5 months, 4 weeks ago (2016-11-02 14:37:30 UTC) #32
Lei Zhang
lgtm
5 months, 4 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
5 months, 4 weeks ago (2016-11-02 20:09:43 UTC) #50
commit-bot: I haz the power
Committed patchset #7 (id:120001)
5 months, 4 weeks ago (2016-11-02 20:44:07 UTC) #52
commit-bot: I haz the power
5 months, 4 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 cc6ac46