Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10456017:
Add WebsiteSettingsUI for Windows (Closed)
|
Created:
8 years, 6 months ago by markusheintz_ Modified:
8 years, 6 months ago CC:
chromium-reviews, tfarina, Ben Goodger (Google) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd views implementation of the WebsiteSettingsUI.
BUG=113688
TEST=PageInfoBubble is replaced by WebsiteSettingsUI if cmd arg --enable-website-settings is used.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141407
Patch Set 1 #
Total comments: 60
Patch Set 2 : Address comments #Patch Set 3 : " #
Total comments: 8
Patch Set 4 : Address comments (Finnur) #Patch Set 5 : Fix nits after proofreading the patchset. #
Total comments: 2
Patch Set 6 : Address comments (Finnur) #
Total comments: 26
Patch Set 7 : Address comments (tfarina, sky) #
Total comments: 1
Patch Set 8 : Rename tab_contents_wrapper_ or wrapper variabels to tab_contents #
Total comments: 18
Patch Set 9 : Address comments (tfarina) #
Total comments: 2
Patch Set 10 : Address comments (tfarina) #Patch Set 11 : s/TabContentsWrapper/TabContents after rebasing on tot #Patch Set 12 : " #Messages
Total messages: 24 (0 generated)
Ben and Finnur, please review my CL. I have a few questions: 1) I don't know why the |WebsiteSettingsPopupView| ignores mouse input but reacts to keyboard input. What am I missing? 2) What is the preferred way of organizing a view UI: A) Smalls classes for individual parts of the UI (see PopupHeader in the CL). B) Methods for creating parts of the UI (see private "CreateYYY()" methods of the |WebsiteSettingsPopupView| in the CL) C) A and B are equally good.
http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... File chrome/browser/website_settings.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:47: #include "chrome/browser/ui/views/website_settings_popup_view.h" you can't do this anymore :) see chrome/browser/DEPS file. http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:528: void WebsiteSettings::Show(views::View* anchor_view, you should implement this function in chrome/browser/ui/views/website_settings_popup_view.cc same thing for the gtk version.
http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... File chrome/browser/website_settings.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:44: #if defined(TOOLKIT_GTK) again, toolkit-specific ifdefs are gross, so think about how to avoid proliferating them here and cleaning up the existing usage. http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_settings.h File chrome/browser/website_settings.h (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.h:78: static void Show(views::View* anchor_view, think about how to make Show() work in a cross platform manner.
Ben might have a stronger opinion on the organization question, but I've given my take. > I have a few questions: > 1) I don't know why the |WebsiteSettingsPopupView| ignores mouse input but > reacts to keyboard input. What am I missing? I don't know. Nothing obvious stuck out during review. > 2) What is the preferred way of organizing a view UI: > A) Smalls classes for individual parts of the UI (see PopupHeader in the CL). > B) Methods for creating parts of the UI (see private "CreateYYY()" methods of > the |WebsiteSettingsPopupView| in the CL) > C) A and B are equally good. I'm not sure there's any general rule for this -- I guess it will depend on the circumstances in each case. I don't have a problem with the way it was used in this CL but I did wonder if the private classes (such as PopupHeader should be moved to the .cc file). http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:55: } These two functions are a bit unfortunate since they are essentially copied (from your GTK implementation of the dialog) and slightly altered to use string16 instead of std::string, but without a comment or mention of needing to keep the two in sync. I'd probably change it to return the int resource ID instead of a string and then call GetStringUTF16 or GetStringUTF8 as necessary at the call sites. Then you can move this code to be shared between the two implementations. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:61: // //////////////////////////////////////////////////////////////////////////// nit: To be consistent with other Chrome code, you should not have a space in these bars (eighty slashes, no space). Same elsewhere. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:98: return gfx::Size(300, 80); Do you mean to reserve space (80 pixels) even if no name/status has been set? Is this to prevent flickering? Can the name be long enough to wrap causing you to need more than that? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:101: void PopupHeader::SetIdentityName(string16 name) { const string16&, same on line 105 http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:109: nit: Extra line break. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:145: + ASCIIToUTF16(")"); String concatenations are generally a bad idea in terms of i18n. Can you convert this to a single string that takes the arguments as parameters? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:158: Profile* profile, Why do you need the |profile|? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:223: new views::GridLayout(site_data_content_); nit: This fits within one line. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:245: label_text += " blocked)"; Same i18n argument here. It is easy to add a string in the resources that takes these as parameters and then you can change the wording later without having to change the code. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:277: permission_info_list.begin(); nit: You could move the initialization of |permission| to happen above the for loop and leave: for (; permission != permission_info_list.end(); ++permission) ... might make this more readable? Up to you. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:284: permission->default_setting)); nit: align indentation of type with default_setting http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:355: // point to the mittle of the icon. Englisch, bitte! s/mittle/middle/. :) http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:357: anchor.Inset(0, anchor_view() ? 5 : 0); Can you create a variable for this magic constant (5)? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:376: } else NOTREACHED? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:415: // Add Identity section nit: End comments in period. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:558: Is there an extra space here at the end? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:20: namespace views { nit: I'd put a line break before this line... http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:29: class PopupHeader : public views::View { Does this need to be defined in the .h file? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:34: void SetIdentityName(string16 identity); Document all members and functions (here and classes below). http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:49: // select a setting for a given site permission. wording: allow to select -> allow selecting http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:50: class PermissionComboboxModel : public ui::ComboboxModel { Same here, does it need to be in the .h file? http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:86: // displayed in a popup that is positioned relative the an anchor element nit: 'the' or 'an', which is it? Also, missing period at the end. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:145: views::Label* CreateTextLabel(string16 text); These Create functions... can you annotate them with the WARN_UNUSED_RESULT, to avoid leaks? http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... File chrome/browser/website_settings.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:528: void WebsiteSettings::Show(views::View* anchor_view, Agree. On 2012/05/29 19:38:57, tfarina wrote: > you should implement this function in > chrome/browser/ui/views/website_settings_popup_view.cc > > same thing for the gtk version.
Sorry for being a bit unresponsive on this thread until now. But I first had to commit CL http://codereview.chromium.org/10440092/ to fix some of the comments in this CL. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:55: } On 2012/05/30 11:20:04, Finnur wrote: > These two functions are a bit unfortunate since they are essentially copied > (from your GTK implementation of the dialog) and slightly altered to use > string16 instead of std::string, but without a comment or mention of needing to > keep the two in sync. I'd probably change it to return the int resource ID > instead of a string and then call GetStringUTF16 or GetStringUTF8 as necessary > at the call sites. Then you can move this code to be shared between the two > implementations. I created two methods for returning the ressource id and added them to the class WebsiteSettingsUI. Since the names of the two new methods are pretty long I kept the two anonymous methods. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:61: // //////////////////////////////////////////////////////////////////////////// On 2012/05/30 11:20:04, Finnur wrote: > nit: To be consistent with other Chrome code, you should not have a space in > these bars (eighty slashes, no space). Same elsewhere. Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:98: return gfx::Size(300, 80); On 2012/05/30 11:20:04, Finnur wrote: > Do you mean to reserve space (80 pixels) even if no name/status has been set? Is > this to prevent flickering? Can the name be long enough to wrap causing you to > need more than that? The width 300 was meant prevent flickering. But it turns out I don't need it. The 80 is wrong. I removed the method. since I don't seem to need it. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:101: void PopupHeader::SetIdentityName(string16 name) { On 2012/05/30 11:20:04, Finnur wrote: > const string16&, same on line 105 Done (Damn it :) ) http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:109: On 2012/05/30 11:20:04, Finnur wrote: > nit: Extra line break. Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:145: + ASCIIToUTF16(")"); On 2012/05/30 11:20:04, Finnur wrote: > String concatenations are generally a bad idea in terms of i18n. Can you convert > this to a single string that takes the arguments as parameters? Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:158: Profile* profile, On 2012/05/30 11:20:04, Finnur wrote: > Why do you need the |profile|? I need it now to create the presenter (WebsiteSettings). http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:223: new views::GridLayout(site_data_content_); On 2012/05/30 11:20:04, Finnur wrote: > nit: This fits within one line. Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:245: label_text += " blocked)"; On 2012/05/30 11:20:04, Finnur wrote: > Same i18n argument here. It is easy to add a string in the resources that takes > these as parameters and then you can change the wording later without having to > change the code. Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:277: permission_info_list.begin(); On 2012/05/30 11:20:04, Finnur wrote: > nit: You could move the initialization of |permission| to happen above the for > loop and leave: > for (; permission != permission_info_list.end(); ++permission) > > ... might make this more readable? Up to you. I don't know. Does this really improve the readability? I don't mind fixing it but would prefer to keep it this way. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:284: permission->default_setting)); On 2012/05/30 11:20:04, Finnur wrote: > nit: align indentation of type with default_setting Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:355: // point to the mittle of the icon. On 2012/05/30 11:20:04, Finnur wrote: > Englisch, bitte! > s/mittle/middle/. > :) Done. You won't believe it. But this is a copy and past error :-) http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:357: anchor.Inset(0, anchor_view() ? 5 : 0); On 2012/05/30 11:20:04, Finnur wrote: > Can you create a variable for this magic constant (5)? Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:376: } On 2012/05/30 11:20:04, Finnur wrote: > else NOTREACHED? Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:415: // Add Identity section On 2012/05/30 11:20:04, Finnur wrote: > nit: End comments in period. Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:558: On 2012/05/30 11:20:04, Finnur wrote: > Is there an extra space here at the end? Removed. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:20: namespace views { On 2012/05/30 11:20:04, Finnur wrote: > nit: I'd put a line break before this line... Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:29: class PopupHeader : public views::View { On 2012/05/30 11:20:04, Finnur wrote: > Does this need to be defined in the .h file? I moved the class definitions back to the .cc file. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:34: void SetIdentityName(string16 identity); On 2012/05/30 11:20:04, Finnur wrote: > Document all members and functions (here and classes below). Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:49: // select a setting for a given site permission. On 2012/05/30 11:20:04, Finnur wrote: > wording: allow to select -> allow selecting Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:50: class PermissionComboboxModel : public ui::ComboboxModel { On 2012/05/30 11:20:04, Finnur wrote: > Same here, does it need to be in the .h file? Same as above in line 29. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:86: // displayed in a popup that is positioned relative the an anchor element On 2012/05/30 11:20:04, Finnur wrote: > nit: 'the' or 'an', which is it? > Also, missing period at the end. Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:145: views::Label* CreateTextLabel(string16 text); On 2012/05/30 11:20:04, Finnur wrote: > These Create functions... can you annotate them with the WARN_UNUSED_RESULT, to > avoid leaks? Done. http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... File chrome/browser/website_settings.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:44: #if defined(TOOLKIT_GTK) On 2012/05/29 19:54:20, Ben Goodger (Google) wrote: > again, toolkit-specific ifdefs are gross, so think about how to avoid > proliferating them here and cleaning up the existing usage. Obsolete since http://codereview.chromium.org/10440092/ http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:47: #include "chrome/browser/ui/views/website_settings_popup_view.h" On 2012/05/29 19:38:57, tfarina wrote: > you can't do this anymore :) see chrome/browser/DEPS file. Obsolete since http://codereview.chromium.org/10440092/ http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:528: void WebsiteSettings::Show(views::View* anchor_view, On 2012/05/29 19:38:57, tfarina wrote: > you should implement this function in > chrome/browser/ui/views/website_settings_popup_view.cc > > same thing for the gtk version. Obsolete since http://codereview.chromium.org/10440092/ http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.cc:528: void WebsiteSettings::Show(views::View* anchor_view, On 2012/05/30 11:20:04, Finnur wrote: > Agree. > > On 2012/05/29 19:38:57, tfarina wrote: > > you should implement this function in > > chrome/browser/ui/views/website_settings_popup_view.cc > > > > same thing for the gtk version. > Obsolete since http://codereview.chromium.org/10440092/ http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_settings.h File chrome/browser/website_settings.h (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/website_setting... chrome/browser/website_settings.h:78: static void Show(views::View* anchor_view, On 2012/05/29 19:54:20, Ben Goodger (Google) wrote: > think about how to make Show() work in a cross platform manner. After a small refactoring: http://codereview.chromium.org/10440092/ I removed the Show method in the class WebsiteSettings. The WebsiteSettingsPopupView has a ShowPopup method now that isu used to open the Popup.
Close. http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:277: permission_info_list.begin(); Like I said, up to you. :) http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:145: views::Label* CreateTextLabel(string16 text); Actually, you should probably also document the fact that the caller is responsible for making sure the return value gets cleaned up. On 2012/06/05 17:02:12, markusheintz_ wrote: > On 2012/05/30 11:20:04, Finnur wrote: > > These Create functions... can you annotate them with the WARN_UNUSED_RESULT, > to > > avoid leaks? > > Done. http://codereview.chromium.org/10456017/diff/12002/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10456017/diff/12002/chrome/app/generated_resou... chrome/app/generated_resources.grd:269: + <message name="IDS_WEBSITE_SETTINGS_PERMISSION_LABEL" desc="The label for a permission value that is used in t permisisons dropdown."> t? You mean 'the', right? http://codereview.chromium.org/10456017/diff/12002/chrome/app/generated_resou... chrome/app/generated_resources.grd:270: + Always $1 I believe you need to be more specific to help out the translators. Example parameter from elsewhere in this file: <ph name="PLATFORM_TEXT">$1<ex>Goto the wrench menu and choose Fix It</ex></ph> This shows the name of the variable and example content (<ex>). Same above. http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:28: const int kBubbleInsets = 5; nit: Document please. :) http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings.h (right): http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.h:20: #endif Is this needed still?
http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.cc:277: permission_info_list.begin(); On 2012/06/06 11:55:50, Finnur wrote: > Like I said, up to you. :) :) http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings_popup_view.h:145: views::Label* CreateTextLabel(string16 text); Done. I also added a comment to the methods above. On 2012/06/06 11:55:50, Finnur wrote: > Actually, you should probably also document the fact that the caller is > responsible for making sure the return value gets cleaned up. > > On 2012/06/05 17:02:12, markusheintz_ wrote: > > On 2012/05/30 11:20:04, Finnur wrote: > > > These Create functions... can you annotate them with the WARN_UNUSED_RESULT, > > to > > > avoid leaks? > > > > Done. > http://codereview.chromium.org/10456017/diff/12002/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/10456017/diff/12002/chrome/app/generated_resou... chrome/app/generated_resources.grd:269: + <message name="IDS_WEBSITE_SETTINGS_PERMISSION_LABEL" desc="The label for a permission value that is used in t permisisons dropdown."> On 2012/06/06 11:55:50, Finnur wrote: > t? You mean 'the', right? Done. http://codereview.chromium.org/10456017/diff/12002/chrome/app/generated_resou... chrome/app/generated_resources.grd:270: + Always $1 On 2012/06/06 11:55:50, Finnur wrote: > I believe you need to be more specific to help out the translators. Example > parameter from elsewhere in this file: > > <ph name="PLATFORM_TEXT">$1<ex>Goto the wrench menu and choose Fix It</ex></ph> > > This shows the name of the variable and example content (<ex>). > > Same above. :( Sorry this slipped into another local git branch I forked. Done. http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:28: const int kBubbleInsets = 5; On 2012/06/06 11:55:50, Finnur wrote: > nit: Document please. :) :). Done. I also improved the constant name. http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings.h (right): http://codereview.chromium.org/10456017/diff/12002/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings.h:20: #endif On 2012/06/06 11:55:50, Finnur wrote: > Is this needed still? No. Removed
LGTM, one nit. http://codereview.chromium.org/10456017/diff/12003/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/12003/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:33: // omnibox). s/omnibox/Omnibox/ (multiple cases in the paragraph above)
Thanks a lot for the review Finnur. Now I also need owners approval for the gtk changes, the UI stuff and a UI string review: @Elliot: Could you please Review the GTK parts? Thanks a lot. @Ben: Could you please review the remaining ui stuff? Thanks a lot. @Glen: Please review the strings I added. Thanks a lot. http://codereview.chromium.org/10456017/diff/12003/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/12003/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:33: // omnibox). On 2012/06/06 14:10:00, Finnur wrote: > s/omnibox/Omnibox/ > (multiple cases in the paragraph above) Done.
It looks like Ben is OOO @sky: Scott would you mind taking a look the this CL as a UI owner? Thanks a lot.
gtk lgtm
http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:159: ContentSettingsType site_permission, ContentSetting default_setting) nit: I'd wrap it argument in its own line. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:168: } nit: Peter (pkasting) insists that single-line statements should not require { } even though the condition spans multiple lines. The opposite is not true, if there are more than one statement for the condition then it requires { } http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:337: new PermissionComboboxModel(permission->type, nit: Please, do not do this way. I have written many patches fixing this leak. views::ComboboxModel does NOT own the model, thus we are leaking here. One way I have fixed some chromeos leaks was: - In header file: namespace internal { class PermissionComboboxModel; } - scoped_ptr<PermissionComboboxModel> permission_combobox_model_; - In source file: namespace internal { class PermissionComboboxModel : public ui::ComboboxModel { ... }; PermissionComboboxModel::PermissionComboboxModel() {} ... And that way you will be able to avoid the static_cast below. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:418: static_cast<PermissionComboboxModel*>(combobox->model()); nice: although I like this trick, see my comment about the ownership/leak of ComboboxModel. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:9: #include "base/compiler_specific.h" nit: basictypes.h for DISALLOW ;) http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:12: #include "ui/base/models/combobox_model.h" are you using this header file here? http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:17: #include "ui/views/view.h" nit: doesn't this come from bubble_delegate.h? http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:33: } // namespace views nit: I'd remove this "// namespace views", in ui/views we are pretty consistent about this. Up to you though. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:48: WebsiteSettingsPopupView(views::View* anchor_view, can you make this constructor private?
http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings_ui.h (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.h:85: static int PermissionTypeToUIStringID(ContentSettingsType type); these two static functions should be declared after the virtual destructor, see crbug.com/68682
http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings_ui.cc (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.cc:25: int WebsiteSettingsUI::PermissionTypeToUIStringID(ContentSettingsType type) { // static
http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:42: static void ShowPopup(views::View* anchor_view, constructor/destructor before statics. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:108: TabContentsWrapper* tab_contents_wrapper_; Document ownership of all non-views classes.
http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:159: ContentSettingsType site_permission, ContentSetting default_setting) On 2012/06/06 16:58:29, tfarina wrote: > nit: I'd wrap it argument in its own line. Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:168: } On 2012/06/06 16:58:29, tfarina wrote: > nit: Peter (pkasting) insists that single-line statements should not require { } > even though the condition spans multiple lines. The opposite is not true, if > there are more than one statement for the condition then it requires { } Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:337: new PermissionComboboxModel(permission->type, On 2012/06/06 16:58:29, tfarina wrote: > nit: Please, do not do this way. I have written many patches fixing this leak. > views::ComboboxModel does NOT own the model, thus we are leaking here. > > One way I have fixed some chromeos leaks was: > - In header file: > namespace internal { > class PermissionComboboxModel; > } > - scoped_ptr<PermissionComboboxModel> permission_combobox_model_; > > - In source file: > namespace internal { > class PermissionComboboxModel : public ui::ComboboxModel { > ... > }; > > PermissionComboboxModel::PermissionComboboxModel() {} > ... > > And that way you will be able to avoid the static_cast below. Thanks a lot for pointing this out! I didn't know this. Since I store the permission type and other permission type related information in the PermissionComboboxModel I need to keep a vector for all Combobox models. I use a ScopedVector now that deletes all the ComboboxModels when the popup is destroyed. I chose "website_settings" as namespace. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:418: static_cast<PermissionComboboxModel*>(combobox->model()); On 2012/06/06 16:58:29, tfarina wrote: > nice: although I like this trick, see my comment about the ownership/leak of > ComboboxModel. :) I'll keep it this way since this allows me to use a ScopedVector instead of a combobox -> model map (see above for why I need to store several models). http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:9: #include "base/compiler_specific.h" On 2012/06/06 16:58:29, tfarina wrote: > nit: basictypes.h for DISALLOW ;) Done. I promiss to create a checklist and the first item will be: "Check if I added base/compiler_specific.h." http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:12: #include "ui/base/models/combobox_model.h" On 2012/06/06 16:58:29, tfarina wrote: > are you using this header file here? moved to .cc file. I forgot this when I move the class definition. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:17: #include "ui/views/view.h" On 2012/06/06 16:58:29, tfarina wrote: > nit: doesn't this come from bubble_delegate.h? Don't we include all header for all classes we explicitly use? I thought implicit includes through other headers are not encouraged to document code dependencies. I'm returning views::View pointers. So I explicitly depend on "View". Hm.. since these I only user Views pointers, I could forward declare it. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:33: } // namespace views On 2012/06/06 16:58:29, tfarina wrote: > nit: I'd remove this "// namespace views", in ui/views we are pretty consistent > about this. Up to you though. Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:42: static void ShowPopup(views::View* anchor_view, On 2012/06/06 17:57:10, sky wrote: > constructor/destructor before statics. Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:48: WebsiteSettingsPopupView(views::View* anchor_view, On 2012/06/06 16:58:29, tfarina wrote: > can you make this constructor private? Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:108: TabContentsWrapper* tab_contents_wrapper_; On 2012/06/06 17:57:10, sky wrote: > Document ownership of all non-views classes. Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings_ui.cc (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.cc:25: int WebsiteSettingsUI::PermissionTypeToUIStringID(ContentSettingsType type) { On 2012/06/06 17:02:20, tfarina wrote: > // static Done. http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... File chrome/browser/ui/website_settings/website_settings_ui.h (right): http://codereview.chromium.org/10456017/diff/16021/chrome/browser/ui/website_... chrome/browser/ui/website_settings/website_settings_ui.h:85: static int PermissionTypeToUIStringID(ContentSettingsType type); On 2012/06/06 17:01:30, tfarina wrote: > these two static functions should be declared after the virtual destructor, see > crbug.com/68682 Done. Thanks a lot for including the bug. http://codereview.chromium.org/10456017/diff/20005/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/20005/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:52: TabContentsWrapper* tab_contents, Since there is an effort going on to rename TabContentsWrapper to TabContents, I s/wrapper/tab_contents/
http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:15: #include "googleurl/src/gurl.h" it's fine, but I seem to recall that if you don't use any functions from GURL API and are just passing it around, either through pointer or reference, you don't need the include. Up to you though. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:206: WebsiteSettingsPopupView::WebsiteSettingsPopupView( the order of the implementation should match with the order of the declaration. So this should be implemented after ShowPopup() function. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:214: presenter_(NULL), you don't need to do this, because |presenter_| is not a raw a pointer. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:347: case CONTENT_SETTING_DEFAULT: combobox->SetSelectedIndex(0); nit: move the SetSelectedIndex call onto the next line. Same for the cases below. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:400: int width = 300; nit: is it worth making this a constant in the unnamed namespace? http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:407: return size; nit: return gfx::Size(width, height); http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:434: if (source != cookie_dialog_link_) I think we don't usually do like this. Scott, correct if I'm wrong. Probably, better to do: DCHECK_EQ(cookie_dialog_link_, source); http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:14: #include "ui/base/models/combobox_model.h" already included in source file, and not used here. please, remove. ;) http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:29: namespace website_settings { nit: sort this, after namespace views.
http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:15: #include "googleurl/src/gurl.h" On 2012/06/07 20:51:59, tfarina wrote: > it's fine, but I seem to recall that if you don't use any functions from GURL > API and are just passing it around, either through pointer or reference, you > don't need the include. Up to you though. True, But I guess I'll just leave it there. It should not harm. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:206: WebsiteSettingsPopupView::WebsiteSettingsPopupView( On 2012/06/07 20:51:59, tfarina wrote: > the order of the implementation should match with the order of the declaration. > So this should be implemented after ShowPopup() function. Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:214: presenter_(NULL), On 2012/06/07 20:51:59, tfarina wrote: > you don't need to do this, because |presenter_| is not a raw a pointer. Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:347: case CONTENT_SETTING_DEFAULT: combobox->SetSelectedIndex(0); On 2012/06/07 20:51:59, tfarina wrote: > nit: move the SetSelectedIndex call onto the next line. Same for the cases > below. Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:400: int width = 300; On 2012/06/07 20:51:59, tfarina wrote: > nit: is it worth making this a constant in the unnamed namespace? Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:407: return size; On 2012/06/07 20:51:59, tfarina wrote: > nit: return gfx::Size(width, height); Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:434: if (source != cookie_dialog_link_) On 2012/06/07 20:51:59, tfarina wrote: > I think we don't usually do like this. Scott, correct if I'm wrong. > > Probably, better to do: > DCHECK_EQ(cookie_dialog_link_, source); Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.h (right): http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:14: #include "ui/base/models/combobox_model.h" On 2012/06/07 20:51:59, tfarina wrote: > already included in source file, and not used here. please, remove. ;) Done. http://codereview.chromium.org/10456017/diff/16023/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.h:29: namespace website_settings { On 2012/06/07 20:51:59, tfarina wrote: > nit: sort this, after namespace views. Done.
LGTM, with the following nit. http://codereview.chromium.org/10456017/diff/23001/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/23001/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:405: int width = kPopupWidth; nit: no need of this temp (non-const) variable. just do: return gfx::Size(kPopupWidth, height);
Thanks a lot Thiago for the review. @Scott: Could you take another look at the CL. Thanks a lot. http://codereview.chromium.org/10456017/diff/23001/chrome/browser/ui/views/we... File chrome/browser/ui/views/website_settings_popup_view.cc (right): http://codereview.chromium.org/10456017/diff/23001/chrome/browser/ui/views/we... chrome/browser/ui/views/website_settings_popup_view.cc:405: int width = kPopupWidth; On 2012/06/07 22:09:41, tfarina wrote: > nit: no need of this temp (non-const) variable. > > just do: > > return gfx::Size(kPopupWidth, height); Done.
LGTM
If the strings match the mocks, you don't need a review from me. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10456017/10022
Change committed as 141407 |
