Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
675023002:
Make extensions that desire to act pop out if in overflow (Closed)
DescriptionMake extensions that desire to act pop out if in overflow
If an extension desires to act, it should pop itself out of
the overflow menu. There should also be a visual queue for
extensions that are already visible, but what exactly that
should be is still being discussed.
BUG=417441
Committed: https://crrev.com/d604171517135387ca3b4c33d7f1774c8d2d38b0
Cr-Commit-Position: refs/heads/master@{#302511}
Patch Set 1 : #Patch Set 2 : Test fixes #
Total comments: 4
Patch Set 3 : #
Total comments: 34
Patch Set 4 : Peter's #
Total comments: 6
Patch Set 5 : Nits and Rebase #Patch Set 6 : Rebase #Messages
Total messages: 23 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + finnur@chromium.org
Finnur - one more for you. :)
Looks like you have some related test failures. Do they require simple augmentations to the code and I should dive in or should I hold off?
On 2014/10/24 09:53:59, Finnur wrote: > Looks like you have some related test failures. Do they require simple > augmentations to the code and I should dive in or should I hold off? All relatively minor fixes to make them pass locally. I think you should be good to review.
Couple of nits. And one test is still failing. But apart from that LGTM https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_toolbar_model.cc (left): https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_toolbar_model.cc:156: if (extension && Is the extension check unnecessary now? Not saying it isn't -- just wondering. https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_toolbar_model.cc:585: size_t overflowed_actions_to_run = 0u; nit: to run? Seems a bit weird to use that phrase here. overflowed_actions_wanting_to_run perhaps?
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_toolbar_model.cc (left): https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_toolbar_model.cc:156: if (extension && On 2014/10/25 14:09:58, Finnur wrote: > Is the extension check unnecessary now? Not saying it isn't -- just wondering. I think it actually always was - if extension is nullptr, it shouldn't be in the array of toolbar_items_ to begin with (or else we'd have much bigger problems) :) https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://codereview.chromium.org/675023002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_toolbar_model.cc:585: size_t overflowed_actions_to_run = 0u; On 2014/10/25 14:09:58, Finnur wrote: > nit: to run? Seems a bit weird to use that phrase here. > overflowed_actions_wanting_to_run perhaps? Done.
rdevlin.cronin@chromium.org changed reviewers: + pkasting@chromium.org
This one too, please, Peter. :)
On 2014/10/29 20:29:53, Devlin wrote: > This one too, please, Peter. :) Peter - friendly ping, just in case it slipped through your inbox.
https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.cc:579: int base_visible = visible_icon_count_; Why do we need this temp? Does visible_icon_count_ change during the course of the function? If not, just use it directly. Or, keep it but eliminate |overflowed_actions_wanting_to_run| below and just increment this value directly. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.cc:614: for (int i = toolbar_items_.size() - 1; i >= base_visible; --i) { This is a very complicated way of doing what I think would be easier with std::rotate(). Just rotate the extensions that wish to run up to the divider point between the visible and overflowed extensions. No need for a spare ExtensionList or two loops. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:81: // Signal that the toolbar needs to be reordered for the given Nit: "Signals" (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... , "comments should be descriptive, not imperative") Same comment applies to other existing functions in this file. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:84: virtual void ToolbarReorderNecessary( Nit: This is a strange name for a function. It sounds as if it should return a bool, but instead it returns void. Maybe "ReorderToolbarIfNecessary", or "OnToolbarReorderNecessary"? (Some of the other functions here have problematic names too... I might add "On" to a lot of them that are event handlers.) https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:112: // Returns the number of visible icons as an absolute value. Nit: I think this name/comment are misleading. They suggest you're going to do a mathematical abs(). I would use some other word than "Absolute" in the function name. Maybe "Actual", or "True"? https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model_unittest.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model_unittest.cc:146: reorder_count_(0u) { Nit: None of these lines should use 0u, they should just be 0. (We only need to use 0u explicitly in certain EXPECT/ASSERT statements due to how the macro expansion works.) https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_action_view.h:45: virtual content::WebContents* GetCurrentWebContents() const = 0; Const functions should not return non-const pointers. Either make the returned pointer const, or leave this non-const. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:206: suppress_animation_ = true; Nit: Use base::AutoReset to be safe against recursion and reduce the chance of future maintainers accidentally bypassing your "set the variable back" statements: ... base::AutoReset<bool> animation_resetter(&suppress_animation_, true); { base::AutoReset<bool> layout_resetter(&suppress_layout_, true); for (BrowserActionView* view : browser_action_views_) view->UpdateState(); } ReorderViews(); } https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:225: suppress_layout_ = true; Nit: AutoReset here too, perhaps https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:391: if (suppress_layout_) Nit: I think this should happen at the top of the function. Otherwise, calling Layout() with |suppress_layout_| set will have a functional effect when we go from some views to no views, but not when we go from no views to some views. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1051: // index, since everything up to this point is correct). Is it conceivable that the two sets of views are not in-sync? If so, the code below would run off the end of the arrays in question. I assume not, but it certainly made me take a closer look at the loop. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1056: std::swap(browser_action_views_[i], browser_action_views_[j]); I wonder if std::rotate() would make more sense than std::swap(). rotate() could be more performant, or it could make no sense at all, depending on what kinds of model changes you have to account for. In general, if you're only needing to account for extensions popping themselves out of overflow, you could conceivably change the API so that you call once for each single such extension, or maybe call with a vector of the extension IDs that are popping out of overflow, or some such. Then that could be implemented here using rotate(). The "something changed" API you have designed in this CL is more general, but it's also less efficient if you don't need that generality. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1076: #if !defined(NDEBUG) Nit: While here: use DCHECK_IS_ON instead of looking at NDEBUG, so release trybots will run these sanity checks. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:327: // Reorder the views to match the toolbar model for the active tab. Nit: "Reorders" https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:382: // True if we should suppress animation. Nit: Does this need a note about when/why we might want this?
https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.cc:579: int base_visible = visible_icon_count_; On 2014/10/30 22:05:59, Peter Kasting wrote: > Why do we need this temp? Does visible_icon_count_ change during the course of > the function? If not, just use it directly. Or, keep it but eliminate > |overflowed_actions_wanting_to_run| below and just increment this value > directly. Whoops, leftover from old implementation. Fixed. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.cc:614: for (int i = toolbar_items_.size() - 1; i >= base_visible; --i) { On 2014/10/30 22:05:59, Peter Kasting wrote: > This is a very complicated way of doing what I think would be easier with > std::rotate(). > > Just rotate the extensions that wish to run up to the divider point between the > visible and overflowed extensions. No need for a spare ExtensionList or two > loops. Ah, yes, that is quite a bit simpler. Done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:81: // Signal that the toolbar needs to be reordered for the given On 2014/10/30 22:05:59, Peter Kasting wrote: > Nit: "Signals" (see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > , "comments should be descriptive, not imperative") > > Same comment applies to other existing functions in this file. Yeah, this and the name itself were to match existing style in this file. Updating comments. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:84: virtual void ToolbarReorderNecessary( On 2014/10/30 22:05:59, Peter Kasting wrote: > Nit: This is a strange name for a function. It sounds as if it should return a > bool, but instead it returns void. Maybe "ReorderToolbarIfNecessary", or > "OnToolbarReorderNecessary"? (Some of the other functions here have problematic > names too... I might add "On" to a lot of them that are event handlers.) /sigh... I know - it's bothered me most times I add a new function here and reach for OnFoo, and realize it's totally different from the rest. But in order to keep this patch small and targeted, can we rename the new function and add a TODO for the rest? https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:112: // Returns the number of visible icons as an absolute value. On 2014/10/30 22:05:59, Peter Kasting wrote: > Nit: I think this name/comment are misleading. They suggest you're going to do > a mathematical abs(). I would use some other word than "Absolute" in the > function name. Maybe "Actual", or "True"? Yeah, I struggled with the name. The problem with "actual" and "true" is that it implies the other is "fake" (and why would we ever want a fake value?). I think the real problem here is that GetVisibleIconCount() is trying to do two things - return the number of icons and return whether or not all icons are visible. Let's just make a function for that. Solves naming, and improves readability in some cases. SGTY? https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model_unittest.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model_unittest.cc:146: reorder_count_(0u) { On 2014/10/30 22:05:59, Peter Kasting wrote: > Nit: None of these lines should use 0u, they should just be 0. (We only need to > use 0u explicitly in certain EXPECT/ASSERT statements due to how the macro > expansion works.) Done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_action_view.h:45: virtual content::WebContents* GetCurrentWebContents() const = 0; On 2014/10/30 22:05:59, Peter Kasting wrote: > Const functions should not return non-const pointers. Either make the returned > pointer const, or leave this non-const. I thought this only applied to direct data members of the class, which this doesn't return? But, it's an easy enough workaround, so done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:206: suppress_animation_ = true; On 2014/10/30 22:06:00, Peter Kasting wrote: > Nit: Use base::AutoReset to be safe against recursion and reduce the chance of > future maintainers accidentally bypassing your "set the variable back" > statements: > > ... > base::AutoReset<bool> animation_resetter(&suppress_animation_, true); > { > base::AutoReset<bool> layout_resetter(&suppress_layout_, true); > for (BrowserActionView* view : browser_action_views_) > view->UpdateState(); > } > ReorderViews(); > } Done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:225: suppress_layout_ = true; On 2014/10/30 22:06:00, Peter Kasting wrote: > Nit: AutoReset here too, perhaps Done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:391: if (suppress_layout_) On 2014/10/30 22:05:59, Peter Kasting wrote: > Nit: I think this should happen at the top of the function. Otherwise, calling > Layout() with |suppress_layout_| set will have a functional effect when we go > from some views to no views, but not when we go from no views to some views. Done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1051: // index, since everything up to this point is correct). On 2014/10/30 22:06:00, Peter Kasting wrote: > Is it conceivable that the two sets of views are not in-sync? If so, the code > below would run off the end of the arrays in question. > > I assume not, but it certainly made me take a closer look at the loop. Hmm...I don't think so. Views are added to the container synchronously once they are added to the model, so we should be good. That said, it doesn't hurt to DCHECK it. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1056: std::swap(browser_action_views_[i], browser_action_views_[j]); On 2014/10/30 22:06:00, Peter Kasting wrote: > I wonder if std::rotate() would make more sense than std::swap(). rotate() > could be more performant, or it could make no sense at all, depending on what > kinds of model changes you have to account for. > > In general, if you're only needing to account for extensions popping themselves > out of overflow, you could conceivably change the API so that you call once for > each single such extension, or maybe call with a vector of the extension IDs > that are popping out of overflow, or some such. Then that could be implemented > here using rotate(). The "something changed" API you have designed in this CL > is more general, but it's also less efficient if you don't need that generality. The reason not to do that is because ReorderViews() is also called when we switch tabs, in which case neither the model itself nor its underlying items change, so we can't call it with the specific extension(s) that update. Theoretically, we could have the model call back into this with the necessary changes whenever tabs change, but that seems a little messier than this way. So if we don't have a known set of extension differences to move forward, I think this way might be easiest (and it's still pretty simple, given that extensions are < 100, and on average around 2). WDYT? https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1076: #if !defined(NDEBUG) On 2014/10/30 22:06:00, Peter Kasting wrote: > Nit: While here: use DCHECK_IS_ON instead of looking at NDEBUG, so release > trybots will run these sanity checks. Good idea. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:327: // Reorder the views to match the toolbar model for the active tab. On 2014/10/30 22:06:00, Peter Kasting wrote: > Nit: "Reorders" Done. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:382: // True if we should suppress animation. On 2014/10/30 22:06:00, Peter Kasting wrote: > Nit: Does this need a note about when/why we might want this? Done.
LGTM with a few substantive comments at the bottom. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:84: virtual void ToolbarReorderNecessary( On 2014/10/31 17:44:09, Devlin wrote: > On 2014/10/30 22:05:59, Peter Kasting wrote: > > Nit: This is a strange name for a function. It sounds as if it should return > a > > bool, but instead it returns void. Maybe "ReorderToolbarIfNecessary", or > > "OnToolbarReorderNecessary"? (Some of the other functions here have > problematic > > names too... I might add "On" to a lot of them that are event handlers.) > > /sigh... I know - it's bothered me most times I add a new function here and > reach for OnFoo, and realize it's totally different from the rest. But in order > to keep this patch small and targeted, can we rename the new function and add a > TODO for the rest? Doesn't bother me. Bonus points if you go write a separate renaming patch to do the renaming immediately, but I'm fine either way. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:112: // Returns the number of visible icons as an absolute value. On 2014/10/31 17:44:09, Devlin wrote: > On 2014/10/30 22:05:59, Peter Kasting wrote: > > Nit: I think this name/comment are misleading. They suggest you're going to > do > > a mathematical abs(). I would use some other word than "Absolute" in the > > function name. Maybe "Actual", or "True"? > > Yeah, I struggled with the name. The problem with "actual" and "true" is that > it implies the other is "fake" (and why would we ever want a fake value?). > > I think the real problem here is that GetVisibleIconCount() is trying to do two > things - return the number of icons and return whether or not all icons are > visible. Let's just make a function for that. Solves naming, and improves > readability in some cases. > > SGTY? Good resolution. I'm happy with it. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_action_view.h:45: virtual content::WebContents* GetCurrentWebContents() const = 0; On 2014/10/31 17:44:09, Devlin wrote: > On 2014/10/30 22:05:59, Peter Kasting wrote: > > Const functions should not return non-const pointers. Either make the > returned > > pointer const, or leave this non-const. > > I thought this only applied to direct data members of the class, which this > doesn't return? But, it's an easy enough workaround, so done. There's a whole long physical/logical constness debate behind this one, but the short answer is that it's too easy in too many situations for even non-member returned pointers to be used to make changes to state that affects the supposedly-const object. Rather than try and debate whether such changes are theoretically possible it's easiest to just have this simple rule. https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/675023002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1056: std::swap(browser_action_views_[i], browser_action_views_[j]); On 2014/10/31 17:44:09, Devlin wrote: > On 2014/10/30 22:06:00, Peter Kasting wrote: > > I wonder if std::rotate() would make more sense than std::swap(). rotate() > > could be more performant, or it could make no sense at all, depending on what > > kinds of model changes you have to account for. > > > > In general, if you're only needing to account for extensions popping > themselves > > out of overflow, you could conceivably change the API so that you call once > for > > each single such extension, or maybe call with a vector of the extension IDs > > that are popping out of overflow, or some such. Then that could be > implemented > > here using rotate(). The "something changed" API you have designed in this CL > > is more general, but it's also less efficient if you don't need that > generality. > > The reason not to do that is because ReorderViews() is also called when we > switch tabs, in which case neither the model itself nor its underlying items > change, so we can't call it with the specific extension(s) that update. > Theoretically, we could have the model call back into this with the necessary > changes whenever tabs change, but that seems a little messier than this way. > > So if we don't have a known set of extension differences to move forward, I > think this way might be easiest (and it's still pretty simple, given that > extensions are < 100, and on average around 2). > > WDYT? Fine with me. https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.cc:634: result.rbegin() + (result.size() - boundary)); Why use rbegin() + result.size() - x in all these? It seems like this could be clearer using begin() everywhere. Furthermore, I suspect you could make your loop index an iterator instead and get an even simpler function body. https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.h (right): https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:113: toolbar_items().size() : static_cast<size_t>(visible_icon_count_); Nit: So... now that we have both these accessors, would it make sense to just have two members as well? That eliminates the casting here, and my hope is that it would make the usage within the class more consistent and less confusing (but maybe not). https://codereview.chromium.org/675023002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/675023002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1041: #if defined(DCHECK_IS_ON) I think you want "#if DCHECK_IS_ON", since I think it's always defined, just to either 0 or 1. See https://code.google.com/p/chromium/codesearch#search/&q=DCHECK_IS_ON&sq=packa... . (2 places)
https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.cc (right): https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.cc:634: result.rbegin() + (result.size() - boundary)); On 2014/10/31 19:01:08, Peter Kasting wrote: > Why use rbegin() + result.size() - x in all these? It seems like this could be > clearer using begin() everywhere. Excellent question. For some reason, I thought a right rotation made more sense, but it really shouldn't matter. Changed. > Furthermore, I suspect you could make your loop index an iterator instead and > get an even simpler function body. Thought std::rotate would invalidate iters. Turns out not. Changed. https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_toolbar_model.h (right): https://codereview.chromium.org/675023002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_toolbar_model.h:113: toolbar_items().size() : static_cast<size_t>(visible_icon_count_); On 2014/10/31 19:01:08, Peter Kasting wrote: > Nit: So... now that we have both these accessors, would it make sense to just > have two members as well? That eliminates the casting here, and my hope is that > it would make the usage within the class more consistent and less confusing (but > maybe not). Yeah, I was thinking the same thing, but I'll postpone that for another patch (hopefully Monday). It'll need a little more work, and we'll need to be careful since we also use the visible icon count in saved preferences. Added a TODO. https://codereview.chromium.org/675023002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/675023002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.cc:1041: #if defined(DCHECK_IS_ON) On 2014/10/31 19:01:08, Peter Kasting wrote: > I think you want "#if DCHECK_IS_ON", since I think it's always defined, just to > either 0 or 1. See > https://code.google.com/p/chromium/codesearch#search/&q=DCHECK_IS_ON&sq=packa... > . (2 places) Ah, right. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/675023002/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d604171517135387ca3b4c33d7f1774c8d2d38b0 Cr-Commit-Position: refs/heads/master@{#302511}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:200001) has been created in https://codereview.chromium.org/700453003/ by benwells@chromium.org. The reason for reverting is: Suspect this patch of causing errors on linux valgrind for LocationBarControllerUnitTest.NavigationClearsState. http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v... Sample valgrind output: Suppression (error hash=#606630BA25518095#): For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem... { <insert_a_suppression_name_here> Memcheck:Unaddressable fun:_ZN16ExtensionService21NotifyExtensionLoadedEPKN10extensions9ExtensionE fun:_ZN16ExtensionService12AddExtensionEPKN10extensions9ExtensionE fun:_ZN10extensions12_GLOBAL__N_129LocationBarControllerUnitTest12AddExtensionEbRKSs fun:_ZN10extensions12_GLOBAL__N_156LocationBarControllerUnitTest_NavigationClearsState_Test8TestBodyEv }. |
