Add icons to the views Website Settings UI.

Add icons to the views Website Settings UI.

Plus sort the methods in website_settings_popup_view.h and website_settings_popup_view.cc in the same order.


BUG=113688
TEST=Icons are visible in the UI on Windows

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

[markusheintz_, 2012-06-14 11:38:03]

@Finnur: Would you mind taking a look at this CL. I'd appreciate your input
this. Thank a lot.

[Finnur, 2012-06-14 12:32:17]

I'd like to see a screenshot before commenting further... Hopefully a
before/after, if you have both....

https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
chrome/browser/ui/website_settings/website_settings_ui.cc:92: case
CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
What's the plan for this?

https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
chrome/browser/ui/website_settings/website_settings_ui.cc:156: }
Neither of those functions handle SITE_IDENTITY_STATUS_INTERNAL_PAGE. Is that
intentional?

[Finnur, 2012-06-14 12:33:18]

I left out a bit of explanation...

I'd like to see a screenshot before commenting further to help me understand
this change better...

On 2012/06/14 12:32:17, Finnur wrote:
> I'd like to see a screenshot before commenting further... Hopefully a
> before/after, if you have both....
> 
>
https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
> File chrome/browser/ui/website_settings/website_settings_ui.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
> chrome/browser/ui/website_settings/website_settings_ui.cc:92: case
> CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
> What's the plan for this?
> 
>
https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
> chrome/browser/ui/website_settings/website_settings_ui.cc:156: }
> Neither of those functions handle SITE_IDENTITY_STATUS_INTERNAL_PAGE. Is that
> intentional?

[markusheintz_, 2012-06-14 12:55:21]

I shared "befor" and "after" screenshots with you.

https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
chrome/browser/ui/website_settings/website_settings_ui.cc:92: case
CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
On 2012/06/14 12:32:17, Finnur wrote:
> What's the plan for this?

I don't have icons yet :(. But I'm working on it. It's just a but difficult
since Glen is still asleep :). But I will add the propper icons asap

https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
chrome/browser/ui/website_settings/website_settings_ui.cc:156: }
On 2012/06/14 12:32:17, Finnur wrote:
> Neither of those functions handle SITE_IDENTITY_STATUS_INTERNAL_PAGE. Is that
> intentional?

Yes. As for internal pages I intend to display different content (To be
implemented asap - like tomorrow)

[markusheintz_, 2012-06-14 12:56:39]

I also shared a mock with you that show how the UI should look in the end.

On 2012/06/14 12:55:21, markusheintz_ wrote:
> I shared "befor" and "after" screenshots with you.
> 
>
https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
> File chrome/browser/ui/website_settings/website_settings_ui.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
> chrome/browser/ui/website_settings/website_settings_ui.cc:92: case
> CONTENT_SETTINGS_TYPE_NOTIFICATIONS:
> On 2012/06/14 12:32:17, Finnur wrote:
> > What's the plan for this?
> 
> I don't have icons yet :(. But I'm working on it. It's just a but difficult
> since Glen is still asleep :). But I will add the propper icons asap
> 
>
https://chromiumcodereview.appspot.com/10534138/diff/10/chrome/browser/ui/web...
> chrome/browser/ui/website_settings/website_settings_ui.cc:156: }
> On 2012/06/14 12:32:17, Finnur wrote:
> > Neither of those functions handle SITE_IDENTITY_STATUS_INTERNAL_PAGE. Is
that
> > intentional?
> 
> Yes. As for internal pages I intend to display different content (To be
> implemented asap - like tomorrow)

[Finnur, 2012-06-14 13:35:46]

http://codereview.chromium.org/10534138/diff/10/chrome/browser/ui/views/websi...
File chrome/browser/ui/views/website_settings_popup_view.cc (right):

http://codereview.chromium.org/10534138/diff/10/chrome/browser/ui/views/websi...
chrome/browser/ui/views/website_settings_popup_view.cc:699: string16 text) {
const string16& ?

http://codereview.chromium.org/10534138/diff/10/chrome/browser/ui/views/websi...
chrome/browser/ui/views/website_settings_popup_view.cc:704:
label->SetAllowCharacterBreak(true);
I forget... What does this do again? Allow you to split a word in the middle of
it?

[markusheintz_, 2012-06-14 14:09:43]

http://codereview.chromium.org/10534138/diff/10/chrome/browser/ui/views/websi...
File chrome/browser/ui/views/website_settings_popup_view.cc (right):

http://codereview.chromium.org/10534138/diff/10/chrome/browser/ui/views/websi...
chrome/browser/ui/views/website_settings_popup_view.cc:699: string16 text) {
On 2012/06/14 13:35:46, Finnur wrote:
> const string16& ?

This is 5 bucks in the piggy banks labeled "stupid things I should not do in my
CLs".

Done

http://codereview.chromium.org/10534138/diff/10/chrome/browser/ui/views/websi...
chrome/browser/ui/views/website_settings_popup_view.cc:704:
label->SetAllowCharacterBreak(true);
On 2012/06/14 13:35:46, Finnur wrote:
> I forget... What does this do again? Allow you to split a word in the middle
of
> it?

Sets whether the label text can be split on words.
Default is false. This only works when is_multi_line is true.

[Finnur, 2012-06-14 14:37:19]

LGTM with the caveat that this UI is a stepping stone to a slightly different UI
so I will not fuzz over needed padding in some places.

Good luck filling the piggy bank. :)

[markusheintz_, 2012-06-14 15:12:21]

Thanks a lot Finnur for the review.

@Scott: Could you please review this CL for OWNERS approval. Thanks a lot.

[sky, 2012-06-14 15:48:40]

https://chromiumcodereview.appspot.com/10534138/diff/1005/chrome/browser/ui/w...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10534138/diff/1005/chrome/browser/ui/w...
chrome/browser/ui/website_settings/website_settings_ui.h:96: static gfx::Image*
GetPermissionIcon(ContentSettingsType type,
Why are you returning pointers here? Ownership is clearer if you don't and it
also means folks don't have to worry about NULL.

[markusheintz_, 2012-06-14 16:41:13]

https://chromiumcodereview.appspot.com/10534138/diff/1005/chrome/browser/ui/w...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10534138/diff/1005/chrome/browser/ui/w...
chrome/browser/ui/website_settings/website_settings_ui.h:96: static gfx::Image*
GetPermissionIcon(ContentSettingsType type,
On 2012/06/14 15:48:40, sky wrote:
> Why are you returning pointers here? Ownership is clearer if you don't and it
> also means folks don't have to worry about NULL.

I changed it to const refs.

[sky, 2012-06-14 17:02:59]

I don't think you even need const since RB doesn't return const. But as long as
this works its fine. LGTM

[markusheintz_, 2012-06-14 18:18:08]

On 2012/06/14 17:02:59, sky wrote:
> I don't think you even need const since RB doesn't return const. But as long
as
> this works its fine. LGTM

Thanks for the review.

[commit-bot: I haz the power, 2012-06-14 18:18:27]

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

[commit-bot: I haz the power, 2012-06-14 19:54:22]

Change committed as 142210

[tfarina, 2012-06-14 19:59:32]

https://chromiumcodereview.appspot.com/10534138/diff/6006/chrome/browser/ui/w...
File chrome/browser/ui/website_settings/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10534138/diff/6006/chrome/browser/ui/w...
chrome/browser/ui/website_settings/website_settings_ui.h:97: ContentSetting
setting);
nit: wrong indentation here!

[tfarina, 2012-06-14 20:00:45]

https://chromiumcodereview.appspot.com/10534138/diff/6006/chrome/browser/ui/w...
File chrome/browser/ui/website_settings/website_settings_ui.cc (right):

https://chromiumcodereview.appspot.com/10534138/diff/6006/chrome/browser/ui/w...
chrome/browser/ui/website_settings/website_settings_ui.cc:98: ResourceBundle& rb
= ResourceBundle::GetSharedInstance();
nit: missing ui::
this just works without it because there is a using ui::ResourceBundle in
resource_bundle.h!