Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2408803002: Make it possible to launch chrome --mash with ozone_platform={wayland|x11}, chromeos=0 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 1 week ago by tonikitoo
Modified:
9 months, 1 week ago
CC:
chromium-reviews, yusukes+watch_chromium.org, feature-media-reviews_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, mcasas+watch+vc_chromium.org, James Su, miu+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make it possible to launch chrome --mash with ozone_platform={wayland|x11}, chromeos=0 As of now, chrome --mash is possible to get launched on regular chrome desktop Linux builds (with enable_package_mash_services = true) and on chromeos builds. CL provides a first step for running with chrome --mash natively on both Wayland and X11, through Ozone. It includes 3 stub'ed methods for now, to be implemented follow up CLs. GN args: use_ozone = true ozone_platform_wayland = true ozone_platform_x11 = true ozone_auto_platforms = false enable_package_mash_services = true Additionally, CL also makes it possible to turn green the FYI linux/ozone bot, worked out by thomasanderson@ et al. TEST= <out>/chrome --mash --ozone-platform=wayland|x11 --user-data-dir=~/tmp BUG=295089 Committed: https://crrev.com/b10ea66fcc43a0eed828eb62ffb2bd141b825fce Cr-Commit-Position: refs/heads/master@{#430424}

Patch Set 1 #

Total comments: 6

Patch Set 2 #

Patch Set 3 : Make it possible to launch chrome --mash with ozone_platform={wayland|x11}, chromeos=0 #

Total comments: 10

Patch Set 4 : Make it possible to launch chrome --mash with ozone_platform={wayland|x11}, chromeos=0 #

Total comments: 6

Patch Set 5 : addressed fred's review #

Patch Set 6 : fixed cast_shell_linux test failures #

Patch Set 7 : reverted changes in base/ime/ #

Total comments: 12

Patch Set 8 : addressed tom's reviews #

Total comments: 8

Patch Set 9 : smaller CL, only with crucial bits (see comment #53) #

Total comments: 2

Patch Set 10 : smaller CL, only with crucial bits (see comment #53) - take 2 #

Patch Set 11 : smaller CL, only with crucial bits (see comment #53) - take 3 #

Total comments: 5

Patch Set 12 : addressed sky's review #

Patch Set 13 : do not bundle ash resources into chrome #

Total comments: 2

Patch Set 14 : addressed sky's review #

Total comments: 2

Patch Set 15 : reverted changes to native_browser_frame_factory_auralinux.cc #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -5 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/browser/fullscreen_ozone.cc View 1 2 3 4 5 6 7 8 10 11 1 chunk +18 lines, -0 lines 1 comment Download
A chrome/browser/media/webrtc/window_icon_util_ozone.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/browser/ui/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +9 lines, -4 lines 1 comment Download
M chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/ui/views/frame/native_browser_frame_factory_ozone.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +18 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/tabs/window_finder_ozone.cc View 1 2 3 4 5 6 7 8 9 12 13 1 chunk +17 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 100 (50 generated)
tonikitoo
10 months, 1 week ago (2016-10-10 17:16:13 UTC) #2
fwang
https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/ozone_stubs.h File chrome/browser/ui/ozone_stubs.h (right): https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/ozone_stubs.h#newcode17 chrome/browser/ui/ozone_stubs.h:17: Extra blank line at the end of the file.
10 months, 1 week ago (2016-10-11 07:30:10 UTC) #4
fwang
This basically l g t m When testing "chrome --mash", many images from ash/resources/ash_resources.grd fails ...
10 months, 1 week ago (2016-10-11 09:34:13 UTC) #5
fwang
On 2016/10/11 09:34:13, fwang wrote: > When testing "chrome --mash", many images from ash/resources/ash_resources.grd > ...
10 months, 1 week ago (2016-10-11 10:04:57 UTC) #6
tonikitoo
On 2016/10/11 10:04:57, fwang wrote: > On 2016/10/11 09:34:13, fwang wrote: > > When testing ...
10 months, 1 week ago (2016-10-11 12:56:28 UTC) #7
tonikitoo
PTAL https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2408803002/diff/1/chrome/browser/ui/BUILD.gn#newcode1543 chrome/browser/ui/BUILD.gn:1543: # These files rely on Gtk+ calls, which ...
10 months, 1 week ago (2016-10-11 12:59:03 UTC) #10
fwang
informal l g t m
10 months, 1 week ago (2016-10-11 13:03:56 UTC) #11
rjkroege
I'm amazed how little change is actually needed to build and run with this. That's ...
10 months, 1 week ago (2016-10-11 18:49:47 UTC) #12
sadrul
https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc (right): https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc#newcode17 chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:17: #if !defined(USE_OZONE) On 2016/10/11 18:49:47, rjkroege wrote: > this ...
10 months, 1 week ago (2016-10-11 19:14:14 UTC) #13
sadrul
Some of the changes in this CL fall in the 'obviously correct' category (e.g. USE_X11 ...
10 months, 1 week ago (2016-10-11 19:26:56 UTC) #14
tonikitoo
Addressed Robert's and Sadrul's reviews. https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/ozone_stubs.cc File chrome/browser/ui/ozone_stubs.cc (right): https://codereview.chromium.org/2408803002/diff/40001/chrome/browser/ui/ozone_stubs.cc#newcode17 chrome/browser/ui/ozone_stubs.cc:17: bool IsFullScreenMode() { On ...
10 months, 1 week ago (2016-10-12 13:44:23 UTC) #16
tonikitoo
On 2016/10/11 19:26:56, sadrul wrote: > (..) > > All of these changes will not ...
10 months, 1 week ago (2016-10-12 13:46:30 UTC) #17
fwang
https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscreen_ozone.cc File chrome/browser/fullscreen_ozone.cc (right): https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscreen_ozone.cc#newcode8 chrome/browser/fullscreen_ozone.cc:8: NOTIMPLEMENTED(); Maybe add a TODO that references https://bugs.chromium.org/p/chromium/issues/detail?id=640390 https://codereview.chromium.org/2408803002/diff/60001/chrome/chrome_paks.gni ...
10 months, 1 week ago (2016-10-12 14:33:15 UTC) #18
tonikitoo
https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscreen_ozone.cc File chrome/browser/fullscreen_ozone.cc (right): https://codereview.chromium.org/2408803002/diff/60001/chrome/browser/fullscreen_ozone.cc#newcode8 chrome/browser/fullscreen_ozone.cc:8: NOTIMPLEMENTED(); On 2016/10/12 14:33:15, fwang wrote: > Maybe add ...
10 months, 1 week ago (2016-10-12 14:45:07 UTC) #19
tonikitoo
This is how Chrome/mash/ozone/wayland/linux_os looks like, with the missing ash resources issues fixed: https://people.igalia.com/agomes/mus+ash/chrome_mash_ozone_wayland_non-chromeos-oct12.png
10 months, 1 week ago (2016-10-12 15:38:06 UTC) #20
rjkroege
On 2016/10/12 15:38:06, tonikitoo wrote: > This is how Chrome/mash/ozone/wayland/linux_os looks like, with the missing ...
10 months, 1 week ago (2016-10-12 18:30:29 UTC) #21
sadrul
On 2016/10/12 18:30:29, rjkroege wrote: > On 2016/10/12 15:38:06, tonikitoo wrote: > > This is ...
10 months, 1 week ago (2016-10-12 18:31:57 UTC) #22
rjkroege
On 2016/10/12 18:31:57, sadrul wrote: > On 2016/10/12 18:30:29, rjkroege wrote: > > On 2016/10/12 ...
10 months, 1 week ago (2016-10-12 19:52:08 UTC) #23
not to use - tonikitoo
On 2016/10/12 19:52:08, rjkroege wrote: > On 2016/10/12 18:31:57, sadrul wrote: > > On 2016/10/12 ...
10 months, 1 week ago (2016-10-12 20:50:23 UTC) #24
fwang
> dumb question maybe: why does it have the ash tray still? i.e. you're not ...
10 months, 1 week ago (2016-10-13 16:13:12 UTC) #25
not to use - tonikitoo
FYI: The trybots caught tests failing in chromecast (linux) builds. I could reproduce it, and ...
10 months, 1 week ago (2016-10-14 19:43:15 UTC) #31
rjkroege
On 2016/10/14 19:43:15, not to use - tonikitoo wrote: > FYI: The trybots caught tests ...
10 months ago (2016-10-16 21:07:38 UTC) #32
Tom (Use chromium acct)
https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_content_browser_client.cc#newcode896 chrome/browser/chrome_content_browser_client.cc:896: main_parts->AddParts(new ChromeBrowserMainExtraPartsViewsLinux()); I'm a bit worried about this. ChromeBrowserMainExtraPartsViewsLinux ...
10 months ago (2016-10-17 23:40:52 UTC) #37
tonikitoo
https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2408803002/diff/120001/chrome/browser/chrome_content_browser_client.cc#newcode896 chrome/browser/chrome_content_browser_client.cc:896: main_parts->AddParts(new ChromeBrowserMainExtraPartsViewsLinux()); Here are the reasons to originally ifdef'out ...
10 months ago (2016-10-18 04:55:06 UTC) #38
Tom (Use chromium acct)
Pinging other reviewers on behalf of tonikitoo@ rjkroge@ maybe if this isn't a step in ...
9 months, 3 weeks ago (2016-10-25 19:30:31 UTC) #47
rjkroege
On 2016/10/25 19:30:31, Tom Anderson wrote: > Pinging other reviewers on behalf of tonikitoo@ > ...
9 months, 3 weeks ago (2016-10-25 19:57:43 UTC) #48
rjkroege
I've been persuaded that so long as we get an FYI builder, it's reasonable to ...
9 months, 3 weeks ago (2016-10-25 20:10:40 UTC) #49
sadrul
Note that to run chrome with mus (chrome --mash), chrome itself does not need to ...
9 months, 3 weeks ago (2016-10-26 04:37:33 UTC) #50
sadrul
On 2016/10/26 04:37:33, sadrul wrote: > Note that to run chrome with mus (chrome --mash), ...
9 months, 3 weeks ago (2016-10-26 04:44:10 UTC) #51
tonikitoo
Started to address Robert's request in separate CLs. Will rebase this one against them, once ...
9 months, 3 weeks ago (2016-10-26 05:37:17 UTC) #52
tonikitoo
On 2016/10/11 19:26:56, sadrul wrote: > Some of the changes in this CL fall in ...
9 months, 2 weeks ago (2016-11-02 22:46:01 UTC) #53
Tom (Use chromium acct)
All of this makes sense to me non-owners LGTM
9 months, 2 weeks ago (2016-11-02 23:02:04 UTC) #56
fwang
https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/views/tabs/window_finder_ozone.cc File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right): https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/views/tabs/window_finder_ozone.cc#newcode6 chrome/browser/ui/views/tabs/window_finder_ozone.cc:6: Why this new line?
9 months, 2 weeks ago (2016-11-03 07:56:22 UTC) #59
tonikitoo
https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/views/tabs/window_finder_ozone.cc File chrome/browser/ui/views/tabs/window_finder_ozone.cc (right): https://codereview.chromium.org/2408803002/diff/160001/chrome/browser/ui/views/tabs/window_finder_ozone.cc#newcode6 chrome/browser/ui/views/tabs/window_finder_ozone.cc:6: On 2016/11/03 07:56:22, fwang wrote: > Why this new ...
9 months, 2 weeks ago (2016-11-03 12:52:29 UTC) #60
tonikitoo
sky@chromium.org: Please review changes. I tried to summarize the work so far in comment #53 ...
9 months, 2 weeks ago (2016-11-04 18:51:37 UTC) #71
sky
https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.gn#newcode3548 chrome/browser/BUILD.gn:3548: "fullscreen_auramus.cc", Wouldn't _ozone be a better name to use ...
9 months, 2 weeks ago (2016-11-04 21:40:37 UTC) #72
tonikitoo
https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2408803002/diff/200001/chrome/browser/BUILD.gn#newcode3548 chrome/browser/BUILD.gn:3548: "fullscreen_auramus.cc", On 2016/11/04 21:40:37, sky wrote: > Wouldn't _ozone ...
9 months, 2 weeks ago (2016-11-04 22:02:57 UTC) #73
sky
https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni File chrome/chrome_paks.gni (right): https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni#newcode57 chrome/chrome_paks.gni:57: if (use_ash || use_ozone) { On 2016/11/04 22:02:56, tonikitoo ...
9 months, 2 weeks ago (2016-11-04 23:18:40 UTC) #76
tonikitoo
On 2016/11/04 23:18:40, sky wrote: > https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni > File chrome/chrome_paks.gni (right): > > https://codereview.chromium.org/2408803002/diff/200001/chrome/chrome_paks.gni#newcode57 > ...
9 months, 2 weeks ago (2016-11-05 00:06:28 UTC) #77
tonikitoo
In patchset #13, I am moving out of this CL the part that bundles ash ...
9 months, 2 weeks ago (2016-11-05 03:55:09 UTC) #80
sky
https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc (right): https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc#newcode16 chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if defined(USE_OZONE) native_browser_frame_factory_ozone?
9 months, 1 week ago (2016-11-07 16:28:00 UTC) #85
tonikitoo
https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc (right): https://codereview.chromium.org/2408803002/diff/240001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc#newcode16 chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if defined(USE_OZONE) On 2016/11/07 16:28:00, sky wrote: > native_browser_frame_factory_ozone? ...
9 months, 1 week ago (2016-11-07 16:36:20 UTC) #86
sky
https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc (right): https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc#newcode16 chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if defined(USE_OZONE) You shouldn't need this ifdef anymore, right?
9 months, 1 week ago (2016-11-07 17:18:52 UTC) #89
tonikitoo
https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc File chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc (right): https://codereview.chromium.org/2408803002/diff/260001/chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc#newcode16 chrome/browser/ui/views/frame/native_browser_frame_factory_auralinux.cc:16: #if defined(USE_OZONE) On 2016/11/07 17:18:52, sky wrote: > You ...
9 months, 1 week ago (2016-11-07 17:22:18 UTC) #90
fwang
informal l g t m
9 months, 1 week ago (2016-11-07 18:12:16 UTC) #91
rjkroege
lgtm This seems reasonable to me. Thanks for your persistence. I'm looking forward to the ...
9 months, 1 week ago (2016-11-07 20:07:59 UTC) #92
sky
LGTM https://codereview.chromium.org/2408803002/diff/280001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2408803002/diff/280001/chrome/browser/ui/BUILD.gn#newcode3055 chrome/browser/ui/BUILD.gn:3055: "views/frame/native_browser_frame_factory_auralinux.cc", For clarity this should be renamed to ...
9 months, 1 week ago (2016-11-07 21:48:11 UTC) #93
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/2408803002/280001
9 months, 1 week ago (2016-11-07 21:52:30 UTC) #96
commit-bot: I haz the power
Committed patchset #15 (id:280001)
9 months, 1 week ago (2016-11-07 23:52:31 UTC) #98
commit-bot: I haz the power
9 months, 1 week ago (2016-11-07 23:58:05 UTC) #100
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/b10ea66fcc43a0eed828eb62ffb2bd141b825fce
Cr-Commit-Position: refs/heads/master@{#430424}
Sign in to reply to this message.

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