Implement the confirm infobar with link for mac....

Implement the confirm infobar with link for mac.

BUG=11246
TEST=browser_tests.exe --gtest_filter=Geol*


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

[Nico, 2010-03-19 19:38:02]

Can you upload a screenshot of how this looks somwhere (e.g. imgur.com)? Thanks
:-)

[rohitrao (ping after 24h), 2010-03-19 19:47:29]

On 2010/03/19 19:38:02, Nico wrote:
> Can you upload a screenshot of how this looks somwhere (e.g. imgur.com)?
Thanks
> :-)

Would also like to see how this looks.  This change generally LGTM, but I really
don't like how we have identical-but-duplicated code between the link infobar
and the confirm infobar.  At this point, the link infobar is basically a
buttonless confirm infobar, so maybe the delegates should reflect that.

[Nico, 2010-03-19 21:57:12]

Can you add an actual screenshot how this looks on OS X with this patch?

On Fri, Mar 19, 2010 at 2:54 PM, Jonathan Dixon <joth@chromium.org> wrote:
> Sorry thought I'd included the mocks link, ah that was probably on the
> windows review.
> There's copy in the design
> doc http://docs.google.com/View?id=dfbnm49n_0dpc7pxpx
> Although currently we have link & buttons swapped for consistency with the
> rest of the confirm info bars. I've attached a windows screenshot.
> Cheers
> Joth
>
> On 19 March 2010 19:47, <rohitrao@chromium.org> wrote:
>>
>> On 2010/03/19 19:38:02, Nico wrote:
>>>
>>> Can you upload a screenshot of how this looks somwhere (e.g. imgur.com)?
>>
>> Thanks
>>>
>>> :-)
>>
>> Would also like to see how this looks.  This change generally LGTM, but I
>> really
>> don't like how we have identical-but-duplicated code between the link
>> infobar
>> and the confirm infobar.  At this point, the link infobar is basically a
>> buttonless confirm infobar, so maybe the delegates should reflect that.
>>
>> http://codereview.chromium.org/1127001
>
>

[joth, 2010-03-19 22:08:19]

Sure, will do as soon as I get chance.

I'm at home now so no easy access to it (can remote into windows & linux
boxes, but not set that up on my mac yet...............)



On 19 March 2010 21:57, Nico Weber <thakis@chromium.org> wrote:

> Can you add an actual screenshot how this looks on OS X with this patch?
>
> On Fri, Mar 19, 2010 at 2:54 PM, Jonathan Dixon <joth@chromium.org> wrote:
> > Sorry thought I'd included the mocks link, ah that was probably on the
> > windows review.
> > There's copy in the design
> > doc http://docs.google.com/View?id=dfbnm49n_0dpc7pxpx
> > Although currently we have link & buttons swapped for consistency with
> the
> > rest of the confirm info bars. I've attached a windows screenshot.
> > Cheers
> > Joth
> >
> > On 19 March 2010 19:47, <rohitrao@chromium.org> wrote:
> >>
> >> On 2010/03/19 19:38:02, Nico wrote:
> >>>
> >>> Can you upload a screenshot of how this looks somwhere (e.g. imgur.com
> )?
> >>
> >> Thanks
> >>>
> >>> :-)
> >>
> >> Would also like to see how this looks.  This change generally LGTM, but
> I
> >> really
> >> don't like how we have identical-but-duplicated code between the link
> >> infobar
> >> and the confirm infobar.  At this point, the link infobar is basically a
> >> buttonless confirm infobar, so maybe the delegates should reflect that.
> >>
> >> http://codereview.chromium.org/1127001
> >
> >
>

[joth, 2010-03-22 09:37:53]

On 2010/03/19 19:47:29, rohitrao wrote:
> On 2010/03/19 19:38:02, Nico wrote:
> > Can you upload a screenshot of how this looks somwhere (e.g. imgur.com)?
> Thanks
> > :-)
> 
> Would also like to see how this looks.  This change generally LGTM, but I
really
> don't like how we have identical-but-duplicated code between the link infobar
> and the confirm infobar.  
Not sure which code you mean? I factored out setLabelToMessage:withLink:
specifically so I could share it between confirm & link infobars  without
duplicating it.

> At this point, the link infobar is basically a
> buttonless confirm infobar, so maybe the delegates should reflect that.

Yeah I'm discussing the hierarchy screwiness on the GTk review thread too. The
significant issue is the alert & confirm delegates have GetMessageText() (pure
virtual) whereas link delegate has GetMessageTextWithOffset()  (pure virtual)
instead. It's not trivial to converge these two.

My appetite for major reworking of this is dampened by the fact we're running
short of time to get this done, and I found out Friday the UI team want to
redesign all the infobars in near future anyway, which would seem to be a much
better time to rework the delegate hierarchy.

[rohitrao (ping after 24h), 2010-03-22 14:45:47]

My "duplicated code" comment was mostly referring to the hierarchy screwiness. 
Can you file a bug for cleaning up the delegates, possibly after the infobar
rewrite?  It'll get ignored anyways, but at least we'll have a bug for it :)

LGTM, but I'd still like to see a screenshot before you submit.  Is the bar
yellow, or did you add code somewhere to make it blue?


> > don't like how we have identical-but-duplicated code between the link
infobar
> > and the confirm infobar.  
> Not sure which code you mean? I factored out setLabelToMessage:withLink:
> specifically so I could share it between confirm & link infobars  without
> duplicating it.
> 
> > At this point, the link infobar is basically a
> > buttonless confirm infobar, so maybe the delegates should reflect that.
> 
> Yeah I'm discussing the hierarchy screwiness on the GTk review thread too. The
> significant issue is the alert & confirm delegates have GetMessageText() (pure
> virtual) whereas link delegate has GetMessageTextWithOffset()  (pure virtual)
> instead. It's not trivial to converge these two.
> 
> My appetite for major reworking of this is dampened by the fact we're running
> short of time to get this done, and I found out Friday the UI team want to
> redesign all the infobars in near future anyway, which would seem to be a much
> better time to rework the delegate hierarchy.

[joth, 2010-03-22 14:54:57]

On 22 March 2010 14:45, <rohitrao@chromium.org> wrote:

> My "duplicated code" comment was mostly referring to the hierarchy
> screwiness.
> Can you file a bug for cleaning up the delegates, possibly after the
> infobar
> rewrite?  It'll get ignored anyways, but at least we'll have a bug for it
> :)
>
> LGTM, but I'd still like to see a screenshot before you submit.  Is the bar
> yellow, or did you add code somewhere to make it blue?
>
>
> Sorry for the delay, I've had to WFH today, and it's taking an age to get a
Mac build on my 2 core laptop...

I've left the bar as yellow. Again, there's been numerous conversations on
yellow vs. blue vs. green with no obvious conclusion, so I've left it as
whatever is consistent with other 'info' type bars for the time being. The
color is much lower priority than getting the link shown which could be beta
blocker as it contains privacy policy details.




>  > don't like how we have identical-but-duplicated code between the link
>>
> infobar
>
>> > and the confirm infobar.
>> Not sure which code you mean? I factored out setLabelToMessage:withLink:
>> specifically so I could share it between confirm & link infobars  without
>> duplicating it.
>>
>
>  > At this point, the link infobar is basically a
>> > buttonless confirm infobar, so maybe the delegates should reflect that.
>>
>
>  Yeah I'm discussing the hierarchy screwiness on the GTk review thread too.
>> The
>> significant issue is the alert & confirm delegates have GetMessageText()
>> (pure
>> virtual) whereas link delegate has GetMessageTextWithOffset()  (pure
>> virtual)
>> instead. It's not trivial to converge these two.
>>
>
>  My appetite for major reworking of this is dampened by the fact we're
>> running
>> short of time to get this done, and I found out Friday the UI team want to
>> redesign all the infobars in near future anyway, which would seem to be a
>> much
>> better time to rework the delegate hierarchy.
>>
>
>
>
> http://codereview.chromium.org/1127001
>

[joth, 2010-03-22 17:23:09]

screenshot: http://i.imgur.com/eDIkw.png

A bug (and maybe a TODO) sounds good way to track this.

Cheers
Joth


On 2010/03/22 14:54:57, joth wrote:
> On 22 March 2010 14:45, <mailto:rohitrao@chromium.org> wrote:
> 
> > My "duplicated code" comment was mostly referring to the hierarchy
> > screwiness.
> > Can you file a bug for cleaning up the delegates, possibly after the
> > infobar
> > rewrite?  It'll get ignored anyways, but at least we'll have a bug for it
> > :)
> >
> > LGTM, but I'd still like to see a screenshot before you submit.  Is the bar
> > yellow, or did you add code somewhere to make it blue?
> >
> >
> > Sorry for the delay, I've had to WFH today, and it's taking an age to get a
> Mac build on my 2 core laptop...
> 
> I've left the bar as yellow. Again, there's been numerous conversations on
> yellow vs. blue vs. green with no obvious conclusion, so I've left it as
> whatever is consistent with other 'info' type bars for the time being. The
> color is much lower priority than getting the link shown which could be beta
> blocker as it contains privacy policy details.
> 
> 
> 
> 
> >  > don't like how we have identical-but-duplicated code between the link
> >>
> > infobar
> >
> >> > and the confirm infobar.
> >> Not sure which code you mean? I factored out setLabelToMessage:withLink:
> >> specifically so I could share it between confirm & link infobars  without
> >> duplicating it.
> >>
> >
> >  > At this point, the link infobar is basically a
> >> > buttonless confirm infobar, so maybe the delegates should reflect that.
> >>
> >
> >  Yeah I'm discussing the hierarchy screwiness on the GTk review thread too.
> >> The
> >> significant issue is the alert & confirm delegates have GetMessageText()
> >> (pure
> >> virtual) whereas link delegate has GetMessageTextWithOffset()  (pure
> >> virtual)
> >> instead. It's not trivial to converge these two.
> >>
> >
> >  My appetite for major reworking of this is dampened by the fact we're
> >> running
> >> short of time to get this done, and I found out Friday the UI team want to
> >> redesign all the infobars in near future anyway, which would seem to be a
> >> much
> >> better time to rework the delegate hierarchy.
> >>
> >
> >
> >
> > http://codereview.chromium.org/1127001
> >
>

[Nico, 2010-03-23 04:50:12]

Does the "Learn more" link open a web page?

On Mon, Mar 22, 2010 at 10:23 AM,  <joth@chromium.org> wrote:
> screenshot: http://i.imgur.com/eDIkw.png
>
> A bug (and maybe a TODO) sounds good way to track this.
>
> Cheers
> Joth
>
>
> On 2010/03/22 14:54:57, joth wrote:
>>
>> On 22 March 2010 14:45, <mailto:rohitrao@chromium.org> wrote:
>
>> > My "duplicated code" comment was mostly referring to the hierarchy
>> > screwiness.
>> > Can you file a bug for cleaning up the delegates, possibly after the
>> > infobar
>> > rewrite?  It'll get ignored anyways, but at least we'll have a bug for
>> > it
>> > :)
>> >
>> > LGTM, but I'd still like to see a screenshot before you submit.  Is the
>> > bar
>> > yellow, or did you add code somewhere to make it blue?
>> >
>> >
>> > Sorry for the delay, I've had to WFH today, and it's taking an age to
>> > get a
>> Mac build on my 2 core laptop...
>
>> I've left the bar as yellow. Again, there's been numerous conversations on
>> yellow vs. blue vs. green with no obvious conclusion, so I've left it as
>> whatever is consistent with other 'info' type bars for the time being. The
>> color is much lower priority than getting the link shown which could be
>> beta
>> blocker as it contains privacy policy details.
>
>
>
>
>> >  > don't like how we have identical-but-duplicated code between the link
>> >>
>> > infobar
>> >
>> >> > and the confirm infobar.
>> >> Not sure which code you mean? I factored out
>> >> setLabelToMessage:withLink:
>> >> specifically so I could share it between confirm & link infobars
>> >>  without
>> >> duplicating it.
>> >>
>> >
>> >  > At this point, the link infobar is basically a
>> >> > buttonless confirm infobar, so maybe the delegates should reflect
>> >> > that.
>> >>
>> >
>> >  Yeah I'm discussing the hierarchy screwiness on the GTk review thread
>> > too.
>> >> The
>> >> significant issue is the alert & confirm delegates have
>> >> GetMessageText()
>> >> (pure
>> >> virtual) whereas link delegate has GetMessageTextWithOffset()  (pure
>> >> virtual)
>> >> instead. It's not trivial to converge these two.
>> >>
>> >
>> >  My appetite for major reworking of this is dampened by the fact we're
>> >> running
>> >> short of time to get this done, and I found out Friday the UI team want
>> >> to
>> >> redesign all the infobars in near future anyway, which would seem to be
>> >> a
>> >> much
>> >> better time to rework the delegate hierarchy.
>> >>
>> >
>> >
>> >
>> > http://codereview.chromium.org/1127001
>> >
>
>
>
>
> http://codereview.chromium.org/1127001
>

[joth, 2010-03-23 09:24:25]

Yes. See LinkClicked in
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/geolocation/ge...


On 23 March 2010 04:50, Nico Weber <thakis@chromium.org> wrote:

> Does the "Learn more" link open a web page?
>
> On Mon, Mar 22, 2010 at 10:23 AM,  <joth@chromium.org> wrote:
> > screenshot: http://i.imgur.com/eDIkw.png
> >
> > A bug (and maybe a TODO) sounds good way to track this.
> >
> > Cheers
> > Joth
> >
> >
> > On 2010/03/22 14:54:57, joth wrote:
> >>
> >> On 22 March 2010 14:45, <mailto:rohitrao@chromium.org> wrote:
> >
> >> > My "duplicated code" comment was mostly referring to the hierarchy
> >> > screwiness.
> >> > Can you file a bug for cleaning up the delegates, possibly after the
> >> > infobar
> >> > rewrite?  It'll get ignored anyways, but at least we'll have a bug for
> >> > it
> >> > :)
> >> >
> >> > LGTM, but I'd still like to see a screenshot before you submit.  Is
> the
> >> > bar
> >> > yellow, or did you add code somewhere to make it blue?
> >> >
> >> >
> >> > Sorry for the delay, I've had to WFH today, and it's taking an age to
> >> > get a
> >> Mac build on my 2 core laptop...
> >
> >> I've left the bar as yellow. Again, there's been numerous conversations
> on
> >> yellow vs. blue vs. green with no obvious conclusion, so I've left it as
> >> whatever is consistent with other 'info' type bars for the time being.
> The
> >> color is much lower priority than getting the link shown which could be
> >> beta
> >> blocker as it contains privacy policy details.
> >
> >
> >
> >
> >> >  > don't like how we have identical-but-duplicated code between the
> link
> >> >>
> >> > infobar
> >> >
> >> >> > and the confirm infobar.
> >> >> Not sure which code you mean? I factored out
> >> >> setLabelToMessage:withLink:
> >> >> specifically so I could share it between confirm & link infobars
> >> >>  without
> >> >> duplicating it.
> >> >>
> >> >
> >> >  > At this point, the link infobar is basically a
> >> >> > buttonless confirm infobar, so maybe the delegates should reflect
> >> >> > that.
> >> >>
> >> >
> >> >  Yeah I'm discussing the hierarchy screwiness on the GTk review thread
> >> > too.
> >> >> The
> >> >> significant issue is the alert & confirm delegates have
> >> >> GetMessageText()
> >> >> (pure
> >> >> virtual) whereas link delegate has GetMessageTextWithOffset()  (pure
> >> >> virtual)
> >> >> instead. It's not trivial to converge these two.
> >> >>
> >> >
> >> >  My appetite for major reworking of this is dampened by the fact we're
> >> >> running
> >> >> short of time to get this done, and I found out Friday the UI team
> want
> >> >> to
> >> >> redesign all the infobars in near future anyway, which would seem to
> be
> >> >> a
> >> >> much
> >> >> better time to rework the delegate hierarchy.
> >> >>
> >> >
> >> >
> >> >
> >> > http://codereview.chromium.org/1127001
> >> >
> >
> >
> >
> >
> > http://codereview.chromium.org/1127001
> >
>

[joth, 2010-03-23 11:24:12]

I'd like to go ahead and commit this - now rohitrao has got the screenshoot I
assume your previous LGTM stands?
If you do have any follow up comments please let me know and I can bug them or
do a quick follow up

Cheers
Joth

[jorlow, 2010-03-23 11:42:56]

I agree we should get this committed and then iterate if there do happen to be
any outstanding issues.  LGTM