Auto select the connection tab of the Website Settings UI in case of an https error or mixed content

chrome/browser/ui/views/website_settings/website_settings_popup_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/website_settings/website_settings_popup_view.h"
 
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/certificate_viewer.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 295 matching lines @@
   layout->AddView(header_);
 
   layout->AddPaddingRow(1, kHeaderMarginBottom);
   tabbed_pane_ = new views::TabbedPane();
   layout->StartRow(1, content_column);
   layout->AddView(tabbed_pane_);
   // Tabs must be added after the tabbed_pane_ was added to the views
   // hierachy.  Adding the |tabbed_pane_| to the views hierachy triggers the
   // initialization of the native tab UI element. If the native tab UI
   // element is not initalized adding a tab will result in a NULL pointer
-  // excetion.
+  // exception.
   tabbed_pane_->AddTab(
       l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_TAB_LABEL_PERMISSIONS),
       CreatePermissionsTab());
   tabbed_pane_->AddTab(
       l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_TAB_LABEL_CONNECTION),
       CreateConnectionTab());
-  tabbed_pane_->SelectTabAt(0);
   tabbed_pane_->set_listener(this);
 
   set_margins(gfx::Insets(kPopupMarginTop, kPopupMarginLeft,
                           kPopupMarginBottom, kPopupMarginRight));
 
   views::BubbleDelegateView::CreateBubble(this);
   this->Show();
   SizeToContents();
 
   presenter_.reset(new WebsiteSettings(this, profile,
@@ skipping 22 matching lines @@
   GetWidget()->Close();
 }
 
 void WebsiteSettingsPopupView::LinkClicked(views::Link* source,
                                            int event_flags) {
   if (source == cookie_dialog_link_) {
     new CollectedCookiesViews(tab_contents_);
   } else if (source == certificate_dialog_link_) {
     gfx::NativeWindow parent =
         anchor_view() ? anchor_view()->GetWidget()->GetNativeWindow() : NULL;
-ShowCertificateViewerByID(tab_contents_->web_contents(), parent, cert_id_);
+    ShowCertificateViewerByID(tab_contents_->web_contents(), parent, cert_id_);
   }
   // The popup closes automatically when the collected cookies dialog or the
   // certificate viewer opens.
 }
 
 void WebsiteSettingsPopupView::TabSelectedAt(int index) {
   tabbed_pane_->GetSelectedTab()->Layout();
   SizeToContents();
 }
 
@@ skipping 138 matching lines @@
       UTF8ToUTF16(identity_info.identity_status_description),
       certificate_dialog_link_);
 
   ResetConnectionSection(
       connection_info_content_,
       WebsiteSettingsUI::GetConnectionIcon(identity_info.connection_status),
       string16(),  // The connection section has no headline.
       UTF8ToUTF16(identity_info.connection_status_description),
       NULL);
 
-  Layout();
-  SizeToContents();
+  // TODO(markusheintz): This is a hack to resize the connection tab contents
+  // correctly. If this is not done then the connection tab does not disaply

[msw, 2012/08/23 01:01:35]

spelling nit: display, grammar nit: "not done, then"

[markusheintz_, 2012/08/23 19:46:21]

Done.

+  // its contents correctly in case it is selected as visible tab when the
+  // |WebsiteSettingsPopupView| is opened. Fix this.

[markusheintz_, 2012/08/23 00:49:14]

@msw,sky: I wish I could have come up with something better. This hack is for
now the minimal solution.

The issue here is, that the view connection_info_content_ and
identity_info_content_ have no children, when the popup is created. The
SetIdentityInfo method then adds all respective child views. When this happens
size of connection_info_content_ and identity_info_content_ is not update.
Switching tabs does the trick. I tried setting the size of the tab contents
manually but that did not work well because I need to provide the proper bounds
of the tabbed pane to set the correct width of the views correctly.

I'm more than happy for any suggestion that allows me to get rid of this hack. I
probably played to long with this code now I miss the obvious things.
 

[msw, 2012/08/23 01:01:35]

This hacks seems okay for now, I suppose. Perhaps you need to set the preferred
size to be as wide as the widest tab or similar? I'm just guessing at first
glance; happy to help/discuss more as needed.

[markusheintz_, 2012/08/23 19:46:21]

This is a good guess. And it's also easy to do since I can set the width to
kPopupWidth. I though to that this would be sufficient, but It was not :( The
height is the tricky part.

+  int tab_index = tabbed_pane_->GetSelectedTabIndex();
+  tabbed_pane_->SelectTabAt(0);
+  tabbed_pane_->SelectTabAt(1);
+  tabbed_pane_->SelectTabAt(tab_index);

[sky, 2012/08/23 04:01:48]

Oy, why the hack? Can't we fix this correctly?

[markusheintz_, 2012/08/23 19:46:21]

That's the plan :) (to fix it correctly).

The problem is:
1) I create some contents (view) for a tab page.
2) I add the page to the tabbed pane
3) Then I alter the content of the page
4) Then I problematically select the corresponding tab (SelectTabAt) and the tab
page is not layouted correctly.

If I switch to the first tab and back using the mouse the page it layouted
correctly.

I think the page is not layouted after calling TabLayout::SwitchToPage because
the bounds of the view that contains the content don't change and the layout is
not invalidated.

I tried to fix this "correctly" but this required more that 4 lines of code and
started to get pretty complex. So I though maybe I'm over complicating things
here. Therefor I though I go with the easiest solution first and solicit some
feedback.

I have another CL here:  
But that uses InvalidateLayout(). According to the comment in the code this
method might be changed to be private in the future . So I'm not so happy with
that change either.

I also tried to call SizeToPreferredSize on the altered parts of tab page and
the tab page. But then the width and height is not correct and parts of the tab
page are clipped.

[msw, 2012/08/23 20:11:30]

TabLayout::SwitchToPage calls SetBoundsRect on the contents being selected for
NativeTabbedPaneWin, perhaps NativeTabbedPaneViews needs to do that or something
similar:
http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/tabbed_p...

[markusheintz_, 2012/08/23 20:34:33]

It does 
http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/tabbed_p...

What you suggest is exactly what I mean. I just haven't figured out a good way
to do it yet.

Maybe we could go with one of the solutions that work. I'll file a bug which
buys me some time to work on this, since the tab auto select part landed on
trunk :)

[msw, 2012/08/23 21:26:39]

Sorry, I missed that... hmm, I'm out of ideas without applying locally and
taking a closer look. Does InvalidateLayout do the trick? (that might be
okay...) What were you trying as a correct fix that you mentioned took more than
4 lines?

[markusheintz_, 2012/08/23 22:21:34]

The InvalidayLayout version works perfect.


Thing that do not work:
-----------------------
- Using SizeToPreferedSize since the wrong width is applied


(I have some screenshots in case you are interessted to see the effect)

I also tried a mix of setting the width and using SizeToPreferedSize

Calling Layout on NativeTabbedPane, on the tab page, on ...  (Requires making
some Layout method public; Sorry for the lack of detail here)

Trying to set the propper Bounds, but this turned into a manually layouting
effort, and so I stopped it.

Things I planed to try
----------------------
I also thought about implementing a class AutoResizeView : public View; that
implements the method "ChildPreferredSizeChanged" and call "InvalidateLayout"
from there.

[msw, 2012/08/23 22:59:41]

Use InvalidateLayout unless Scott objects :)

[sky, 2012/08/24 17:41:30]

Mike's right, InvalidateLayout is what you want. Additionally I don't think its
going away any time soon. I'll see about removing the comment.

 }
 
 void WebsiteSettingsPopupView::SetFirstVisit(const string16& first_visit) {
   ResetConnectionSection(
       page_info_content_,
       WebsiteSettingsUI::GetFirstVisitIcon(first_visit),
       l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_INFO_TITLE),
       first_visit,
       NULL);
   Layout();
   SizeToContents();
 }
 
+void WebsiteSettingsPopupView::SetSelectedTab(TabId tab_id) {
+  DCHECK_EQ(TAB_ID_PERMISSIONS, 0);

[msw, 2012/08/23 01:01:35]

nit: it's silly to DCHECK this on each platform separately each time a tab is
changed. Move this to a x-platform WebsiteSettingsTest (or I guess
WebsiteSettings ctor if it's *really* needed). Do you mean to DCHECK_LT(tab_id,
NUM_TAB_IDS)?

[markusheintz_, 2012/08/23 19:46:21]

1) I think the DCHECKS document pretty nice the relation between tabbed pane
indexes and the TabIds. And they document this in the place where it matters.

2) In theory the relation between TabId and the plattform specific tabbed pane
widget could be different on different platform.

2 is not true so we can ignore it. But I think 1 is nice to have because the
dependency is immediately visible in the code. Somebody unfamiliar with the code
would not need to find out about this through a failing test.

The DCHECKs come with a zero performance penalty.

I think WebsiteSetting ctor is logically the wrong place to have the DCHECKS.
WebsiteSettings should not need to care about the relation between TabId and
tabbed pane indexes.

[msw, 2012/08/23 20:11:30]

These (and the GTK and [future] Mac) DCHECKs just duplicate 'documentation' of
the enum itself, since tab indices and ids are equivalent. It's easy for anyone
to look up the TabID enum as needed and good IDEs will even show the named enum
values while debugging. I think these add little besides redundancy and
maintenance costs.

[markusheintz_, 2012/08/23 20:34:33]

I definitely agree that duplicated code sucks :).

Since Elliot asked for the DCHECKS (See comment in patchset 3 for
website_settings_popup_gtk.cc) and you ask me to delete them, I have
contradicting comments :-D. Nice! :) :).

So ... I guess I just move the DCHECKs to a test like you suggested :) would
that be ok?

[msw, 2012/08/23 21:26:39]

Elliot's okay with my new plan:
1) Remove these and the corresponding DCHECKs in GTK.
2) Use enum values when adding tabs for each platform (see my new comments).
3) Feel free to DCHECK_EQ(#tabs, NUM_TAB_IDS) on each platform directly after
adding all the tabs (or on tab switching if you really think that adds value).

[markusheintz_, 2012/08/23 22:21:34]

Thanks a lot for checking with Elliot! The new is great!
I guess we don't need such a DCHECK.

Thanks a lot for checking with Elliot!

[msw, 2012/08/23 22:59:41]

'twas my idea, Elliot liked it too :)

[markusheintz_, 2012/08/24 12:42:43]

+1 done :-) 

+  DCHECK_EQ(TAB_ID_CONNECTION, 1);
+  tabbed_pane_->SelectTabAt(tab_id);
+}
+
 views::View* WebsiteSettingsPopupView::CreatePermissionsTab() {
   views::View* pane = new views::View();
   pane->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1));
   // Add cookies and site data section.
   cookie_dialog_link_ = new views::Link(
       l10n_util::GetStringUTF16(IDS_WEBSITE_SETTINGS_SHOW_SITE_DATA));
   cookie_dialog_link_->set_listener(this);
   site_data_content_ = new views::View();
   views::View* site_data_section =
@@ skipping 144 matching lines @@
 
   if (link) {
     content_layout->StartRow(1, 0);
     content_layout->AddView(link);
   }
 
   layout->AddView(content_pane, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::LEADING);
   layout->AddPaddingRow(0, kConnectionSectionPaddingBottom);
 }