Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
795053003:
[Password Manager] Close the bubble when fullscreen state gets change. (Closed)
|
Created:
6 years ago by Pritam Nikam Modified:
5 years, 11 months ago CC:
chromium-reviews, vabr (Chromium) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Close the bubble when full screen state gets change.
BUG=425993
Committed: https://crrev.com/bef9a565814ecf1c571640fea39a8e49754016ec
Cr-Commit-Position: refs/heads/master@{#309613}
Patch Set 1 : #
Total comments: 31
Patch Set 2 : Incorporated Vasilii's review comments. #
Total comments: 6
Patch Set 3 : Addresses Vasilii's review comments. #
Total comments: 28
Patch Set 4 : Addresses Peter's review comments. #
Total comments: 14
Patch Set 5 : Addresses Peter's review comments. #Messages
Total messages: 24 (8 generated)
Patchset #1 (id:1) has been deleted
pritam.nikam@samsung.com changed reviewers: + mkwst@chromium.org, vabr@chromium.org, vasilii@chromium.org
Hi Vaclav, Vasilii & Mike, I could see many bubble view implementations (derived from views::BubbleDelegateView); and number of these are interested in knowing the full screen state change as well (apart from password save and translate); I assume this patch shall take care of removing all code duplicates (if any). So this patch is more about starting a discussion for the same. Please let me know whether my understanding and this patch is on right track? Thanks!
Hi Pritam, I'm unavailable in the next couple of weeks. You don't need 3 reviewers :), and Vasilii knows the problem well enough (having reported it :)). I'll pass the reviewing to him and/or Mike. Cheers, Vaclav
vabr@chromium.org changed reviewers: - vabr@chromium.org
https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:144: // Add observers immersive fullscreen revealed state changes. nit: grammar. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:12: #include "content/public/browser/web_contents.h" Is it needed? https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:22: AddAccelerator(ui::Accelerator(ui::VKEY_F11, ui::EF_NONE)); Why do we need it? https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:34: void ManagedFullScreenBubbleDelegateView::AdjustForFullscreen( Order of definitions should match the order of declarations in the header. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:40: const int kFullscreenPaddingEnd = 20; the constant should go to the beginning of the file. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:41: const size_t bubble_half_width = width() / 2; should be int. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:65: Close(); Why would F11 close the bubble? https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:12: class ManagePasswordsIconView; unused. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:17: class NotificationRegistrar; No need to forward declare classes which you already included. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:24: class View; The same as above. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:27: class ManagedFullScreenBubbleDelegateView Add a comment. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:35: // Close the buuble. bubble https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:36: virtual void Close(); This method should be neither public nor virtual. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:9: #include "chrome/browser/ui/browser_finder.h" Clean up the unused headers. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/translate/translate_bubble_view.h:18: #include "ui/views/bubble/bubble_delegate.h" unused
Thanks vasilii for your review inputs. I've addressed these in my new patch set, PTAL. Thanks! https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:144: // Add observers immersive fullscreen revealed state changes. On 2014/12/17 13:40:11, vasilii wrote: > nit: grammar. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:12: #include "content/public/browser/web_contents.h" On 2014/12/17 13:40:12, vasilii wrote: > Is it needed? Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:22: AddAccelerator(ui::Accelerator(ui::VKEY_F11, ui::EF_NONE)); On 2014/12/17 13:40:11, vasilii wrote: > Why do we need it? Cases where bubble view is having focus, F11 wont do anything, i.e. does not toggle the browser full screen state. I've registered F11 to capture full screen state change event, however I'm not sure whether the counter action i.e. closing a bubble is right or wrong here. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:34: void ManagedFullScreenBubbleDelegateView::AdjustForFullscreen( On 2014/12/17 13:40:11, vasilii wrote: > Order of definitions should match the order of declarations in the header. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:40: const int kFullscreenPaddingEnd = 20; On 2014/12/17 13:40:12, vasilii wrote: > the constant should go to the beginning of the file. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:41: const size_t bubble_half_width = width() / 2; On 2014/12/17 13:40:11, vasilii wrote: > should be int. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:65: Close(); On 2014/12/17 13:40:11, vasilii wrote: > Why would F11 close the bubble? I'm not sure whether we should close the bubble or not. But to keep consistent behavior of closing the bubble on full-screen toggling I have added the Close() call here. May be appropriate behavior would be pass F11 to root view, & through notification observer Close() this bubble. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:12: class ManagePasswordsIconView; On 2014/12/17 13:40:12, vasilii wrote: > unused. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:17: class NotificationRegistrar; On 2014/12/17 13:40:12, vasilii wrote: > No need to forward declare classes which you already included. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:24: class View; On 2014/12/17 13:40:12, vasilii wrote: > The same as above. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:27: class ManagedFullScreenBubbleDelegateView On 2014/12/17 13:40:12, vasilii wrote: > Add a comment. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:35: // Close the buuble. On 2014/12/17 13:40:12, vasilii wrote: > bubble Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:36: virtual void Close(); On 2014/12/17 13:40:12, vasilii wrote: > This method should be neither public nor virtual. subclasses having specific implementations hence need to be virtual. |ManagePasswordsBubbleView::Close|, |ZoomBubbleView::Close| etc... https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:9: #include "chrome/browser/ui/browser_finder.h" On 2014/12/17 13:40:12, vasilii wrote: > Clean up the unused headers. Done. https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/translate/translate_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/translate/translate_bubble_view.h:18: #include "ui/views/bubble/bubble_delegate.h" On 2014/12/17 13:40:12, vasilii wrote: > unused Done.
https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:22: AddAccelerator(ui::Accelerator(ui::VKEY_F11, ui::EF_NONE)); On 2014/12/17 16:06:26, Pritam Nikam wrote: > On 2014/12/17 13:40:11, vasilii wrote: > > Why do we need it? > > Cases where bubble view is having focus, F11 wont do anything, i.e. does not > toggle the browser full screen state. > I've registered F11 to capture full screen state change event, however I'm not > sure whether the counter action i.e. closing a bubble is right or wrong here. Indeed, but closing the bubble on F11 isn't what users expect. In this patch I'd leave the problem as is. The bubble with focus will swallow all the accelerators not only F11. If we decide to solve it we should fix it in another way. https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:31: if (web_contents) { Can it be NULL? https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:19: class View; it's already included. https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:23: // state transition notifications. It closes on browser's full screen transition notifications.
Thanks Visilii for review inputs. I've addressed these along newest patch set, PTAL. Thanks! https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:31: if (web_contents) { On 2014/12/18 16:16:22, vasilii wrote: > Can it be NULL? I don't think it can be NULL in practical cases, however this check is specifically added to address unit_tests failures: TranslateBubbleViewTest.TryAgainButton TranslateBubbleViewTest.DoneButton TranslateBubbleViewTest.AlwaysTranslateCheckboxAndCancelButton TranslateBubbleViewTest.ShowOriginalButton TranslateBubbleViewTest.TranslateButton TranslateBubbleViewTest.CancelButtonReturningAfterTranslate TranslateBubbleViewTest.AlwaysTranslateCheckboxAndDoneButton TranslateBubbleViewTest.AdvancedLink TranslateBubbleViewTest.CancelButtonReturningBeforeTranslate TranslateBubbleViewTest.CancelButtonReturningError TranslateBubbleViewTest.DoneButtonWithoutTranslating https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:19: class View; On 2014/12/18 16:16:22, vasilii wrote: > it's already included. Done. https://codereview.chromium.org/795053003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:23: // state transition notifications. On 2014/12/18 16:16:22, vasilii wrote: > It closes on browser's full screen transition notifications. Done.
lgtm You also need LGTMs from other bubble's owners.
pritam.nikam@samsung.com changed reviewers: + pkasting@chromium.org
Hi Peter, please review changes in - chrome/browser/ui/views/location_bar/zoom_bubble_view.h/cc - chrome/browser/ui/views/translate/translate_bubble_view.h/cc Thanks!
Mostly trivial stuff https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:144: // Add observer to immerse fullscreen revealed state changes. This comment does not make sense. I don't think there's anything to say here the code doesn't already say, so I'd just remove it entirely. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:179: GetWidget()->Close(); Nit: Consider calling ManagedFullScreenBubbleDelegateView::Close() instead as this last line, in case we start wanting all these classes to do something else during closing down the road. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:92: // ManagedFullScreenBubbleDelegateView: Nit: Normally we try to encourage placing all overrides together, in the same order as the parent classes are declared. In this case things are already scattered around pretty badly and we list overrides for classes that aren't direct parents (e.g. views::View), so there's really no good place to put this, but if you feel inclined to clean all this up you're welcome to do so. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:17: const int kFullscreenPaddingEnd = 20; Nit: Declare this in the lone function where it's used. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:26: : views::BubbleBorder::NONE)) { Nit: Wrap like: : BubbleDelegateView( anchor_view, anchor_view ? views::BubbleBorder::TOP_RIGHT : views::BubbleBorder::NONE) { https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:43: DCHECK_EQ(type, chrome::NOTIFICATION_FULLSCREEN_CHANGED); Nit: (expected, actual) https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:53: const int bubble_half_width = width() / 2; Nit: While I'm fine with them, most Chromium code omits const declarations on local non-pointer/ref variables. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:55: (base::i18n::IsRTL() Nit: Don't parenthesize entire RHS. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:58: kFullscreenPaddingEnd)); Nit: Wrap like: const int x_pos = base::i18n::IsRTL() ? (screen_bounds.x() + bubble_half_width + kFullscreenPaddingEnd) : (screen_bounds.right() - bubble_half_width - kFullscreenPaddingEnd); https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:19: // transition notifications. This comment has grammar issues. Perhaps you want something like this: View used to automatically close a bubble when the browser transitions in or out of fullscreen mode. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:33: protected: I only see Close() getting overridden. Does AdjustForFullscreen() really need to be protected as well, or can it be private? https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:40: // Close the bubble. Nit: Closes I also suggest listing virtual methods before non-virtual ones. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1006: GetWidget()->Close(); Nit: Same comment as in other class https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:67: // ManagedFullScreenBubbleDelegateView: See earlier comments regarding override order.
Patchset #4 (id:80001) has been deleted
Thanks Peter for inputs. I've addressed same with patch set #4, PTAL. Thanks! https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:144: // Add observer to immerse fullscreen revealed state changes. On 2014/12/19 22:11:35, Peter Kasting wrote: > This comment does not make sense. > > I don't think there's anything to say here the code doesn't already say, so I'd > just remove it entirely. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:179: GetWidget()->Close(); On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: Consider calling ManagedFullScreenBubbleDelegateView::Close() instead as > this last line, in case we start wanting all these classes to do something else > during closing down the road. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:92: // ManagedFullScreenBubbleDelegateView: On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: Normally we try to encourage placing all overrides together, in the same > order as the parent classes are declared. In this case things are already > scattered around pretty badly and we list overrides for classes that aren't > direct parents (e.g. views::View), so there's really no good place to put this, > but if you feel inclined to clean all this up you're welcome to do so. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:17: const int kFullscreenPaddingEnd = 20; On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: Declare this in the lone function where it's used. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:26: : views::BubbleBorder::NONE)) { On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: Wrap like: > > : BubbleDelegateView( > anchor_view, > anchor_view ? > views::BubbleBorder::TOP_RIGHT : views::BubbleBorder::NONE) { Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:43: DCHECK_EQ(type, chrome::NOTIFICATION_FULLSCREEN_CHANGED); On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: (expected, actual) Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:53: const int bubble_half_width = width() / 2; On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: While I'm fine with them, most Chromium code omits const declarations on > local non-pointer/ref variables. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:55: (base::i18n::IsRTL() On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: Don't parenthesize entire RHS. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:58: kFullscreenPaddingEnd)); On 2014/12/19 22:11:35, Peter Kasting wrote: > Nit: Wrap like: > > const int x_pos = base::i18n::IsRTL() ? > (screen_bounds.x() + bubble_half_width + kFullscreenPaddingEnd) : > (screen_bounds.right() - bubble_half_width - kFullscreenPaddingEnd); Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:19: // transition notifications. On 2014/12/19 22:11:36, Peter Kasting wrote: > This comment has grammar issues. Perhaps you want something like this: > > View used to automatically close a bubble when the browser transitions in or out > of fullscreen mode. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:33: protected: On 2014/12/19 22:11:36, Peter Kasting wrote: > I only see Close() getting overridden. Does AdjustForFullscreen() really need > to be protected as well, or can it be private? Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:40: // Close the bubble. On 2014/12/19 22:11:36, Peter Kasting wrote: > Nit: Closes > > I also suggest listing virtual methods before non-virtual ones. Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1006: GetWidget()->Close(); On 2014/12/19 22:11:36, Peter Kasting wrote: > Nit: Same comment as in other class Done. https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:67: // ManagedFullScreenBubbleDelegateView: On 2014/12/19 22:11:36, Peter Kasting wrote: > See earlier comments regarding override order. Done.
https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:63: // views::View methods. Since you're trying to clean this stuff up anyway, please group together all overrides that come from a single immediate parent. This would mean not having separate sections for views::View, views::BubbleDelegateView, or ui::EventHandler, but instead putting those methods in whatever immediate parent actually includes those classes in its own inheritance hierarchy. Standardize the comment above that section as just "ParentClassName:" For example, let's say we have three parents A, B, C, and a child D: class A { virtual void Method1(); virtual void Method2(); }; class B { virtual void Method3(); }; class C : public A { // A: virtual void Method2(); virtual void Method4(); }; class D : public B, public C { // First list any overrides from B in a section headed "B:" // Then list any overrides from C in a section headed "C:" // Within the C section, list things first declared in A first, // then things first declared in C // So you'd have something like the following: // B: virtual void Method3(); // C: virtual void Method1(); virtual void Method2(); virtual void Method4(); }; https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:72: using ManagedFullScreenBubbleDelegateView::AdjustForFullscreen; Don't do this, just make the method public in the parent class. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:105: struct ZoomBubbleExtensionInfo { Member structs/classes should probably go above methods (along with any typedefs and enums, not that those are present here), like the order was previously. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:47: // The bubble's padding from the screen edge, used in fullscreen. Nit: Instead of the somewhat-redundant comment, how about just a better constant name: const int kBubblePaddingFromScreenEdge = 20; https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:38: virtual void AdjustForFullscreen(const gfx::Rect& screen_bounds); Why is this virtual? No subclass overrides it AFAICT. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:67: // views::WidgetDelegate: Same comments regarding override groups. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:75: using ManagedFullScreenBubbleDelegateView::AdjustForFullscreen; Again, don't do this.
Patchset #5 (id:120001) has been deleted
Thanks Peter, I've incorporated review inputs along patch set #5, PTAL. Thanks! https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:63: // views::View methods. On 2014/12/23 00:49:02, Peter Kasting wrote: > Since you're trying to clean this stuff up anyway, please group together all > overrides that come from a single immediate parent. This would mean not having > separate sections for views::View, views::BubbleDelegateView, or > ui::EventHandler, but instead putting those methods in whatever immediate parent > actually includes those classes in its own inheritance hierarchy. Standardize > the comment above that section as just "ParentClassName:" > > For example, let's say we have three parents A, B, C, and a child D: > > class A { > virtual void Method1(); > virtual void Method2(); > }; > > class B { > virtual void Method3(); > }; > > class C : public A { > // A: > virtual void Method2(); > > virtual void Method4(); > }; > > class D : public B, public C { > // First list any overrides from B in a section headed "B:" > // Then list any overrides from C in a section headed "C:" > // Within the C section, list things first declared in A first, > // then things first declared in C > // So you'd have something like the following: > > // B: > virtual void Method3(); > > // C: > virtual void Method1(); > virtual void Method2(); > virtual void Method4(); > }; Done. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:72: using ManagedFullScreenBubbleDelegateView::AdjustForFullscreen; On 2014/12/23 00:49:02, Peter Kasting wrote: > Don't do this, just make the method public in the parent class. Done. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/zoom_bubble_view.h:105: struct ZoomBubbleExtensionInfo { On 2014/12/23 00:49:02, Peter Kasting wrote: > Member structs/classes should probably go above methods (along with any typedefs > and enums, not that those are present here), like the order was previously. Done. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.cc:47: // The bubble's padding from the screen edge, used in fullscreen. On 2014/12/23 00:49:02, Peter Kasting wrote: > Nit: Instead of the somewhat-redundant comment, how about just a better constant > name: > > const int kBubblePaddingFromScreenEdge = 20; Done. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/managed_full_screen_bubble_delegate_view.h:38: virtual void AdjustForFullscreen(const gfx::Rect& screen_bounds); On 2014/12/23 00:49:02, Peter Kasting wrote: > Why is this virtual? No subclass overrides it AFAICT. Done. Kept as non-virtual protected method (called from static functions from |ZoomBubbleView::ShowBubble| & |ManagePasswordsBubbleView::ShowBubble|). https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:67: // views::WidgetDelegate: On 2014/12/23 00:49:03, Peter Kasting wrote: > Same comments regarding override groups. Done. https://codereview.chromium.org/795053003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:75: using ManagedFullScreenBubbleDelegateView::AdjustForFullscreen; On 2014/12/23 00:49:03, Peter Kasting wrote: > Again, don't do this. Done.
LGTM
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/795053003/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bef9a565814ecf1c571640fea39a8e49754016ec Cr-Commit-Position: refs/heads/master@{#309613}
Message was sent while issue was closed.
Patchset #6 (id:160001) has been deleted |
