Separate the URL of an infobar link with clicking on it.

Separate getting the URL of an infobar from clicking on it. Also make a similar change to bubbles, as they have the same interface.

BUG=528228
TEST=existing tests

Committed: https://crrev.com/d73c8cc05e6e4e6656c88a4715ac67e35abfd58f
Cr-Commit-Position: refs/heads/master@{#351799}

[Avi (use Gerrit), 2015-09-23 22:52:16]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-23 22:54:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/1

[Avi (use Gerrit), 2015-09-23 23:03:33]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-23 23:07:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/20001

[commit-bot: I haz the power, 2015-09-24 00:10:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 00:10:16]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2015-09-24 14:27:31]

avi@chromium.org changed reviewers:
+ pkasting@chromium.org

[Avi (use Gerrit), 2015-09-24 14:27:32]

Peter:

As we discussed. This is a first pass, just separating out the URLs. You had a
question about unifying the LinkClicked implementations. If you look at the
AutofillCCInfoBarDelegate, which is the only one in components today, it
custom-sends the link click to the autofill_client_ because everyone else
basically does

InfoBarService::WebContentsFromInfoBar(infobar())->OpenURL()

and both the InfoBarService and WebContents are outside components. If you have
thoughts on how to proceed here, I'm open ears.

[Peter Kasting, 2015-09-24 21:18:27]

On 2015/09/24 14:27:32, Avi wrote:
> Peter:
> 
> As we discussed. This is a first pass, just separating out the URLs. You had a
> question about unifying the LinkClicked implementations. If you look at the
> AutofillCCInfoBarDelegate, which is the only one in components today, it
> custom-sends the link click to the autofill_client_ because everyone else
> basically does
> 
> InfoBarService::WebContentsFromInfoBar(infobar())->OpenURL()
> 
> and both the InfoBarService and WebContents are outside components. If you
have
> thoughts on how to proceed here, I'm open ears.

The obvious way would be to add a method on InfoBarManager to open a URL, which
would be pure virtual and implemented by InfoBarService.  I think this would
allow replacing the autofill client implementation above too.

https://codereview.chromium.org/1361253002/diff/20001/chrome/browser/plugins/...
File chrome/browser/plugins/plugin_infobar_delegates.cc (right):

https://codereview.chromium.org/1361253002/diff/20001/chrome/browser/plugins/...
chrome/browser/plugins/plugin_infobar_delegates.cc:85: return
GURL(GetLearnMoreURL());
We should just eliminate GetLearnMoreURL() and either move its implementation to
here (if there is only one implementation) or override GetLinkURL() in
subclasses as needed.

https://codereview.chromium.org/1361253002/diff/20001/chrome/browser/ui/confi...
File chrome/browser/ui/confirm_bubble_model.h (right):

https://codereview.chromium.org/1361253002/diff/20001/chrome/browser/ui/confi...
chrome/browser/ui/confirm_bubble_model.h:57: virtual GURL GetLinkURL() const;
I don't think you should be modifying this class or its subclasses, as they're
not infobar delegates.

This also means reverting the Mac changes that called this new method.

[Avi (use Gerrit), 2015-09-30 17:49:13]

On 2015/09/24 21:18:27, Peter Kasting wrote:
> The obvious way would be to add a method on InfoBarManager to open a URL,
which
> would be pure virtual and implemented by InfoBarService.  I think this would
> allow replacing the autofill client implementation above too.

I'm running into trouble here.

What I wanted to do is make an InfoBar::LinkClicked, and have the view
controllers call it. It would have the logic of calling OpenURL on the
InfoBarService, and then LinkWasClicked (optionally overridable) on the
delegate. Problem is that the view controllers only have the InfoBarDelegate,
and the call to get back to the InfoBar object is protected.

So then I thought about making the view controller do both calls, but again the
view controllers only have the InfoBarDelegate.

If we want to consolidate this code, we need to expose a bit. Would you mind
making the InfoBar* accessor on InfoBarDelegate public? Then the view
controllers can tell the InfoBar it was clicked, and it can do all the opening,
etc.

[Avi (use Gerrit), 2015-09-30 18:29:49]

On 2015/09/30 17:49:13, Avi wrote:
> On 2015/09/24 21:18:27, Peter Kasting wrote:
> > The obvious way would be to add a method on InfoBarManager to open a URL,
> which
> > would be pure virtual and implemented by InfoBarService.  I think this would
> > allow replacing the autofill client implementation above too.
> 
> I'm running into trouble here.
> 
> What I wanted to do is make an InfoBar::LinkClicked, and have the view
> controllers call it. It would have the logic of calling OpenURL on the
> InfoBarService, and then LinkWasClicked (optionally overridable) on the
> delegate. Problem is that the view controllers only have the InfoBarDelegate,
> and the call to get back to the InfoBar object is protected.
> 
> So then I thought about making the view controller do both calls, but again
the
> view controllers only have the InfoBarDelegate.
> 
> If we want to consolidate this code, we need to expose a bit. Would you mind
> making the InfoBar* accessor on InfoBarDelegate public? Then the view
> controllers can tell the InfoBar it was clicked, and it can do all the
opening,
> etc.

Actually, never mind. I'll do the work in ConfirmInfoBarDelegate::LinkClicked.

[Avi (use Gerrit), 2015-09-30 18:59:49]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 19:01:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/40001

[commit-bot: I haz the power, 2015-09-30 19:12:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 19:12:55]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Avi (use Gerrit), 2015-09-30 19:14:13]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 19:16:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/40001

[commit-bot: I haz the power, 2015-09-30 19:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 19:28:56]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[Avi (use Gerrit), 2015-09-30 19:36:13]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 19:38:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/60001

[commit-bot: I haz the power, 2015-09-30 19:50:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 19:50:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Avi (use Gerrit), 2015-09-30 20:13:36]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-30 20:15:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/80001

[commit-bot: I haz the power, 2015-09-30 20:23:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-30 20:23:02]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Avi (use Gerrit), 2015-09-30 22:58:33]

https://codereview.chromium.org/1361253002/diff/20001/chrome/browser/ui/confi...
File chrome/browser/ui/confirm_bubble_model.h (right):

https://codereview.chromium.org/1361253002/diff/20001/chrome/browser/ui/confi...
chrome/browser/ui/confirm_bubble_model.h:57: virtual GURL GetLinkURL() const;
On 2015/09/24 21:18:27, Peter Kasting wrote:
> I don't think you should be modifying this class or its subclasses, as they're
> not infobar delegates.
> 
> This also means reverting the Mac changes that called this new method.

This interface is identical to the toolbar interface, down to the typos in the
comment for GetLinkText(). It suffers exactly the same problem as the toolbar
interface, and I need to provide the exact same solution for it. I think it
makes sense to include it in this change if we're making the same changes to it.

[Peter Kasting, 2015-10-01 00:10:58]

LGTM

https://chromiumcodereview.appspot.com/1361253002/diff/20001/chrome/browser/u...
File chrome/browser/ui/confirm_bubble_model.h (right):

https://chromiumcodereview.appspot.com/1361253002/diff/20001/chrome/browser/u...
chrome/browser/ui/confirm_bubble_model.h:57: virtual GURL GetLinkURL() const;
On 2015/09/30 22:58:33, Avi wrote:
> On 2015/09/24 21:18:27, Peter Kasting wrote:
> > I don't think you should be modifying this class or its subclasses, as
they're
> > not infobar delegates.
> > 
> > This also means reverting the Mac changes that called this new method.
> 
> This interface is identical to the toolbar interface, down to the typos in the
> comment for GetLinkText(). It suffers exactly the same problem as the toolbar
> interface, and I need to provide the exact same solution for it. I think it
> makes sense to include it in this change if we're making the same changes to
it.

I'm not objecting to the fix, I just would have prefered not to touch
non-infobar-related APIs in a change nominally about infobars.

It doesn't matter much; the change is clearly correct.

https://chromiumcodereview.appspot.com/1361253002/diff/80001/chrome/browser/g...
File chrome/browser/gpu/three_d_api_observer.cc (left):

https://chromiumcodereview.appspot.com/1361253002/diff/80001/chrome/browser/g...
chrome/browser/gpu/three_d_api_observer.cc:162: 
Nit: Leave this blank line

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
File components/infobars/core/confirm_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
components/infobars/core/confirm_infobar_delegate.h:72: // infobar to begin
closing.
Nit: Move this last sentence to end of the previous paragraph?

Maybe add something here about how the default implementation uses a disposition
of NEW_FOREGROUND_TAB if |disposition| is CURRENT_TAB, so that normal clicks on
the link won't automatically blow away the user's page.

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
File components/infobars/core/infobar_manager.h (right):

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
components/infobars/core/infobar_manager.h:104: // Opens a URL in whatever
manner is appropriate.
Nit: in whatever manner is appropriate -> according to the specified
|disposition|?

[Avi (use Gerrit), 2015-10-01 00:20:16]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[Avi (use Gerrit), 2015-10-01 00:20:34]

https://codereview.chromium.org/1361253002/diff/80001/chrome/browser/gpu/thre...
File chrome/browser/gpu/three_d_api_observer.cc (left):

https://codereview.chromium.org/1361253002/diff/80001/chrome/browser/gpu/thre...
chrome/browser/gpu/three_d_api_observer.cc:162: 
On 2015/10/01 00:10:58, Peter Kasting wrote:
> Nit: Leave this blank line

Done.

https://codereview.chromium.org/1361253002/diff/80001/components/infobars/cor...
File components/infobars/core/confirm_infobar_delegate.h (right):

https://codereview.chromium.org/1361253002/diff/80001/components/infobars/cor...
components/infobars/core/confirm_infobar_delegate.h:72: // infobar to begin
closing.
On 2015/10/01 00:10:58, Peter Kasting wrote:
> Nit: Move this last sentence to end of the previous paragraph?

Done.

> Maybe add something here about how the default implementation uses a
disposition
> of NEW_FOREGROUND_TAB if |disposition| is CURRENT_TAB, so that normal clicks
on
> the link won't automatically blow away the user's page.

True, though the problem is that it's the InfoBarService that does that
fanciness, not the ConfirmInfoBarDelegate, and I feel weird describing the
function of chrome/ code in comments in components/.

https://codereview.chromium.org/1361253002/diff/80001/components/infobars/cor...
File components/infobars/core/infobar_manager.h (right):

https://codereview.chromium.org/1361253002/diff/80001/components/infobars/cor...
components/infobars/core/infobar_manager.h:104: // Opens a URL in whatever
manner is appropriate.
On 2015/10/01 00:10:58, Peter Kasting wrote:
> Nit: in whatever manner is appropriate -> according to the specified
> |disposition|?

Done.

[commit-bot: I haz the power, 2015-10-01 00:21:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/100001

[Avi (use Gerrit), 2015-10-01 00:23:51]

avi@chromium.org changed reviewers:
+ mnaganov@chromium.org, thestig@chromium.org

[Avi (use Gerrit), 2015-10-01 00:23:52]

mnaganov@: Please review changes in android_webview/
thestig@: Please review changes in chrome/
isherman@: Please review changes in components/autofill/
droger@: Please review changes in ios/

[Peter Kasting, 2015-10-01 00:23:53]

pkasting@chromium.org changed reviewers:
- mnaganov@chromium.org, thestig@chromium.org

[Peter Kasting, 2015-10-01 00:23:54]

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
File components/infobars/core/confirm_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
components/infobars/core/confirm_infobar_delegate.h:72: // infobar to begin
closing.
On 2015/10/01 00:20:33, Avi wrote:
> On 2015/10/01 00:10:58, Peter Kasting wrote:
> > Nit: Move this last sentence to end of the previous paragraph?
> 
> Done.
> 
> > Maybe add something here about how the default implementation uses a
> disposition
> > of NEW_FOREGROUND_TAB if |disposition| is CURRENT_TAB, so that normal clicks
> on
> > the link won't automatically blow away the user's page.
> 
> True, though the problem is that it's the InfoBarService that does that
> fanciness, not the ConfirmInfoBarDelegate, and I feel weird describing the
> function of chrome/ code in comments in components/.

Oh, my mistake.  Put the comment on the InfoBarService method then and here
maybe just note that we pass it on to that function?

[Avi (use Gerrit), 2015-10-01 00:25:38]

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
File components/infobars/core/confirm_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
components/infobars/core/confirm_infobar_delegate.h:72: // infobar to begin
closing.
On 2015/10/01 00:23:54, Peter Kasting wrote:
> On 2015/10/01 00:20:33, Avi wrote:
> > On 2015/10/01 00:10:58, Peter Kasting wrote:
> > > Nit: Move this last sentence to end of the previous paragraph?
> > 
> > Done.
> > 
> > > Maybe add something here about how the default implementation uses a
> > disposition
> > > of NEW_FOREGROUND_TAB if |disposition| is CURRENT_TAB, so that normal
clicks
> > on
> > > the link won't automatically blow away the user's page.
> > 
> > True, though the problem is that it's the InfoBarService that does that
> > fanciness, not the ConfirmInfoBarDelegate, and I feel weird describing the
> > function of chrome/ code in comments in components/.
> 
> Oh, my mistake.  Put the comment on the InfoBarService method then and here
> maybe just note that we pass it on to that function?

My latest patchset puts a comment in InfoBarService. Again, do we comment in
components about a call to a specific function in chrome?

[Avi (use Gerrit), 2015-10-01 00:28:03]

avi@chromium.org changed reviewers:
+ droger@chromium.org, isherman@chromium.org, mnaganov@chromium.org,
thestig@chromium.org

[Avi (use Gerrit), 2015-10-01 00:28:04]

mnaganov@: Please review changes in android_webview/
thestig@: Please review changes in chrome/
isherman@: Please review changes in components/autofill/
droger@: Please review changes in ios/

[Evan Stade, 2015-10-01 00:34:48]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-10-01 00:34:49]

(standing in for Ilya since I've been vaguely following this CL anyway) autofill
lgtm

[Lei Zhang, 2015-10-01 00:58:48]

lgtm

chrome -> I wonder if we started with 1 infobar, and everyone else just copy +
pasted.

[mnaganov (inactive), 2015-10-01 01:02:54]

android_webview/ LGTM

[commit-bot: I haz the power, 2015-10-01 01:10:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 01:10:49]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2015-10-01 01:12:17]

On 2015/10/01 00:58:48, Lei Zhang wrote:
> lgtm
> 
> chrome -> I wonder if we started with 1 infobar, and everyone else just copy +
> pasted.

Isn't that how you write code?

[Peter Kasting, 2015-10-01 01:20:22]

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
File components/infobars/core/confirm_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/1361253002/diff/80001/components/infob...
components/infobars/core/confirm_infobar_delegate.h:72: // infobar to begin
closing.
On 2015/10/01 00:25:38, Avi wrote:
> On 2015/10/01 00:23:54, Peter Kasting wrote:
> > On 2015/10/01 00:20:33, Avi wrote:
> > > On 2015/10/01 00:10:58, Peter Kasting wrote:
> > > > Nit: Move this last sentence to end of the previous paragraph?
> > > 
> > > Done.
> > > 
> > > > Maybe add something here about how the default implementation uses a
> > > disposition
> > > > of NEW_FOREGROUND_TAB if |disposition| is CURRENT_TAB, so that normal
> clicks
> > > on
> > > > the link won't automatically blow away the user's page.
> > > 
> > > True, though the problem is that it's the InfoBarService that does that
> > > fanciness, not the ConfirmInfoBarDelegate, and I feel weird describing the
> > > function of chrome/ code in comments in components/.
> > 
> > Oh, my mistake.  Put the comment on the InfoBarService method then and here
> > maybe just note that we pass it on to that function?
> 
> My latest patchset puts a comment in InfoBarService. Again, do we comment in
> components about a call to a specific function in chrome?

I don't tend to be hard-and-fast about this one, but I think what you have now
is fine.

[droger, 2015-10-01 08:16:40]

//ios LGTM

[Avi (use Gerrit), 2015-10-01 14:39:19]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-10-01 14:39:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1361253002/#ps100001 (title: "pkasting")

[commit-bot: I haz the power, 2015-10-01 14:39:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1361253002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1361253002/100001

[commit-bot: I haz the power, 2015-10-01 14:46:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-01 14:47:28]

Patchset 6 (id:??) landed as
https://crrev.com/d73c8cc05e6e4e6656c88a4715ac67e35abfd58f
Cr-Commit-Position: refs/heads/master@{#351799}