Use a global info bar in Chrome for Testing [chromium/src : main]
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson.
Patch set 2:Commit-Queue +1
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
1 comment:
Patchset:
iiuc, this is not ready to land on account of a crash when the link is clicked. is this still the case? if so, please flip this back to a WIP until it's ready for review. thanks.
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson.
1 comment:
Patchset:
iiuc, this is not ready to land on account of a crash when the link is clicked. […]
Yes. Technically the CL is ready, it's just blocked on the open bug. I put `COMMIT=false` in the CL description with the hopes of getting your +1 early so that when the bug is fixed I can just immediately submit it without waiting for approval. (This CL is final and won't change)
If you prefer, I could just mark it as WIP and get your +1 later on. Just trying to speed things up a bit :-)
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
1 comment:
Patchset:
Yes. Technically the CL is ready, it's just blocked on the open bug. […]
does the crash repro with the any other global infobars, or only with this one?
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson.
1 comment:
Patchset:
does the crash repro with the any other global infobars, or only with this one?
I patched chrome/browser/ui/startup/automation_infobar_delegate.h to add a button link with a URL to it and it has also crashed.
Link to patch CL for reference: https://chromium-review.googlesource.com/c/chromium/src/+/4066579
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
1 comment:
Patchset:
I patched chrome/browser/ui/startup/automation_infobar_delegate. […]
great. if other infobars don't suffer from the crash because they don't have links, then that explains why we haven't seen this before. i think we need to fix the crash before we land an infobar that will crash the browser when users interact with it. or land the infobar without the link until the link-click crash is resolved. one or the other. we shouldn't land a cl that we know will crash the browser.
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
Patchset:
great. […]
Totally agreed! Let's see if we can get the bug fixed. If we cannot, our options for CfT are:
(i) use global info bar, but remove the download chrome button/link
(ii) do not use a global info bar (i.e. use an info bar that only shows up in the first browser tab)From a product perspective, (i) is the less worse alternative (this should be confirmed with stakeholders first though)
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Greg Thompson (Gerrit)
Attention is currently required from: Thiago Perrotta.
1 comment:
Patchset:
Totally agreed! Let's see if we can get the bug fixed. If we cannot, our options for CfT are: […]
i encourage you to dig into the crash yourself. asking others for advice is great, of course, but it seems that you have a reliable repro, so adding a few DCHECKs and reproing the failure should very quickly show you exactly what ptr is null. knowing that, you can sprinkle more DCHECKs around the code to try to understand where it's happening. i think you could get pretty far without involving a debugger.
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson.
1 comment:
Patchset:
https://chromium-review.googlesource.com/c/chromium/src/+/4117513 shall unblock this CL. Could I get your +1 in advance? I will wait until the CL is submitted before landing this one.
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Patchset:
i encourage you to dig into the crash yourself. […]
Obsoleted (see other comment)
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kasting (Gerrit)
Attention is currently required from: Greg Thompson, Thiago Perrotta.
2 comments:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #9, Line 89:
ChromeForTestingInfoBarDelegate::Create(infobar_manager);Does anything use the old-style API anymore? If not, can it be removed entirely?
Patch Set #9, Line 83:
infobars::ContentInfoBarManager* infobar_manager =Nit: Move this down to just above first use
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Greg Thompson, Peter Kasting.
2 comments:
File chrome/browser/ui/startup/infobar_utils.cc:
Patch Set #9, Line 89:
ChromeForTestingInfoBarDelegate::Create(infobar_manager);Does anything use the old-style API anymore? If not, can it be removed entirely?
Done
Patch Set #9, Line 83:
infobars::ContentInfoBarManager* infobar_manager =Nit: Move this down to just above first use
Done
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kasting (Gerrit)
Attention is currently required from: Greg Thompson, Thiago Perrotta.
Patch set 11:Code-Review +1
1 comment:
File chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.cc:
Patch Set #11, Line 24:
GlobalConfirmInfoBar::Show(std::move(delegate));Nit: Since the constructor is public, seems like this could just be:
```
GlobalConfirmInfoBar::Show(std::make_unique<ChromeForTestingInfoBarDelegate>());
```
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 11:Commit-Queue +1
1 comment:
File chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.cc:
Patch Set #11, Line 24:
GlobalConfirmInfoBar::Show(std::move(delegate));
Nit: Since the constructor is public, seems like this could just be: […]
Done
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
1 comment:
Patchset:
https://chromium-review.googlesource.com/c/chromium/src/+/4117513 shall unblock this CL. […]
Obsoleted given Peter's +1
To view, visit change 4057898. To unsubscribe, or for help writing mail filters, visit settings.
Thiago Perrotta (Gerrit)
Patch set 13:Commit-Queue +2
Thiago Perrotta (Gerrit)
Attention is currently required from: Thiago Perrotta.
Patch set 14:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.cc
Insertions: 2, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/ui/startup/infobar_utils.cc
Insertions: 3, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Use a global info bar in Chrome for Testing
Previously the info bar was displayed only in the first browser tab.
Design document:
https://goo.gle/chrome-for-testing#bookmark=id.8oia0mpldm1d
This is a follow-up of:
https://chromium-review.googlesource.com/c/chromium/src/+/3922097
Bug: 1336611, 1393765
Change-Id: Icca2129363bc38f25a3aa40846810f464b89cd5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4057898
Reviewed-by: Peter Kasting <pkas...@chromium.org>
Commit-Queue: Thiago Perrotta <tper...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1086118}
---
M chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.cc
M chrome/browser/ui/startup/chrome_for_testing_infobar_delegate.h
M chrome/browser/ui/startup/infobar_utils.cc
3 files changed, 28 insertions(+), 21 deletions(-)