Fixes bug where a Page Action icon was not being properly displayed upon page...

Fixes bug where a Page Action icon was not being properly displayed upon page load unless the Omnibar was being forced to redraw (via clicking within it).

BUG=12281
TEST=Load a page that has a page action, observe that it appears without having to select the text in the omnibar.

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

[Nico, 2009-12-24 01:45:04]

The code looks alright, but I'm not sure why you need this: The windows and
linux versions don't do anything like this as far as I can tell. Do they have
the same bug? Or is the mac plumbing just somehow wrong so that the mac version
needs this while win/linux don't? In the latter case, fixing the plumbing might
be better.

[andybons, 2009-12-24 02:16:18]

I agree, but we have win, which has each image be it's own proper
subview and can therefore call schedulePaint, linux, which seems to
redraw if the layout changes regardless and mac, where the image view
classes are not proper subclasses of NSView and therefore cannot call
redraw in the same way that windows does.

If I were to change things to be more consistent, I would change the
other implementations to be looking for the page action visibility
notification change instead of redrawing in every function where it
_may_ have changed.

A

On Wed, Dec 23, 2009 at 8:45 PM,  <thakis@chromium.org> wrote:
> The code looks alright, but I'm not sure why you need this: The windows and
> linux versions don't do anything like this as far as I can tell. Do they
> have
> the same bug? Or is the mac plumbing just somehow wrong so that the mac
> version
> needs this while win/linux don't? In the latter case, fixing the plumbing
> might
> be better.
>
> http://codereview.chromium.org/515027
>

[Nico, 2009-12-24 02:37:12]

Your call. LG if you don't want to do that, but it'd be nicer IMHO.

On Dec 23, 2009 6:16 PM, "Andrew Bonventre (Bons)" <andybons@google.com>
wrote:

I agree, but we have win, which has each image be it's own proper
subview and can therefore call schedulePaint, linux, which seems to
redraw if the layout changes regardless and mac, where the image view
classes are not proper subclasses of NSView and therefore cannot call
redraw in the same way that windows does.

If I were to change things to be more consistent, I would change the
other implementations to be looking for the page action visibility
notification change instead of redrawing in every function where it
_may_ have changed.

A

On Wed, Dec 23, 2009 at 8:45 PM, <thakis@chromium.org> wrote: > The code
looks alright, but I'm no...

[andybons, 2009-12-24 02:43:54]

I'll submit and wait for comments from Pam, who was also looking into
this, as well as look for a way to fix the plumbing since I'll be in
this code doing tooltips and cursor changes as well.

On Wed, Dec 23, 2009 at 9:36 PM, Nico Weber <thakis@chromium.org> wrote:
> Your call. LG if you don't want to do that, but it'd be nicer IMHO.
>
> On Dec 23, 2009 6:16 PM, "Andrew Bonventre (Bons)" <andybons@google.com>
> wrote:
>
> I agree, but we have win, which has each image be it's own proper
> subview and can therefore call schedulePaint, linux, which seems to
> redraw if the layout changes regardless and mac, where the image view
> classes are not proper subclasses of NSView and therefore cannot call
> redraw in the same way that windows does.
>
> If I were to change things to be more consistent, I would change the
> other implementations to be looking for the page action visibility
> notification change instead of redrawing in every function where it
> _may_ have changed.
>
> A
>
> On Wed, Dec 23, 2009 at 8:45 PM, <thakis@chromium.org> wrote: > The code
> looks alright, but I'm no...

[Pam (message me for reviews), 2009-12-28 23:19:51]

I'd propose leaving this as it is, with a TODO and/or bug to make the three
implementations more consistent. There's already a ton of duplicated code in the
location bar, and it'd be nice to straighten out the whole mess at once rather
than piecemeal.

Aside: would it be helpful to make the Mac image "view" classes truly subclasses
of NSView, or is that more complication than it's worth?

- Pam

On 2009/12/24 02:43:54, andybons_google.com wrote:
> I'll submit and wait for comments from Pam, who was also looking into
> this, as well as look for a way to fix the plumbing since I'll be in
> this code doing tooltips and cursor changes as well.
> 
> On Wed, Dec 23, 2009 at 9:36 PM, Nico Weber <mailto:thakis@chromium.org>
wrote:
> > Your call. LG if you don't want to do that, but it'd be nicer IMHO.
> >
> > On Dec 23, 2009 6:16 PM, "Andrew Bonventre (Bons)"
<mailto:andybons@google.com>
> > wrote:
> >
> > I agree, but we have win, which has each image be it's own proper
> > subview and can therefore call schedulePaint, linux, which seems to
> > redraw if the layout changes regardless and mac, where the image view
> > classes are not proper subclasses of NSView and therefore cannot call
> > redraw in the same way that windows does.
> >
> > If I were to change things to be more consistent, I would change the
> > other implementations to be looking for the page action visibility
> > notification change instead of redrawing in every function where it
> > _may_ have changed.
> >
> > A
> >
> > On Wed, Dec 23, 2009 at 8:45 PM, <mailto:thakis@chromium.org> wrote: > The
code
> > looks alright, but I'm no...
>

[andybons, 2009-12-29 03:25:35]

On Mon, Dec 28, 2009 at 6:19 PM,  <pam@chromium.org> wrote:
> I'd propose leaving this as it is, with a TODO and/or bug to make the three
> implementations more consistent. There's already a ton of duplicated code in
> the
> location bar, and it'd be nice to straighten out the whole mess at once
> rather
> than piecemeal.
Agreed.

> Aside: would it be helpful to make the Mac image "view" classes truly
> subclasses
> of NSView, or is that more complication than it's worth?

It would simplify the tooltip code that I'm about to send out and
maybe fix this cursor bug I'm running into, but it depends on
implementation cost. If you have time, then go for it :).

>
> - Pam
>
> On 2009/12/24 02:43:54, andybons_google.com wrote:
>>
>> I'll submit and wait for comments from Pam, who was also looking into
>> this, as well as look for a way to fix the plumbing since I'll be in
>> this code doing tooltips and cursor changes as well.
>
>> On Wed, Dec 23, 2009 at 9:36 PM, Nico Weber <mailto:thakis@chromium.org>
>
> wrote:
>>
>> > Your call. LG if you don't want to do that, but it'd be nicer IMHO.
>> >
>> > On Dec 23, 2009 6:16 PM, "Andrew Bonventre (Bons)"
>
> <mailto:andybons@google.com>
>>
>> > wrote:
>> >
>> > I agree, but we have win, which has each image be it's own proper
>> > subview and can therefore call schedulePaint, linux, which seems to
>> > redraw if the layout changes regardless and mac, where the image view
>> > classes are not proper subclasses of NSView and therefore cannot call
>> > redraw in the same way that windows does.
>> >
>> > If I were to change things to be more consistent, I would change the
>> > other implementations to be looking for the page action visibility
>> > notification change instead of redrawing in every function where it
>> > _may_ have changed.
>> >
>> > A
>> >
>> > On Wed, Dec 23, 2009 at 8:45 PM, <mailto:thakis@chromium.org> wrote: >
>> > The
>
> code
>>
>> > looks alright, but I'm no...
>
>
>
>
> http://codereview.chromium.org/515027
>