Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
649653003:
Close the password bubble when the fullscreen state has changed. (Closed)
DescriptionClose the password bubble when the fullscreen state has changed.
BUG=401707
Committed: https://crrev.com/8aecd2dd1071108f3ae1060b366d39b220935b1b
Cr-Commit-Position: refs/heads/master@{#300707}
Patch Set 1 #Patch Set 2 : no animate #
Total comments: 8
Patch Set 3 : comments #
Messages
Total messages: 23 (8 generated)
vasilii@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, please review.
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
Looks good, but a few questions before you land it. In particular, it would be nice if we could do this higher up such that all bubbles could hook into the functionality without all individually registering themselves for notifications. https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:19: #include "content/public/browser/notification_source.h" You never use NotificationSource. I think you can get away with the forward declaration. https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:877: wm::SetWindowVisibilityAnimationTransition(window, wm::ANIMATE_NONE); Is this not the default? https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:29: public content::NotificationObserver { It's worth asking views OWNERS whether or not registering for these kinds of events should be higher up in the stack (e.g. in BubbleDelegateView). All bubbles need to deal with this, right?
+1 to Mike's comments and questions and statement that it looks otherwise good. :) https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:99: // content::NotificationObserver method. nit: There should be a single format of this type of comment, at least within a single file. Please choose either the one used for views::WidgetDelegate or views::BubbleDelegateView above and make all three follow the same pattern.
vasilii@chromium.org changed reviewers: + msw@chromium.org
msw@, do you agree to move this code up in the hierarchy to BubbleDelegateView?
https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:29: public content::NotificationObserver { On 2014/10/14 11:09:47, Mike West wrote: > It's worth asking views OWNERS whether or not registering for these kinds of > events should be higher up in the stack (e.g. in BubbleDelegateView). All > bubbles need to deal with this, right? This seems like a good signal for the browser-scoped 'popup' manager, where work has stalled due to sufficient dev resources, etc. <http://crbug.com/375393>. Otherwise, not all bubbles are related to web content (eg. extension popups, global errors, avatar/profile bubble), so observing a particular web contents doesn't necessarily make sense as a base class feature.
Mike (mkwst@), Vaclav, as Mike (msw@) pointed out some of the bubbles aren't tied to the web_content. Indeed the bubble manager should take care of this for all the bubbles. However, its status is unclear for now. https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:19: #include "content/public/browser/notification_source.h" On 2014/10/14 11:09:47, Mike West wrote: > You never use NotificationSource. I think you can get away with the forward > declaration. Line 788? https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:877: wm::SetWindowVisibilityAnimationTransition(window, wm::ANIMATE_NONE); On 2014/10/14 11:09:47, Mike West wrote: > Is this not the default? By default the bubble animates both showing and hiding. It takes 200ms. https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/649653003/diff/110001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:99: // content::NotificationObserver method. On 2014/10/14 11:26:30, vabr (Chromium) wrote: > nit: There should be a single format of this type of comment, at least within a > single file. Please choose either the one used for views::WidgetDelegate or > views::BubbleDelegateView above and make all three follow the same pattern. Done.
On 2014/10/22 12:14:35, vasilii wrote: > Mike (mkwst@), Vaclav, as Mike (msw@) pointed out some of the bubbles aren't > tied to the web_content. Indeed the bubble manager should take care of this for > all the bubbles. However, its status is unclear for now. Alright. LGTM. That said, it may still make sense to subclass BubbleDelegateView with a WebContents-specific variant in the short term so that Translate, Mixed Content, and the other omnibox bubbles won't have to reimplement this logic. That can certainly be followup refactoring.
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649653003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649653003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vasilii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649653003/130001
LGTM. Vasilii, If you find time to file the refactoring bug mentioned by Mike, that would help both keeping the track of that TODO, as well as having a "Good second bug" for our Nooglers. Cheers, Vaclav
Message was sent while issue was closed.
Committed patchset #3 (id:130001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8aecd2dd1071108f3ae1060b366d39b220935b1b Cr-Commit-Position: refs/heads/master@{#300707} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
