Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(46)

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

Created:
4 years, 1 month ago by tonikitoo
Modified:
4 years, 1 month 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

Messages

Total messages: 56 (30 generated)
tonikitoo
PTAL
4 years, 1 month ago (2016-11-01 14:40:56 UTC) #2
Tom (Use chromium acct)
I think we still want CBMEPVL for ozone builds. There some important stuff in there, ...
4 years, 1 month ago (2016-11-01 16:48:14 UTC) #8
fwang
4 years, 1 month 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 years, 1 month 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 years, 1 month ago (2016-11-01 17:56:54 UTC) #12
Tom (Use chromium acct)
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 years, 1 month ago (2016-11-01 18:10:07 UTC) #14
Tom (Use chromium acct)
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 years, 1 month 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 years, 1 month ago (2016-11-01 18:29:54 UTC) #16
Tom (Use chromium acct)
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 years, 1 month ago (2016-11-01 18:38:27 UTC) #17
Tom (Use chromium acct)
lgtm Pinging thestig@. Not sure if you saw my earlier comment
4 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month ago (2016-11-01 21:40:21 UTC) #25
Tom (Use chromium acct)
On 2016/11/01 21:40:21, tonikitoo wrote: > On 2016/11/01 21:36:26, Lei Zhang wrote: > > Looking ...
4 years, 1 month 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 years, 1 month ago (2016-11-01 22:36:11 UTC) #27
Tom (Use chromium acct)
On 2016/11/01 22:36:11, Lei Zhang wrote: > On 2016/11/01 22:29:25, Tom Anderson wrote: > > ...
4 years, 1 month 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 years, 1 month 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 years, 1 month 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 years, 1 month ago (2016-11-02 14:37:30 UTC) #32
Lei Zhang
lgtm
4 years, 1 month 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 years, 1 month ago (2016-11-02 20:09:43 UTC) #50
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 1 month ago (2016-11-02 20:44:07 UTC) #52
commit-bot: I haz the power
4 years, 1 month 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}

Powered by Google App Engine
This is Rietveld 408576698