Add a persistent infobar warning when launching Chrome for Testing [chromium/src : main]
Thiago Perrotta (Gerrit)
Attention is currently required from: Maksim Sadym.
1 comment:
File chrome/app/generated_resources.grd:
Patch Set #17, Line 12563:
This version of Chrome doesn't update automatically and you shouldn't use it unless you're a developer testing new features.nit: Since this is a UI text string, we should follow go/punctuation-standards and use proper apostr […]
Done
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sadym (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 19:Code-Review +1
Thiago Perrotta (Gerrit)
1 comment:
Patchset:
The review of the wording is happening here: http://doc/1kVPKTPn0pnJa2OAL9_L7Ei8A2C4zjzYO8lQFblztdLI
Update: wording discussion is still happening.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 22:Code-Review +1
1 comment:
Patchset:
Christian, Greg, I know you’ve both reviewed and discussed this CL. Could you please LGTM/no-LGTM?
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
Patchset:
Update: wording discussion is still happening.
Latest mock lives here: https://bugs.chromium.org/p/chromium/issues/detail?id=1336611#c56
If this wording becomes final, I'll follow up by internationalizing the "Download Chrome" string.
Do we also need to extract the download URL to the GRD file?
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Christian Flach (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 23:Code-Review +1
1 comment:
Patchset:
This looks good to me. A test would be nice, but I'm not sure if there are even builders that run with `BUILDFLAG(GOOGLE_CHROME_FOR_TESTING_BRANDING)`, so no idea if that is feasible.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
4 comments:
File chrome/app/generated_resources.grd:
Patch Set #23, Line 12580:
<message name="IDS_CHROME_FOR_TESTING_DISCLAIMER" desc="Message shown whenever Chrome for Testing is launched.">please include a screenshot for this for translators as per https://chromium.googlesource.com/chromium/src/+/main/docs/translation_screenshots.md
File chrome/browser/ui/BUILD.gn:
Patch Set #23, Line 1309:
"startup/chrome_for_testing_infobar_delegate.cc",can these be conditionally built based on whether or not the build is targeting CfT?
File chrome/browser/ui/startup/infobar_utils.cc:
nit: omit braces for consistency with the rest of this file
Patch Set #23, Line 92:
AutomationInfoBarDelegate::Create();i see that the Automation infobar is a GlobalConfirmInfoBar shown for every tab in every browser. don't we want that for the CfT infobar, too? if you Ctrl-N to open a new browser window, does it not have the infobar?
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
2 comments:
File chrome/app/generated_resources.grd:
Patch Set #23, Line 12580:
<message name="IDS_CHROME_FOR_TESTING_DISCLAIMER" desc="Message shown whenever Chrome for Testing is launched.">
please include a screenshot for this for translators as per https://chromium.googlesource. […]
Will do once the final string is finalized.
File chrome/browser/ui/startup/infobar_utils.cc:
nit: omit braces for consistency with the rest of this file
Done
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
2 comments:
File chrome/browser/ui/BUILD.gn:
Patch Set #23, Line 1309:
"startup/chrome_for_testing_infobar_delegate.cc",can these be conditionally built based on whether or not the build is targeting CfT?
Sure, this is a nice quality-of-life improvement. Done.
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #23, Line 92:
AutomationInfoBarDelegate::Create();i see that the Automation infobar is a GlobalConfirmInfoBar shown for every tab in every browser. […]
Currently, no. This is a good observation, and a design decision. Let me ask around to see whether it's needed.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach, Maksim Sadym, Mathias Bynens.
1 comment:
File chrome/app/generated_resources.grd:
Patch Set #23, Line 12580:
<message name="IDS_CHROME_FOR_TESTING_DISCLAIMER" desc="Message shown whenever Chrome for Testing is launched.">
Will do once the final string is finalized.
Done
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Christian Flach (Gerrit)
Attention is currently required from: Maksim Sadym, Mathias Bynens, Thiago Perrotta.
1 comment:
Patchset:
Removing myself from attention set for now, Thiago, please let me know once I should take a look again.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Maksim Sadym, Thiago Perrotta.
Patch set 29:Code-Review +1
1 comment:
Patchset:
Still LGTM
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach, Maksim Sadym.
2 comments:
Patchset:
Latest mock lives here: https://bugs.chromium.org/p/chromium/issues/detail?id=1336611#c56 […]
This wording is now final. The question above remains. Do you happen to know, Christian?
Patchset:
Removing myself from attention set for now, Thiago, please let me know once I should take a look aga […]
Yes, could you please take a look at this open comment (https://chromium-review.googlesource.com/c/chromium/src/+/3922097/comments/038c3e9c_17b4fa9b)? Let me know if you have the answer for my question there. That's all for now, I'll readd you to the attention set again once I finish writing tests for this CL.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Christian Flach (Gerrit)
Attention is currently required from: Maksim Sadym, Thiago Perrotta.
1 comment:
Patchset:
The question above remains. Do you happen to know, Christian?
Ah, sorry, I didn't realize that you wanted me to answer that question.
Do we also need to extract the download URL to the GRD file?
What do you mean, exactly? The link text of the download button (`ChromeForTestingInfoBarDelegate::GetLinkText`), or literally the download URL (`ChromeForTestingInfoBarDelegate::GetLinkURL`)? I feel like the link text should get translated, whereas the URL itself probably shouldn't.
(take this with a grain of salt, though, given that I have never added a translated string to Chrome myself)
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Christian Flach, Maksim Sadym, Thiago Perrotta.
1 comment:
Patchset:
> The question above remains. Do you happen to know, Christian? […]
taking chrome/browser/ui/dialogs/outdated_upgrade_bubble.cc as an example, the URL there is not in a grd file. i don't think we need to in this case, since we don't need, for example, different URLs for Chromium vs. Chrome. (this infobar is never shown in production scenarios from a non-CfT build, right?)
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach, Maksim Sadym.
1 comment:
Patchset:
>(this infobar is never shown in production scenarios from a non-CfT build, right?)
That's right.
What do you mean, exactly?
`GetLinkText` should be added to the GRD file for sure. My question was about the URL itself (`GetLinkURL`), but sounds like we shouldn't move it to the GRD file.
The original motivation behind my question wasn't to translate the URL (it doesn't make sense in this context), but rather not to hard-code URLs into source code (*.cc). Somehow adding URLS to *.grd seemed a bit more elegant. But maybe it makes no difference at all. I'll leave it untouched then.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach, Maksim Sadym, Mathias Bynens.
2 comments:
Patchset:
>(this infobar is never shown in production scenarios from a non-CfT build, right?) […]
Done
Patchset:
Yes, could you please take a look at this open comment (https://chromium-review.googlesource. […]
Ack
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #23, Line 92:
AutomationInfoBarDelegate::Create();
Currently, no. This is a good observation, and a design decision. […]
Added a note and an AI to the design doc for this, but will defer to the next CL to unblock.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
1 comment:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #23, Line 92:
AutomationInfoBarDelegate::Create();
Added a note and an AI to the design doc for this, but will defer to the next CL to unblock.
@tper...@chromium.org: i'm not sure what you mean. are you planning to change this CL so that it uses a global infobar?
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #23, Line 92:
AutomationInfoBarDelegate::Create();
@tper...@chromium.org: i'm not sure what you mean. […]
No, the opposite, I'll leave that to a follow-up CL. There are some issues when using a global bar that need to be investigated separately.
For example, clicking the 'Download Chrome' link makes the browser CRASH when using a global info bar, but that doesn't happen when using a non-global info bar.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 34:Code-Review +1
2 comments:
Patchset:
lgtm w/ a TODO
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #34, Line 88:
ChromeForTestingInfoBarDelegate::Create(infobar_manager);this looks like a good place for a comment along the lines of `// TODO(crbug.com/NNNN): Switch to a global infobar.`
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Colin Blundell.
1 comment:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #34, Line 88:
ChromeForTestingInfoBarDelegate::Create(infobar_manager);
this looks like a good place for a comment along the lines of `// TODO(crbug. […]
Done
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Colin Blundell (Gerrit)
Attention is currently required from: Mathias Bynens, Thiago Perrotta.
Patch set 35:Code-Review +1
2 comments:
Patchset:
LGTM with nit, thanks!
File components/infobars/core/infobar_delegate.h:
Patch Set #35, Line 65:
// KEEP IN SYNC WITH THE InfoBarIdentifier ENUM IN enums.xml.nit: Add an element to the enum (I think it's also missing the element for 111 as well).
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Thiago Perrotta.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Colin Blundell, Greg Thompson, Mathias Bynens.
1 comment:
File components/infobars/core/infobar_delegate.h:
Patch Set #35, Line 65:
// KEEP IN SYNC WITH THE InfoBarIdentifier ENUM IN enums.xml.nit: Add an element to the enum (I think it's also missing the element for 111 as well).
Done. I've also added one value that https://chromium-review.googlesource.com/c/chromium/src/+/3865974 seems to have missed therein.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Mathias Bynens (Gerrit)
Attention is currently required from: Colin Blundell, Greg Thompson, Thiago Perrotta.
Patch set 36:Code-Review +1
Greg Thompson (Gerrit)
Attention is currently required from: Colin Blundell, Thiago Perrotta.
Patch set 36:Code-Review +1
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Colin Blundell (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 36:Code-Review +1
1 comment:
Patchset:
//components lgtm again, thanks!
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach.
1 comment:
Patchset:
Unresolving
I am going to skip this for now, because:
(i) we do not currently have plans to add a presubmit CI for CfT, so even if these tests are added, they wouldn't be run automatically
(ii) I tested this change one last time, manually, and it is currently passing / working as expected
(iii) this is blocking CfT progress for a while
That said, this will likely not be the final change to the infobar, there's at least one other AI that is still open, I am just deferring it to a follow-up CL.
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach.
Patch set 36:Commit-Queue +2
Thiago Perrotta (Gerrit)
Attention is currently required from: Christian Flach.
1 comment:
Patchset:
I am going to skip this for now, because: […]
Also linking supporting comment thread: https://docs.google.com/document/d/1e98Ag9sRyEx4do9T3GcIOmH0VuK-JNZnBTusdP8s5aU/edit?disco=AAAAdoJXt0k
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Christian Flach (Gerrit)
2 comments:
Patchset:
Also linking supporting comment thread: https://docs.google. […]
Makes sense :+1: And I didn't intend to hold up anything with that comment, I was just wondering whether it would easily be possible to add a test 😊
Patchset:
LGTM
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Add a persistent infobar warning when launching Chrome for Testing
The CfT infobar is meant to warn developers/users that CfT is an unsupported product that does not auto-update.
Design document: https://goo.gle/chrome-for-testing#bookmark=id.8oia0mpldm1d
Preview: https://bugs.chromium.org/p/chromium/issues/detail?id=1336611#c56
Bug: 1336611
Change-Id: Ia4a017c2828a5fcaae97de57dfe8c6ae27136c24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3922097
Reviewed-by: Mathias Bynens <mat...@chromium.org>
Commit-Queue: Thiago Perrotta <tper...@chromium.org>
Reviewed-by: Greg Thompson <g...@chromium.org>
Reviewed-by: Colin Blundell <blun...@chromium.org>
Reviewed-by: Christian Flach <cmf...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1072750}
---
M chrome/app/generated_resources.grd
A chrome/app/generated_resources_grd/IDS_CHROME_FOR_TESTING_DISCLAIMER.png.sha1
A chrome/app/generated_resources_grd/IDS_DOWNLOAD_CHROME.png.sha1
M chrome/browser/ui/BUILD.gn
A chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.cc
A chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.h
M chrome/browser/ui/startup/infobar_utils.cc
M components/infobars/core/infobar_delegate.h
M tools/metrics/histograms/enums.xml
9 files changed, 184 insertions(+), 9 deletions(-)
Thiago Perrotta (Gerrit)
1 comment:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #34, Line 88:
ChromeForTestingInfoBarDelegate::Create(infobar_manager);
To view, visit change 3922097. To unsubscribe, or for help writing mail filters, visit settings.