Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828201:
(Views only) Create a separate class for the contents of the Website Settings UI that is displayed … (Closed)
|
Created:
8 years, 4 months ago by markusheintz_ Modified:
8 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description(Views only) Create a separate class for the contents of the Website Settings UI that is displayed for internal Chrome pages.
BUG=113688
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150790
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #
Total comments: 14
Patch Set 3 : Address comments. #
Total comments: 4
Patch Set 4 : Address Comments. #
Total comments: 10
Messages
Total messages: 15 (0 generated)
Please review this CL. Thanks :) This refactoring is a follow up for CL https://chromiumcodereview.appspot.com/10826185/.
https://chromiumcodereview.appspot.com/10828201/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: views::GridLayout* layout = new views::GridLayout(this); GridLayout seems overkill here, please try a BoxLayout.
http://codereview.chromium.org/10828201/diff/1/chrome/browser/ui/views/websit... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): http://codereview.chromium.org/10828201/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:146: virtual gfx::Rect GetAnchorRect() OVERRIDE; nit: can this be private? please. http://codereview.chromium.org/10828201/diff/1/chrome/browser/ui/views/websit... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: views::GridLayout* layout = new views::GridLayout(this); I don't like doing many things in constructors. Is there another way to initialize all this stuff outside of the constructor?
https://chromiumcodereview.appspot.com/10828201/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:146: virtual gfx::Rect GetAnchorRect() OVERRIDE; On 2012/08/08 02:32:46, tfarina wrote: > nit: can this be private? please. Done. https://chromiumcodereview.appspot.com/10828201/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: views::GridLayout* layout = new views::GridLayout(this); On 2012/08/08 02:32:46, tfarina wrote: > I don't like doing many things in constructors. Is there another way to > initialize all this stuff outside of the constructor? In general I agree with you. But in this particular case what would be the benefit? I would need to add an additional Init method that is called right after the object is created. This means I add additional code with no benefit. Please correct me if I'm wrong :). https://chromiumcodereview.appspot.com/10828201/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: views::GridLayout* layout = new views::GridLayout(this); On 2012/08/07 22:21:49, msw wrote: > GridLayout seems overkill here, please try a BoxLayout. Done.
LGTM with nits. https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:138: // |WebsiteSettingsPopupView| the |WebsiteSettingsInternalPageView| is nit: add a comma: "|WebsiteSettingsPopupView|, the" https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:226: // WebsiteSettingsInternalPageView nit: I don't love the name... but lack a great suggestion, InternalPagePopupView? https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: const int spacing = 4; nit: this should be kSpacing, i think. (perhaps same in your other review, did I miss that?) https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:238: icon_view->SetImage(icon.ToImageSkia()); nit: consider the shorter version (very optional): icon_view->SetImage(rb. GetImageSkiaNamed(IDR_PRODUCT_LOGO_26)); https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:242: IDS_PAGE_INFO_INTERNAL_PAGE); nit: fits on line above.
https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:138: // |WebsiteSettingsPopupView| the |WebsiteSettingsInternalPageView| is On 2012/08/08 17:27:08, msw wrote: > nit: add a comma: "|WebsiteSettingsPopupView|, the" Done. https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:226: // WebsiteSettingsInternalPageView On 2012/08/08 17:27:08, msw wrote: > nit: I don't love the name... but lack a great suggestion, > InternalPagePopupView? How about InternalPageInfoView? https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: const int spacing = 4; On 2012/08/08 17:27:08, msw wrote: > nit: this should be kSpacing, i think. (perhaps same in your other review, did I > miss that?) Done. I'm not sure which other CL you mean. I'll skipped through some recent CLs and it looks like you caught all issues ;-). https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:238: icon_view->SetImage(icon.ToImageSkia()); On 2012/08/08 17:27:08, msw wrote: > nit: consider the shorter version (very optional): > icon_view->SetImage(rb. GetImageSkiaNamed(IDR_PRODUCT_LOGO_26)); Done. https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:242: IDS_PAGE_INFO_INTERNAL_PAGE); On 2012/08/08 17:27:08, msw wrote: > nit: fits on line above. Done.
LGTM with nits; thanks! https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:226: // WebsiteSettingsInternalPageView On 2012/08/08 18:00:35, markusheintz_ wrote: > On 2012/08/08 17:27:08, msw wrote: > > nit: I don't love the name... but lack a great suggestion, > > InternalPagePopupView? > How about InternalPageInfoView? It's still a 'Popup' bubble, like WebsiteSettings, and both bubbles contain 'Info'; but InternalPageInfoView is an improvement, thanks! https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: const int spacing = 4; On 2012/08/08 18:00:35, markusheintz_ wrote: > On 2012/08/08 17:27:08, msw wrote: > > nit: this should be kSpacing, i think. (perhaps same in your other review, did > I > > miss that?) > > Done. I'm not sure which other CL you mean. I'll skipped through some recent > CLs and it looks like you caught all issues ;-). found it and sent another nit :p http://codereview.chromium.org/10823229/diff/4001/ui/views/controls/tabbed_pa... https://chromiumcodereview.appspot.com/10828201/diff/8002/chrome/browser/ui/v... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/8002/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:230: views::View* anchor_view) nit: fits on line above. https://chromiumcodereview.appspot.com/10828201/diff/8002/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:235: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); nit: move down a line.
https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:226: // WebsiteSettingsInternalPageView On 2012/08/08 18:34:10, msw wrote: > On 2012/08/08 18:00:35, markusheintz_ wrote: > > On 2012/08/08 17:27:08, msw wrote: > > > nit: I don't love the name... but lack a great suggestion, > > > InternalPagePopupView? > > How about InternalPageInfoView? > > It's still a 'Popup' bubble, like WebsiteSettings, and both bubbles contain > 'Info'; but InternalPageInfoView is an improvement, thanks! Well then let's use the descriptive name: InternalPageInfoPopupView. This name reminds me somehow of the times when I used to code in Java :) https://chromiumcodereview.appspot.com/10828201/diff/1003/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:232: const int spacing = 4; On 2012/08/08 18:34:10, msw wrote: > On 2012/08/08 18:00:35, markusheintz_ wrote: > > On 2012/08/08 17:27:08, msw wrote: > > > nit: this should be kSpacing, i think. (perhaps same in your other review, > did > > I > > > miss that?) > > > > Done. I'm not sure which other CL you mean. I'll skipped through some recent > > CLs and it looks like you caught all issues ;-). > > found it and sent another nit :p > http://codereview.chromium.org/10823229/diff/4001/ui/views/controls/tabbed_pa... And I changed the code so that I don't need to address the nit :-D https://chromiumcodereview.appspot.com/10828201/diff/8002/chrome/browser/ui/v... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/8002/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:230: views::View* anchor_view) On 2012/08/08 18:34:10, msw wrote: > nit: fits on line above. Done. https://chromiumcodereview.appspot.com/10828201/diff/8002/chrome/browser/ui/v... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:235: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); On 2012/08/08 18:34:10, msw wrote: > nit: move down a line. Done.
LGTM; thanks!
On 2012/08/08 23:16:34, msw wrote: > LGTM; thanks! Hey Thiago, I'll land this CL now. Let me know if you still have any concerns about the constructor being to heavy weight. I'm more than happy to address them.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10828201/11001
Change committed as 150790
https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:157: : name_(NULL), status_(NULL) { indent 4 spaces https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:220: SkColor text_color) { wrong indentation here https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:239: string16 text = l10n_util::GetStringUTF16(IDS_PAGE_INFO_INTERNAL_PAGE); could you avoid this temporary variable and fold it below? Looks like you did created the temp var for readability ;) https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:278: new WebsiteSettingsPopupView(anchor_view, profile, tab_contents, url, ssl); Looking now, and seeing that this inherits from BubbleDelegateView, would be more appropriate to name it WebsiteSettingsBubbleView? For me popups are the irritating windows that we usually block. Same thing for InternalPageInfoPopupView (may be InternalWebsiteSettingsBubbleView or something). https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:364: presenter_->OnUIClosing(); Is the comment about |presenter_| being NULL still true, in another form, will be possible for |presenter_| to be NULL after this refactoring? If not, can you update it?
Also TEST=none is not usefull, there was a recent thread on chromium-dev about this line! :)
Thanks a lot for the comments. I addressed them in CL http://codereview.chromium.org/10854079/ https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:157: : name_(NULL), status_(NULL) { On 2012/08/09 16:37:40, tfarina wrote: > indent 4 spaces Done. https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:220: SkColor text_color) { On 2012/08/09 16:37:40, tfarina wrote: > wrong indentation here Done. https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:239: string16 text = l10n_util::GetStringUTF16(IDS_PAGE_INFO_INTERNAL_PAGE); On 2012/08/09 16:37:40, tfarina wrote: > could you avoid this temporary variable and fold it below? Looks like you did > created the temp var for readability ;) Done. https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:278: new WebsiteSettingsPopupView(anchor_view, profile, tab_contents, url, ssl); On 2012/08/09 16:37:40, tfarina wrote: > Looking now, and seeing that this inherits from BubbleDelegateView, would be > more appropriate to name it WebsiteSettingsBubbleView? For me popups are the > irritating windows that we usually block. Same thing for > InternalPageInfoPopupView (may be InternalWebsiteSettingsBubbleView or > something). I see that inheriting from BubbleDelegetView suggest to name that WebsiteSettings thingy bubble. But :) : 1) Some people think bubble is confusing because of the notification bubbles and other small bubbles that are used to notify users about ...things :). I agree with that :) 2) We also have an ExtensionPopup (which is also a bubble delegate - if I remember correctly ) and we call it popup in the UI and in the Extension documentation. 3) I tried to find a name that everybody likes. At one point in time I just gave up on this. I call it a popup and try to use that term consistently. :-) 4) Also renaming it now would be such a pain as there are also GTK and cocoa implementations. :( https://chromiumcodereview.appspot.com/10828201/diff/11001/chrome/browser/ui/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:364: presenter_->OnUIClosing(); On 2012/08/09 16:37:40, tfarina wrote: > Is the comment about |presenter_| being NULL still true, in another form, will > be possible for |presenter_| to be NULL after this refactoring? If not, can you > update it? Good catch. Fixed. |
