Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issue 195050: Linux: implement Page Actions support. (Closed)

Created:
11 years, 3 months ago by mattm
Modified:
9 years, 7 months ago
Reviewers:
Finnur, Elliot Glaysher
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 18

Patch Set 2 : review updates #

Total comments: 2

Patch Set 3 : comment clarification #

Unified diffs Side-by-side diffs Delta from patch set Stats (+449 lines, -172 lines) Patch
M chrome/browser/extensions/extension_browsertests_misc.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extensions_service.h View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extensions_service.cc View 1 chunk +16 lines, -0 lines 0 comments Download
M chrome/browser/gtk/browser_toolbar_gtk.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/gtk/location_bar_view_gtk.h View 6 chunks +63 lines, -1 line 0 comments Download
M chrome/browser/gtk/location_bar_view_gtk.cc View 9 chunks +155 lines, -4 lines 0 comments Download
A chrome/browser/image_loading_tracker.h View 1 2 1 chunk +72 lines, -0 lines 0 comments Download
A chrome/browser/image_loading_tracker.cc View 1 chunk +114 lines, -0 lines 0 comments Download
M chrome/browser/views/location_bar_view.h View 1 4 chunks +6 lines, -13 lines 0 comments Download
M chrome/browser/views/location_bar_view.cc View 1 8 chunks +13 lines, -151 lines 0 comments Download
M chrome/chrome.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
mattm
Is passing Browser to the LocationBarViewGtk ugly? Views has a LocationBarView::Delegate which ToolbarView implements and ...
11 years, 3 months ago (2009-09-10 05:00:38 UTC) #1
Elliot Glaysher
On 2009/09/10 05:00:38, mattm wrote: > Is passing Browser to the LocationBarViewGtk ugly? Views has ...
11 years, 3 months ago (2009-09-10 16:39:17 UTC) #2
Finnur
I didn't review the Linux specific parts thoroughly, I assumed that's what Elliot was for. ...
11 years, 3 months ago (2009-09-10 20:14:23 UTC) #3
mattm
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, ...
11 years, 3 months ago (2009-09-10 21:08:09 UTC) #4
Finnur
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, ...
11 years, 3 months ago (2009-09-10 21:40:32 UTC) #5
mattm
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 ...
11 years, 3 months ago (2009-09-10 22:10:22 UTC) #6
Finnur
I see. There has been a subtle change in how we keep track of what's ...
11 years, 3 months ago (2009-09-10 22:34:59 UTC) #7
mattm
11 years, 3 months ago (2009-09-10 22:39:11 UTC) #8
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
>

Powered by Google App Engine
This is Rietveld 408576698