Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457443004:
Build some x11 dependent files only if 'use_x11' (Closed)
DescriptionBuild some x11 dependent files only if 'use_x11'
This will make it easier to make desktop linux chrome
builds, through Ozone.
BUG=295089
Committed: https://crrev.com/8e861c0932dc392508ab64284ce160d1a58c700e
Cr-Commit-Position: refs/heads/master@{#428174}
Patch Set 1 #Patch Set 2 : Build some x11 dependent files if 'use_x11' #
Total comments: 1
Patch Set 3 : Build some x11 dependent files if 'use_x11' #
Total comments: 2
Patch Set 4 : Build some x11 dependent files if 'use_x11' #Patch Set 5 : Build some x11 dependent files if 'use_x11' #Patch Set 6 : Build some x11 dependent files if 'use_x11' #Messages
Total messages: 29 (19 generated)
The CQ bit was checked by tonikitoo@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.
The CQ bit was checked by tonikitoo@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.
Description was changed from ========== Build some x11 dependent files if 'use_x11' This will make it easier to make desktop linux chrome builds, through Ozone. BUG=295089 ========== to ========== Build some x11 dependent files only if 'use_x11' This will make it easier to make desktop linux chrome builds, through Ozone. BUG=295089 ==========
tonikitoo@chromium.org changed reviewers: + rjkroege@chromium.org, thestig@chromium.org - tonikitoo@chromium.org
tonikitoo@chromium.org changed reviewers: + tonikitoo@chromium.org
PTAL
Can you check the unit tests for the files in the use_x11 block, if any, are also built under the same condition? https://codereview.chromium.org/2457443004/diff/20001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2457443004/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:3508: "password_manager/native_backend_kwallet_x.cc", I don't see any x11 usage here. I think the 'x' means unix but not mac.
On 2016/10/27 17:00:16, Lei Zhang wrote: > Can you check the unit tests for the files in the use_x11 block, if any, are > also built under the same condition? Checked chrome/test/BUILD.gn, where theme_service_unittest.cc is included in the GN build. No x11 checks. I added a comment on the code. no unittests I could find for the other files being guarded with use_x11. > https://codereview.chromium.org/2457443004/diff/20001/chrome/browser/BUILD.gn > File chrome/browser/BUILD.gn (right): > > https://codereview.chromium.org/2457443004/diff/20001/chrome/browser/BUILD.gn... > chrome/browser/BUILD.gn:3508: "password_manager/native_backend_kwallet_x.cc", > I don't see any x11 usage here. I think the 'x' means unix but not mac. Done. It works fine for ozone as you guessed (no x11 dependency). "bleh_x.cc/h" indicating unix/non-mac isn't too clear, but it makes sense. I have seen _posix.cc or _unix suffixed files for this though IIRC.
fwang@igalia.com changed reviewers: + fwang@igalia.com
informal l g t m https://codereview.chromium.org/2457443004/diff/40001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2457443004/diff/40001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:3511: # is conditioned by USE_X11. I think you should check wording here. Do you mean "does not depend on X11" ... "its instantiation/inclusion" ?
https://codereview.chromium.org/2457443004/diff/40001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2457443004/diff/40001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:3511: # is conditioned by USE_X11. On 2016/10/27 18:19:16, fwang wrote: > I think you should check wording here. Do you mean > > "does not depend on X11" ... > "its instantiation/inclusion" > > ? Done.
The CQ bit was checked by thestig@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...
On 2016/10/27 18:11:38, tonikitoo wrote: > "bleh_x.cc/h" indicating unix/non-mac isn't too clear, but it makes sense. I > have seen _posix.cc or _unix suffixed files for this though IIRC. I bet some of these files were named a long time ago. _posix isn't clear either because Macs are POSIX. In base/, we have one directory named 'nix'.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tonikitoo@igalia.com
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 ========== Build some x11 dependent files only if 'use_x11' This will make it easier to make desktop linux chrome builds, through Ozone. BUG=295089 ========== to ========== Build some x11 dependent files only if 'use_x11' This will make it easier to make desktop linux chrome builds, through Ozone. BUG=295089 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== Build some x11 dependent files only if 'use_x11' This will make it easier to make desktop linux chrome builds, through Ozone. BUG=295089 ========== to ========== Build some x11 dependent files only if 'use_x11' This will make it easier to make desktop linux chrome builds, through Ozone. BUG=295089 Committed: https://crrev.com/8e861c0932dc392508ab64284ce160d1a58c700e Cr-Commit-Position: refs/heads/master@{#428174} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8e861c0932dc392508ab64284ce160d1a58c700e Cr-Commit-Position: refs/heads/master@{#428174} |
