Linux: implement Page Actions support.

Linux: implement Page Actions support.

BUG=11973
TEST=load an extension with page actions, it should work like on windows.

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

[mattm, 2009-09-10 05:00:38]

Is passing Browser to the LocationBarViewGtk ugly?  Views has a
LocationBarView::Delegate which ToolbarView implements and it has a browser
pointer.  Dunno if it's worth implementing something similar for gtk?

[Elliot Glaysher, 2009-09-10 16:39:17]

On 2009/09/10 05:00:38, mattm wrote:
> Is passing Browser to the LocationBarViewGtk ugly?  Views has a
> LocationBarView::Delegate which ToolbarView implements and it has a browser
> pointer.  Dunno if it's worth implementing something similar for gtk?

It may be cleaner, but I'd say don't bother.

Also, LGTM.

[Finnur, 2009-09-10 20:14:23]

I didn't review the Linux specific parts thoroughly, I assumed that's what
Elliot was for.

LGTM, with nits (below).

http://codereview.chromium.org/195050/diff/1/6
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/195050/diff/1/6#newcode245
Line 245: gtk_box_pack_end(GTK_BOX(hbox_.get()), page_action_hbox_, FALSE,
FALSE, 0);
I'm not clear on where the page action icons will be painted? Will they appear
to the right or to the left of the security icon?

http://codereview.chromium.org/195050/diff/1/9
File chrome/browser/image_loading_tracker.h (right):

http://codereview.chromium.org/195050/diff/1/9#newcode25
Line 25: explicit ImageLoadingTracker(Observer* observer, size_t image_count)
This doesn't need to be 'explicit'.

http://codereview.chromium.org/195050/diff/1/9#newcode31
Line 31: virtual ~ImageLoadingTracker() {}
I don't feel strongly about this, but it seems to me that since you are not
deleting this class through the base class nor do you have virtual functions so
marking the constructor virtual is not needed.

http://codereview.chromium.org/195050/diff/1/9#newcode37
Line 37: void PostLoadImageTask(FilePath path);
Please add a comment. This may be me forgetting to comment originally, but since
you are touching this...

http://codereview.chromium.org/195050/diff/1/9#newcode42
Line 42: void OnImageLoaded(SkBitmap* image, size_t index);
Can you comment this as well?

http://codereview.chromium.org/195050/diff/1/9#newcode51
Line 51: size_t posted_count_;
Why do you keep track of this?

http://codereview.chromium.org/195050/diff/1/10
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/195050/diff/1/10#newcode659
Line 659: page_actions = profile_->GetExtensionsService()->GetPageActions();
Make sure you test this in Incognito mode (see bug 12326).
Also, indentation here is off (2, not 4 spaces)

http://codereview.chromium.org/195050/diff/1/11
File chrome/browser/views/location_bar_view.h (right):

http://codereview.chromium.org/195050/diff/1/11#newcode357
Line 357: virtual void OnImageLoaded(SkBitmap* image, size_t index);
nit: To keep with the conventions of this file, this function should be moved
above UpdateVisibility with a comment saying "Overridden from
ImageLoadingTracker."

I think the current comment above it belongs to the interface definition, not
the override function.

[mattm, 2009-09-10 21:08:09]

http://codereview.chromium.org/195050/diff/1/6
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/195050/diff/1/6#newcode245
Line 245: gtk_box_pack_end(GTK_BOX(hbox_.get()), page_action_hbox_, FALSE,
FALSE, 0);
On 2009/09/10 20:14:23, Finnur wrote:
> I'm not clear on where the page action icons will be painted? Will they appear
> to the right or to the left of the security icon?

They appear to the left of the security icon.  (gtk_box_pack_end packs from the
right edge and moves left).

Example: http://www/~mattm/no_crawl/chrome/location_bar_padding/comparison1.png

http://codereview.chromium.org/195050/diff/1/9
File chrome/browser/image_loading_tracker.h (right):

http://codereview.chromium.org/195050/diff/1/9#newcode25
Line 25: explicit ImageLoadingTracker(Observer* observer, size_t image_count)
On 2009/09/10 20:14:23, Finnur wrote:
> This doesn't need to be 'explicit'.

Done.

http://codereview.chromium.org/195050/diff/1/9#newcode31
Line 31: virtual ~ImageLoadingTracker() {}
On 2009/09/10 20:14:23, Finnur wrote:
> I don't feel strongly about this, but it seems to me that since you are not
> deleting this class through the base class nor do you have virtual functions
so
> marking the constructor virtual is not needed.

Ah, hm.  I see you're right that RefCountedThreadSafe does cast it to the actual
type before deleting.  Removed the virtual.

http://codereview.chromium.org/195050/diff/1/9#newcode37
Line 37: void PostLoadImageTask(FilePath path);
On 2009/09/10 20:14:23, Finnur wrote:
> Please add a comment. This may be me forgetting to comment originally, but
since
> you are touching this...

Done.

http://codereview.chromium.org/195050/diff/1/9#newcode42
Line 42: void OnImageLoaded(SkBitmap* image, size_t index);
On 2009/09/10 20:14:23, Finnur wrote:
> Can you comment this as well?

Done.

http://codereview.chromium.org/195050/diff/1/9#newcode51
Line 51: size_t posted_count_;
On 2009/09/10 20:14:23, Finnur wrote:
> Why do you keep track of this?

It's how we know what |index| to pass to LoadImageTask (and thus to
OnImageLoaded.)  It was originally tracked by the caller, but I moved that into
PostloadImageTask.

http://codereview.chromium.org/195050/diff/1/10
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/195050/diff/1/10#newcode659
Line 659: page_actions = profile_->GetExtensionsService()->GetPageActions();
On 2009/09/10 20:14:23, Finnur wrote:
> Make sure you test this in Incognito mode (see bug 12326).
Thanks, tested linux with incognito mode, looks good.  By code inspection it
seems windows should be the same as before (haven't gotten a windows box to test
on.) 

> Also, indentation here is off (2, not 4 spaces)

Fixed.

http://codereview.chromium.org/195050/diff/1/11
File chrome/browser/views/location_bar_view.h (right):

http://codereview.chromium.org/195050/diff/1/11#newcode357
Line 357: virtual void OnImageLoaded(SkBitmap* image, size_t index);
On 2009/09/10 20:14:23, Finnur wrote:
> nit: To keep with the conventions of this file, this function should be moved
> above UpdateVisibility with a comment saying "Overridden from
> ImageLoadingTracker."
> 
> I think the current comment above it belongs to the interface definition, not
> the override function.

Done.

[Finnur, 2009-09-10 21:40:32]

LG with one nit.

http://codereview.chromium.org/195050/diff/1/6
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/195050/diff/1/6#newcode245
Line 245: gtk_box_pack_end(GTK_BOX(hbox_.get()), page_action_hbox_, FALSE,
FALSE, 0);
Excellent.

As a side note I thought I should mention that you might have a future in UI
design, judging by those icons. :)

http://codereview.chromium.org/195050/diff/1/9
File chrome/browser/image_loading_tracker.h (right):

http://codereview.chromium.org/195050/diff/1/9#newcode51
Line 51: size_t posted_count_;
Ah, right. Of course. I missed that.

http://codereview.chromium.org/195050/diff/3003/33#newcode44
Line 44: // The view that is waiting for the image to load.
I don't understand the latter part of this comment. |image_count| keeps track of
how many time this function has been called, not the other way around...?

[mattm, 2009-09-10 22:10:22]

http://codereview.chromium.org/195050/diff/3003/33
File chrome/browser/image_loading_tracker.h (right):

http://codereview.chromium.org/195050/diff/3003/33#newcode44
Line 44: // exactly |image_count| times.)
On 2009/09/10 21:40:33, Finnur wrote:
> I don't understand the latter part of this comment. |image_count| keeps track
of
> how many time this function has been called, not the other way around...?


posted_count_ keeps track of how many times it's been called, image_count keeps
track of how many images have yet to load.  Clarified the comment to indicate
this is the image_count passed into the constructor, not the member variable.
(If you call it too few times the tracker object will leak, too many and it'll
delete itself while it still has pending tasks..)

[Finnur, 2009-09-10 22:34:59]

I see. 

There has been a subtle change in how we keep track of what's left to load. 

LGTM.

On 2009/09/10 22:10:22, mattm wrote:
> http://codereview.chromium.org/195050/diff/3003/33
> File chrome/browser/image_loading_tracker.h (right):
> 
> http://codereview.chromium.org/195050/diff/3003/33#newcode44
> Line 44: // exactly |image_count| times.)
> On 2009/09/10 21:40:33, Finnur wrote:
> > I don't understand the latter part of this comment. |image_count| keeps
track
> of
> > how many time this function has been called, not the other way around...?
> 
> 
> posted_count_ keeps track of how many times it's been called, image_count
keeps
> track of how many images have yet to load.  Clarified the comment to indicate
> this is the image_count passed into the constructor, not the member variable.
> (If you call it too few times the tracker object will leak, too many and it'll
> delete itself while it still has pending tasks..)

[mattm, 2009-09-10 22:39:11]

I don't think there's any change, other than previously the caller did
the PostTask directly and now they go through PostLoadImageTask.  In
both cases they have to call it the right number of times.

On Thu, Sep 10, 2009 at 15:34,  <finnur@chromium.org> wrote:
> I see.
>
> There has been a subtle change in how we keep track of what's left to loa=
d.
>
> LGTM.
>
> On 2009/09/10 22:10:22, mattm wrote:
>>
>> http://codereview.chromium.org/195050/diff/3003/33
>> File chrome/browser/image_loading_tracker.h (right):
>
>> http://codereview.chromium.org/195050/diff/3003/33#newcode44
>> Line 44: // exactly |image_count| times.)
>> On 2009/09/10 21:40:33, Finnur wrote:
>> > I don't understand the latter part of this comment. |image_count| keep=
s
>
> track
>>
>> of
>> > how many time this function has been called, not the other way
>> > around...?
>
>
>> posted_count_ keeps track of how many times it's been called, image_coun=
t
>
> keeps
>>
>> track of how many images have yet to load. =A0Clarified the comment to
>> indicate
>> this is the image_count passed into the constructor, not the member
>> variable.
>> (If you call it too few times the tracker object will leak, too many and
>> it'll
>> delete itself while it still has pending tasks..)
>
>
>
> http://codereview.chromium.org/195050
>