Integrate Brave omaha #109

simonhong · 2018-05-21T14:34:16Z

Testing with https://github.com/brave/omaha

When refresh settings/help, BraveUpdate.exe process runs and terminated.

Work together with brave/brave-browser#303.

Closes: brave/brave-browser#179

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

bridiver · 2018-06-14T21:02:38Z

patches/chrome-installer-util-util_constants.cc.patch

+#if defined(BRAVE_CHROMIUM_BUILD) && defined(OFFICIAL_BUILD)
+// Reflect brave branded-omaha variable name.
+// Company name(BraveSoftware) + Product name(Update).
+const char kGoogleUpdateIsMachineEnvVar[] = "BraveSoftwareUpdateIsMachine";


is this necessary? I think it would be better to have consistent behavior for this env var with Chrome

This var name comes from omaha.
To customize omaha for brave, company name should be changed to BraveSoftware because it is related with install directory. Then omaha makes env var by using it.
So, I think we should follow omaha var name in here.

can be done with the later string substitution method.

bridiver · 2018-06-14T21:05:07Z

patches/components-update_client-updater_state_win.cc.patch

+const wchar_t kGoogleUpdatePoliciesKey[] =
+    L"SOFTWARE\\Policies\\BraveSoftware\\Update";
+const wchar_t kCheckPeriodOverrideMinutes[] = L"AutoUpdateCheckPeriodMinutes";
+const wchar_t kUpdatePolicyValue[] = L"UpdateDefault";


most of these values look identical to the google versions. Seems like it would be better to use the undefine/redefine override. @bbondy can help with that

Is it good way to use def/undef override for variable change?
I think the result would be same with the copying.

const overriding should work just like method overriding. same def/undef applies

why would the result be the same as copying if we're only changing some of the values?

I referred browser_about_handler.cc in chromium_src.
In that file, upstream method name was changed and reused with additional implementation(FixupBrowserAboutURLBraveImpl).

I don't know how to adopt this technique to this file w/o changing functions implementation in this file.
Do you mean variable like re-define kGoogleUpdatePoliciesKey to kGoogleUpdatePoliciesKey_ChromiumImpl?

I think I don't know how to adopt undefine/redefine technique here.

but whether we patch or do something else, we only need the values we are overriding. We should not include things like kCheckPeriodOverrideMinutes or kCheckPeriodOverrideMinutesMax that we are not changing

does that work correctly? It doesn't seem like it would

It doesn't work

Do you want to apply override technique like this (b23256e) to other files in this PR?

well, it won't work if they're in an anonymous namespace like that. There's not way to reference/override them afaik

Ok, I'll revert b23256e to patch, and will try override to non-anonymous symbols.

bridiver · 2018-06-14T21:06:25Z

patches/chrome-installer-util-google_update_settings.cc.patch

+ using installer::InstallationState;
+
+ const wchar_t GoogleUpdateSettings::kPoliciesKey[] =
+#if defined(BRAVE_CHROMIUM_BUILD) && defined(OFFICIAL_BUILD)


same as below undefine/redefine override

Can you advice how to override variable? I still don't have any idea how to do it with separate file in chormium_src.

I think it's difficult to use override technique to member var/method.

I tried to override ClassA::MethodA, in new class_a.cc in chromium_src like below.

#define MethodA MethodA_ChromiumImpl #include ".../class_a.cc" #undef MethodA void ClassA::MethodA() { ... }

Compiler will complain like this error: no member named MethodA in ClassA.
I think the reason is header file(class_a.h) included in this file was also affected by macro.
So, there is no MethodA in this file.
Overriding member variable will have same error.
@bbondy

replied below.

bridiver · 2018-06-14T21:07:25Z

patches/chrome-installer-mini_installer-appid.h.patch

+diff --git a/chrome/installer/mini_installer/appid.h b/chrome/installer/mini_installer/appid.h
+index c408242814504aae086fc332f0d8bbd7872821a2..deadf14c9c8a17d7d83fcf3428cd3734a92d70b1 100644
+--- a/chrome/installer/mini_installer/appid.h
+++ b/chrome/installer/mini_installer/appid.h


why are we modifying this when we already have brave_appid?

Done.
appid.h is copied into chromium_src.

bridiver · 2018-06-14T21:07:54Z

patches/chrome-installer-mini_installer-BUILD.gn.patch

+     "mini_installer.h",
+     "mini_installer.rc",
+-    "mini_installer_constants.cc",
+    "//brave/installer/mini_installer/mini_installer_constants.cc",


if we are replacing the entire file it's better to use chromium_src which will work automatically without any patching

Done.
mini_installer_constants.cc is moved into chromium_src.

bridiver · 2018-06-14T21:13:01Z

patches/chrome-installer-util-google_update_settings.cc.patch

+
+ const GoogleUpdateSettings::UpdatePolicy
+ GoogleUpdateSettings::kDefaultUpdatePolicy =
+-#if defined(GOOGLE_CHROME_BUILD)


are we doing this for all GOOGLE_CHROME_BUILD guards in this file? If so it would be easier to just define GOOGLE_CHROME_BUILD if BRAVE_CHROMIUM_BUILD && OFFICIAL_BUILD and then undef at the end

I'm worrying about that situation when upstream add more GOOGLE_CHROME_BUILD guard in this file and it isn't related with brave. WDYT?

isn't the reverse just as likely?

done with #define GOOGLE_CHROME_BUILD

bridiver · 2018-06-14T21:14:34Z

patches/chrome-installer-util-google_chrome_distribution.cc.patch

+ }
+
+ std::string GoogleChromeDistribution::GetSafeBrowsingName() {
+#if defined(BRAVE_CHROMIUM_BUILD)


why is this needed?

bridiver · 2018-06-14T21:14:51Z

patches/chrome-installer-util-google_chrome_distribution.cc.patch

+   return base::ReplaceStringPlaceholders(url, language, NULL);
+ }
+
+// TODO(simonhong): Use brave's url.


please don't patch for comments. Open a ticket instead

bridiver · 2018-06-14T21:16:05Z

patches/chrome-installer-util-browser_distribution.cc.patch

+
+-#if defined(GOOGLE_CHROME_BUILD)
+#if defined(GOOGLE_CHROME_BUILD) || (defined(BRAVE_CHROMIUM_BUILD) && defined(OFFICIAL_BUILD))
+   dist = GetOrCreateBrowserDistribution<GoogleChromeDistribution>(


can't we just patch here to create a BraveDistribution and get rid of most of the patches below?

After addressing your review, all modification of GoogleChromeDistribution is gone.
So, just enabling this seems fine instead of creating BraveDistribution because we are using GoogleChromeDistribution w/o any change.

bridiver · 2018-06-14T21:17:57Z

patches/chrome-install_static-install_modes.cc.patch

+   return std::wstring(L"Software\\").append(product);
+ }
+
+#if defined(BRAVE_CHROMIUM_BUILD)


override with undefine/redefine

Can't do for functions that are used by other functions in the same file.
Also, this is in anonymous namespace.

you can do it for functions that are used by other functions in the same file, but you would also have to do the functions that use that function. You can just only do the functions that use that function I think in this case.

Left some comment for the record.
This is the reason why patching technique is used instead of overriding in this case.
In case of install_modes.cc, override technique causes modifying all functions in that file and can't reuse upstream functions. It is similar to copying technique.
IMO, patching technique is more better to copying for future rebase.
I thought def/undef technique is only useful when we can re-use upstream implementation

bridiver · 2018-06-14T21:19:03Z

patches/chrome-browser-ui-BUILD.gn.patch

+         "webui/settings/incompatible_applications_handler_win.h",
+       ]
+       deps += [ "//google_update" ]
+    } else if (brave_chromium_build && is_official_build) {


can't we just add these in our own gn file?

like you did here https://github.com/brave/brave-core/pull/109/files#diff-84f8700538180eec90bb2f4c9dff4da4R42

bridiver · 2018-06-14T21:37:02Z

win_build_output/midl/google_update/x64/google_update_idl.h

@@ -0,0 +1,5747 @@
+
+
+/* this ALWAYS GENERATED file contains the definitions for the interfaces */


how is this file generated?

During the build, midl.py generates these file in temp directory and compares it with pre-installed version.

then why are we copying the output? Shouldn't we be modifying the inputs so we get the correct output?

Modifying input files in src/third_party/win_bulid_output causes creating a lot of patch files.
So, I decided to use copying from our directory

brave/brave-browser#303 handles this.

Enable Omaha connection with UpgradeDetector and VersionUpdater. Also, guids and registry pathes are changed to use brave branding.

TODO: uninstall survey url should be changed.

Our pre-generated midl files are copied to overwrite upstream pre-installed midl files.

Moves the IDL file into chromium_src, patches BUILD.gn to use chromium_src override. IDL output rebuilt at `./src` directory using `python3 ./tools/win/update_idl.py` I used #109 as a guide

Moves the IDL file into chromium_src, patches BUILD.gn to use chromium_src override. IDL output rebuilt at `./src` directory using `python3 ./tools/win/update_idl.py` Using #109 as a guide Also includes `.gitattributes` fix which prevents differences found w/ filecmp.cmpfiles in MIDL compilation.

simonhong self-assigned this May 21, 2018

simonhong added the pr/work-in-progress label May 21, 2018

simonhong force-pushed the integrate_omaha branch 3 times, most recently from 0e71e58 to 12564dd Compare May 30, 2018 02:58

simonhong force-pushed the integrate_omaha branch 2 times, most recently from 4e7777a to 3aa703a Compare June 7, 2018 15:53

simonhong force-pushed the integrate_omaha branch 2 times, most recently from 6587bdc to 0c13a11 Compare June 14, 2018 05:00

simonhong changed the title ~~WIP: Integrate Brave omaha~~ Integrate Brave omaha Jun 14, 2018

simonhong removed the pr/work-in-progress label Jun 14, 2018

simonhong requested review from bbondy, bridiver and RyanJarv June 14, 2018 20:22

bridiver reviewed Jun 14, 2018

View reviewed changes

simonhong added 4 commits June 18, 2018 12:56

Initial Omaha integration to brave core

f4eaa77

Enable Omaha connection with UpgradeDetector and VersionUpdater. Also, guids and registry pathes are changed to use brave branding.

Change guids and reg pathes in installer

e5f6fca

Use GoogleChromeDistribution for official build

36ee9e3

TODO: uninstall survey url should be changed.

Rebase on C67 and refactoring the handling of generated midl files

18a660c

Our pre-generated midl files are copied to overwrite upstream pre-installed midl files.

bsclifton added a commit that referenced this pull request Nov 13, 2023

Fixing up IDL files

18b027b

Moves the IDL file into chromium_src, patches BUILD.gn to use chromium_src override. IDL output rebuilt at `./src` directory using `python3 ./tools/win/update_idl.py` I used #109 as a guide

		@@ -0,0 +1,5747 @@


		/* this ALWAYS GENERATED file contains the definitions for the interfaces */

Integrate Brave omaha #109

Integrate Brave omaha #109

Conversation

simonhong commented May 21, 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

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 Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

bridiver Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

simonhong Jun 14, 2018 • edited Loading

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

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 Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong Jun 27, 2018 • edited Loading

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 Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong commented May 21, 2018 •

edited

Loading

simonhong Jun 14, 2018 •

edited

Loading

bridiver Jun 14, 2018 •

edited

Loading

simonhong Jun 14, 2018 •

edited

Loading

simonhong Jun 20, 2018 •

edited

Loading

simonhong Jun 27, 2018 •

edited

Loading

simonhong Jun 19, 2018 •

edited

Loading