(GTK only) Add icons to the "Permissions" tab of the Website Settings UI.

(GTK only) Add icons to the "Permissions" tab of the Website Settings UI.


BUG=140450
TBR=ben@chromium.org


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

[markusheintz_, 2012-08-16 11:54:30]

Hey Elliot please review this CL. Thanks a lot.

FYI: I'd like to create a separate directory
"chrome/browser/ui/gtk/website_settings" and move all the files for the GTK
implementation of the Website Settings UI there.

[tfarina, 2012-08-16 12:09:31]

On Thu, Aug 16, 2012 at 8:54 AM,  <markusheintz@chromium.org> wrote:
> Reviewers: Elliot Glaysher,
>
> Message:
> Hey Elliot please review this CL. Thanks a lot.
>
> FYI: I'd like to create a separate directory
> "chrome/browser/ui/gtk/website_settings" and move all the files for the GTK
> implementation of the Website Settings UI there.
>
Yes, please do so! This is to mirror chrome/browser/ui/views/website_settings/

> Description:
> (GTK only) Add icons to the "Permissions" tab of the Website Settings UI.
>
>
> BUG=140450
>
>
> Please review this at http://codereview.chromium.org/10827374/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   A chrome/browser/ui/gtk/permission_selector.h
>   A chrome/browser/ui/gtk/permission_selector.cc
>   A chrome/browser/ui/gtk/permission_selector_observer.h
>   M chrome/browser/ui/gtk/website_settings_popup_gtk.h
>   M chrome/browser/ui/gtk/website_settings_popup_gtk.cc
>   M chrome/browser/ui/views/website_settings/permission_selector_view.cc
>   M chrome/chrome_browser.gypi
>
>



-- 
Thiago

[tfarina, 2012-08-16 12:11:14]

http://codereview.chromium.org/10827374/diff/9/chrome/browser/ui/views/websit...
File chrome/browser/ui/views/website_settings/permission_selector_view.cc
(right):

http://codereview.chromium.org/10827374/diff/9/chrome/browser/ui/views/websit...
chrome/browser/ui/views/website_settings/permission_selector_view.cc:323:
menu_button_model_->current_setting(),
nit: indentation is wrong here. indent just 4 spaces.

[markusheintz_, 2012-08-16 14:53:08]

I added a the dir "chrome/browser/ui/gtk/website_settings" and moved the
corresponding files to the new dir.

http://codereview.chromium.org/10827374/diff/9/chrome/browser/ui/views/websit...
File chrome/browser/ui/views/website_settings/permission_selector_view.cc
(right):

http://codereview.chromium.org/10827374/diff/9/chrome/browser/ui/views/websit...
chrome/browser/ui/views/website_settings/permission_selector_view.cc:323:
menu_button_model_->current_setting(),
On 2012/08/16 12:11:14, tfarina wrote:
> nit: indentation is wrong here. indent just 4 spaces.

Done.

[Elliot Glaysher, 2012-08-16 16:37:42]

gtk lgtm

[tfarina, 2012-08-16 17:16:09]

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
File chrome/browser/ui/gtk/website_settings/permission_selector.h (right):

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/permission_selector.h:22: class
PermissionSelector {
If I recall, we have PermissionSelectorView which inherits from views::View,
right? So, just as a naming suggestion, I'd name this PermissionSelectorGtk to
pair with the view one. But seems fine as is though, since this does not inherit
from anything.

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/permission_selector.h:65: GtkWidget*
icon_;
can you forward declare this instead of including gtk/gtk.h?
typedef struct GtkWidget_ GtkWidget;

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.h
(right):

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.h:55: virtual
void OnPermissionChanged(PermissionSelector* selector);
these overriddes are missing the OVERRIDE in the end.

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.h:56: virtual
void OnComboboxShown();
and the order should match with the other of the inheritance. i.e, first the
overriddes from WebsiteSettingsUI, then the overriddes from
PermissionSelectorObserver and so on.

[markusheintz_, 2012-08-17 12:54:03]

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
File chrome/browser/ui/gtk/website_settings/permission_selector.h (right):

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/permission_selector.h:22: class
PermissionSelector {
On 2012/08/16 17:16:09, tfarina wrote:
> If I recall, we have PermissionSelectorView which inherits from views::View,
> right? So, just as a naming suggestion, I'd name this PermissionSelectorGtk to
> pair with the view one. But seems fine as is though, since this does not
inherit
> from anything.

This is exactly why I didn't add the GTK suffix. It does not inherit for a
widget or anyother GTK class.

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/permission_selector.h:65: GtkWidget*
icon_;
On 2012/08/16 17:16:09, tfarina wrote:
> can you forward declare this instead of including gtk/gtk.h?
> typedef struct GtkWidget_ GtkWidget;

hm ... the compiler does not like that there is another forward declaration in:
chrome/browser/ui/gtk/gtk_theme_service.h

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.h
(right):

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.h:55: virtual
void OnPermissionChanged(PermissionSelector* selector);
On 2012/08/16 17:16:09, tfarina wrote:
> these overriddes are missing the OVERRIDE in the end.

Done.

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.h:56: virtual
void OnComboboxShown();
On 2012/08/16 17:16:09, tfarina wrote:
> and the order should match with the other of the inheritance. i.e, first the
> overriddes from WebsiteSettingsUI, then the overriddes from
> PermissionSelectorObserver and so on.

Done.

[tfarina, 2012-08-17 13:58:59]

lgtm with the following comment.

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
File chrome/browser/ui/gtk/website_settings/permission_selector.h (right):

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/permission_selector.h:65: GtkWidget*
icon_;
On 2012/08/17 12:54:03, markusheintz_ wrote:
> On 2012/08/16 17:16:09, tfarina wrote:
> > can you forward declare this instead of including gtk/gtk.h?
> > typedef struct GtkWidget_ GtkWidget;
> 
> hm ... the compiler does not like that there is another forward declaration
in:
> chrome/browser/ui/gtk/gtk_theme_service.h
so we safely remove the include to gtk.h :)

[markusheintz_, 2012-08-17 16:10:37]

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
File chrome/browser/ui/gtk/website_settings/permission_selector.h (right):

http://codereview.chromium.org/10827374/diff/3009/chrome/browser/ui/gtk/websi...
chrome/browser/ui/gtk/website_settings/permission_selector.h:65: GtkWidget*
icon_;
Sorry ignore what I wrote here. Apparently I can't read compiler error messages.
I should not have worked on this after coming from a b-day party late at night
;-).

I addressed your initial comment :-).

On 2012/08/17 13:58:59, tfarina wrote:
> On 2012/08/17 12:54:03, markusheintz_ wrote:
> > On 2012/08/16 17:16:09, tfarina wrote:
> > > can you forward declare this instead of including gtk/gtk.h?
> > > typedef struct GtkWidget_ GtkWidget;
> > 
> > hm ... the compiler does not like that there is another forward declaration
> in:
> > chrome/browser/ui/gtk/gtk_theme_service.h
> so we safely remove the include to gtk.h :)

[commit-bot: I haz the power, 2012-08-17 16:11:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10827374/9006

[commit-bot: I haz the power, 2012-08-17 18:11:43]

Try job failure for 10827374-9006 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-17 19:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10827374/8010

[commit-bot: I haz the power, 2012-08-17 20:41:38]

Try job failure for 10827374-8010 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-17 22:18:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10827374/9009

[commit-bot: I haz the power, 2012-08-18 02:21:59]

Change committed as 152220