Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478323003:
Ozone Linux: Fix linker errors in interactive_ui_tests (Closed)
|
Created:
4 years, 1 month ago by Tom (Use chromium acct) Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOzone Linux: Fix linker errors in interactive_ui_tests
BUG=295089
R=sky@chromium.org
Committed: https://crrev.com/0f6913b12bd932712e21b88a9668726c26374152
Cr-Commit-Position: refs/heads/master@{#430825}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add CreateBrowserDesktopWindowTreeHost stub for ozone #
Total comments: 11
Patch Set 3 : NOTIMPLEMENTED->NOTREACHED, clean up interactive_ui_tests_main.cc #Patch Set 4 : Rebase #
Total comments: 8
Patch Set 5 : Fix up macros #
Messages
Total messages: 43 (20 generated)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Description was changed from ========== Ozone Linux: Fix linker errors in interactive_ui_tests BUG=295089 R=sky@chromium.org ========== to ========== Ozone Linux: Fix linker errors in interactive_ui_tests BUG=295089 R=sky@chromium.org ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sky PTAL Once this change, along with https://codereview.chromium.org/2408803002/ https://codereview.chromium.org/2476663002/ lands, we should have a green Linux Ozone bot
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/04 18:20:33, Tom Anderson wrote: > sky PTAL > > Once this change, along with > https://codereview.chromium.org/2408803002/ > https://codereview.chromium.org/2476663002/ > lands, we should have a green Linux Ozone bot non-owner lgtm
https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:312: #if defined(USE_OZONE) Without TestDesktopBrowserFrameAura TabDragCaptureLostTest isn't useful. So, I would rather see you DISABLE said test with a comment and a bug. https://codereview.chromium.org/2478323003/diff/1/chrome/test/base/interactiv... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/1/chrome/test/base/interactiv... chrome/test/base/interactive_ui_tests_main.cc:53: NOTIMPLEMENTED(); Won't this basically make a bunch of tests not work/compile? Shouldn't those tests be disabled or perhaps ifdefd?
https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:312: #if defined(USE_OZONE) On 2016/11/04 21:33:02, sky wrote: > Without TestDesktopBrowserFrameAura TabDragCaptureLostTest isn't useful. So, I > would rather see you DISABLE said test with a comment and a bug. That would not fix the linker error. The error is because there's no DesktopBrowserFrameAura* on Linux Ozone builds atm. Maybe I can stub one out as an alternative? https://codereview.chromium.org/2478323003/diff/1/chrome/test/base/interactiv... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/1/chrome/test/base/interactiv... chrome/test/base/interactive_ui_tests_main.cc:53: NOTIMPLEMENTED(); On 2016/11/04 21:33:02, sky wrote: > Won't this basically make a bunch of tests not work/compile? Shouldn't those > tests be disabled or perhaps ifdefd? Sorry I should have provided more context on this. Currently the Linux Ozone bot is compile-only. The project is still in its very early stages so basically none of the tests will work anyway. This is to get around a linker error where CreateBrowserDesktopWindowTreeHost is missing because it's not implemented yet on ozone. The alternative would be to stub out a CreateBrowserDesktopWindowTreeHostOzone with a bunch of NOTIMPLEMENTED()'s.
https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:312: #if defined(USE_OZONE) On 2016/11/04 21:54:21, Tom Anderson wrote: > On 2016/11/04 21:33:02, sky wrote: > > Without TestDesktopBrowserFrameAura TabDragCaptureLostTest isn't useful. So, I > > would rather see you DISABLE said test with a comment and a bug. > > That would not fix the linker error. The error is because there's no > DesktopBrowserFrameAura* on Linux Ozone builds atm. Maybe I can stub one out as > an alternative? If that's the case, don't you need an ifdef around TestDesktopBrowserFrameAura? I'm ok with that, just trying to understand.
https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:312: #if defined(USE_OZONE) On 2016/11/04 23:15:39, sky wrote: > On 2016/11/04 21:54:21, Tom Anderson wrote: > > On 2016/11/04 21:33:02, sky wrote: > > > Without TestDesktopBrowserFrameAura TabDragCaptureLostTest isn't useful. So, > I > > > would rather see you DISABLE said test with a comment and a bug. > > > > That would not fix the linker error. The error is because there's no > > DesktopBrowserFrameAura* on Linux Ozone builds atm. Maybe I can stub one out > as > > an alternative? > > If that's the case, don't you need an ifdef around TestDesktopBrowserFrameAura? > I'm ok with that, just trying to understand. Oops, I meant there's no CreateBrowserDesktopWindowTreeHost function, and maybe I can stub out a BrowserDesktopWindowTreeHostOzone The linker error I get is: ../../chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc:54: error: undefined reference to 'BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost(views::internal::NativeWidgetDelegate*, views::DesktopNativeWidgetAura*, BrowserView*, BrowserFrame*)' TestDesktopBrowserFrameAura is a subclass of DesktopBrowserFrameAura, which has DesktopBrowserFrameAura::InitNativeWidget which calls CreateBrowserDesktopWindowTreeHost. So even though we compile desktop_browser_frame_aura.cc, we never use it in the ozone build, so by some crazy linker magic it all works ¯\_(ツ)_/¯
On 2016/11/04 23:34:06, Tom Anderson wrote: > https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... > File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc > (right): > > https://codereview.chromium.org/2478323003/diff/1/chrome/browser/ui/views/tab... > chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:312: #if > defined(USE_OZONE) > On 2016/11/04 23:15:39, sky wrote: > > On 2016/11/04 21:54:21, Tom Anderson wrote: > > > On 2016/11/04 21:33:02, sky wrote: > > > > Without TestDesktopBrowserFrameAura TabDragCaptureLostTest isn't useful. > So, > > I > > > > would rather see you DISABLE said test with a comment and a bug. > > > > > > That would not fix the linker error. The error is because there's no > > > DesktopBrowserFrameAura* on Linux Ozone builds atm. Maybe I can stub one > out > > as > > > an alternative? > > > > If that's the case, don't you need an ifdef around > TestDesktopBrowserFrameAura? > > I'm ok with that, just trying to understand. > > Oops, I meant there's no CreateBrowserDesktopWindowTreeHost function, and maybe > I can stub out a BrowserDesktopWindowTreeHostOzone > > The linker error I get is: > ../../chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc:54: error: > undefined reference to > 'BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost(views::internal::NativeWidgetDelegate*, > views::DesktopNativeWidgetAura*, BrowserView*, BrowserFrame*)' > > TestDesktopBrowserFrameAura is a subclass of DesktopBrowserFrameAura, which has > DesktopBrowserFrameAura::InitNativeWidget which calls > CreateBrowserDesktopWindowTreeHost. > > So even though we compile desktop_browser_frame_aura.cc, we never use it in the > ozone build, so by some crazy linker magic it all works ¯\_(ツ)_/¯ Which is not what you want. I encourage you to go through and exclude files you know shouldn't be used in this environment and then add the missing functionality. Patching code in the fashion you have here isn't getting you closer to the end goal of ozone.
thomasanderson@google.com changed reviewers: + rjkroege@chromium.org
https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:18: NOTIMPLEMENTED(); I'm not sure what the right way to stub out BrowserDesktopWindowTreeHost* is. I can see two possible implementations here 1. Have a single BrowserDesktopWindowTreeHostOzone which takes a BrowserDesktopWindowTreeHost in the constructor, and proxies all calls to it. 2. Have BrowserDesktopWindowTreeHostOzoneX11 : public DesktopWindowTreeHostOzoneX11, BrowserDesktopWindowTreeHostOzoneWayland : public DesktopWindowTreeHostOzoneWayland, etc... +rjkroege@ What do you guys think?
https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:13: BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost( What does --mash Chrome use in place of this? It already operates in "desktop-like" mode. But perhaps can't draw itself without ash submitting a top-level compositor frame / per display. But maybe that's ok pending a TODO(tonikitoo) here? Maybe this code should submit the top-level compositor frame when running without ash? https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:51: #if defined(OS_LINUX) Does OS_CHROMEOS never imply OS_LINUX? It use to in various places. But the existing could would seem to suggest that it doesn't?
https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:13: BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost( On 2016/11/07 19:54:15, rjkroege wrote: > What does --mash Chrome use in place of this? It already operates in > "desktop-like" mode. But perhaps can't draw itself without ash submitting a > top-level compositor frame / per display. But maybe that's ok pending a > TODO(tonikitoo) here? Maybe this code should submit the top-level compositor > frame when running without ash? > The only impls I see are BDWTHWin and BDWTHX11, so I guess mash chrome just never calls CreateBrowserDWTH, and this solves the linker error? https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:51: #if defined(OS_LINUX) On 2016/11/07 19:54:15, rjkroege wrote: > Does OS_CHROMEOS never imply OS_LINUX? It use to in various places. But the > existing could would seem to suggest that it doesn't? OS_CHROMEOS implies OS_LINUX. In this mess of ifdefs, the path changed is !defined(OS_CHROMEOS) && defined(use_aura) && defined(OS_LINUX)
nit: change the NOTIMPLEMENTED to NOTREACHED. otherwise lgtm https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:13: BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost( On 2016/11/07 20:07:00, Tom Anderson wrote: > On 2016/11/07 19:54:15, rjkroege wrote: > > What does --mash Chrome use in place of this? It already operates in > > "desktop-like" mode. But perhaps can't draw itself without ash submitting a > > top-level compositor frame / per display. But maybe that's ok pending a > > TODO(tonikitoo) here? Maybe this code should submit the top-level compositor > > frame when running without ash? > > > > The only impls I see are BDWTHWin and BDWTHX11, so I guess mash chrome just > never calls CreateBrowserDWTH, and this solves the linker error? Yes. WindowPort and friends is the solution for this with the --mash config. https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:18: NOTIMPLEMENTED(); On 2016/11/07 19:40:45, Tom Anderson wrote: > I'm not sure what the right way to stub out BrowserDesktopWindowTreeHost* is. > > I can see two possible implementations here > 1. Have a single BrowserDesktopWindowTreeHostOzone which takes a > BrowserDesktopWindowTreeHost in the constructor, and proxies all calls to it. > 2. Have BrowserDesktopWindowTreeHostOzoneX11 : public > DesktopWindowTreeHostOzoneX11, BrowserDesktopWindowTreeHostOzoneWayland : public > DesktopWindowTreeHostOzoneWayland, etc... > > +rjkroege@ > What do you guys think? NOTREACHED(); We should never get here. https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:51: #if defined(OS_LINUX) On 2016/11/07 20:07:00, Tom Anderson wrote: > On 2016/11/07 19:54:15, rjkroege wrote: > > Does OS_CHROMEOS never imply OS_LINUX? It use to in various places. But the > > existing could would seem to suggest that it doesn't? > > OS_CHROMEOS implies OS_LINUX. > > In this mess of ifdefs, the path changed is !defined(OS_CHROMEOS) && > defined(use_aura) && defined(OS_LINUX) OK. Can I persuade you to make this easier to read so that others don't misread the code?
https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:13: BrowserDesktopWindowTreeHost::CreateBrowserDesktopWindowTreeHost( On 2016/11/07 23:01:22, rjkroege wrote: > On 2016/11/07 20:07:00, Tom Anderson wrote: > > On 2016/11/07 19:54:15, rjkroege wrote: > > > What does --mash Chrome use in place of this? It already operates in > > > "desktop-like" mode. But perhaps can't draw itself without ash submitting a > > > top-level compositor frame / per display. But maybe that's ok pending a > > > TODO(tonikitoo) here? Maybe this code should submit the top-level > compositor > > > frame when running without ash? > > > > > > > The only impls I see are BDWTHWin and BDWTHX11, so I guess mash chrome just > > never calls CreateBrowserDWTH, and this solves the linker error? > > Yes. WindowPort and friends is the solution for this with the --mash config. Acknowledged. https://codereview.chromium.org/2478323003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:18: NOTIMPLEMENTED(); On 2016/11/07 23:01:22, rjkroege wrote: > On 2016/11/07 19:40:45, Tom Anderson wrote: > > I'm not sure what the right way to stub out BrowserDesktopWindowTreeHost* is. > > > > I can see two possible implementations here > > 1. Have a single BrowserDesktopWindowTreeHostOzone which takes a > > BrowserDesktopWindowTreeHost in the constructor, and proxies all calls to it. > > 2. Have BrowserDesktopWindowTreeHostOzoneX11 : public > > DesktopWindowTreeHostOzoneX11, BrowserDesktopWindowTreeHostOzoneWayland : > public > > DesktopWindowTreeHostOzoneWayland, etc... > > > > +rjkroege@ > > What do you guys think? > > NOTREACHED(); We should never get here. Done. https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/20001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:51: #if defined(OS_LINUX) On 2016/11/07 23:01:22, rjkroege wrote: > On 2016/11/07 20:07:00, Tom Anderson wrote: > > On 2016/11/07 19:54:15, rjkroege wrote: > > > Does OS_CHROMEOS never imply OS_LINUX? It use to in various places. But the > > > existing could would seem to suggest that it doesn't? > > > > OS_CHROMEOS implies OS_LINUX. > > > > In this mess of ifdefs, the path changed is !defined(OS_CHROMEOS) && > > defined(use_aura) && defined(OS_LINUX) > > OK. > > > Can I persuade you to make this easier to read so that others don't misread the > code? Done. Also I realized I forgot to 'git add' the revert of this in PS2, but I've cleaned this up anyway
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fwang@igalia.com changed reviewers: + fwang@igalia.com
tonikitoo@igalia.com changed reviewers: + tonikitoo@igalia.com
https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:16: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(USE_X11) maybe we can omit OS_LINUX, as USE_X11 implies it. https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:55: ui_controls::InstallUIControlsAura( ui_controls::InstallUIControlsAura is declared/defined ui/views/test/ui_controls_factory_desktop_aurax11.h/cc Maybe should rename it in a follow up to ui_controls::InstallUIControlsAuraX11 ? https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/view_e... File chrome/test/base/view_event_test_platform_part_default.cc (right): https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/view_e... chrome/test/base/view_event_test_platform_part_default.cc:26: #if defined(USE_X11) would not the proper check here be #if !defined(OS_CHROMEOS) && defined(USE_X11) .. just like my comment to chrome/test/base/interactive_ui_tests_main.cc ?
https://codereview.chromium.org/2478323003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:18: NOTIMPLEMENTED(); not sure if the "rebase" version got NOTIMPLEMENTED back here intentionally (differently from ps3), or accidentally.
On 2016/11/04 18:20:33, Tom Anderson wrote: > sky PTAL > > Once this change, along with > https://codereview.chromium.org/2408803002/ > https://codereview.chromium.org/2476663002/ > lands, we should have a green Linux Ozone bot FYI I get a build failure in libangle when ozone_platform_gbm = 0 see https://chromium-review.googlesource.com/#/c/408100/ ; I guess GBM is enabled for the bots?
On 2016/11/08 17:12:37, fwang wrote: > On 2016/11/04 18:20:33, Tom Anderson wrote: > > sky PTAL > > > > Once this change, along with > > https://codereview.chromium.org/2408803002/ > > https://codereview.chromium.org/2476663002/ > > lands, we should have a green Linux Ozone bot > > FYI I get a build failure in libangle when ozone_platform_gbm = 0 see > https://chromium-review.googlesource.com/#/c/408100/ ; I guess GBM is enabled > for the bots? Yes, that's correct. Please see https://chromium-review.googlesource.com/#/c/407301/ and also https://codereview.chromium.org/2472213002 https://codereview.chromium.org/2480573003/ https://codereview.chromium.org/2478323003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc (right): https://codereview.chromium.org/2478323003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_ozone.cc:18: NOTIMPLEMENTED(); On 2016/11/08 15:47:57, tonikitoo wrote: > not sure if the "rebase" version got NOTIMPLEMENTED back here intentionally > (differently from ps3), or accidentally. oops, changed back to NOTREACHED https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/intera... File chrome/test/base/interactive_ui_tests_main.cc (right): https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:16: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(USE_X11) On 2016/11/08 15:44:56, tonikitoo wrote: > maybe we can omit OS_LINUX, as USE_X11 implies it. Done. https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/intera... chrome/test/base/interactive_ui_tests_main.cc:55: ui_controls::InstallUIControlsAura( On 2016/11/08 15:44:56, tonikitoo wrote: > ui_controls::InstallUIControlsAura is declared/defined > ui/views/test/ui_controls_factory_desktop_aurax11.h/cc > > Maybe should rename it in a follow up to ui_controls::InstallUIControlsAuraX11 ? > I was thinking the same thing actually.. But I agree this would be better for a separate CL https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/view_e... File chrome/test/base/view_event_test_platform_part_default.cc (right): https://codereview.chromium.org/2478323003/diff/60001/chrome/test/base/view_e... chrome/test/base/view_event_test_platform_part_default.cc:26: #if defined(USE_X11) On 2016/11/08 15:44:56, tonikitoo wrote: > would not the proper check here be > > #if !defined(OS_CHROMEOS) && defined(USE_X11) > > .. just like my comment to chrome/test/base/interactive_ui_tests_main.cc ? Done.
On 2016/11/08 18:21:49, Tom Anderson wrote: > Yes, that's correct. > Please see > https://chromium-review.googlesource.com/#/c/407301/ It seems that this CL has -2 because it will break chromecast build. I guess https://codereview.chromium.org/2480573003/ should be fine to make the build work on the bots... However, https://chromium-review.googlesource.com/#/c/408100/ is also useful to make Linux Desktop builds with gbm disabled work and angle reviewers seem more likely to accept it.
Pinging sky
LGTM
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tonikitoo@igalia.com, rjkroege@chromium.org Link to the patchset: https://codereview.chromium.org/2478323003/#ps80001 (title: "Fix up macros")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Ozone Linux: Fix linker errors in interactive_ui_tests BUG=295089 R=sky@chromium.org ========== to ========== Ozone Linux: Fix linker errors in interactive_ui_tests BUG=295089 R=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Ozone Linux: Fix linker errors in interactive_ui_tests BUG=295089 R=sky@chromium.org ========== to ========== Ozone Linux: Fix linker errors in interactive_ui_tests BUG=295089 R=sky@chromium.org Committed: https://crrev.com/0f6913b12bd932712e21b88a9668726c26374152 Cr-Commit-Position: refs/heads/master@{#430825} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0f6913b12bd932712e21b88a9668726c26374152 Cr-Commit-Position: refs/heads/master@{#430825} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
