Add GTK Website Settings UI

Add GTK Website Settings UI

BUG=113688
TEST=none

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

[Elliot Glaysher, 2012-03-19 19:56:06]

Not sure what you're asking me to look at.

https://chromiumcodereview.appspot.com/9732007/diff/1/chrome/browser/ui/gtk/w...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

https://chromiumcodereview.appspot.com/9732007/diff/1/chrome/browser/ui/gtk/w...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:125: */
Commented out?

https://chromiumcodereview.appspot.com/9732007/diff/1/chrome/browser/ui/gtk/w...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:144:
gtk_box_pack_start(GTK_BOX(site_info_contents_), site_info_entry_box, FALSE,
FALSE, 0);
nit: lots of 80 col errors in this patch. won't point them all out.

https://chromiumcodereview.appspot.com/9732007/diff/1/chrome/browser/ui/gtk/w...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:244: permission_text =
"Popups";
shouldn't this be localized?

[markusheintz_, 2012-03-20 20:44:11]

In my email I just wanted to give you a heads up and ask whether you are willing
to review my CLs :).

So please review this CL now.

I will move the grd changes to a separate CL tomorrow (EMEA time)

http://codereview.chromium.org/9732007/diff/1/chrome/browser/ui/gtk/website_s...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

http://codereview.chromium.org/9732007/diff/1/chrome/browser/ui/gtk/website_s...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:125: */
On 2012/03/19 19:56:06, Elliot Glaysher wrote:
> Commented out?

removed

http://codereview.chromium.org/9732007/diff/1/chrome/browser/ui/gtk/website_s...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:144:
gtk_box_pack_start(GTK_BOX(site_info_contents_), site_info_entry_box, FALSE,
FALSE, 0);
On 2012/03/19 19:56:06, Elliot Glaysher wrote:
> nit: lots of 80 col errors in this patch. won't point them all out.

Done.

http://codereview.chromium.org/9732007/diff/1/chrome/browser/ui/gtk/website_s...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:244: permission_text =
"Popups";
On 2012/03/19 19:56:06, Elliot Glaysher wrote:
> shouldn't this be localized?

Done.

[Elliot Glaysher, 2012-03-20 23:46:19]

http://codereview.chromium.org/9732007/diff/8001/chrome/browser/ui/gtk/websit...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

http://codereview.chromium.org/9732007/diff/8001/chrome/browser/ui/gtk/websit...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:193:
gtk_util::SetLabelWidth(info, 200);
Can you get away without manually setting the width here?

http://codereview.chromium.org/9732007/diff/8001/chrome/browser/ui/gtk/websit...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:254:
l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_TYPE_NOTIFICATIONS));
GetStringUTF8. Also in the rest of the file.

http://codereview.chromium.org/9732007/diff/8001/chrome/browser/ui/gtk/websit...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:334: Browser* browser =
BrowserList::GetLastActiveWithProfile(profile_);
May want to get this from BrowserWindow that you have in your ctor to make sure
you always match the window that owns this bubble (though theoretically that
should never happen). Also in your other handlers.

[markusheintz_, 2012-03-21 10:54:24]

I removed one of the buttons as it is not clear yet where this button should be.

https://chromiumcodereview.appspot.com/9732007/diff/8001/chrome/browser/ui/gt...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

https://chromiumcodereview.appspot.com/9732007/diff/8001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:193:
gtk_util::SetLabelWidth(info, 200);
On 2012/03/20 23:46:19, Elliot Glaysher wrote:
> Can you get away without manually setting the width here?
Yes. removed.

https://chromiumcodereview.appspot.com/9732007/diff/8001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:254:
l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_TYPE_NOTIFICATIONS));
On 2012/03/20 23:46:19, Elliot Glaysher wrote:
> GetStringUTF8. Also in the rest of the file.

Done.

https://chromiumcodereview.appspot.com/9732007/diff/8001/chrome/browser/ui/gt...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:334: Browser* browser =
BrowserList::GetLastActiveWithProfile(profile_);
On 2012/03/20 23:46:19, Elliot Glaysher wrote:
> May want to get this from BrowserWindow that you have in your ctor to make
sure
> you always match the window that owns this bubble (though theoretically that
> should never happen). Also in your other handlers.

Done.

[Elliot Glaysher, 2012-03-21 18:29:29]

lgtm

[markusheintz_, 2012-03-27 08:48:32]

@Ben: Could you please review the UI changes:
chrome/browser/ui/website_settings_ui.h

Thanks a lot

[Ben Goodger (Google), 2012-03-27 15:24:37]

lgtm

[commit-bot: I haz the power, 2012-03-27 15:41:47]

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

[commit-bot: I haz the power, 2012-03-27 19:28:04]

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

[commit-bot: I haz the power, 2012-03-28 08:59:35]

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

[commit-bot: I haz the power, 2012-03-28 09:08:23]

Try job failure for 9732007-20001 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.