Implement ConfirmInfoBar link support on GTK...

Implement ConfirmInfoBar link support on GTK
this is the follow up to http://codereview.chromium.org/1037006 which added it on windows, and http://codereview.chromium.org/1127001 which added it to Mac.

BUG=11246
TEST=run browser with --enable-geolocation, open maps.google.com and click my location.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42337

[joth, 2010-03-19 12:33:33]

Marcus, could you have a quick scan over this and then I'll track down a GTK
expert to have a look :)

Cheers!

[bulach, 2010-03-19 14:24:23]

overall lgtm, but please get some GTK advise..

couple of comments below:

http://codereview.chromium.org/1081007/diff/8001/9002
File chrome/browser/gtk/infobar_gtk.cc (right):

http://codereview.chromium.org/1081007/diff/8001/9002#newcode250
chrome/browser/gtk/infobar_gtk.cc:250: explicit
AlertInfoBar(AlertInfoBarDelegate* delegate)
since this class is pvt within this cc, I think it'd be clearer to just have a
single ctor with the extra param, and fix the one or two calling sites..

http://codereview.chromium.org/1081007/diff/8001/9002#newcode323
chrome/browser/gtk/infobar_gtk.cc:323: }
\n

[joth, 2010-03-19 15:02:57]

Cheers!

http://codereview.chromium.org/1081007/diff/8001/9002
File chrome/browser/gtk/infobar_gtk.cc (right):

http://codereview.chromium.org/1081007/diff/8001/9002#newcode250
chrome/browser/gtk/infobar_gtk.cc:250: explicit
AlertInfoBar(AlertInfoBarDelegate* delegate)
On 2010/03/19 14:24:23, bulach wrote:
> since this class is pvt within this cc, I think it'd be clearer to just have a
> single ctor with the extra param, and fix the one or two calling sites..

Done.

http://codereview.chromium.org/1081007/diff/8001/9002#newcode323
chrome/browser/gtk/infobar_gtk.cc:323: }
On 2010/03/19 14:24:23, bulach wrote:
> \n

Done.

[joth, 2010-03-19 15:06:30]

Evan, I think you're the main author for this file, would you be able to have a
look over this CL for me?

Thanks in advance,
Joth

[Evan Stade, 2010-03-19 17:00:56]

why don't you just change the base class of confirminfobar to linkinfobar?

[joth, 2010-03-19 17:13:17]

"click"

Excellent suggestion. I was blinkered by the windows implementation where
ConfirmInfoBar is highly dependent  on (and implementation intertwinded
with) AlertInfoBar.

Thanks.

On 19 March 2010 17:00, <estade@chromium.org> wrote:

> why don't you just change the base class of confirminfobar to linkinfobar?
>
>
> http://codereview.chromium.org/1081007
>

[joth, 2010-03-19 17:30:57]

only .............  the catch is LinkInfoBar wants to deal with a
LinkInfoBarDelegate, which is not a subclass of ConfirmInfoBarDelegate, so
that doesn't work.


I either need to converge these two interfaces, which is awkward as
most ConfirmInfoBarDelegates don't want to have to deal with
the GetMessageTextWithOffset() method (the offset is not needed), or I need
to factor out a common base class (or function) for the gtk LinkInfoBar and
ConfirmInfoBar which is effectively what I'm using InfoBar as.

Still a very good point that AlertInfoBar is doing nothing for us, so I made
Confirm derive direct from InfoBar. -- New snapshot uploaded.

Thoughts?

Joth


On 19 March 2010 17:12, Jonathan Dixon <joth@chromium.org> wrote:

> "click"
>
> Excellent suggestion. I was blinkered by the windows implementation where
> ConfirmInfoBar is highly dependent  on (and implementation intertwinded
> with) AlertInfoBar.
>
> Thanks.
>
>
> On 19 March 2010 17:00, <estade@chromium.org> wrote:
>
>> why don't you just change the base class of confirminfobar to linkinfobar?
>>
>>
>> http://codereview.chromium.org/1081007
>>
>
>

[Evan Stade, 2010-03-19 23:42:26]

On 2010/03/19 17:30:57, joth wrote:
> only .............  the catch is LinkInfoBar wants to deal with a
> LinkInfoBarDelegate, which is not a subclass of ConfirmInfoBarDelegate, so
> that doesn't work.
> 
> 
> I either need to converge these two interfaces, which is awkward as
> most ConfirmInfoBarDelegates don't want to have to deal with
> the GetMessageTextWithOffset() method (the offset is not needed), or I need
> to factor out a common base class (or function) for the gtk LinkInfoBar and
> ConfirmInfoBar which is effectively what I'm using InfoBar as.
> 
> Still a very good point that AlertInfoBar is doing nothing for us, so I made
> Confirm derive direct from InfoBar. -- New snapshot uploaded.
> 
> Thoughts?
> 
> Joth

Sorry for slow response. It seems that confirminfobardelegate should be a sub of
linkinfobardelegate as well, now that it can have links, which I guess is new as
of r41951.

The infobars were added as they were needed, so maybe their current structure
isn't super great. I don't actually see why an AlertInfoBar is necessary. Seems
like you could just use a LinkInfoBar instead and convert all current
AlertInfoBars to LinkInfoBars (where GetLinkText() defaults to returning an
empty string, and no link is added in that case).

> 
> 
> On 19 March 2010 17:12, Jonathan Dixon <mailto:joth@chromium.org> wrote:
> 
> > "click"
> >
> > Excellent suggestion. I was blinkered by the windows implementation where
> > ConfirmInfoBar is highly dependent  on (and implementation intertwinded
> > with) AlertInfoBar.
> >
> > Thanks.
> >
> >
> > On 19 March 2010 17:00, <mailto:estade@chromium.org> wrote:
> >
> >> why don't you just change the base class of confirminfobar to linkinfobar?
> >>
> >>
> >> http://codereview.chromium.org/1081007
> >>
> >
> >
>

[Evan Stade, 2010-03-19 23:44:20]

Also, I kind of think that the name LinkInfoBar doesn't make much sense without
AlertInfoBar. So maybe the new merged class should retain the name AlertInfoBar.

[joth, 2010-03-22 17:41:14]

Thanks for looking again.

I agree AlertInfoBar seems spurious. I'm tempted to go as far as say
any hierarchy is overkill: we could easily have a single infobar with
optional buttons and/or link that would suit everybody's needs. However
I'm hesitant to this change now as:
- On other platforms (esp. windows) Confirm & Alert info bars share a lot of
code, but none shared with link infobar, so changing the hierarchy as
suggested (merging alert and info) will require major rework (in fact on
windows it would be much easier to merge Alert into Confirm info bar
instead)
- the alert & confirm delegates have GetMessageText() (pure virtual) whereas
link delegate has GetMessageTextWithOffset() instead, it will not be a quick
job to converge these two.
- I just found out Friday there's a plan underway by the UI team to
completely overhaul all info bars in the not too distant future (move
confirm buttons over to the left, to join with the text) and when this is
done the code-sharing will need a major overhaul anyway (again, esp. on
windows)

But most of all, we're on a very tight schedule to get this into the next
beta (due to the UI being redesigned at last moment), so I really need a way
to get this implemented without being held up on major refactoring.

For Mac we're looking at raising a bug to tidy up the delegate hierarchy,
linked off the UI bug. Would this cover your concerns too?

Cheers
Joth



On 19 March 2010 23:44, <estade@chromium.org> wrote:

> Also, I kind of think that the name LinkInfoBar doesn't make much sense
> without
> AlertInfoBar. So maybe the new merged class should retain the name
> AlertInfoBar.
>
>
> http://codereview.chromium.org/1081007
>

[Evan Stade, 2010-03-22 17:55:09]

ok. lgtm. thanks for doing this.

[joth, 2010-03-22 19:53:33]

On 2010/03/22 17:55:09, Evan Stade wrote:
> ok. lgtm. thanks for doing this.

Great, thanks. Glad to learn some GTK
Bug raised here: http://crbug.com/38924 I'll add a todo & link to that in the
code.