Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
550313002:
Pop extensions out of the action overflow menu (Closed)
DescriptionPop extensions out of the action overflow menu
Extension actions that are dragged out of the overflow menu should not force
another inside; rather they should "pop out" and cause the container to grow.
Analogously, actions dragged in should reduce the size of the container.
BUG=411394
Committed: https://crrev.com/4d6a24d9dfaf85826a0158a35d84b76da8ec31b3
Cr-Commit-Position: refs/heads/master@{#294623}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Finnur's #Patch Set 3 : #
Total comments: 2
Patch Set 4 : Fixed incognito mode #
Total comments: 6
Patch Set 5 : name change + latest master for CQ #
Messages
Total messages: 23 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + finnur@chromium.org
Finnur, please take a look when you can. :)
https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:544: if (i > data.index()) { I understand why dragging an icon to the left subtracts 1 from the index |i|, but it isn't immediately apparent why dragging left/right would modify the |size_modifier|. I think you are referring to container -> overflow as right and overflow -> container as left. It would probably be clearer if you stated that instead of left and right. It would also make the comment correct for RTL. But lets take a step back. This size_modifier code is a bit hard to grok. And it seems to me like |size_modifier| is redundant. We only modify it when dragging between containers and it seems to me like it can in all cases be set to in_overflow_mode() ? -1 : 1 Right? If so, I think we can delete a block of code (before MoveExtensionIcon) if we simply do: bool drag_between_containers = !browser_action_views_[data.index()]->visible()); model_->MoveExtensionIcon( browser_action_views_[data.index()]->extension(), i); if (drag_between_containers) { // Do what you do inside if(size_modifier) but use: // |in_overflow_mode() ? -1 : 1| instead of size_modifier when // setting the visible count. } https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc (right): https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:206: nit: Extra line. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:240: // BrowserActionOverflowMenuController. Umm... the comment for the newly added test at the bottom says you are doing just that (dragging from overflow to main). https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:647: // Order should now b B [A C]. nit: Missing e in be. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:669: VerifyVisibleCount(2u); Want to drag C to the main bar as well (verify no icon left in overflow)?
https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:544: if (i > data.index()) { On 2014/09/10 09:48:01, Finnur wrote: > I understand why dragging an icon to the left subtracts 1 from the index |i|, > but it isn't immediately apparent why dragging left/right would modify the > |size_modifier|. I think you are referring to container -> overflow as right and > overflow -> container as left. It would probably be clearer if you stated that > instead of left and right. It would also make the comment correct for RTL. > > But lets take a step back. This size_modifier code is a bit hard to grok. And it > seems to me like |size_modifier| is redundant. We only modify it when dragging > between containers and it seems to me like it can in all cases be set to > in_overflow_mode() ? -1 : 1 > > Right? If so, I think we can delete a block of code (before MoveExtensionIcon) > if we simply do: > > bool drag_between_containers = !browser_action_views_[data.index()]->visible()); > model_->MoveExtensionIcon( > browser_action_views_[data.index()]->extension(), i); > > if (drag_between_containers) { > // Do what you do inside if(size_modifier) but use: > // |in_overflow_mode() ? -1 : 1| instead of size_modifier when > // setting the visible count. > } Wow. Yeah, that's easier. Shoulda seen that... thanks. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc (right): https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:206: On 2014/09/10 09:48:01, Finnur wrote: > nit: Extra line. Whoops, removed. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:240: // BrowserActionOverflowMenuController. On 2014/09/10 09:48:01, Finnur wrote: > Umm... the comment for the newly added test at the bottom says you are doing > just that (dragging from overflow to main). Should have been more clear - the legacy overflow container (i.e., the chevron). The test at the bottom tests with the new overflow behavior. Updated the comment. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:647: // Order should now b B [A C]. On 2014/09/10 09:48:01, Finnur wrote: > nit: Missing e in be. u r right. :P Fixed. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:669: VerifyVisibleCount(2u); On 2014/09/10 09:48:01, Finnur wrote: > Want to drag C to the main bar as well (verify no icon left in overflow)? Sure, can't hurt.
rdevlin.cronin@chromium.org changed reviewers: + sky@chromium.org
+sky@, mind taking a look when you can?
I'll take a look once Finnur is happy.
Almost there. If the answer is "it works", then LGTM. https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc (right): https://codereview.chromium.org/550313002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:240: // BrowserActionOverflowMenuController. Well, the chevron is going away anyway. https://codereview.chromium.org/550313002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:558: else if (!profile_->IsOffTheRecord()) // This is the main container. What happens if you drop an icon onto the main container in incognito? Does the size increase by one?
s/answer/answer to the question below/
LGTM
https://codereview.chromium.org/550313002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:558: else if (!profile_->IsOffTheRecord()) // This is the main container. On 2014/09/11 09:23:48, Finnur wrote: > What happens if you drop an icon onto the main container in incognito? Does the > size increase by one? tl;dr: All fixed. :) As it turns out, this particular code was fine - but there was a bug in the GetIconCount() code for incognito in general, because it did all its calculations based off the model. Additionally, the behavior was a little unpredictable to begin with. Per offline chat, I tweaked the behavior of incognito window bars to make any actions which were initially hidden, remain hidden. Now the behavior is correct. - A new incognito window will have only visible actions for those extensions which are visible in the main window (and have incognito privileges). - Changing incognito visible action count doesn't modify regular visible action count. - Popping in/out in incognito works as expected.
https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1098: int model_size = model_->GetVisibleIconCount(); Maybe it is just too early in the morning for me, but I find this variable name (and the one below) a little confusing. :) This isn't really model_size, it is how many are visible, right? Would visible_size and absolute_visible_size work for you? I think it would make the code on line 1111 easier to understand. https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1118: if (initialized_ && profile_->IsOffTheRecord()) { Out of curiosity, under what circumstances is |initialized_| false when we get here? Also, when it is false, doesn't this function return some incorrect value? Should we early return 0u if it is false, like we do with !model_?
https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1098: int model_size = model_->GetVisibleIconCount(); On 2014/09/12 10:30:24, Finnur wrote: > Maybe it is just too early in the morning for me, but I find this variable name > (and the one below) a little confusing. :) > > This isn't really model_size, it is how many are visible, right? > > Would visible_size and absolute_visible_size work for you? I think it would make > the code on line 1111 easier to understand. Well, it's actually how many are visible, according to the model (which, due to incognito mode, could be different from how many are *actually* visible). So it's both. :) I'll change the name when I get into the office. https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1118: if (initialized_ && profile_->IsOffTheRecord()) { On 2014/09/12 10:30:24, Finnur wrote: > Out of curiosity, under what circumstances is |initialized_| false when we get > here? Also, when it is false, doesn't this function return some incorrect value? > Should we early return 0u if it is false, like we do with !model_? Initialized is false when we call this method from GetPreferredWidth() called from Init() - and it's important to do that. It actually returns the proper value, because this is how we initialize the container. So the behavior for incognito mode windows is "When you open a new incognito window, display only browser actions that were displayed in a regular window. After the window is open and set up (init'd), trust what's there because it might have adjusted without the model knowing."
https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1098: int model_size = model_->GetVisibleIconCount(); LGTM, with name change.
The CQ bit was checked by rdevlin.cronin@chromium.org
https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/550313002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1098: int model_size = model_->GetVisibleIconCount(); On 2014/09/12 15:29:52, Finnur wrote: > LGTM, with name change. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/550313002/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as 51419e758e045dad960ccd61b198b31e80156a4a
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4d6a24d9dfaf85826a0158a35d84b76da8ec31b3 Cr-Commit-Position: refs/heads/master@{#294623} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
