Make different channel apps run simultaneously on MacOS #298

simonhong · 2018-07-30T12:44:23Z

To execute different channel apps simultaneously, each app should
have different app name and different id.
This PR added different BRANDING files for each channel because
Brave on MacOS refers BRANDING file to get unique app name and id.

Issue brave/brave-browser#469

Did all four channel apps installation and execution simultaneously.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.

Test Plan:

Create multiple channel dmg and install/run them.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions

simonhong · 2018-07-31T05:21:18Z

patches/chrome-common-BUILD.gn.patch

+     "//printing/buildflags",
+     "//ui/base:ui_features",
+   ]
+  if (brave_chromium_build) { configs += [ "//brave/common:constants_configs" ] }


Added to use BRAVE_PRODUCT_STRING define in chrome_constants.cc.
This define has different string for different channel.(Brave-Browser, Brave-Browser-Nightly)

see comment below

simonhong · 2018-07-31T05:22:21Z

patches/chrome-installer-mac-sign_versioned_dir.sh.in.patch

+     "${xpc_plist}" "CFBundleIdentifier")"
+
+framework_plist="${framework}/Resources/Info"
+framework_bundle_id="$(__CFPREFERENCES_AVOID_DAEMON=1 defaults read \


Fetch id from plist because framework of different channel have different id.
(ex, org.brave.Brave.dev.framework)

isn't this also the case with chrome?

it seems like we're patching some of these files to the point that we might as well have our own version of them?

simonhong · 2018-07-31T05:30:50Z

patches/chrome-common-chrome_constants.cc.patch

 --- a/chrome/common/chrome_constants.cc
 +++ b/chrome/common/chrome_constants.cc
 @@ -10,11 +10,11 @@
 #define FPL FILE_PATH_LITERAL

 #if defined(OS_MACOSX)
 -#define CHROMIUM_PRODUCT_STRING "Chromium"
-+#define CHROMIUM_PRODUCT_STRING "Brave"
+#define CHROMIUM_PRODUCT_STRING BRAVE_PRODUCT_STRING


Framework/Helper app's name reflects PRODUCT_FULL_NAME in BRANDING file.
(Ex, Brave-Browser-Dev Framework.framework, Brave-Browser-Dev Helper.app).
Their framework/helper name is made by using CHROMIUM_PRODCUT_STRING and PRODUCT_STRING.

can't we just use a chromium_src override here to redefine?

I'm pretty sure this entire patch file can be replaced with overrides cc @bbondy

simonhong · 2018-07-31T05:40:48Z

@RyanJarv To sign each channel, I created cert for each channel like below because each channel has different id.
spctl --add --requirement '(identifier "org.brave.Brave.nightly") and (certificate leaf = H"40D43D0858B39D93BA0A5EEFCA57CE9D5E169526")' --label simon-dev

bridiver · 2018-07-31T15:58:43Z

patches/chrome-tools-build-mac-infoplist_strings_util.mm.patch

+
+     // Fetch the strings
+     NSString* name =
+-          LoadStringFromDataPack(branded_data_pack.get(), cur_lang,


this doesn't seem right to me because presumably this works with multi-channel chrome

bridiver · 2018-07-31T15:58:57Z

patches/chrome-tools-build-mac-BUILD.gn.patch

+
+ executable("infoplist_strings_tool") {
+   configs += [ "//build/config/compiler:wexit_time_destructors" ]
+  configs += [ "//brave/common:constants_configs" ]


same as other comments for BRAVE_PRODUCT_STRING

bridiver

see comments

bbondy · 2018-07-31T16:12:05Z

I was hoping to include this in the new dev-channel builds tonight, are these blockers @bridiver ?

bbondy · 2018-07-31T16:16:50Z

I'll wait on it, FYI the patches are rebased for C69 in the branch channels if you want to use those as a base.

simonhong · 2018-08-01T01:35:53Z

@bridiver , Chrome product has same problem with us. We can't run different channel chrome product simultaneously. They don't support it for now. Only one channel app(among stable, beta and dev) is visible in launch pad because they use same id(because of this, sign script patch is introduced). I'm not sure this is bug or intended behavior.
So, we can run only two channel(one of them(stable, beta and dev) and canary) chrome app simultenously. I think canary is treated as a different product. I assume they might use different branding, resources and etc internally for canary.

Without infoplist_string patch, there is no facility for using different product name. To do this, we might have different grd files(such as brave_strings.grd, brave_dev_strings.grd and etc.)

I think this PR is minimal change for supporting multi channel.

RyanJarv · 2018-08-08T02:09:03Z

I think we'll want to add "TEAM_ID=KL8N8XSYF4" in the branding files.

This was added recently for entitlements during signing upstream

Edit: Well I suppose they just added MAC_TEAM_ID=, not sure if we'll want to add our team id here or if that get's set somewhere else.

simonhong · 2018-08-08T07:23:00Z

@RyanJarv Thanks for checking this. I added it to all BRANDING files.

To execute different channel apps simultaneously, each app should have different app name and different id. This PR added different BRANDING files for each channel because Brave on MacOS refers BRANDING file to get unique app name and id.

Each channel app should have different bundle id to install/run them simultaneously.

Brave browser should show product name that includes channel because different channel apps can be run simultaneously. To do this, BRAVE_PRODUCT_STRING is set to CFBundleDisplayName in localized infoplist string instead of using IDS_PRODUCT_NAME only.

This property is newly introduced in C69.

bbondy

lgtm
@bridiver if you have more comments pls just post an issue or ask @simonhong to here and he'll do them as a follow up.

Changes: ``` * cee363d - (44 minutes ago) Merge pull request #314 from brave/revert-sync-master — Pete Miller (HEAD -> master, origin/master, origin/HEAD) |\ | * 0ca4b73 - (56 minutes ago) Revert "sync v2" — Cezar Augusto | * a479cea - (56 minutes ago) Revert "Merge pull request #308 from brave/sync_img" — Cezar Augusto |/ * dd46eab - (19 hours ago) Merge pull request #247 from brave/c71-clock-period — Pete Miller |\ | * 20d2a61 - (7 weeks ago) Fix NTP clock for c71 — petemill (origin/c71-clock-period, c71-clock-period) * | 69a3399 - (19 hours ago) Merge pull request #305 from ryanml/android-rewards-title-fix — Pete Miller |\ \ | |/ |/| | * d92ce8f - (7 days ago) Fixing rewards toggle for long translations — ryanml (ryanml/android-rewards-title-fix, android-rewards-title-fix) |/ * 4692dfd - (25 hours ago) readme typos — Ross Moody * ca3e285 - (29 hours ago) Merge pull request #312 from emerick/rewards-backup-wallet-notification — Nejc Zdovc |\ | * 4dda1bc - (4 days ago) Add support for backupWallet notification — Emerick Rogul * | 0a5f56e - (4 days ago) Emoji Life — Ross Moody * | be16816 - (4 days ago) Readme formatting — Ross Moody * | 35b166d - (4 days ago) Brave UI Logo — Ross Moody * | fddeb8a - (4 days ago) Merge pull request #311 from brave/now-deploy-static — Cezar Augusto |\ \ | * | 6685301 - (5 days ago) Use new static deploy config for now instead of docker — petemill (origin/now-deploy-static, now-deploy-static) * | | eb49e8b - (4 days ago) remove list from README file — Cezar Augusto * | | 59eae8d - (4 days ago) remove outdated docs — Cezar Augusto * | | f04fecc - (4 days ago) do not count first letter if space in textareaclipboard — Cezar Augusto | |/ |/| * | 7ea9178 - (5 days ago) Merge pull request #308 from brave/sync_img — Cezar Augusto |\ \ | * | 470233f - (6 days ago) make sync images exportable — Cezar Augusto | |/ * | aa520cf - (5 days ago) Merge pull request #309 from brave/welcome_img — Pete Miller |\ \ | * | 870d3ba - (6 days ago) animation dial in — Ross Moody (origin/welcome_img, welcome_img) | * | 1c2525b - (6 days ago) make welcome page images exportable — Cezar Augusto | |/ * | 96ccb12 - (6 days ago) Merge pull request #307 from kirkins/patch-1 — Ryan Lanese |\ \ | * | f98a665 - (6 days ago) small typo — Philip Kirkbride * | | f3c38e1 - (6 days ago) v0.36.0 — ryanml (tag: v0.36.0) |/ / * | 46b3501 - (6 days ago) Merge pull request #304 from ryanml/disabled-panel — Nejc Zdovc |\ \ | * | 6be56a3 - (7 days ago) Adding disabledPanel component — ryanml (ryanml/disabled-panel) |/ / * | 79b16fb - (6 days ago) Merge pull request #299 from ryanml/fix-298 — Nejc Zdovc |\ \ | |/ |/| | * c79530c - (12 days ago) Fixes #298, uses string type for amount/tokens — ryanml (ryanml/fix-298, fix-298) * 6bf8054 - (8 days ago) v0.35.0 — Cezar Augusto (tag: v0.35.0, now-deploy-reliable) * 3535427 - (3 weeks ago) sync v2 — Cezar Augusto * 10f07c0 - (9 days ago) Merge pull request #300 from brave/site-banner-icon — Ryan Lanese |\ | * da3cc4b - (11 days ago) Site banner icon update — Ross Moody * 9064f12 - (9 days ago) Merge pull request #301 from emerick/rewards-insufficient-funds-notification — Ryan Lanese * 5300262 - (11 days ago) Add support for insufficientFunds notification — Emerick Rogul ```

…unt/tokens

simonhong added the pr/work-in-progress label Jul 30, 2018

simonhong self-assigned this Jul 30, 2018

simonhong mentioned this pull request Jul 30, 2018

Enable different channel apps run simultaneously on MacOS brave/brave-browser#659

Merged

9 tasks

simonhong changed the title ~~WIP: Use different BRANDING files for each channel on MacOS~~ WIP: Make different channel apps run simultaneously on MacOS Jul 30, 2018

simonhong requested review from RyanJarv, bbondy and bridiver July 31, 2018 05:31

simonhong changed the title ~~WIP: Make different channel apps run simultaneously on MacOS~~ Make different channel apps run simultaneously on MacOS Jul 31, 2018

simonhong removed the pr/work-in-progress label Jul 31, 2018

simonhong commented Jul 31, 2018

View reviewed changes

simonhong force-pushed the multi_channel_install_on_mac branch from 0d042f9 to 92a2d1c Compare July 31, 2018 11:53

bbondy previously approved these changes Jul 31, 2018

View reviewed changes

bridiver reviewed Jul 31, 2018

View reviewed changes

bridiver requested changes Jul 31, 2018

View reviewed changes

simonhong dismissed bbondy’s stale review via 70e64d4 August 1, 2018 03:01

simonhong force-pushed the multi_channel_install_on_mac branch 2 times, most recently from 64826d0 to a0bdef7 Compare August 1, 2018 23:20

simonhong added 6 commits August 8, 2018 18:47

Consider multiple bundle id for signing on MacOS

72e5e4f

Each channel app should have different bundle id to install/run them simultaneously.

Make product name visible to users

15d5a3b

Brave browser should show product name that includes channel because different channel apps can be run simultaneously. To do this, BRAVE_PRODUCT_STRING is set to CFBundleDisplayName in localized infoplist string instead of using IDS_PRODUCT_NAME only.

Override chrome_constants.cc and remove its patch

415627e

Add simple test for changes in chrome_constants.cc

9569752

Add newly added constants to chrome_constants.cc

8dc0b85

Add empty MAC_TEAM_ID to BRANDING files

061c53b

This property is newly introduced in C69.

simonhong force-pushed the multi_channel_install_on_mac branch from 7a24535 to 061c53b Compare August 8, 2018 14:50

bbondy approved these changes Aug 8, 2018

View reviewed changes

bbondy merged commit 1b5fd09 into master Aug 8, 2018

simonhong deleted the multi_channel_install_on_mac branch August 19, 2018 23:22

NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019

Rewards: Imported from brave-ui: Fixes #298, uses string type for amo…

a0a1ca5

…unt/tokens

mkarolin mentioned this pull request Feb 3, 2021

Upgrade from Chromium 88 to Chromium 89. #7460

Merged

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make different channel apps run simultaneously on MacOS #298

Make different channel apps run simultaneously on MacOS #298

simonhong commented Jul 30, 2018 •

edited

Loading

simonhong Jul 31, 2018

bridiver Jul 31, 2018

simonhong Jul 31, 2018

bridiver Jul 31, 2018

bridiver Jul 31, 2018

simonhong Jul 31, 2018

bridiver Jul 31, 2018

bridiver Jul 31, 2018

simonhong Aug 1, 2018

simonhong commented Jul 31, 2018

bridiver Jul 31, 2018

bridiver Jul 31, 2018

bridiver left a comment

bbondy commented Jul 31, 2018

bbondy commented Jul 31, 2018

simonhong commented Aug 1, 2018 •

edited

Loading

RyanJarv commented Aug 8, 2018 •

edited

Loading

simonhong commented Aug 8, 2018

bbondy left a comment

Make different channel apps run simultaneously on MacOS #298

Make different channel apps run simultaneously on MacOS #298

Conversation

simonhong commented Jul 30, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

bbondy commented Jul 31, 2018

bbondy commented Jul 31, 2018

simonhong commented Aug 1, 2018 • edited Loading

RyanJarv commented Aug 8, 2018 • edited Loading

simonhong commented Aug 8, 2018

bbondy left a comment

Choose a reason for hiding this comment

simonhong commented Jul 30, 2018 •

edited

Loading

simonhong commented Aug 1, 2018 •

edited

Loading

RyanJarv commented Aug 8, 2018 •

edited

Loading