Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764813002:
Make extensions_browsertest work on mac. (Closed)
DescriptionMake extensions_browsertest work on mac.
Make app_shell, app_shell_framework and related targets as bundles on
mac. Mostly copy-paste from content/shell/BUILD.gn.
BUG=
R=steel@chromium.org
Review-Url: https://codereview.chromium.org/2764813002
Cr-Commit-Position: refs/heads/master@{#460012}
Committed: https://chromium.googlesource.com/chromium/src/+/dadec57db629fb81fea6341d949d5a3f0f27eb9c
Patch Set 1 #
Messages
Total messages: 16 (6 generated)
rkc@chromium.org changed reviewers: + michaelpg@chromium.org, rkc@chromium.org
Michael, I forget, was there a reason why we have a mac build around for app_shell?
akalugin: Sorry, I'm not familiar enough with Mac or GN to properly review this change. I'm happy to provide an owners stamp after a review from someone more knowledgeable than me. brettw@ may know who to direct this to (or could review himself). On 2017/03/21 08:33:29, rkc wrote: > Michael, I forget, was there a reason why we have a mac build around for > app_shell? We haven't made a clear decision on what to do with app_shell on Mac and Windows. My understanding is that we'd keep it around but not officially support it, meaning patches like this are welcome, but we won't go out of our way to implement features on those platforms. The situation might change as it becomes clearer how much effort it will take to keep Mac and Windows passing tests. I'll look into it and start a thread with Zel.
Other than the ability to run/debug apps on other platforms, I don't see any reason at all to keep a Mac/Win port. If it is going to be a zero to minimal time investment, I'm fine with keeping support but if it mounts to anything more than that, we'll drop these ports.
Description was changed from ========== Make extensions_browsertest work on mac. Make app_shell, app_shell_framework and related targets as bundles on mac. Mostly copy-paste from content/shell/BUILD.gn. BUG= R=steel@chromium.org ========== to ========== Make extensions_browsertest work on mac. Make app_shell, app_shell_framework and related targets as bundles on mac. Mostly copy-paste from content/shell/BUILD.gn. BUG= R=steel@chromium.org ==========
akalugin@yandex-team.ru changed reviewers: + brettw@chromium.org
On 2017/03/22 20:40:24, michaelpg wrote: > akalugin: Sorry, I'm not familiar enough with Mac or GN to properly review this > change. I'm happy to provide an owners stamp after a review from someone more > knowledgeable than me. brettw@ may know who to direct this to (or could review > himself). Added brettw@ to reviewers.
Making extensions_browsertests work on mac sounds great. Is the app_shell stuff required for that or just ancillary? If it's not required for this, I'd rather conditionally not define them on Mac: the fix is nontrivial and it seems confusing to people to have all of this around (as it's confusing to the people here).
On 2017/03/23 22:12:06, brettw (plz ping after 24h) wrote: > Making extensions_browsertests work on mac sounds great. Is the app_shell stuff > required for that or just ancillary? If it's not required for this, I'd rather > conditionally not define them on Mac: the fix is nontrivial and it seems > confusing to people to have all of this around (as it's confusing to the people > here). At first, I tried to remove dependency on //extensions/shell:browser_tests from //extension:extensions_browsertests ( https://cs.chromium.org/chromium/src/extensions/BUILD.gn?l=240 ) but this does not work because all browsertests from /extensions/browser use AppShellTest as their base class and use various stuff from it. So the answer is yes, now app_shell is required for extensions_browsertest to work. Before migration to GN app_shell was actually built as bundle, but this has not been ported to GN https://codereview.chromium.org/1413783003 . Therefore I decided to fix TODOs in /extensions/shell/BUILD.gn and port bundle support similar to the way it was done here https://codereview.chromium.org/1915863003/patch/120001/130004 .
lgtm
The CQ bit was checked by akalugin@yandex-team.ru
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": 1, "attempt_start_ts": 1490671404448050, "parent_rev":
"1cfd7c66fd4befbd62f10526965413e682e84e13", "commit_rev":
"dadec57db629fb81fea6341d949d5a3f0f27eb9c"}
Message was sent while issue was closed.
Description was changed from ========== Make extensions_browsertest work on mac. Make app_shell, app_shell_framework and related targets as bundles on mac. Mostly copy-paste from content/shell/BUILD.gn. BUG= R=steel@chromium.org ========== to ========== Make extensions_browsertest work on mac. Make app_shell, app_shell_framework and related targets as bundles on mac. Mostly copy-paste from content/shell/BUILD.gn. BUG= R=steel@chromium.org Review-Url: https://codereview.chromium.org/2764813002 Cr-Commit-Position: refs/heads/master@{#460012} Committed: https://chromium.googlesource.com/chromium/src/+/dadec57db629fb81fea6341d949d... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/dadec57db629fb81fea6341d949d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
