Rename Chrome for Testing GN args, GRIT variables and C++ buildflags [chromium/src : main]
Lei Zhang (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Mathias Bynens, Rebekah Potter, Thiago Perrotta.
Patch set 8:Code-Review +1Commit-Queue +2
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, Mathias Bynens, Thiago Perrotta.
2 comments:
File chrome/browser/chrome_for_testing/BUILD.gn:
Patch Set #8, Line 12:
GOOGLE_i think we should remove "GOOGLE" from this -- this knob isn't about selecting a branded product, but rather the CfT feature set. i also like using the same spelling for the GN arg and the buildflag -- this makes it more clear that there's a 1:1 correspondence between the two.
File chrome/common/chrome_constants.cc:
Patch Set #8, Line 16:
#if BUILDFLAG(GOOGLE_CHROME_FOR_TESTING)this looks like a place where we should use GOOGLE_CHROME_FOR_TESTING_BRANDING -- i.e., use "Chromium" here for all unbranded builds, even if the CfT feature set is selected. wdyt?
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Mathias Bynens.
Patch set 8:Auto-Submit +1
2 comments:
File chrome/browser/chrome_for_testing/BUILD.gn:
Patch Set #8, Line 12:
GOOGLE_
i think we should remove "GOOGLE" from this -- this knob isn't about selecting a branded product, bu […]
I couldn't find the supporting CL, but I vaguely remember in the beginning there was some flag or variable named "chrome for testing" and I got a reviewer suggestion to prepend "google" to it. This comment asks me to do the opposite.
From my side, I am indifferent and happy to oblige, but we should have more buy-in from other reviewers, otherwise this just goes back-and-forth.
Will wait until at least one more reviewer agrees (or not) to this change.
File chrome/common/chrome_constants.cc:
Patch Set #8, Line 16:
#if BUILDFLAG(GOOGLE_CHROME_FOR_TESTING)
this looks like a place where we should use GOOGLE_CHROME_FOR_TESTING_BRANDING -- i.e. […]
I see. It makes sense, but let me test it first.
If I remember correctly, this change needs to align with the BRANDING file, otherwise it crashes on macOS. If it passes my local tests I will update it.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
Patch set 9:-Auto-Submit
1 comment:
File chrome/common/chrome_constants.cc:
Patch Set #8, Line 16:
#if BUILDFLAG(GOOGLE_CHROME_FOR_TESTING)
I see. It makes sense, but let me test it first. […]
You're totally right here, and this comment caught a bug. Thanks for your due diligence.
Done.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, Lei Zhang, Mathias Bynens, Thiago Perrotta.
1 comment:
File chrome/browser/chrome_for_testing/BUILD.gn:
Patch Set #8, Line 12:
GOOGLE_
I couldn't find the supporting CL, but I vaguely remember in the beginning there was some flag or va […]
back when we had only one arg for CfT, that made sense. things are different now that we have two args -- one that picks the feature set and can be used in normal Chromium builds, and another that that builds the Google product known as "Google Chrome for Testing" (and requires both src-internal and the first arg). i don't think we should have "Google" in the name for the first arg (we don't), and i think we should have the arg names match the buildflags.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
1 comment:
File chrome/browser/chrome_for_testing/BUILD.gn:
Patch Set #8, Line 12:
GOOGLE_
back when we had only one arg for CfT, that made sense. […]
OK, this is a reasonable argument. Updated.
Should I also update the GN variable right below?
is_chrome_for_testing_branded -> is_google_chrome_for_testing_branded
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, Lei Zhang, Mathias Bynens, Thiago Perrotta.
1 comment:
File chrome/browser/chrome_for_testing/BUILD.gn:
Patch Set #8, Line 12:
GOOGLE_
OK, this is a reasonable argument. Updated. […]
why is life so difficult? :-)
today, we have `is_chrome_branded` -> `GOOGLE_CHROME_BRANDING`. this makes me sad.
i guess i like `is_chrome_for_testing_branded` since it's close to `is_chrome_branded` (and this is what's in the design doc), and then i guess we should have `GOOGLE_CHROME_FOR_TESTING_BRANDING` to go along with it.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
Patch set 14:Auto-Submit +1
1 comment:
File chrome/browser/chrome_for_testing/BUILD.gn:
Patch Set #8, Line 12:
GOOGLE_
why is life so difficult? :-) […]
Then there are no further changes to be done here :-)
Resolving. Time to collect all +1s again
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Thiago Perrotta.
Patch set 14:Code-Review +1
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, Lei Zhang, Thiago Perrotta.
Patch set 14:Code-Review +1
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
Patch set 15:Auto-Submit +1
1 comment:
Patchset:
Unfortunately all +1s will be once again reset because I had to merge the changes incorporated by https://chromium-review.googlesource.com/c/chromium/src/+/4000196
I'd _really_ like to submit it this Monday ASAP, otherwise this will keep happening.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, Lei Zhang, Mathias Bynens, Thiago Perrotta.
1 comment:
File chrome/install_static/install_modes.h:
Patch Set #15, Line 44:
#elif BUILDFLAG(CHROME_FOR_TESTING)i think we want to continue to select on the brand here, not on the feature set.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
1 comment:
File chrome/install_static/install_modes.h:
Patch Set #15, Line 44:
#elif BUILDFLAG(CHROME_FOR_TESTING)i think we want to continue to select on the brand here, not on the feature set.
Sure. Should I change it for this file only, or for all //chrome/install* files?
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
Patch set 16:-Auto-Submit
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Lei Zhang, Mathias Bynens.
1 comment:
File chrome/install_static/install_modes.h:
Patch Set #15, Line 44:
#elif BUILDFLAG(CHROME_FOR_TESTING)
Sure. […]
I'll make an educated guess for the sake of moving forward quickly: Changed for all `//chrome/install*` files (except for `chrome/installer/BUILD.gn` because it should still be based on the feature flag, IMHO).
Note to self: If this is incorrect, revert back to patch set 17.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens.
1 comment:
Patchset:
Updated CL description to make old -> new variable names clearer.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Will Harris (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Thiago Perrotta.
1 comment:
Patchset:
There's a few tests failing on trunk when the CfT gn flag is enabled
see - https://chromium-review.googlesource.com/c/chromium/src/+/4060573
I don't think we should land this CL until we get these failures fixed, and then ensure that the build remains green when landing new CLs (including this one). Without this testing - it's hard to reason about the correctness of any CLs.
For the time being, we can verify this with dependent CLs with the flag enabled and just CQ those, but in the near-term we should try and get an optional trybot.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens.
1 comment:
Patchset:
There's a few tests failing on trunk when the CfT gn flag is enabled […]
Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1394143 to request optional trybots for CfT
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Thiago Perrotta.
Patch set 18:Code-Review +1
Lei Zhang (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Thiago Perrotta.
Patch set 18:Code-Review +1
1 comment:
Commit Message:
Patch Set #18, Line 12:
- Brand: use_internal_chrome_for_testing_icons -> is_chrome_for_testing_brandedWrap at 72 columns.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
John Lee (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Thiago Perrotta.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Will Harris.
Thiago Perrotta removed Rebekah Potter from this change.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, Will Harris.
1 comment:
Commit Message:
Patch Set #18, Line 12:
- Brand: use_internal_chrome_for_testing_icons -> is_chrome_for_testing_brandedWrap at 72 columns.
Done
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Dirk Pranke (Gerrit)
Attention is currently required from: Greg Thompson, Thiago Perrotta, Will Harris.
Patch set 19:Code-Review +1
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta, Will Harris.
3 comments:
File chrome/browser/chrome_browser_main_mac.mm:
if (base::FeatureList::IsEnabled(features::kUseChromiumUpdater)) {
EnsureUpdater(base::DoNothing(), base::DoNothing());
} else {
// This is a no-op if the KeystoneRegistration framework is not present.
// The framework is only distributed with branded Google Chrome builds.
[[KeystoneGlue defaultKeystoneGlue] registerWithKeystone];
}
updater::SchedulePeriodicTasks();should this block be protected by `BUILDFLAG(ENABLE_UPDATER)`?
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}should we have a BUILDFLAG for `enable_mac_installer` to cover this block? wdyt, @ma...@chromium.org?
File chrome/install_static/install_modes.h:
Patch Set #15, Line 44:
#elif BUILDFLAG(CHROME_FOR_TESTING)
I'll make an educated guess for the sake of moving forward quickly: Changed for all `//chrome/instal […]
rule of thumb:
- the `is_chrome_for_testing` knob controls behaviors. when set for a normal unbranded Chromium build, it should disable/enable features, but the generated browser should still be plain-old Chromium.
- the `is_chrome_for_testing_branded` knob controls the branding of the generated browser. this includes strings, icons, the name of the User Data directory, etc.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson, Will Harris.
2 comments:
File chrome/browser/chrome_browser_main_mac.mm:
if (base::FeatureList::IsEnabled(features::kUseChromiumUpdater)) {
EnsureUpdater(base::DoNothing(), base::DoNothing());
} else {
// This is a no-op if the KeystoneRegistration framework is not present.
// The framework is only distributed with branded Google Chrome builds.
[[KeystoneGlue defaultKeystoneGlue] registerWithKeystone];
}
updater::SchedulePeriodicTasks();should this block be protected by `BUILDFLAG(ENABLE_UPDATER)`?
Even if it is, can we look into this in another CL? Ideally this CL should only change variables/flags names (Reasoning: (i) keeps this CL cleaner / focused in doing just one thing (go/small-cls) and (ii) does not introduce yet another contentious point which could reset all +1s once again).
Regarding your question anyway: https://chromium-review.googlesource.com/c/chromium/src/+/4026292 was the last one to touch this build flag, so I'd ask for @waf...@chromium.org's input for that.
File chrome/install_static/install_modes.h:
Patch Set #15, Line 44:
#elif BUILDFLAG(CHROME_FOR_TESTING)
rule of thumb: […]
Then I _believe_ the current implementation is correct, that was my interpretation of the flags as well.
Let me know if you spot any mistakes.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta, Will Harris.
1 comment:
File chrome/browser/chrome_browser_main_mac.mm:
if (base::FeatureList::IsEnabled(features::kUseChromiumUpdater)) {
EnsureUpdater(base::DoNothing(), base::DoNothing());
} else {
// This is a no-op if the KeystoneRegistration framework is not present.
// The framework is only distributed with branded Google Chrome builds.
[[KeystoneGlue defaultKeystoneGlue] registerWithKeystone];
}
updater::SchedulePeriodicTasks();
Even if it is, can we look into this in another CL? Ideally this CL should only change variables/fla […]
one piece of feedback earlier was that we should try to avoid sprinking "if (!)CfT" around the codebase when it makes sense to add knobs for features and then have the CfT switch set those features as appropriate. my suggestion here is in line with that feedback, and i don't think it would cause you to lose +1s since we have `ENABLE_UPDATER` in `//chrome/browser:buildflags` -- no new files are needed, no?
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Will Harris (Gerrit)
Attention is currently required from: Thiago Perrotta.
1 comment:
Patchset:
Filed https://bugs.chromium. […]
re: this blocker
I patched out the totally failing mac_signing_tests and run the CQ, it seems like there are fewer (maybe? different?) tests failing now, especially on mac_rel.
https://chromium-review.googlesource.com/c/chromium/src/+/4060573
Purely from the angle of "being happy for CfT CLs to continue landing", I would consider patching out mac_signing_tests "for the time being" in a dependent patchset and having green bots before/after as sufficient for landing this CL and others, and unblocking development.
Clearly we would need some sort of automated try/testing/CI in the longer term, but that work can continue in parallel, assuming we can coax a green CQ run from a patchset like 4060573.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Dirk Pranke (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 20:Code-Review +1
1 comment:
Patchset:
It sounds like we're still on the fence as to what to do in this CL.
It seems like there is a clear long-term solution, which is to set up CfT bots
that can have different flags and run different sets of tests.The question is, as I understand it, what to do in the short term, since mac_signing_tests fails with CfT enabled. Here we have a couple options:
1. disable mac_signing_tests just in this CL (on mac-rel) to show that everything else is green.
2. figure out how to get the tests to pass with GN wizardry.
3. Wait for the bots to be set up properly.
I don't know if folks (other than wfh) are fine with (1), so maybe others can weigh in on that. I can help with (2), but it'd be throwaway work if the bots can be set up in a couple days. I can also help with (3) tomorrow.
So, I'd vote for (1) + (3), and not do (2) unless we have to for some reason I'm not aware of yet.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Mark Mentovai.
1 comment:
File chrome/browser/chrome_browser_main_mac.mm:
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
should we have a BUILDFLAG for `enable_mac_installer` to cover this block? wdyt, @mark@chromium. […]
CC'ed and added to attention set (not sure if Gerrit mentions alone sends emails)
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson, Mark Mentovai.
1 comment:
File chrome/browser/chrome_browser_main_mac.mm:
if (base::FeatureList::IsEnabled(features::kUseChromiumUpdater)) {
EnsureUpdater(base::DoNothing(), base::DoNothing());
} else {
// This is a no-op if the KeystoneRegistration framework is not present.
// The framework is only distributed with branded Google Chrome builds.
[[KeystoneGlue defaultKeystoneGlue] registerWithKeystone];
}
updater::SchedulePeriodicTasks();
one piece of feedback earlier was that we should try to avoid sprinking "if (!)CfT" around the codeb […]
Your suggestion is totally valid, and I agree with it, it's just that it pollutes the CL with a change that is more than just renaming, making it harder to reason about it and invalidating its "no-op, just refactoring" aspect.
If we have to revert this suggestion for some reason (e.g. it breaks something), then we shouldn't have to revert the whole renaming refactoring (and, especially, collect all needed +1s again).
I am spinning this off to a separate CL for the sake of purity: https://chromium-review.googlesource.com/c/chromium/src/+/4080862
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
File chrome/browser/chrome_browser_main_mac.mm:
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
CC'ed and added to attention set (not sure if Gerrit mentions alone sends emails)
Note: This is related to https://chromium-review.googlesource.com/c/chromium/src/+/4080862 (which was created as per another comment of Greg)
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Patchset:
re: this blocker […]
It will take a long time, likely O(weeks), to get a fully green CQ. Back-fixing the tests is reasonable and the right thing to do, however it could (and arguably should, but that's just my opinion) be done in parallel.
I personally still think that blocking the submission of this CL is a mistake and adds confusion and unnecessary context switching to my side (as I need to rename the GN args back-and-forth), however I prefer to spend my energy in fixing the tests and moving forward in other ways rather than trying to (possibly endlessly) argue my side.
This CL is effectively a no-op refactoring. We shouldn't be afraid of submitting no-op CLs, when the CQ is already red.We could initiate a whole separate discussion in how the CQ became red in the first place, the summary is that we didn't have CfT trybots before so the tests weren't exhaustively monitored/tested - the credits to catching them are all yours
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens.
1 comment:
Patchset:
I'll retire this CL otherwise the +1s will keep being reset and this will just waste everyone's time. I just really wish I could submit it today.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
File chrome/common/chrome_constants.cc:
Patch Set #8, Line 16:
#if BUILDFLAG(GOOGLE_CHROME_FOR_TESTING)You're totally right here, and this comment caught a bug. Thanks for your due diligence. […]
This diff got lost in this CL because of the variable renaming, but it actually fixes a bug.
I sent https://chromium-review.googlesource.com/c/chromium/src/+/4100712 separately to submit it and unblock by fixing more of the macOS test failures.
Once that CL is submitted, and this one rebases atop it, I will need to rename this comment back to `GOOGLE_CHROME_FOR_TESTING_BRANDING` again.
Keeping this unresolved to remind me to do so.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Greg Thompson, Thiago Perrotta.
1 comment:
File chrome/browser/chrome_browser_main_mac.mm:
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
Note: This is related to https://chromium-review.googlesource. […]
Re `enable_mac_installer`: no, we don’t really want a build flag for that.
The first half of this should be `BUILDFLAG(ENABLE_UPDATER)` as it is now. For the second half of this, regarding installation from a read-only disk image, it’s fine to test `BUILDFLAG(CHROME_FOR_TESTING)` directly. This is one of those weird rare one-offs where there isn’t really a generic feature that can be extracted and used as the gate.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Mark Mentovai.
1 comment:
File chrome/browser/chrome_browser_main_mac.mm:
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
Re `enable_mac_installer`: no, we don’t really want a build flag for that. […]
Do you mean to wrap up the inner if? If so, then it's done now.
```
+#if !BUILDFLAG(CHROME_FOR_TESTING)
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
+#endif // BUILDFLAG(CHROME_FOR_TESTING)
```
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
File chrome/common/chrome_constants.cc:
Patch Set #8, Line 16:
#if BUILDFLAG(GOOGLE_CHROME_FOR_TESTING)
This diff got lost in this CL because of the variable renaming, but it actually fixes a bug. […]
Done
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Greg Thompson, Thiago Perrotta.
4 comments:
File chrome/browser/chrome_browser_main_mac.mm:
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
Do you mean to wrap up the inner if? If so, then it's done now.
```
+#if !BUILDFLAG(CHROME_FOR_TESTING)
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
+#endif // BUILDFLAG(CHROME_FOR_TESTING)
```
No, but comments in-line showing what I meant. Hopefully that’ll be clearer.
File chrome/browser/chrome_browser_main_mac.mm:
Here, write:
```
#endif // BUILDFLAG(ENABLE_UPDATER)#if !BUILDFLAG(CHROME_FOR_TESTING)
```Patch Set #30, Line 106:
#if !BUILDFLAG(CHROME_FOR_TESTING)Get rid of this.
#endif // BUILDFLAG(CHROME_FOR_TESTING)
#endif // BUILDFLAG(ENABLE_UPDATER)And this becomes:
```
#endif // !BUILDFLAG(CHROME_FOR_TESTING)
```
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson.
4 comments:
File chrome/browser/chrome_browser_main_mac.mm:
// Disk image installation is sort of a first-run task, so it shares the
// no first run switches.
//
// This needs to be done after the resource bundle is initialized (for
// access to localizations in the UI) and after Keystone is initialized
// (because the installation may need to promote Keystone) but before the
// app controller is set up (and thus before MainMenu.nib is loaded, because
// the app controller assumes that a browser has been set up and will crash
// upon receipt of certain notifications if no browser exists), before
// anyone tries doing anything silly like firing off an import job, and
// before anything creating preferences like Local State in order for the
// relaunched installed application to still consider itself as first-run.
if (!first_run::IsFirstRunSuppressed(
*base::CommandLine::ForCurrentProcess())) {
if (MaybeInstallFromDiskImage()) {
// The application was installed and the installed copy has been
// launched. This process is now obsolete. Exit.
exit(0);
}
}
> Do you mean to wrap up the inner if? If so, then it's done now. […]
Ack
File chrome/browser/chrome_browser_main_mac.mm:
Here, write: […]
Done
Patch Set #30, Line 106:
#if !BUILDFLAG(CHROME_FOR_TESTING)Get rid of this.
Done
#endif // BUILDFLAG(CHROME_FOR_TESTING)
#endif // BUILDFLAG(ENABLE_UPDATER)
And this becomes: […]
Done
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Will Harris.
1 comment:
Patchset:
This is ready to collect +1s again. Hopefully no more unforeseen blockers will appear.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Will Harris.
1 comment:
Patchset:
It will take a long time, likely O(weeks), to get a fully green CQ. […]
At this point, pretty much most tests are fixed: go/cft-problematic-tests
The two open ones - gpu_tests and installer_test - have nothing to do with this CL and may take longer to fix than the already fixed ones.
I can only hope to count on good faith to not remain unblocked on this CL anymore because of the tests.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Will Harris.
1 comment:
Patchset:
At this point, pretty much most tests are fixed: go/cft-problematic-tests […]
remain blocked**
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Will Harris.
1 comment:
Patchset:
Leaving `Commit: False` for now, due to:
Running Python 3 presubmit upload checks ...
** Presubmit ERRORS: 1 **
There is a prod freeze in effect from 2022/12/16 08:00 +0000 until 2023/01/02 08:00 +0000, files in //tools/mb cannot be modified
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Will Harris.
Patch set 41:Commit-Queue +1
1 comment:
Patchset:
Leaving `Commit: False` for now, due to: […]
Done
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Mathias Bynens, Thiago Perrotta, Will Harris.
1 comment:
File chrome/test/BUILD.gn:
Patch Set #41, Line 2379:
if (is_chrome_branded) {isn't this duplicating line 4629?
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Will Harris.
1 comment:
File chrome/test/BUILD.gn:
Patch Set #41, Line 2379:
if (is_chrome_branded) {isn't this duplicating line 4629?
Bad merge. Fixed!
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Mathias Bynens, Thiago Perrotta, Will Harris.
Patch set 42:Code-Review +1
1 comment:
Patchset:
please wait for mmentovai to take a look at the changes he's asked for. thanks.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Will Harris.
1 comment:
Patchset:
please wait for mmentovai to take a look at the changes he's asked for. thanks.
moved to R= then (@ma...@chromium.org FYI)
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Dirk Pranke (Gerrit)
Attention is currently required from: John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Thiago Perrotta, Will Harris.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Thiago Perrotta, Will Harris.
Set Ready For Review
Mark Mentovai (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mathias Bynens, Thiago Perrotta, Will Harris.
Patch set 44:Code-Review +1
1 comment:
Patchset:
LGTM. I only re-reviewed chrome_browser_main_mac.mm. Thanks.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Mathias Bynens, Thiago Perrotta, Will Harris.
1 comment:
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)as mentioned in the installer test CL, this is branching on the feature set rather than the brand. i would argue that `chrome/install_static/google_chrome_for_testing_install_modes.h` should be conditional on the brand rather than the feature set, in which case this should be `GOOGLE_CHROME_FOR_TESTING_BRANDING`.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Thiago Perrotta, Will Harris.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Will Harris.
2 comments:
Patchset:
Update: The try bots are ~almost green:
- mac is effectively passing
- linux is effectively passing (currently there's one failing test, that needs to be filtered out)
- windows is effectively passing, with remarks
- the installer test still needs to be fixed (WIP, see the other open comment)
- we do not care about telemetry tests for cft
- all other tests have already been filtered out (e.g. https://source.chromium.org/search?q=-f:%5Egen%2F%7C%5Eout%2F%20BrowserFocusTest.AppLocationBar) - but there must be some issue with the `interactive_ui_tests` filter, because they are still showing up as red in the CI
For the sake of being practical and not lose more time I propose we just go ahead and submit this CL, knowing that the only known failure(s) we _actually_ care about is the installer test (which is WIP, and can be done orthogonally, without the need to block this CL).
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)
as mentioned in the installer test CL, this is branching on the feature set rather than the brand. […]
Hmm, we _never_ want Chrome for Testing to be able to be set as the default browser, regardless whether it's using the internal branding or not.
I'd argue the current form of the unit test is correct (i.e. depending on the feature set), and if the test is failing then it's the implementation that needs to change.
That said, for the sake of minimizing friction in this CL I'll go ahead and address your suggestion, but it may be revisited in the future (in a smaller CL that only touches the installer files).
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Will Harris.
1 comment:
Patchset:
but there must be some issue with the interactive_ui_tests
OK, I found what the issue is. Will send a fix tomorrow. If all goes as predicted, only the installer-related tests will remain in the failed tests log.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Will Harris (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Thiago Perrotta.
1 comment:
Patchset:
> but there must be some issue with the interactive_ui_tests […]
(re: win tests)
if I recall correctly, telemetry tests are the only tests that actually launch and run chrome.exe - the others are launching test binaries.
do we know why the telemetry tests are failing?
are you planning to fix the installer test before landing this CL?
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Will Harris (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Thiago Perrotta.
1 comment:
Patchset:
(re: win tests) […]
(re: other tests)
I took a look at the output for the three cft trybots and I am seeing quite a lot of unusual failures, I'm not even sure they are running correctly:
e.g. for browser_tests, they are just failing immediately with "[0111/032932.971453:ERROR:test_launcher.cc(1401)] Failed to read the filter file."
e.g. for blink_wpt_tests and blink_web_tests they seem to be failing immediately with "Unable to find test driver"
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Pawlicki (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Thiago Perrotta, Will Harris.
1 comment:
Patchset:
(re: other tests) […]
FWIW I suggest we don't take shortcuts and get the bots green before trying to proceed with code-side changes. Even if the changes are reasonably safe, the extra comms and overhead of double-checking the red bots (which seem to be failing in an unexpected way, as Will points out) is unnecessary stress on the project and contributors.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
Patchset:
quite a lot of unusual failures,
those were fixed by https://chromium-review.googlesource.com/c/chromium/src/+/4161594 and were the same issue that I had previously mentioned
are you planning to fix the installer test before landing this CL?
ideally no, but it seems that I have no other choice, so yes (CLs currently WIP)
will change this CL from "WIP" to active again once it's ready, but my original intent was to collect all "+1s" in advance to speed up
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)
Hmm, we _never_ want Chrome for Testing to be able to be set as the default browser, regardless whet […]
Done
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Thiago Perrotta, Will Harris.
Set Ready For Review
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Thiago Perrotta, Will Harris.
1 comment:
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)
Done
re: "we never want Chrome for Testing to be able to be set as the default browser, regardless whether it's using the internal branding or not." i disagree. whether or not a build can be made default is a branding issue. building Chromium with the CfT feature set is still a build of Chromium.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Will Harris.
1 comment:
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)
re: "we never want Chrome for Testing to be able to be set as the default browser, regardless whethe […]
The main motivation for this is Security, not Branding (c.f. https://docs.google.com/document/d/1XJvxyqAQjhPfJ0rX84PjfXXb5sBx3m8DXzMxR0ipQNs/edit#bookmark=id.22u0z977iy40)
Anyway: From my perspective I am just stating this for completeness; no extra action is needed here.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Dirk Pranke, John Lee, Lei Zhang, Mark Mentovai, Mathias Bynens, Thiago Perrotta, Will Harris.
2 comments:
File chrome/install_static/install_util_unittest.cc:
Patch Set #53, Line 312:
#elif BUILDFLAG(GOOGLE_CHROME_FOR_TESTING_BRANDING)CfT does not use a distinct policies key from Google Chrome, so we should change line 309 to `#if BUILDFLAG(GOOGLE_CHROME_BRANDING) || BUILDFLAG(GOOGLE_CHROME_FOR_TESTING_BRANDING)`
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)
The main motivation for this is Security, not Branding (c.f. https://docs.google. […]
for security of the shipping CfT product, not for security of a chromium build for use by developers to work on features of CfT. this is why i assert that it's a property of the brand rather than of the feature set.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Dirk Pranke, Greg Thompson, John Lee, Joshua Pawlicki, Lei Zhang, Mark Mentovai, Mathias Bynens, Will Harris.
3 comments:
Patchset:
> quite a lot of unusual failures, […]
I will split this CL into two, one for each flag, in order to try to make it easier to review and to move forward (go/small-cls).
The first CL is: https://chromium-review.googlesource.com/c/chromium/src/+/4172729
Unfortunately it still requires the review of 5 distinct OWNERS, but at least it's smaller
File chrome/install_static/install_util_unittest.cc:
Patch Set #53, Line 312:
#elif BUILDFLAG(GOOGLE_CHROME_FOR_TESTING_BRANDING)
CfT does not use a distinct policies key from Google Chrome, so we should change line 309 to `#if BU […]
Done
File chrome/installer/util/shell_util_interactive_uitest.cc:
Patch Set #44, Line 167:
#if BUILDFLAG(CHROME_FOR_TESTING)
for security of the shipping CfT product, not for security of a chromium build for use by developers […]
Ack
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Thiago Perrotta abandoned this change.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Thiago Perrotta restored this change.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Thiago Perrotta abandoned this change.
To view, visit change 4027840. To unsubscribe, or for help writing mail filters, visit settings.