Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2631623003:
Remove some ifdefs from ash since it only supports ChromeOS now. (Closed)
|
Created:
3 years, 11 months ago by Evan Stade Modified:
3 years, 11 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove some ifdefs from ash since it only supports ChromeOS now.
BUG=666773
Review-Url: https://codereview.chromium.org/2631623003
Cr-Commit-Position: refs/heads/master@{#444590}
Committed: https://chromium.googlesource.com/chromium/src/+/4471855e032876c6194f5a414fa72f28b30e057d
Patch Set 1 #
Total comments: 5
Patch Set 2 : no export #Patch Set 3 : revert x11 change #
Messages
Total messages: 25 (15 generated)
The CQ bit was checked by estade@chromium.org 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...
https://codereview.chromium.org/2631623003/diff/1/ash/display/display_util.h File ash/display/display_util.h (right): https://codereview.chromium.org/2631623003/diff/1/ash/display/display_util.h#... ash/display/display_util.h:39: ASH_EXPORT gfx::Rect GetNativeEdgeBounds(AshWindowTreeHost* ash_host, I don't actually know why these EXPORTs are necessary. The exported functions are used by ash_unittests, and yet when I remove the exports it still works (even a component build). Do unit tests have always link statically or something? https://codereview.chromium.org/2631623003/diff/1/ash/display/mirror_window_c... File ash/display/mirror_window_controller.cc (left): https://codereview.chromium.org/2631623003/diff/1/ash/display/mirror_window_c... ash/display/mirror_window_controller.cc:42: #if defined(USE_X11) Am I correct in thinking OS_CHROMEOS implies USE_X11? It seems the only linux config that doesn't use x11 is chromecast.
Description was changed from ========== Remove some ifdefs from ash since it only supports ChromeOS now. BUG=666773 ========== to ========== Remove some ifdefs from ash since it only supports ChromeOS now. BUG=666773 ==========
estade@chromium.org changed reviewers: + jamescook@chromium.org
oops, +jamescook for real this time. See comment above, patchset 1 has changes I'm more sure of whereas patchset 2 additionally removes a couple ASH_EXPORTs.
The CQ bit was checked by estade@chromium.org 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2631623003/diff/1/ash/display/display_util.h File ash/display/display_util.h (right): https://codereview.chromium.org/2631623003/diff/1/ash/display/display_util.h#... ash/display/display_util.h:39: ASH_EXPORT gfx::Rect GetNativeEdgeBounds(AshWindowTreeHost* ash_host, On 2017/01/13 17:19:34, Evan Stade wrote: > I don't actually know why these EXPORTs are necessary. The exported functions > are used by ash_unittests, and yet when I remove the exports it still works > (even a component build). Do unit tests have always link statically or > something? Huh. I thought they were necessary for unit tests too. If it works with component build, and it passes on the bots, feel free to remove them. Or leave them, I don't feel strongly. https://codereview.chromium.org/2631623003/diff/1/ash/display/mirror_window_c... File ash/display/mirror_window_controller.cc (left): https://codereview.chromium.org/2631623003/diff/1/ash/display/mirror_window_c... ash/display/mirror_window_controller.cc:42: #if defined(USE_X11) On 2017/01/13 17:19:34, Evan Stade wrote: > Am I correct in thinking OS_CHROMEOS implies USE_X11? It seems the only linux > config that doesn't use x11 is chromecast. No, real Chromebooks don't run X. They use our own Ozone code backed directly by the Linux kernel DRM system. So we still need these ifdefs.
The CQ bit was checked by estade@chromium.org 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.
https://codereview.chromium.org/2631623003/diff/1/ash/display/mirror_window_c... File ash/display/mirror_window_controller.cc (left): https://codereview.chromium.org/2631623003/diff/1/ash/display/mirror_window_c... ash/display/mirror_window_controller.cc:42: #if defined(USE_X11) On 2017/01/13 20:19:31, James Cook wrote: > On 2017/01/13 17:19:34, Evan Stade wrote: > > Am I correct in thinking OS_CHROMEOS implies USE_X11? It seems the only linux > > config that doesn't use x11 is chromecast. > > No, real Chromebooks don't run X. They use our own Ozone code backed directly by > the Linux kernel DRM system. So we still need these ifdefs. ah, ok. restored
LGTM
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484787071424860,
"parent_rev": "89ac94386ebb1a4e316549cab76fe490e66f9b34", "commit_rev":
"4471855e032876c6194f5a414fa72f28b30e057d"}
Message was sent while issue was closed.
Description was changed from ========== Remove some ifdefs from ash since it only supports ChromeOS now. BUG=666773 ========== to ========== Remove some ifdefs from ash since it only supports ChromeOS now. BUG=666773 Review-Url: https://codereview.chromium.org/2631623003 Cr-Commit-Position: refs/heads/master@{#444590} Committed: https://chromium.googlesource.com/chromium/src/+/4471855e032876c6194f5a414fa7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4471855e032876c6194f5a414fa7...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2639323003/ by estade@chromium.org. The reason for reverting is: ASH_EXPORT was necessary after all..
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 444590 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
I thought it would be. Otherwise places like chrome_switches wouldn't need it. On Wed, Jan 18, 2017 at 6:09 PM, <estade@chromium.org> wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2639323003/ by estade@chromium.org. > > The reason for reverting is: ASH_EXPORT was necessary after all.. > > https://codereview.chromium.org/2631623003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
