Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829452:
Auto select the connection tab of the Website Settings UI in case of an https error or mixed content (Closed)
|
Created:
8 years, 4 months ago by markusheintz_ Modified:
8 years, 3 months ago CC:
chromium-reviews, tfarina, markusheintz_ Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIf a site contains mixed content or the https connection has an error then the Website Settings UI automatically selects the connection when it is opened.
BUG=143922
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154137
Patch Set 1 #Patch Set 2 : " #Patch Set 3 : " #
Total comments: 4
Patch Set 4 : Address comments (erg). #
Total comments: 8
Patch Set 5 : Address comments (wtc). #
Total comments: 2
Patch Set 6 : Address comments (wtc). #Patch Set 7 : Add views specific parts. #
Total comments: 25
Patch Set 8 : Address comments (msw). #Patch Set 9 : " #
Total comments: 11
Patch Set 10 : Address comments (msw). #
Total comments: 4
Patch Set 11 : Address comments (msw). #Patch Set 12 : " #Patch Set 13 : " #
Total comments: 6
Patch Set 14 : " #Patch Set 15 : Address comments and really upload the CL :) #
Total comments: 2
Patch Set 16 : Rebase tot. #Patch Set 17 : Fix build issues. #Messages
Total messages: 38 (0 generated)
First review round: Please review the appropriate part of this CL. Thanks a lot :) @wtc: chrome/browser/ui/website_settings/website_settings.cc @erg: *.gtk @dubroy: */ui/cocoa/*
LGTM. On 2012/08/21 16:57:32, markusheintz_ wrote: > First review round: > > Please review the appropriate part of this CL. Thanks a lot :) > > @wtc: chrome/browser/ui/website_settings/website_settings.cc > @erg: *.gtk > @dubroy: */ui/cocoa/*
http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/gtk/websi... File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc (right): http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/gtk/websi... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:531: tab_id_index_map_[tab_id]); @erg: I wonder whether I'm not over complicating things here. I could just use the tab_id and cast it. The tab ids follow the order in which the tabs are added. Since we only have two tabs I guess we could keep things simple. WDYT? http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/website_s... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/website_s... chrome/browser/ui/website_settings/website_settings.cc:425: site_identity_status_ == SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN) @wtc: I'm not sure if we should check for the revocation status here. I remember a post from Scott Violet (http://www.imperialviolet.org/2012/02/05/crlsets.html) where he discussed cert revocation checks. So I wonder whether I'm not better off removing this check.
http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/gtk/websi... File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc (right): http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/gtk/websi... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:531: tab_id_index_map_[tab_id]); On 2012/08/21 17:07:07, markusheintz_ wrote: > @erg: I wonder whether I'm not over complicating things here. I could just use > the tab_id and cast it. The tab ids follow the order in which the tabs are > added. Since we only have two tabs I guess we could keep things simple. WDYT? I'm fine with that, but add DCHECKs to make sure that tabs do follow the order.
http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/gtk/websi... File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc (right): http://codereview.chromium.org/10829452/diff/4006/chrome/browser/ui/gtk/websi... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:531: tab_id_index_map_[tab_id]); On 2012/08/21 17:12:36, Elliot Glaysher wrote: > On 2012/08/21 17:07:07, markusheintz_ wrote: > > @erg: I wonder whether I'm not over complicating things here. I could just use > > the tab_id and cast it. The tab ids follow the order in which the tabs are > > added. Since we only have two tabs I guess we could keep things simple. WDYT? > > I'm fine with that, but add DCHECKs to make sure that tabs do follow the order. Done. Also added a comment.
lgtm
Patch set 4 LGTM. As instructed, I only reviewed chrome/browser/ui/website_settings/website_settings.cc. http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:417: // certificate presented by the website select the connection tab to draw to Nit: add a comma (,) between "the website" and "select". I believe "draw to users attention" should be changed to "draw the user's attention". http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:426: tab_id = WebsiteSettingsUI::TAB_ID_CONNECTION; Nit: add curly braces {} because the conditional expression in this "if" statement is multi-line.
@wtc: I'm not sure if we should check for the revocation status here. I remember a post from Scott Violet (http://www.imperialviolet.org/2012/02/05/crlsets.html) where he discussed cert revocation checks. So I wonder whether I'm not better off removing this check. See line website_settings.cc:425 http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:417: // certificate presented by the website select the connection tab to draw to On 2012/08/21 21:45:32, wtc wrote: > > Nit: add a comma (,) between "the website" and "select". > > I believe "draw to users attention" should be changed to > "draw the user's attention". Done. http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:425: site_identity_status_ == SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN) @wtc: I'm not sure if we should check for the revocation status here. I remember a post from Scott Violet (http://www.imperialviolet.org/2012/02/05/crlsets.html) where he discussed cert revocation checks. So I wonder whether I'm not better off removing this check. http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:426: tab_id = WebsiteSettingsUI::TAB_ID_CONNECTION; On 2012/08/21 21:45:32, wtc wrote: > > Nit: add curly braces {} because the conditional expression > in this "if" statement is multi-line. I was recently told that multi line conditions don't require {}. I'll see if I can find the CL. For now I would stick to the no {} solution to be consistent with my other UI code. http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:426: tab_id = WebsiteSettingsUI::TAB_ID_CONNECTION; On 2012/08/21 21:45:32, wtc wrote: > > Nit: add curly braces {} because the conditional expression > in this "if" statement is multi-line. I was recently told that multi line conditions don't require { }. Personally my heart is not tight to either way. Since I omitted the {} in other places (I need to look up the CL) I would prefer to omit the braces to be consistent.
Patch set 5 LGTM. http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:425: site_identity_status_ == SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN) Not checking for SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN here is fine by me, because the two errors are considered minor errors (net::IsCertStatusMinorError() is true). The question is whether the UI for SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN may make the user curious and want to click the lock icon to find out more. I forgot what the UI looks like, so I can't make a good judgment on this. http://codereview.chromium.org/10829452/diff/19/chrome/browser/ui/website_set... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/19/chrome/browser/ui/website_set... chrome/browser/ui/website_settings/website_settings.cc:417: // certificate presented by the website, select the connection tab to draw to Nit: draw to => draw the
http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/13001/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.cc:425: site_identity_status_ == SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN) On 2012/08/22 19:02:27, wtc wrote: > > Not checking for SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN > here is fine by me, because the two errors are considered > minor errors (net::IsCertStatusMinorError() is true). > > The question is whether the UI for SITE_IDENTITY_STATUS_CERT_REVOCATION_UNKNOWN > may make the user curious and want to click the lock icon > to find out more. I forgot what the UI looks like, so I > can't make a good judgment on this. Good point. I'll keep the check for now. http://codereview.chromium.org/10829452/diff/19/chrome/browser/ui/website_set... File chrome/browser/ui/website_settings/website_settings.cc (right): http://codereview.chromium.org/10829452/diff/19/chrome/browser/ui/website_set... chrome/browser/ui/website_settings/website_settings.cc:417: // certificate presented by the website, select the connection tab to draw to On 2012/08/22 19:02:28, wtc wrote: > > Nit: draw to => draw the Done.
@Michael: Could you please review the views specific parts of this CL? Thanks a lot
Also adding Scott. @Scott: Could you take a look at the comment in website_settings_popup_view.cc:527. Thanks a lot. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:527: // |WebsiteSettingsPopupView| is opened. Fix this. @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.
Some nits, and an idea. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:525: // correctly. If this is not done then the connection tab does not disaply spelling nit: display, grammar nit: "not done, then" http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:527: // |WebsiteSettingsPopupView| is opened. Fix this. On 2012/08/23 00:49:14, markusheintz_ wrote: > @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. > 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. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); 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)? http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings_ui.h (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.h:40: TAB_ID_PERMISSIONS, nit: explicitly specify TAB_ID_PERMISSIONS = 0 (especially if you're DCHECKing this elsewhere), you can let the others implicitly inherit incremental values. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.h:42: NUM_TAB_IDS nit: always use trailing commas in enum decls.
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); Oy, why the hack? Can't we fix this correctly?
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:525: // correctly. If this is not done then the connection tab does not disaply On 2012/08/23 01:01:35, msw wrote: > spelling nit: display, grammar nit: "not done, then" Done. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:527: // |WebsiteSettingsPopupView| is opened. Fix this. On 2012/08/23 01:01:35, msw wrote: > On 2012/08/23 00:49:14, markusheintz_ wrote: > > @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. > > > > 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 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. > glance; happy to help/discuss more as needed. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 04:01:48, sky wrote: > Oy, why the hack? Can't we fix this correctly? 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. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 01:01:35, msw wrote: > 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)? 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. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings_ui.h (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.h:40: TAB_ID_PERMISSIONS, On 2012/08/23 01:01:35, msw wrote: > nit: explicitly specify TAB_ID_PERMISSIONS = 0 (especially if you're DCHECKing > this elsewhere), you can let the others implicitly inherit incremental values. Done. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.h:42: NUM_TAB_IDS On 2012/08/23 01:01:35, msw wrote: > nit: always use trailing commas in enum decls. Done
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 19:46:21, markusheintz_ wrote: > On 2012/08/23 04:01:48, sky wrote: > > Oy, why the hack? Can't we fix this correctly? > > 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. > 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... http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 19:46:21, markusheintz_ wrote: > On 2012/08/23 01:01:35, msw wrote: > > 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)? > > 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. > > > 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.
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 20:11:30, msw wrote: > On 2012/08/23 19:46:21, markusheintz_ wrote: > > On 2012/08/23 04:01:48, sky wrote: > > > Oy, why the hack? Can't we fix this correctly? > > > > 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. > > > > 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... 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 :) http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 20:11:30, msw wrote: > On 2012/08/23 19:46:21, markusheintz_ wrote: > > On 2012/08/23 01:01:35, msw wrote: > > > 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)? > > > > 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. > > > > > > > > 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. 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?
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 20:34:33, markusheintz_ wrote: > On 2012/08/23 20:11:30, msw wrote: > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > On 2012/08/23 04:01:48, sky wrote: > > > > Oy, why the hack? Can't we fix this correctly? > > > > > > 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. > > > > > > > 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... > > 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 :) 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? http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 20:34:33, markusheintz_ wrote: > On 2012/08/23 20:11:30, msw wrote: > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > On 2012/08/23 01:01:35, msw wrote: > > > > 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)? > > > > > > 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. > > > > > > > > > > > > > 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. > > 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? 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). http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:348: [segmentedControl_ setSegmentCount:2]; s/2/NUM_TAB_IDS/ http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:363: [segmentedControl_ setLabel:label forSegment:0]; s/0/TAB_ID_PERMISSIONS/ here and similar for lines 364, 369, 376, and 377 http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/gtk/webs... File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc (right): http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/gtk/webs... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:327: gtk_notebook_append_page( s/gtk_notebook_append_page/gtk_notebook_insert_page/ with enum values for the indices, here and line 334. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/gtk/webs... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:551: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); Remove these two DCHECKs. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:317: tabbed_pane_->AddTab( s/AddTab/AddTabAtIndex/, using enum values for indicies, same for line 320.
Thanks a lot for all the comments and patience. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 21:26:39, msw wrote: > On 2012/08/23 20:34:33, markusheintz_ wrote: > > On 2012/08/23 20:11:30, msw wrote: > > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > > On 2012/08/23 04:01:48, sky wrote: > > > > > Oy, why the hack? Can't we fix this correctly? > > > > > > > > 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. > > > > > > > > > > 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... > > > > 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 :) > > 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? 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. http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 21:26:39, msw wrote: > On 2012/08/23 20:34:33, markusheintz_ wrote: > > On 2012/08/23 20:11:30, msw wrote: > > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > > On 2012/08/23 01:01:35, msw wrote: > > > > > 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)? > > > > > > > > 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. > > > > > > > > > > > > > > > > > > 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. > > > > 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? > > Elliot's okay with my new plan: Thanks a lot for checking with Elliot! The new is great! > 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). I guess we don't need such a DCHECK. Thanks a lot for checking with Elliot! http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:348: [segmentedControl_ setSegmentCount:2]; On 2012/08/23 21:26:39, msw wrote: > s/2/NUM_TAB_IDS/ Done. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:363: [segmentedControl_ setLabel:label forSegment:0]; On 2012/08/23 21:26:39, msw wrote: > s/0/TAB_ID_PERMISSIONS/ here and similar for lines 364, 369, 376, and 377 Done. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/gtk/webs... File chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc (right): http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/gtk/webs... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:327: gtk_notebook_append_page( On 2012/08/23 21:26:39, msw wrote: > s/gtk_notebook_append_page/gtk_notebook_insert_page/ with enum values for the > indices, here and line 334. Done. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/gtk/webs... chrome/browser/ui/gtk/website_settings/website_settings_popup_gtk.cc:551: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 21:26:39, msw wrote: > Remove these two DCHECKs. Done. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:317: tabbed_pane_->AddTab( On 2012/08/23 21:26:39, msw wrote: > s/AddTab/AddTabAtIndex/, using enum values for indicies, same for line 320. Done. http://codereview.chromium.org/10829452/diff/20011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); Removed the DCHECKS
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 22:21:34, markusheintz_ wrote: > On 2012/08/23 21:26:39, msw wrote: > > On 2012/08/23 20:34:33, markusheintz_ wrote: > > > On 2012/08/23 20:11:30, msw wrote: > > > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > > > On 2012/08/23 04:01:48, sky wrote: > > > > > > Oy, why the hack? Can't we fix this correctly? > > > > > > > > > > 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. > > > > > > > > > > > > > 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... > > > > > > 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 :) > > > > 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? > > 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. Use InvalidateLayout unless Scott objects :) http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 22:21:34, markusheintz_ wrote: > On 2012/08/23 21:26:39, msw wrote: > > On 2012/08/23 20:34:33, markusheintz_ wrote: > > > On 2012/08/23 20:11:30, msw wrote: > > > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > > > On 2012/08/23 01:01:35, msw wrote: > > > > > > 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)? > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > 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? > > > > Elliot's okay with my new plan: > > Thanks a lot for checking with Elliot! The new is great! > > > 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). > > I guess we don't need such a DCHECK. > > Thanks a lot for checking with Elliot! > > 'twas my idea, Elliot liked it too :) http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:362: CGFloat tabWidth = textSize.width + 2 * kTabLabelXPadding; nit: the width determination code sux, but leave it alone for now, I guess. It should be: set 1st label; set 2nd label; determine widest; apply widths.
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:546: DCHECK_EQ(TAB_ID_PERMISSIONS, 0); On 2012/08/23 22:59:41, msw wrote: > On 2012/08/23 22:21:34, markusheintz_ wrote: > > On 2012/08/23 21:26:39, msw wrote: > > > On 2012/08/23 20:34:33, markusheintz_ wrote: > > > > On 2012/08/23 20:11:30, msw wrote: > > > > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > > > > On 2012/08/23 01:01:35, msw wrote: > > > > > > > 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)? > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > 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? > > > > > > Elliot's okay with my new plan: > > > > Thanks a lot for checking with Elliot! The new is great! > > > > > 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). > > > > I guess we don't need such a DCHECK. > > > > Thanks a lot for checking with Elliot! > > > > > > 'twas my idea, Elliot liked it too :) +1 done :-) http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:362: CGFloat tabWidth = textSize.width + 2 * kTabLabelXPadding; On 2012/08/23 22:59:41, msw wrote: > nit: the width determination code sux, but leave it alone for now, I guess. > It should be: set 1st label; set 2nd label; determine widest; apply widths. @Patrick: Is this something you want to fix in a followup CL or should I do it?
http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:362: CGFloat tabWidth = textSize.width + 2 * kTabLabelXPadding; On 2012/08/23 22:59:41, msw wrote: > nit: the width determination code sux, but leave it alone for now, I guess. > It should be: set 1st label; set 2nd label; determine widest; apply widths. I don't understand what you mean. Isn't that what the code already does? Are you suggesting a functional change, or just shuffling the lines around to read a bit differently?
http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: tabbed_pane_->SelectTabAt(tab_index); On 2012/08/23 22:59:41, msw wrote: > On 2012/08/23 22:21:34, markusheintz_ wrote: > > On 2012/08/23 21:26:39, msw wrote: > > > On 2012/08/23 20:34:33, markusheintz_ wrote: > > > > On 2012/08/23 20:11:30, msw wrote: > > > > > On 2012/08/23 19:46:21, markusheintz_ wrote: > > > > > > On 2012/08/23 04:01:48, sky wrote: > > > > > > > Oy, why the hack? Can't we fix this correctly? > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > 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... > > > > > > > > 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 :) > > > > > > 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? > > > > 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. > > Use InvalidateLayout unless Scott objects :) 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.
Great! Thanks Scott. Mike think I addressed your comments. Do you have any other suggestions? Scott I might also need an Owners lgtm from you. Thanks all for all the great comments and the reviews so far. -- Sent from my mobil Am 24.08.2012 19:41 schrieb <sky@chromium.org>: > > http://codereview.chromium.**org/10829452/diff/18011/** > chrome/browser/ui/views/**website_settings/website_** > settings_popup_view.cc<http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc> > File > chrome/browser/ui/views/**website_settings/website_** > settings_popup_view.cc > (right): > > http://codereview.chromium.**org/10829452/diff/18011/** > chrome/browser/ui/views/**website_settings/website_** > settings_popup_view.cc#**newcode531<http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc#newcode531> > chrome/browser/ui/views/**website_settings/website_** > settings_popup_view.cc:531: > tabbed_pane_->SelectTabAt(tab_**index); > On 2012/08/23 22:59:41, msw wrote: > >> On 2012/08/23 22:21:34, markusheintz_ wrote: >> > On 2012/08/23 21:26:39, msw wrote: >> > > On 2012/08/23 20:34:33, markusheintz_ wrote: >> > > > On 2012/08/23 20:11:30, msw wrote: >> > > > > On 2012/08/23 19:46:21, markusheintz_ wrote: >> > > > > > On 2012/08/23 04:01:48, sky wrote: >> > > > > > > Oy, why the hack? Can't we fix this correctly? >> > > > > > >> > > > > > 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. >> > > > > > >> > > > > >> > > > > 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_pane/**native_tabbed_pane_win.cc&**type=cs&l=58<http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/tabbed_pane/native_tabbed_pane_win.cc&type=cs&l=58> > >> > > > >> > > > It does >> > > > >> > > >> > >> > > http://code.google.com/**searchframe#OAMlx_jo-ck/src/** > ui/views/controls/tabbed_pane/**native_tabbed_pane_views.cc&l=**250<http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc&l=250> > >> > > > >> > > > 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 :) >> > > >> > > 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? >> > >> > 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. >> > > Use InvalidateLayout unless Scott objects :) >> > > 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. > > http://codereview.chromium.**org/10829452/<http://codereview.chromium.org/108... >
http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/17002/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:362: CGFloat tabWidth = textSize.width + 2 * kTabLabelXPadding; On 2012/08/24 12:48:15, dubroy wrote: > On 2012/08/23 22:59:41, msw wrote: > > nit: the width determination code sux, but leave it alone for now, I guess. > > It should be: set 1st label; set 2nd label; determine widest; apply widths. > > I don't understand what you mean. Isn't that what the code already does? Are you > suggesting a functional change, or just shuffling the lines around to read a bit > differently? Ah, don't worry about it, I'd just remove the first unnecessary setWidth, do maxWidth = max(sizeWithAttributes(...),...) before padding, then add padding and call setWidth, seems just slightly cleaner. http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:529: // TODO(markusheintz): This is a hack to resize the connection tab contents Remove this and use InvalidateLayout instead.
http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:372: DCHECK_EQ( [segmentedControl_ segmentCount ], NUM_TAB_IDS); nit: no space after open paren.
http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:372: DCHECK_EQ( [segmentedControl_ segmentCount ], NUM_TAB_IDS); On 2012/08/24 18:59:14, msw wrote: > nit: no space after open paren. Doh, also no space before close square bracket
It looks like you haven't updated the cl yet. -Scott On Fri, Aug 24, 2012 at 11:02 AM, Markus Heintz <markusheintz@chromium.org> wrote: > Great! Thanks Scott. > > Mike think I addressed your comments. Do you have any other suggestions? > > Scott I might also need an Owners lgtm from you. > > Thanks all for all the great comments and the reviews so far. > > -- > Sent from my mobil > > Am 24.08.2012 19:41 schrieb <sky@chromium.org>: > >> >> >> http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... >> File >> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc >> (right): >> >> >> http://codereview.chromium.org/10829452/diff/18011/chrome/browser/ui/views/we... >> >> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:531: >> tabbed_pane_->SelectTabAt(tab_index); >> On 2012/08/23 22:59:41, msw wrote: >>> >>> On 2012/08/23 22:21:34, markusheintz_ wrote: >>> > On 2012/08/23 21:26:39, msw wrote: >>> > > On 2012/08/23 20:34:33, markusheintz_ wrote: >>> > > > On 2012/08/23 20:11:30, msw wrote: >>> > > > > On 2012/08/23 19:46:21, markusheintz_ wrote: >>> > > > > > On 2012/08/23 04:01:48, sky wrote: >>> > > > > > > Oy, why the hack? Can't we fix this correctly? >>> > > > > > >>> > > > > > 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. >>> > > > > > >>> > > > > >>> > > > > 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... >>> >>> > > > >>> > > > 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 :) >>> > > >>> > > 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? >>> > >>> > 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. >> >> >>> Use InvalidateLayout unless Scott objects :) >> >> >> 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. >> >> http://codereview.chromium.org/10829452/
Ahh ... sorry for forgetting to update the CL. Done. http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right): http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:372: DCHECK_EQ( [segmentedControl_ segmentCount ], NUM_TAB_IDS); On 2012/08/24 19:00:01, msw wrote: > On 2012/08/24 18:59:14, msw wrote: > > nit: no space after open paren. > > Doh, also no space before close square bracket Done. http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/cocoa/we... chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:372: DCHECK_EQ( [segmentedControl_ segmentCount ], NUM_TAB_IDS); On 2012/08/24 18:59:14, msw wrote: > nit: no space after open paren. Done. http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/19017/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:529: // TODO(markusheintz): This is a hack to resize the connection tab contents On 2012/08/24 18:56:40, msw wrote: > Remove this and use InvalidateLayout instead. Done.
LGTM
LGTM with an optional nit to investigate. http://codereview.chromium.org/10829452/diff/19019/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/19019/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:533: SizeToContents(); nit: I doubt all three (invalidate, layout, and size to contents) are needed.
Thanks a for the reviews. http://codereview.chromium.org/10829452/diff/19019/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10829452/diff/19019/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:533: SizeToContents(); On 2012/08/27 17:41:33, msw wrote: > nit: I doubt all three (invalidate, layout, and size to contents) are needed. I've tried all sensible permutations. :(
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10829452/30001
Try job failure for 10829452-30001 (retry) (previous was lost) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10829452/32006
Try job failure for 10829452-32006 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10829452/32006
Change committed as 154137 |
