Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
102163005:
Do not define use_clipboard_aurax11 on ozone builds (Closed)
|
Created:
7 years ago by dnicoara Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDo not define use_clipboard_aurax11 on ozone builds
On Ozone builds clipboard_aurax11 is excluded since Ozone
builds should not pull any X11 dependencies.
This patch fixes failing ui_unittests build on Ozone.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241390
Patch Set 1 #Patch Set 2 : use_ozone #Patch Set 3 : . #
Total comments: 4
Patch Set 4 : check os==win #
Total comments: 5
Patch Set 5 : desktop_linux #Messages
Total messages: 31 (0 generated)
Please take a look. sky@ for OWNERS oshima@ FYI
https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and use_ozone==0', { Shouldn't the test for use_clipboard_aurax11==1 be enough?
https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and use_ozone==0', { On 2013/12/13 17:49:10, sky wrote: > Shouldn't the test for use_clipboard_aurax11==1 be enough? This may look a bit weird, but this is side effect of win_aura not using clipboard_aura.cc.
https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and use_ozone==0', { On 2013/12/13 17:51:35, oshima wrote: > On 2013/12/13 17:49:10, sky wrote: > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > This may look a bit weird, but this is side effect of > win_aura not using clipboard_aura.cc. Shouldn't the test then be os==win and use_aura or use_clipboard_aurax11?
https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and use_ozone==0', { On 2013/12/13 17:55:31, sky wrote: > On 2013/12/13 17:51:35, oshima wrote: > > On 2013/12/13 17:49:10, sky wrote: > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > > > This may look a bit weird, but this is side effect of > > win_aura not using clipboard_aura.cc. > > Shouldn't the test then be os==win and use_aura or use_clipboard_aurax11? If you ask my preference, I prefer not to include "for all" case and include each file for each configuration. That is far less confusing IMO.
On 2013/12/13 18:03:43, oshima wrote: > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > File ui/ui.gyp (right): > > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and use_ozone==0', { > On 2013/12/13 17:55:31, sky wrote: > > On 2013/12/13 17:51:35, oshima wrote: > > > On 2013/12/13 17:49:10, sky wrote: > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > > > > > This may look a bit weird, but this is side effect of > > > win_aura not using clipboard_aura.cc. > > > > Shouldn't the test then be os==win and use_aura or use_clipboard_aurax11? > > If you ask my preference, I prefer not to include "for all" case > and include each file for each configuration. That is far less confusing IMO. Not sure if we've reached a consensus. Should I change the condition as per sky@'s suggestion?
On 2013/12/13 18:30:26, dnicoara wrote: > On 2013/12/13 18:03:43, oshima wrote: > > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > > File ui/ui.gyp (right): > > > > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and use_ozone==0', { > > On 2013/12/13 17:55:31, sky wrote: > > > On 2013/12/13 17:51:35, oshima wrote: > > > > On 2013/12/13 17:49:10, sky wrote: > > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > > > > > > > This may look a bit weird, but this is side effect of > > > > win_aura not using clipboard_aura.cc. > > > > > > Shouldn't the test then be os==win and use_aura or use_clipboard_aurax11? > > > > If you ask my preference, I prefer not to include "for all" case > > and include each file for each configuration. That is far less confusing IMO. > > Not sure if we've reached a consensus. Should I change the condition as per > sky@'s suggestion? Actually I think we should rename clipboard_aura to clipboard_chromeos. sky@, WDTY?
SGTM On Fri, Dec 13, 2013 at 10:43 AM, <oshima@chromium.org> wrote: > On 2013/12/13 18:30:26, dnicoara wrote: >> >> On 2013/12/13 18:03:43, oshima wrote: >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp >> > File ui/ui.gyp (right): >> > >> > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 >> > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and >> > use_ozone==0', > > { >> >> > On 2013/12/13 17:55:31, sky wrote: >> > > On 2013/12/13 17:51:35, oshima wrote: >> > > > On 2013/12/13 17:49:10, sky wrote: >> > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? >> > > > >> > > > This may look a bit weird, but this is side effect of >> > > > win_aura not using clipboard_aura.cc. >> > > >> > > Shouldn't the test then be os==win and use_aura or >> > > use_clipboard_aurax11? >> > >> > If you ask my preference, I prefer not to include "for all" case >> > and include each file for each configuration. That is far less confusing > > IMO. > >> Not sure if we've reached a consensus. Should I change the condition as >> per >> sky@'s suggestion? > > > Actually I think we should rename clipboard_aura to clipboard_chromeos. > sky@, > WDTY? > > https://codereview.chromium.org/102163005/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/13 20:23:34, sky wrote: > SGTM > > On Fri, Dec 13, 2013 at 10:43 AM, <mailto:oshima@chromium.org> wrote: > > On 2013/12/13 18:30:26, dnicoara wrote: > >> > >> On 2013/12/13 18:03:43, oshima wrote: > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > >> > File ui/ui.gyp (right): > >> > > >> > > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > >> > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and > >> > use_ozone==0', > > > > { > >> > >> > On 2013/12/13 17:55:31, sky wrote: > >> > > On 2013/12/13 17:51:35, oshima wrote: > >> > > > On 2013/12/13 17:49:10, sky wrote: > >> > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > >> > > > > >> > > > This may look a bit weird, but this is side effect of > >> > > > win_aura not using clipboard_aura.cc. > >> > > > >> > > Shouldn't the test then be os==win and use_aura or > >> > > use_clipboard_aurax11? > >> > > >> > If you ask my preference, I prefer not to include "for all" case > >> > and include each file for each configuration. That is far less confusing > > > > IMO. > > > >> Not sure if we've reached a consensus. Should I change the condition as > >> per > >> sky@'s suggestion? > > > > > > Actually I think we should rename clipboard_aura to clipboard_chromeos. > > sky@, > > WDTY? > > > > https://codereview.chromium.org/102163005/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I was trying that, but the file naming rules (and the gyp inclusion order) are preventing me to selectively include the file for Ozone. Would there be a better suffix such that I'd avoid making an Ozone specific copy?
On 2013/12/13 21:31:24, dnicoara wrote: > On 2013/12/13 20:23:34, sky wrote: > > SGTM > > > > On Fri, Dec 13, 2013 at 10:43 AM, <mailto:oshima@chromium.org> wrote: > > > On 2013/12/13 18:30:26, dnicoara wrote: > > >> > > >> On 2013/12/13 18:03:43, oshima wrote: > > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > > >> > File ui/ui.gyp (right): > > >> > > > >> > > > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > > >> > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and > > >> > use_ozone==0', > > > > > > { > > >> > > >> > On 2013/12/13 17:55:31, sky wrote: > > >> > > On 2013/12/13 17:51:35, oshima wrote: > > >> > > > On 2013/12/13 17:49:10, sky wrote: > > >> > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > >> > > > > > >> > > > This may look a bit weird, but this is side effect of > > >> > > > win_aura not using clipboard_aura.cc. > > >> > > > > >> > > Shouldn't the test then be os==win and use_aura or > > >> > > use_clipboard_aurax11? > > >> > > > >> > If you ask my preference, I prefer not to include "for all" case > > >> > and include each file for each configuration. That is far less confusing > > > > > > IMO. > > > > > >> Not sure if we've reached a consensus. Should I change the condition as > > >> per > > >> sky@'s suggestion? > > > > > > > > > Actually I think we should rename clipboard_aura to clipboard_chromeos. > > > sky@, > > > WDTY? > > > > > > https://codereview.chromium.org/102163005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I was trying that, but the file naming rules (and the gyp inclusion order) are > preventing me to selectively include the file for Ozone. Would there be a better > suffix such that I'd avoid making an Ozone specific copy? don't we have other files that are (chromeos | ozone) && !win ?
On 2013/12/13 21:36:16, oshima wrote: > On 2013/12/13 21:31:24, dnicoara wrote: > > On 2013/12/13 20:23:34, sky wrote: > > > SGTM > > > > > > On Fri, Dec 13, 2013 at 10:43 AM, <mailto:oshima@chromium.org> wrote: > > > > On 2013/12/13 18:30:26, dnicoara wrote: > > > >> > > > >> On 2013/12/13 18:03:43, oshima wrote: > > > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > > > >> > File ui/ui.gyp (right): > > > >> > > > > >> > > > > >> > > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > > > >> > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and > > > >> > use_ozone==0', > > > > > > > > { > > > >> > > > >> > On 2013/12/13 17:55:31, sky wrote: > > > >> > > On 2013/12/13 17:51:35, oshima wrote: > > > >> > > > On 2013/12/13 17:49:10, sky wrote: > > > >> > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > > >> > > > > > > >> > > > This may look a bit weird, but this is side effect of > > > >> > > > win_aura not using clipboard_aura.cc. > > > >> > > > > > >> > > Shouldn't the test then be os==win and use_aura or > > > >> > > use_clipboard_aurax11? > > > >> > > > > >> > If you ask my preference, I prefer not to include "for all" case > > > >> > and include each file for each configuration. That is far less > confusing > > > > > > > > IMO. > > > > > > > >> Not sure if we've reached a consensus. Should I change the condition as > > > >> per > > > >> sky@'s suggestion? > > > > > > > > > > > > Actually I think we should rename clipboard_aura to clipboard_chromeos. > > > > sky@, > > > > WDTY? > > > > > > > > https://codereview.chromium.org/102163005/ > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > I was trying that, but the file naming rules (and the gyp inclusion order) are > > preventing me to selectively include the file for Ozone. Would there be a > better > > suffix such that I'd avoid making an Ozone specific copy? > > don't we have other files that are (chromeos | ozone) && !win ? I looked at where we use use_ozone in gyp files, but couldn't find anything to go by. We're mostly excluding files rather than including.
On 2013/12/13 21:44:56, dnicoara wrote: > On 2013/12/13 21:36:16, oshima wrote: > > On 2013/12/13 21:31:24, dnicoara wrote: > > > On 2013/12/13 20:23:34, sky wrote: > > > > SGTM > > > > > > > > On Fri, Dec 13, 2013 at 10:43 AM, <mailto:oshima@chromium.org> wrote: > > > > > On 2013/12/13 18:30:26, dnicoara wrote: > > > > >> > > > > >> On 2013/12/13 18:03:43, oshima wrote: > > > > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > > > > >> > File ui/ui.gyp (right): > > > > >> > > > > > >> > > > > > >> > > > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > > > > >> > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and > > > > >> > use_ozone==0', > > > > > > > > > > { > > > > >> > > > > >> > On 2013/12/13 17:55:31, sky wrote: > > > > >> > > On 2013/12/13 17:51:35, oshima wrote: > > > > >> > > > On 2013/12/13 17:49:10, sky wrote: > > > > >> > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > > > >> > > > > > > > >> > > > This may look a bit weird, but this is side effect of > > > > >> > > > win_aura not using clipboard_aura.cc. > > > > >> > > > > > > >> > > Shouldn't the test then be os==win and use_aura or > > > > >> > > use_clipboard_aurax11? > > > > >> > > > > > >> > If you ask my preference, I prefer not to include "for all" case > > > > >> > and include each file for each configuration. That is far less > > confusing > > > > > > > > > > IMO. > > > > > > > > > >> Not sure if we've reached a consensus. Should I change the condition as > > > > >> per > > > > >> sky@'s suggestion? > > > > > > > > > > > > > > > Actually I think we should rename clipboard_aura to clipboard_chromeos. > > > > > sky@, > > > > > WDTY? > > > > > > > > > > https://codereview.chromium.org/102163005/ > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > I was trying that, but the file naming rules (and the gyp inclusion order) > are > > > preventing me to selectively include the file for Ozone. Would there be a > > better > > > suffix such that I'd avoid making an Ozone specific copy? > > > > don't we have other files that are (chromeos | ozone) && !win ? > > I looked at where we use use_ozone in gyp files, but couldn't find anything to > go by. We're mostly excluding files rather than including. can't you just force to include clipboard_chromeos.cc when ozone=1 ?
On 2013/12/13 22:13:23, oshima wrote: > On 2013/12/13 21:44:56, dnicoara wrote: > > On 2013/12/13 21:36:16, oshima wrote: > > > On 2013/12/13 21:31:24, dnicoara wrote: > > > > On 2013/12/13 20:23:34, sky wrote: > > > > > SGTM > > > > > > > > > > On Fri, Dec 13, 2013 at 10:43 AM, <mailto:oshima@chromium.org> wrote: > > > > > > On 2013/12/13 18:30:26, dnicoara wrote: > > > > > >> > > > > > >> On 2013/12/13 18:03:43, oshima wrote: > > > > > >> > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp > > > > > >> > File ui/ui.gyp (right): > > > > > >> > > > > > > >> > > > > > > >> > > > > https://codereview.chromium.org/102163005/diff/30001/ui/ui.gyp#newcode428 > > > > > >> > ui/ui.gyp:428: ['(use_x11==0 or use_clipboard_aurax11==1) and > > > > > >> > use_ozone==0', > > > > > > > > > > > > { > > > > > >> > > > > > >> > On 2013/12/13 17:55:31, sky wrote: > > > > > >> > > On 2013/12/13 17:51:35, oshima wrote: > > > > > >> > > > On 2013/12/13 17:49:10, sky wrote: > > > > > >> > > > > Shouldn't the test for use_clipboard_aurax11==1 be enough? > > > > > >> > > > > > > > > >> > > > This may look a bit weird, but this is side effect of > > > > > >> > > > win_aura not using clipboard_aura.cc. > > > > > >> > > > > > > > >> > > Shouldn't the test then be os==win and use_aura or > > > > > >> > > use_clipboard_aurax11? > > > > > >> > > > > > > >> > If you ask my preference, I prefer not to include "for all" case > > > > > >> > and include each file for each configuration. That is far less > > > confusing > > > > > > > > > > > > IMO. > > > > > > > > > > > >> Not sure if we've reached a consensus. Should I change the condition > as > > > > > >> per > > > > > >> sky@'s suggestion? > > > > > > > > > > > > > > > > > > Actually I think we should rename clipboard_aura to > clipboard_chromeos. > > > > > > sky@, > > > > > > WDTY? > > > > > > > > > > > > https://codereview.chromium.org/102163005/ > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send > an > > > > email > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > I was trying that, but the file naming rules (and the gyp inclusion order) > > are > > > > preventing me to selectively include the file for Ozone. Would there be a > > > better > > > > suffix such that I'd avoid making an Ozone specific copy? > > > > > > don't we have other files that are (chromeos | ozone) && !win ? > > > > I looked at where we use use_ozone in gyp files, but couldn't find anything to > > go by. We're mostly excluding files rather than including. > > can't you just force to include clipboard_chromeos.cc when ozone=1 ? As per the offline discussion with oshima@, going with sky@'s first suggestion to use os==win to avoid code duplication.
LGTM
https://codereview.chromium.org/102163005/diff/30002/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30002/ui/ui.gyp#newcode430 ui/ui.gyp:430: 'base/clipboard/clipboard_aura.cc', perhaps we could rename the file to clipboard_linuxaura.cc ?? Then the file would be excluded on non linuxaura builds (condition defined in build/filename_rules.gypi)
https://codereview.chromium.org/102163005/diff/30002/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30002/ui/ui.gyp#newcode430 ui/ui.gyp:430: 'base/clipboard/clipboard_aura.cc', On 2013/12/14 00:44:06, kalyank wrote: > perhaps we could rename the file to clipboard_linuxaura.cc ?? > > Then the file would be excluded on non linuxaura builds (condition defined in > build/filename_rules.gypi) This is not for linux_aura. This is used for chromeos and ozone. We tried to rename to clipboard_chromeos, but it didn't work for ozone because of gyp rules.
https://codereview.chromium.org/102163005/diff/30002/ui/ui.gyp File ui/ui.gyp (right): https://codereview.chromium.org/102163005/diff/30002/ui/ui.gyp#newcode430 ui/ui.gyp:430: 'base/clipboard/clipboard_aura.cc', On 2013/12/14 00:47:11, oshima wrote: > On 2013/12/14 00:44:06, kalyank wrote: > > perhaps we could rename the file to clipboard_linuxaura.cc ?? > > > > Then the file would be excluded on non linuxaura builds (condition defined in > > build/filename_rules.gypi) Sorry, it should have been _auralinux ending and not linuxaura. > This is not for linux_aura. This is used for chromeos and ozone. > We tried to rename to clipboard_chromeos, but it didn't work for ozone because > of gyp rules. Aha, k. From filename_rules.gypi _auralinux files are excluded when OS!=linux or use_aura=0. So, we fail these checks on ChromeOS?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnicoara@chromium.org/102163005/30002
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnicoara@chromium.org/102163005/30002
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnicoara@chromium.org/102163005/30002
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
https://codereview.chromium.org/102163005/diff/30002/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/102163005/diff/30002/build/common.gypi#newcod... build/common.gypi:555: ['OS=="linux" and use_aura==1 and chromeos==0 and use_ozone==0', { I don't think this even belongs in common.gypi, but in any case shouldn't it be: desktop_linux==1 and use_aura==1 and use_x11==1 It's MUCH cleaner to include X11 code based on use_x11 than (linux, !use_ozone).
sky@, oshima@ please take another look. I've updated it based on spang@'s comment. https://codereview.chromium.org/102163005/diff/30002/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/102163005/diff/30002/build/common.gypi#newcod... build/common.gypi:555: ['OS=="linux" and use_aura==1 and chromeos==0 and use_ozone==0', { On 2013/12/16 19:20:59, spang wrote: > I don't think this even belongs in common.gypi, but in any case shouldn't it be: > > desktop_linux==1 and use_aura==1 and use_x11==1 > > It's MUCH cleaner to include X11 code based on use_x11 than (linux, !use_ozone). Done.
On 2013/12/17 16:41:43, dnicoara wrote: > sky@, oshima@ please take another look. I've updated it based on spang@'s > comment. > > https://codereview.chromium.org/102163005/diff/30002/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/102163005/diff/30002/build/common.gypi#newcod... > build/common.gypi:555: ['OS=="linux" and use_aura==1 and chromeos==0 and > use_ozone==0', { > On 2013/12/16 19:20:59, spang wrote: > > I don't think this even belongs in common.gypi, but in any case shouldn't it > be: > > > > desktop_linux==1 and use_aura==1 and use_x11==1 > > > > It's MUCH cleaner to include X11 code based on use_x11 than (linux, > !use_ozone). > > Done. lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnicoara@chromium.org/102163005/60001
On 2013/12/17 16:41:43, dnicoara wrote: > sky@, oshima@ please take another look. I've updated it based on spang@'s > comment. > Let's send it in and fix our build. Can revert if sky or oshima object to the new conditional.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dnicoara@chromium.org/102163005/60001
LGTM
Message was sent while issue was closed.
Change committed as 241390 |
