[Web Intents] CWS inline install for GTK

[Web Intents] CWS inline install for GTK

BUG=113476
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130238

[binji, 2012-03-30 21:11:11]

screenshots: http://code.google.com/p/chromium/issues/detail?id=113476#c25

[James Hawkins, 2012-03-30 21:35:01]

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/gtk_ch...
File chrome/browser/ui/gtk/gtk_chrome_link_button.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/gtk_ch...
chrome/browser/ui/gtk/gtk_chrome_link_button.cc:82: "black",
Do we need to read system prefs for this color?

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:20: }
}  // namespace

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:28: theme_service_->InitThemesFor(this);
Move these two to Init()?

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:57:
DCHECK(chrome::NOTIFICATION_BROWSER_THEME_CHANGED == type);
nit: DCHECK_EQ

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:97: DCHECK_EQ(width % height, 0);
nit: Constant value should be the first parameter.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.h (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:24: class ThrobberGtk : public
ui::AnimationDelegate,
This hasn't existed previously?

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:27: explicit ThrobberGtk(ThemeServiceGtk*
theme_service);
nit: Document |theme_service|.  Probably must not be NULL.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:56: // The theme service.
Document ownership (probably weak pointer).

[Evan Stade, 2012-03-30 21:38:25]

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/gtk_ch...
File chrome/browser/ui/gtk/gtk_chrome_link_button.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/gtk_ch...
chrome/browser/ui/gtk/gtk_chrome_link_button.cc:82: "black",
I don't think this will look too good on dark GTK themes.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:69: gfx::CairoCachedSurface* ccs =
frames_->ToCairo();
no opaque variable name abbreviations

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.h (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:63: ui::SlideAnimation animation_;
ThrobAnimation?

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:324: EnableWidgetCallback(hbox,
&enabled);
not really necessary; imo decreases readability as opposed to just making the
set_sensitive call from here

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:415:
theme_service->UsingNativeTheme());
AFAIK the picker is always natively themed (does the background ever change?)

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:420: // Throbber, which will be
added to the heirarchy when necessary.
sp.

[binji, 2012-03-30 23:03:51]

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/gtk_ch...
File chrome/browser/ui/gtk/gtk_chrome_link_button.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/gtk_ch...
chrome/browser/ui/gtk/gtk_chrome_link_button.cc:82: "black",
On 2012/03/30 21:38:25, Evan Stade wrote:
> I don't think this will look too good on dark GTK themes.

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:20: }
On 2012/03/30 21:35:02, James Hawkins wrote:
> }  // namespace

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:28: theme_service_->InitThemesFor(this);
On 2012/03/30 21:35:02, James Hawkins wrote:
> Move these two to Init()?

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:57:
DCHECK(chrome::NOTIFICATION_BROWSER_THEME_CHANGED == type);
On 2012/03/30 21:35:02, James Hawkins wrote:
> nit: DCHECK_EQ

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:69: gfx::CairoCachedSurface* ccs =
frames_->ToCairo();
On 2012/03/30 21:38:25, Evan Stade wrote:
> no opaque variable name abbreviations

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.cc:97: DCHECK_EQ(width % height, 0);
On 2012/03/30 21:35:02, James Hawkins wrote:
> nit: Constant value should be the first parameter.

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.h (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:24: class ThrobberGtk : public
ui::AnimationDelegate,
On 2012/03/30 21:35:02, James Hawkins wrote:
> This hasn't existed previously?

Not as a standalone widget, AFAICT.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:27: explicit ThrobberGtk(ThemeServiceGtk*
theme_service);
On 2012/03/30 21:35:02, James Hawkins wrote:
> nit: Document |theme_service|.  Probably must not be NULL.

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:56: // The theme service.
On 2012/03/30 21:35:02, James Hawkins wrote:
> Document ownership (probably weak pointer).

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:63: ui::SlideAnimation animation_;
On 2012/03/30 21:38:25, Evan Stade wrote:
> ThrobAnimation?

Tried that first, seems like it autoreverses.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:324: EnableWidgetCallback(hbox,
&enabled);
On 2012/03/30 21:38:25, Evan Stade wrote:
> not really necessary; imo decreases readability as opposed to just making the
> set_sensitive call from here

Done.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:415:
theme_service->UsingNativeTheme());
On 2012/03/30 21:38:25, Evan Stade wrote:
> AFAIK the picker is always natively themed (does the background ever change?)

Yes, when I click through GTK themes, it changes the background.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:420: // Throbber, which will be
added to the heirarchy when necessary.
On 2012/03/30 21:38:25, Evan Stade wrote:
> sp.

Done.

[Evan Stade, 2012-03-31 01:24:35]

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.h (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:63: ui::SlideAnimation animation_;
On 2012/03/30 23:03:51, binji wrote:
> On 2012/03/30 21:38:25, Evan Stade wrote:
> > ThrobAnimation?
> 
> Tried that first, seems like it autoreverses.

in what way? it goes counter clockwise?

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:415:
theme_service->UsingNativeTheme());
On 2012/03/30 23:03:51, binji wrote:
> On 2012/03/30 21:38:25, Evan Stade wrote:
> > AFAIK the picker is always natively themed (does the background ever
change?)
> 
> Yes, when I click through GTK themes, it changes the background.

those are native themes. It is always natively themed. It never follows the
browser theme.

[binji, 2012-04-02 18:22:35]

I've also made UI changes as suggested:
http://code.google.com/p/chromium/issues/detail?id=113476#c27

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
File chrome/browser/ui/gtk/throbber_gtk.h (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/throbb...
chrome/browser/ui/gtk/throbber_gtk.h:63: ui::SlideAnimation animation_;
On 2012/03/31 01:24:35, Evan Stade wrote:
> On 2012/03/30 23:03:51, binji wrote:
> > On 2012/03/30 21:38:25, Evan Stade wrote:
> > > ThrobAnimation?
> > 
> > Tried that first, seems like it autoreverses.
> 
> in what way? it goes counter clockwise?

Yes. As I understand it, the ThrobAnimation is useful for pulsing, not cycling.

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/9837011/diff/7006/chrome/browser/ui/gtk/web_in...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:415:
theme_service->UsingNativeTheme());
On 2012/03/31 01:24:35, Evan Stade wrote:
> On 2012/03/30 23:03:51, binji wrote:
> > On 2012/03/30 21:38:25, Evan Stade wrote:
> > > AFAIK the picker is always natively themed (does the background ever
> change?)
> > 
> > Yes, when I click through GTK themes, it changes the background.
> 
> those are native themes. It is always natively themed. It never follows the
> browser theme.

OK, removed.

[James Hawkins, 2012-04-02 20:59:05]

lgtm

[binji, 2012-04-02 21:10:11]

On 2012/04/02 20:59:05, James Hawkins wrote:
> lgtm

evan: Can I handle further review feedback in another CL? We'd like to get this
in soon.

[Evan Stade, 2012-04-02 21:16:40]

lgtm

http://codereview.chromium.org/9837011/diff/7009/chrome/browser/ui/gtk/gtk_ch...
File chrome/browser/ui/gtk/gtk_chrome_link_button.cc (right):

http://codereview.chromium.org/9837011/diff/7009/chrome/browser/ui/gtk/gtk_ch...
chrome/browser/ui/gtk/gtk_chrome_link_button.cc:44:
g_free(button->insensitive_markup);
nit: add this after pressed_markup

[binji, 2012-04-02 21:36:05]

http://codereview.chromium.org/9837011/diff/7009/chrome/browser/ui/gtk/gtk_ch...
File chrome/browser/ui/gtk/gtk_chrome_link_button.cc (right):

http://codereview.chromium.org/9837011/diff/7009/chrome/browser/ui/gtk/gtk_ch...
chrome/browser/ui/gtk/gtk_chrome_link_button.cc:44:
g_free(button->insensitive_markup);
On 2012/04/02 21:16:40, Evan Stade wrote:
> nit: add this after pressed_markup

Done.

[Evan Stade, 2012-04-03 03:23:58]

ah I think ThrobAnimation would be better named PulseAnimation (it's used for
the tab color pulsing, e.g. when you get a gchat message on a pinned tab)