Support multi channel release on Windows

[simonhong]

Use different user dir and app icon on Windows.
To do this, some dependency rule is refactored in first commit.
Please see each commit's description.

Issue: brave/brave-browser#10

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:

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]

Just copied guids from upstream. which value should I use?

[bbondy]

You should use the ones above as I created all new IDs for them.

[bbondy]

And made sure other places that used those IDs were also updated.

[bbondy]

I think when we compile with no brave chromium override we could skip the cc_wrapper check for override files. That way we don't need to maintain separate chrome stuff in our src/brave dir.

[simonhong]

Oops.. I didn't know about our cc_wrapper.
That script replaces original file with our files when same file exists.
So, my thought was wrong - I changed PR description.

install_static_util target includes our version and other targets that depends on //brave:browser_dependencies also includes ours , too.

I want to make only install_static_util target dependent on our chromium_install_modes.* files.
We should make install_static_util depends on directly our chromium_install_modes files.
If not, that target build isn't triggered by modifying our files.

I'm not sure another ways is exists to solve this problem.

Regarding to guid, should I use one of IDs above you created in official build guard? or need to generate new one?

[simonhong]

When we use our redirect-cc.py, there is one problem - dependency check.
To trigger re-build, original file should be touched if we don't add additional dependency.
In install_static_util case, we added another dependency to brave:browser_dependencies.
How about making dependency directly to the target like below instead of using cc-wrapper?

else {
  if (brave_chromium_build) {
    sources += [
      "//brave/chromium_src/chrome/install_static/chromium_install_modes.cc",
      "//brave/chromium_src/chrome/install_static/chromium_install_modes.h",
    ]
  } else {
    sources += [
      "chromium_install_modes.cc",
      "chromium_install_modes.h",
    ]
  }

With this, I think we don't need to introduce another dependency and can know dependency easily instead of adding another dependencies into some other places.
I think this kind of modification can be rebased easily.

[bbondy]

Regarding to guid, should I use one of IDs above you created in official build guard? or need to generate new one?

No they were already all newly generated, no need to generate any new IDs here.

[bbondy]

Good point about the dependencies. The whole reason to introduce cc_wrapper hack was to avoid having to patch extra gn files with the real source path though. Maybe we can just have the dev changing these files touch the original ones too, the overrides should very rarely be used.

@bridiver thoughts?

[simonhong]

@bbondy , updated. Please check again.
In addition, I added some missed dependency of our copied chrome_icon_resources_win.h.

[bridiver]

I think in an ideal world we would have a way to reference gn targets and add/remove source files without actually modifying them. For the cc_wrapper files we can create a dummy script target and mark them as inputs to a no-op script or something like that

[bridiver]

I don't think this is going to be sufficient because we'll also have beta, etc.. why wouldn't we just modify the branding_path_component in the args.gn?

[simonhong]

Upstream chrome uses two kinds of brand strings - google_chrome and chromium.
For specific channel support like beta or dev, they handled under google_chrome.
So, I think introducing brave-development or similar name for unofficial build.
We should cleanup/refactoring to use both 'braveandbrave-development` in other places.

[bridiver]

ok, I think I understand what you're trying to do here, but I still think we should modify the branding_path_component here https://github.com/brave/brave-browser/blob/master/lib/config.js#L51 like we do in muon. It was only commented out as a temporary workaround and I'm not sure that it's a good idea to modify it here

[simonhong]

Yep, I agree with you.
How about cleanup/refactoring it as a separate PR?
If we change branding_path_component in args.gn for now, many places would be broken such as brave_strings.grd and mostly under app/theme/.

In the separate PR, we can start it by below in chrome_build.gni

if (brave_chromium_build) {
  if (is_official_build) {
    branding_path_component = "brave"
  } else {
    branding_path_component = "brave-development' # or "brave-dev"
  }
} else {
  if (is_chrome_branded) {
    branding_path_component = "google_chrome"
  } else {
    branding_path_component = "chromium"
  }
}

[bridiver]

can you create an issue for it and I'll remove the "changes requested"?

[simonhong]

brave/brave-browser#153 is created for it.

[bridiver]

looks like I can't directly remove my request for changes, so just ignore it :)

[simonhong]

@bridiver , can you confirm this PR? or should I wait more from other reviewers?

[bridiver]

see branding_path_component comment

[bridiver]

I thought we had most of this stuff sorted out already? We left the name as chrome early on to avoid other issues and renamed later I think. @AlexeyBarabash I think you did the earlier work on this?

[simonhong]

I tested on official/unofficial/component build with this change and and works well :)

[bridiver]

if this isn't already handled by @AlexeyBarabash's work, I'd still prefer to rename in a brave build step vs patching here, again unless there is some other compelling reason to change it

[bridiver]

Do we need to make this change? I don't think we care if the filename is chrome.dll vs brave.dll. If that's the only reason we are changing it then I think we should leave it alone, or is there something else?

[bridiver]

I think there are also more references to chrome.dll and chrome_child.dll that will break with this change.

[simonhong]

Yep, it would be happen if we try to build all targets. I just tested only brave target.
I'll revert to use chrome.dll.

[bridiver]

I don't see any reason to patch the old/new exes because they get renamed. Only the final brave.exe matters

[simonhong]

Yep, I also don't care about the name of dll.
If we only want to use brave.exe, I'll revert them to chrome.dll and chrome_child.dll.

[bridiver]

Same thing here with chrome/brave. Unless there is a compelling reason to rename these dlls I don't think we should

[bridiver]

I believe brave_command_ids.h is included in chrome_command_ids.h and only contains brave-specific ids so most of the ones listed here won't be there. This is one of the few cases where I think it's better to patch (vs large-scale copying)

[simonhong]

All comments are addressed.

[bridiver]

this patch is not needed. The header file in chromium_src will automatically override the one in src/chrome

[simonhong]

IIRU, dependency problem was happened. I'll check this again!

[bridiver]

this is also not necessary for the same reason as above