Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
419023002:
Move ShowPopup logic from BrowserActionsContainer to BrowserActionView (Closed)
DescriptionMove ShowPopup logic from BrowserActionsContainer to BrowserActionView
Since we're going to be showing both page actions and browser actions in the
same view, we need the BrowserActionsContainer to be able to support both. This
is very unwieldy if we also have the BAC know how to execute both. Instead, move
the logic to execute a browser action (and show the popup) into the
BrowserActionView.
Also consolidate the four (!) different "ShowPopup" methods in BrowserActionsContainer into two.
BUG=397259
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286704
Patch Set 1 #
Total comments: 30
Patch Set 2 : Peter's #Patch Set 3 : #
Total comments: 10
Patch Set 4 : nits + remove InspectPopup arg #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 26 (0 generated)
Peter, mind taking a look? Predominantly a refactor.
I feel like I'm not really able to review this functionally, since I haven't ever really worked with the browser action container/button/etc. code before. I can do a style review, but if you really need someone to check the functionality, you may need to find a more knowledgeable reviewer. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:63: ->button()->ExecuteBrowserAction(); Nit: Personally, "operators go on the ends of lines" suggests to me that we should wrap after ->, and not before (or wrap before one of the function arguments, or something) https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:285: // If we're showing the same popup, just hide it and return. Nit: The word "same" in this sentence is slightly confusing to me. What does it mean to have a "same" versus a "different" popup? Here "same" just seems to mean "any popup". Is that because this is a specific browser action's button? Would this sentence read better as "already showing the popup for this browser action" or something similar? https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { This code sure looks a lot like HidePopup(). Can we make the two into one function and/or implement one in terms of the other? https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:163: // through a user action (and not, e.g., an API). Tiny nit: Remove commas (slightly more readable) https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:170: // Executes the browser action (also showing the popup, if one exists). Nit: also showing -> and also shows? https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:241: virtual void InspectPopup(ExtensionAction* action) OVERRIDE; It seems like we should make this public, so that BrowserActionTestUtil::InspectPopup() can call it without having to do an ugly cast. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:285: // The browser action's popup, if it is visible, NULL otherwise. Tiny nit: Change second comma to a semicolon or period https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:662: ->enabled_extensions().GetByID(extension_id); Nit: See prior nit about -> wrapping https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:993: // If the popup cannot override other views, then no other popups can be Nit: Hoist this comment above the above conditional, then combine the nested conditionals. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1008: grant_tab_permissions); Nit: Same length, but feels a little nicer to me: BrowserActionButton* button = (*iter)->button(); if (button->extension() == extension) return button->ShowPopup(ExtensionPopup::SHOW, grant_tab_permissions); https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/toolbar_view.cc:344: false); // Don't override the current view. Nit: I'm not a huge fan of comments on function args. If the meaning would be clear enough (possibly with a reference to the function declaration) to the typical reader, we don't need them. If not, we may want to consider named enum values in place of bools. My normal answer here is to just remove the comments.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc:63: ->button()->ExecuteBrowserAction(); On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: Personally, "operators go on the ends of lines" suggests to me that we > should wrap after ->, and not before (or wrap before one of the function > arguments, or something) Yeah, git cl format formed a bad habit before we decided we didn't trust it. Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:285: // If we're showing the same popup, just hide it and return. On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: The word "same" in this sentence is slightly confusing to me. What does it > mean to have a "same" versus a "different" popup? Here "same" just seems to > mean "any popup". Is that because this is a specific browser action's button? > Would this sentence read better as "already showing the popup for this browser > action" or something similar? Yeah - same means "the popup for this browser action", which is a much clearer phrasing. Changed. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/26 02:33:20, Peter Kasting wrote: > This code sure looks a lot like HidePopup(). Can we make the two into one > function and/or implement one in terms of the other? I'm not 100% sure. The problem being the call to widget->Close(). If it's safe to try and close a widget even if it's destroying, we can combine easily (tentatively done this, but not sure it's always safe to do so). Otherwise, we could call OnWidgetDestroying from HidePopup(), but that seems odd if we destroy the widget as a result of the Close() call (and I think it looks very odd to self-call OnWidgetDestroying anywhere). The other option is to have a Cleanup(bool close_widget) method, but that seems a bit excessive. Thoughts/preferences? https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:163: // through a user action (and not, e.g., an API). On 2014/07/26 02:33:20, Peter Kasting wrote: > Tiny nit: Remove commas (slightly more readable) Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:170: // Executes the browser action (also showing the popup, if one exists). On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: also showing -> and also shows? Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:241: virtual void InspectPopup(ExtensionAction* action) OVERRIDE; On 2014/07/26 02:33:20, Peter Kasting wrote: > It seems like we should make this public, so that > BrowserActionTestUtil::InspectPopup() can call it without having to do an ugly > cast. Yeah - I was torn. I like hiding these when possible (because usually it means you're doing something wrong if you need to call it directly) and I didn't really wanna change that just for a testing method. But... it is a *really* ugly cast. ;) Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.h:285: // The browser action's popup, if it is visible, NULL otherwise. On 2014/07/26 02:33:20, Peter Kasting wrote: > Tiny nit: Change second comma to a semicolon or period Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:662: ->enabled_extensions().GetByID(extension_id); On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: See prior nit about -> wrapping Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:993: // If the popup cannot override other views, then no other popups can be On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: Hoist this comment above the above conditional, then combine the nested > conditionals. Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1008: grant_tab_permissions); On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: Same length, but feels a little nicer to me: > > BrowserActionButton* button = (*iter)->button(); > if (button->extension() == extension) > return button->ShowPopup(ExtensionPopup::SHOW, grant_tab_permissions); Done. https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/toolbar_view.cc:344: false); // Don't override the current view. On 2014/07/26 02:33:20, Peter Kasting wrote: > Nit: I'm not a huge fan of comments on function args. If the meaning would be > clear enough (possibly with a reference to the function declaration) to the > typical reader, we don't need them. If not, we may want to consider named enum > values in place of bools. > > My normal answer here is to just remove the comments. I typically view them as providing a reference to the function declaration, to save any reader from having to look it up (because, as you said, they should be clear with that). Tentatively removed the comments, but out of curiosity, would you feel any better about a shorter "grant_tab_permissions" and "can_override" (simply documenting which variables each bool is associated with)?
Yoyo, since Finnur's out and you've done some UI work, mind taking a look at this from an extensions perspective?
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 19:07:14, Devlin wrote: > On 2014/07/26 02:33:20, Peter Kasting wrote: > > This code sure looks a lot like HidePopup(). Can we make the two into one > > function and/or implement one in terms of the other? > > I'm not 100% sure. The problem being the call to widget->Close(). If it's safe > to try and close a widget even if it's destroying, we can combine easily > (tentatively done this, but not sure it's always safe to do so). Widget::Close()'s definition checks whether the widget is closing and returns, but at the same time the comments suggest that's not expected to be done intentionally. What if HidePopup() _only_ calls Close()? Won't that eventually trigger OnWidgetDestroying() to be reached, so we run the rest of this code? Or are there problems with that flow? https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/toolbar_view.cc:344: false); // Don't override the current view. On 2014/07/29 19:07:15, Devlin wrote: > On 2014/07/26 02:33:20, Peter Kasting wrote: > > Nit: I'm not a huge fan of comments on function args. If the meaning would be > > clear enough (possibly with a reference to the function declaration) to the > > typical reader, we don't need them. If not, we may want to consider named > enum > > values in place of bools. > > > > My normal answer here is to just remove the comments. > > I typically view them as providing a reference to the function declaration, to > save any reader from having to look it up (because, as you said, they should be > clear with that). Tentatively removed the comments, but out of curiosity, would > you feel any better about a shorter "grant_tab_permissions" and "can_override" > (simply documenting which variables each bool is associated with)? I think those are probably slightly worse than the comments you originally had. If we really need to say something because the reader can't look up the function definition or whatever, it's better to just spell out what we're talking about than to leave comments that may require the reader to look up the prototype anyway just to figure them out.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 19:18:01, Peter Kasting wrote: > On 2014/07/29 19:07:14, Devlin wrote: > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > This code sure looks a lot like HidePopup(). Can we make the two into one > > > function and/or implement one in terms of the other? > > > > I'm not 100% sure. The problem being the call to widget->Close(). If it's > safe > > to try and close a widget even if it's destroying, we can combine easily > > (tentatively done this, but not sure it's always safe to do so). > > Widget::Close()'s definition checks whether the widget is closing and returns, > but at the same time the comments suggest that's not expected to be done > intentionally. > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > OnWidgetDestroying() to be reached, so we run the rest of this code? Or are > there problems with that flow? That may still be problematic. In particular, widget_observer.h says for OnWidgetDestroying "This typically occurs asynchronously with respect the the close request" (side note: we should fix that typo). Since it occurs async, we might think there's a popup when, for all intents and purposes, there isn't.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 19:51:46, Devlin wrote: > On 2014/07/29 19:18:01, Peter Kasting wrote: > > On 2014/07/29 19:07:14, Devlin wrote: > > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > > This code sure looks a lot like HidePopup(). Can we make the two into one > > > > function and/or implement one in terms of the other? > > > > > > I'm not 100% sure. The problem being the call to widget->Close(). If it's > > safe > > > to try and close a widget even if it's destroying, we can combine easily > > > (tentatively done this, but not sure it's always safe to do so). > > > > Widget::Close()'s definition checks whether the widget is closing and returns, > > but at the same time the comments suggest that's not expected to be done > > intentionally. > > > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > > OnWidgetDestroying() to be reached, so we run the rest of this code? Or are > > there problems with that flow? > > That may still be problematic. In particular, widget_observer.h says for > OnWidgetDestroying "This typically occurs asynchronously with respect the the > close request" (side note: we should fix that typo). Since it occurs async, we > might think there's a popup when, for all intents and purposes, there isn't. There isn't from the user's perspective, but there is from the code's perspective. What are the concrete potential negative consequences? I'd imagine that, as long as we're not going to deref NULL somewhere or something, we would probably be OK.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 19:56:05, Peter Kasting wrote: > On 2014/07/29 19:51:46, Devlin wrote: > > On 2014/07/29 19:18:01, Peter Kasting wrote: > > > On 2014/07/29 19:07:14, Devlin wrote: > > > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > > > This code sure looks a lot like HidePopup(). Can we make the two into > one > > > > > function and/or implement one in terms of the other? > > > > > > > > I'm not 100% sure. The problem being the call to widget->Close(). If > it's > > > safe > > > > to try and close a widget even if it's destroying, we can combine easily > > > > (tentatively done this, but not sure it's always safe to do so). > > > > > > Widget::Close()'s definition checks whether the widget is closing and > returns, > > > but at the same time the comments suggest that's not expected to be done > > > intentionally. > > > > > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > > > OnWidgetDestroying() to be reached, so we run the rest of this code? Or are > > > there problems with that flow? > > > > That may still be problematic. In particular, widget_observer.h says for > > OnWidgetDestroying "This typically occurs asynchronously with respect the the > > close request" (side note: we should fix that typo). Since it occurs async, > we > > might think there's a popup when, for all intents and purposes, there isn't. > > There isn't from the user's perspective, but there is from the code's > perspective. What are the concrete potential negative consequences? I'd > imagine that, as long as we're not going to deref NULL somewhere or something, > we would probably be OK. In theory, if we have a popup showing, and then we double click the associated browser action, the popup should close and re-open. If we wait for the async close, then we would only close (and then try to close again). Certainly not a crash, but still a negative consequence.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 19:59:48, Devlin wrote: > On 2014/07/29 19:56:05, Peter Kasting wrote: > > On 2014/07/29 19:51:46, Devlin wrote: > > > On 2014/07/29 19:18:01, Peter Kasting wrote: > > > > On 2014/07/29 19:07:14, Devlin wrote: > > > > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > > > > This code sure looks a lot like HidePopup(). Can we make the two into > > one > > > > > > function and/or implement one in terms of the other? > > > > > > > > > > I'm not 100% sure. The problem being the call to widget->Close(). If > > it's > > > > safe > > > > > to try and close a widget even if it's destroying, we can combine easily > > > > > (tentatively done this, but not sure it's always safe to do so). > > > > > > > > Widget::Close()'s definition checks whether the widget is closing and > > returns, > > > > but at the same time the comments suggest that's not expected to be done > > > > intentionally. > > > > > > > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > > > > OnWidgetDestroying() to be reached, so we run the rest of this code? Or > are > > > > there problems with that flow? > > > > > > That may still be problematic. In particular, widget_observer.h says for > > > OnWidgetDestroying "This typically occurs asynchronously with respect the > the > > > close request" (side note: we should fix that typo). Since it occurs async, > > we > > > might think there's a popup when, for all intents and purposes, there isn't. > > > > There isn't from the user's perspective, but there is from the code's > > perspective. What are the concrete potential negative consequences? I'd > > imagine that, as long as we're not going to deref NULL somewhere or something, > > we would probably be OK. > > In theory, if we have a popup showing, and then we double click the associated > browser action, the popup should close and re-open. If we wait for the async > close, then we would only close (and then try to close again). Certainly not a > crash, but still a negative consequence. The async close should happen with the next message loop iteration, no? I'd think it would finish before the user could manage to click again, in most cases. I think you should bring sky into these discussions; he understands widget closure better than I do.
On 2014/07/29 20:03:02, Peter Kasting wrote: > https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... > File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): > > https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... > chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void > BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { > On 2014/07/29 19:59:48, Devlin wrote: > > On 2014/07/29 19:56:05, Peter Kasting wrote: > > > On 2014/07/29 19:51:46, Devlin wrote: > > > > On 2014/07/29 19:18:01, Peter Kasting wrote: > > > > > On 2014/07/29 19:07:14, Devlin wrote: > > > > > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > > > > > This code sure looks a lot like HidePopup(). Can we make the two > into > > > one > > > > > > > function and/or implement one in terms of the other? > > > > > > > > > > > > I'm not 100% sure. The problem being the call to widget->Close(). If > > > it's > > > > > safe > > > > > > to try and close a widget even if it's destroying, we can combine > easily > > > > > > (tentatively done this, but not sure it's always safe to do so). > > > > > > > > > > Widget::Close()'s definition checks whether the widget is closing and > > > returns, > > > > > but at the same time the comments suggest that's not expected to be done > > > > > intentionally. > > > > > > > > > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > > > > > OnWidgetDestroying() to be reached, so we run the rest of this code? Or > > are > > > > > there problems with that flow? > > > > > > > > That may still be problematic. In particular, widget_observer.h says for > > > > OnWidgetDestroying "This typically occurs asynchronously with respect the > > the > > > > close request" (side note: we should fix that typo). Since it occurs > async, > > > we > > > > might think there's a popup when, for all intents and purposes, there > isn't. > > > > > > There isn't from the user's perspective, but there is from the code's > > > perspective. What are the concrete potential negative consequences? I'd > > > imagine that, as long as we're not going to deref NULL somewhere or > something, > > > we would probably be OK. > > > > In theory, if we have a popup showing, and then we double click the associated > > browser action, the popup should close and re-open. If we wait for the async > > close, then we would only close (and then try to close again). Certainly not > a > > crash, but still a negative consequence. > > The async close should happen with the next message loop iteration, no? I'd > think it would finish before the user could manage to click again, in most > cases. > > I think you should bring sky into these discussions; he understands widget > closure better than I do. +sky. Scott, mind clearing this up for us real quick? :) (What started as trying to save a few lines of code led to some interesting questions.)
On 2014/07/29 20:03:02, Peter Kasting wrote: > https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... > File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): > > https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... > chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void > BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { > On 2014/07/29 19:59:48, Devlin wrote: > > On 2014/07/29 19:56:05, Peter Kasting wrote: > > > On 2014/07/29 19:51:46, Devlin wrote: > > > > On 2014/07/29 19:18:01, Peter Kasting wrote: > > > > > On 2014/07/29 19:07:14, Devlin wrote: > > > > > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > > > > > This code sure looks a lot like HidePopup(). Can we make the two > into > > > one > > > > > > > function and/or implement one in terms of the other? > > > > > > > > > > > > I'm not 100% sure. The problem being the call to widget->Close(). If > > > it's > > > > > safe > > > > > > to try and close a widget even if it's destroying, we can combine > easily > > > > > > (tentatively done this, but not sure it's always safe to do so). > > > > > > > > > > Widget::Close()'s definition checks whether the widget is closing and > > > returns, > > > > > but at the same time the comments suggest that's not expected to be done > > > > > intentionally. It's safe to invoke Close() multiple times. It is not safe to invoke CloseNow() multiple times (CloseNow ends up deleting the widget). > > > > > > > > > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > > > > > OnWidgetDestroying() to be reached, so we run the rest of this code? Or > > are > > > > > there problems with that flow? > > > > > > > > That may still be problematic. In particular, widget_observer.h says for > > > > OnWidgetDestroying "This typically occurs asynchronously with respect the > > the > > > > close request" (side note: we should fix that typo). Since it occurs > async, > > > we > > > > might think there's a popup when, for all intents and purposes, there > isn't. > > > > > > There isn't from the user's perspective, but there is from the code's > > > perspective. What are the concrete potential negative consequences? I'd > > > imagine that, as long as we're not going to deref NULL somewhere or > something, > > > we would probably be OK. > > > > In theory, if we have a popup showing, and then we double click the associated > > browser action, the popup should close and re-open. If we wait for the async > > close, then we would only close (and then try to close again). Certainly not > a > > crash, but still a negative consequence. > > The async close should happen with the next message loop iteration, no? Assuming we're talking about widget->Close(). It's effectively widget->Hide() with a delayed task that does a widget->CloseNow(). Close() and CloseNow() are one way operations, they can't be aborted. Did that answer your question? -Scott > I'd > think it would finish before the user could manage to click again, in most > cases. > > I think you should bring sky into these discussions; he understands widget > closure better than I do.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 20:03:02, Peter Kasting wrote: > On 2014/07/29 19:59:48, Devlin wrote: > > On 2014/07/29 19:56:05, Peter Kasting wrote: > > > On 2014/07/29 19:51:46, Devlin wrote: > > > > On 2014/07/29 19:18:01, Peter Kasting wrote: > > > > > On 2014/07/29 19:07:14, Devlin wrote: > > > > > > On 2014/07/26 02:33:20, Peter Kasting wrote: > > > > > > > This code sure looks a lot like HidePopup(). Can we make the two > into > > > one > > > > > > > function and/or implement one in terms of the other? > > > > > > > > > > > > I'm not 100% sure. The problem being the call to widget->Close(). If > > > it's > > > > > safe > > > > > > to try and close a widget even if it's destroying, we can combine > easily > > > > > > (tentatively done this, but not sure it's always safe to do so). > > > > > > > > > > Widget::Close()'s definition checks whether the widget is closing and > > > returns, > > > > > but at the same time the comments suggest that's not expected to be done > > > > > intentionally. > > > > > > > > > > What if HidePopup() _only_ calls Close()? Won't that eventually trigger > > > > > OnWidgetDestroying() to be reached, so we run the rest of this code? Or > > are > > > > > there problems with that flow? > > > > > > > > That may still be problematic. In particular, widget_observer.h says for > > > > OnWidgetDestroying "This typically occurs asynchronously with respect the > > the > > > > close request" (side note: we should fix that typo). Since it occurs > async, > > > we > > > > might think there's a popup when, for all intents and purposes, there > isn't. > > > > > > There isn't from the user's perspective, but there is from the code's > > > perspective. What are the concrete potential negative consequences? I'd > > > imagine that, as long as we're not going to deref NULL somewhere or > something, > > > we would probably be OK. > > > > In theory, if we have a popup showing, and then we double click the associated > > browser action, the popup should close and re-open. If we wait for the async > > close, then we would only close (and then try to close again). Certainly not > a > > crash, but still a negative consequence. > > The async close should happen with the next message loop iteration, no? I'd > think it would finish before the user could manage to click again, in most > cases. > > I think you should bring sky into these discussions; he understands widget > closure better than I do. - PatchSet 2 (just calling HidePopup() from OnWidgetDestroyed) won't work, because Scott confirms that it's a bad idea to call Close() from OnWidgetDestroyed() (asked/answered offline - only definitely safe if it was triggered from a Widget::Close()). - Having HidePopup() only call widget->Close() also won't work well, because this messes up the BrowserActionsContainer's idea of popup owner, since we can't have delegate_->SetPopupOwner(NULL) happen after ShowPopup() finishes, which will be the case in an async pattern. (Didn't notice this one before). Having a few lines of duplicate code (as in PatchSet 1) will work, as would having a third function which takes an argument "bool close_widget". Thoughts?
On 2014/07/29 21:25:43, Devlin wrote: > Having a few lines of duplicate code (as in PatchSet 1) will work, as would > having a third function which takes an argument "bool close_widget". I weakly prefer the third function idea, with comments along the lines of what you just said to explain why a simpler solution is unwise.
https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_action_view.cc:481: void BrowserActionButton::OnWidgetDestroying(views::Widget* widget) { On 2014/07/29 21:26:59, Peter Kasting wrote: > I weakly prefer the third function idea, with comments along the lines of what > you just said to explain why a simpler solution is unwise. Done!
LGTM https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_action_view.cc:285: // If we're showing the popup for this browser action, just hide it and Nit: showing -> already showing
LGTM https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... chrome/browser/ui/views/toolbar/browser_action_view.cc:296: // a reference view other than this view. If so, use the overflow view. "other than this button"? https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... chrome/browser/ui/views/toolbar/browser_action_view.cc:474: ShowPopup(ExtensionPopup::SHOW_AND_INSPECT, true); // grant active tab May as well be consistent about commenting on boolean parameters. https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... chrome/browser/ui/views/toolbar/browser_action_view.h:212: virtual void InspectPopup(ExtensionAction* action) OVERRIDE; The action parameter seems slightly redundant here, but it's fine if you can't get rid of it because of its other uses. https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://chromiumcodereview.appspot.com/419023002/diff/40001/chrome/browser/ui... chrome/browser/ui/views/toolbar/browser_actions_container.h:250: // was shown. Showing the popup will grant tab permissions if Should this say active tab permissions?
https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_action_view.cc (right): https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_action_view.cc:285: // If we're showing the popup for this browser action, just hide it and On 2014/07/29 21:49:48, Peter Kasting wrote: > Nit: showing -> already showing Done. https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_action_view.cc:296: // a reference view other than this view. If so, use the overflow view. On 2014/07/30 01:46:33, Yoyo Zhou wrote: > "other than this button"? Done. https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_action_view.cc:474: ShowPopup(ExtensionPopup::SHOW_AND_INSPECT, true); // grant active tab On 2014/07/30 01:46:33, Yoyo Zhou wrote: > May as well be consistent about commenting on boolean parameters. Done. https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_action_view.h:212: virtual void InspectPopup(ExtensionAction* action) OVERRIDE; On 2014/07/30 01:46:33, Yoyo Zhou wrote: > The action parameter seems slightly redundant here, but it's fine if you can't > get rid of it because of its other uses. Didn't remove it initially because I thought it would mean tweaking a lot of other implementors. Turns out not. Removed. :) https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/419023002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:250: // was shown. Showing the popup will grant tab permissions if On 2014/07/30 01:46:33, Yoyo Zhou wrote: > Should this say active tab permissions? For clarity, probably. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/419023002/...
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/419023002/...
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/419023002/...
Message was sent while issue was closed.
Change committed as 286704 |
