Add tabs UI to WebsiteSettingsUI (GTK only).

Display site permissions in a tab and add a tab to display  information about the site's identity and the site's connection.

UI string changes: https://chromiumcodereview.appspot.com/10180002/

BUG=113688
TEST=WebsiteSettingsTest*

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

[markusheintz_, 2012-04-23 14:42:46]

Please review the following parts of my CL:

@Finnur: All except GTK changes.

@Elliot: GTK changes in:
chrome/browser/ui/gtk/website_settings_popup_gtk.*

@Ben: UI changes in:
chrome/browser/ui/website_settings_ui.*

Thanks a lot.

[Ben Goodger (Google), 2012-04-23 15:03:05]

LGTM for browser/ui

[Finnur, 2012-04-23 15:41:25]

This LGTM, but I have to point out that the more we flesh out the details of the
WebsiteSettingsUI I feel less and less qualified to review it given that my work
in the infobubble was more to change the container and not to decide on its
contents (which had already been decided when I took over). Wan-Teh, were you
the person that did that part?

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:98: GtkWidget*
identity_tab_contents_;
This member variable is missing from the initialization list in the ctor, no?

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
chrome/browser/ui/website_settings_ui.h:95: virtual void SetIdentityInfo(const
IdentityInfo& identity_info) = 0;
Looks like this breaks unit test compilation...

[markusheintz_, 2012-04-23 15:52:08]

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:98: GtkWidget*
identity_tab_contents_;
On 2012/04/23 15:41:25, Finnur wrote:
> This member variable is missing from the initialization list in the ctor, no?

Yes it was. Thanks. I added it.

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10163002/diff/12001/chrome/browser/ui/...
chrome/browser/ui/website_settings_ui.h:95: virtual void SetIdentityInfo(const
IdentityInfo& identity_info) = 0;
On 2012/04/23 15:41:25, Finnur wrote:
> Looks like this breaks unit test compilation...

Yes it did :) saw it to late. Fixed.

[Elliot Glaysher, 2012-04-23 17:25:49]

lgtm

Do you have newer mocks or screenshots? I don't remember seeing a notebook in
the previous iterations...

[markusheintz_, 2012-04-23 18:49:20]

On 2012/04/23 17:25:49, Elliot Glaysher wrote:
> lgtm
> 
> Do you have newer mocks or screenshots? I don't remember seeing a notebook in
> the previous iterations...

Thanks all for the review.

@Elliot: I shared two mocks with you, that were created by Glen Murphy.
Please let me know if you have more questions.

[wtc, 2012-04-23 21:51:55]

On 2012/04/23 15:41:25, Finnur wrote:
> This LGTM, but I have to point out that the more we flesh out the details of
the
> WebsiteSettingsUI I feel less and less qualified to review it given that my
work
> in the infobubble was more to change the container and not to decide on its
> contents (which had already been decided when I took over). Wan-Teh, were you
> the person that did that part?

It was jcivelli who did the contents of the page info dialog.
I reviewed his changelists.  So I am a suitable reviewer for
that code.

[wtc, 2012-04-23 22:01:22]

Drive-by review comments on patch set 10:

I didn't review chrome/browser/ui/gtk/website_settings_popup_gtk.cc.

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/ui/...
chrome/browser/ui/website_settings_ui.h:73: std::string
connection_status_description;

Nit: move these two connnection-related members to the end
of the struct, so that the three identity-related members
are contiguous.

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
File chrome/browser/website_settings.cc (right):

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:90: DCHECK_NE(site_connection_status_,
SITE_CONNECTION_STATUS_UNKNOWN);

I think lines 87-90 should be moved to the new
PresentSiteIdentity() method.

[markusheintz_, 2012-04-24 08:49:41]

Thanks a lot for the drive by Wan-Teh.

I will send future CLs about site identity and connection status to you.

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/ui/...
chrome/browser/ui/website_settings_ui.h:73: std::string
connection_status_description;
On 2012/04/23 22:01:23, wtc wrote:
> 
> Nit: move these two connnection-related members to the end
> of the struct, so that the three identity-related members
> are contiguous.

Done.

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
File chrome/browser/website_settings.cc (right):

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:90: DCHECK_NE(site_connection_status_,
SITE_CONNECTION_STATUS_UNKNOWN);
On 2012/04/23 22:01:23, wtc wrote:
> 
> I think lines 87-90 should be moved to the new
> PresentSiteIdentity() method.

Done. 

I saw these as a kind of post condition check for the Init method. That was my
original motivation for leaving them here.

I plan to integrate the Init() method with the PresentSiteIdentity(). This will
group the code for connection and identity info together. There is no need to
keep two separate methods. I didn't do it in this CL to keep the CL at a
reasonable size. The overall theme of this CL was adding tabs to the UI.

[wtc, 2012-04-24 17:14:25]

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
File chrome/browser/website_settings.cc (right):

https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
chrome/browser/website_settings.cc:90: DCHECK_NE(site_connection_status_,
SITE_CONNECTION_STATUS_UNKNOWN);
On 2012/04/24 08:49:41, markusheintz_ wrote:
>
> I saw these as a kind of post condition check for the Init method. That was my
> original motivation for leaving them here.

Ah, I see.  I didn't notice that.  I think you're right.
Let's move these lines back here, unless you think they
also work well as preconditions for the PresentSiteIdentity()
method.

Sorry about my confusion.

[markusheintz_, 2012-04-24 19:41:00]

On 2012/04/24 17:14:25, wtc wrote:
>
https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
> File chrome/browser/website_settings.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10163002/diff/17001/chrome/browser/web...
> chrome/browser/website_settings.cc:90: DCHECK_NE(site_connection_status_,
> SITE_CONNECTION_STATUS_UNKNOWN);
> On 2012/04/24 08:49:41, markusheintz_ wrote:
> >
> > I saw these as a kind of post condition check for the Init method. That was
my
> > original motivation for leaving them here.
> 
> Ah, I see.  I didn't notice that.  I think you're right.
> Let's move these lines back here, unless you think they
> also work well as preconditions for the PresentSiteIdentity()
> method.

Currently they work both ways equally good :).

Landing this CL is currently blocked by the pending string review
(https://chromiumcodereview.appspot.com/10180002/). Once I landed the UI strings
I'll commit this CL.

> 
> Sorry about my confusion.