Fixes for re-enabling more MSVC level 4 warnings: chrome/browser/ edition

chrome/browser/extensions/extension_toolbar_model.cc

Index: chrome/browser/extensions/extension_toolbar_model.cc
diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc
index 4e1a472ff29b9f1c8d99e2c210ebd31e095fb78b..86a64d7f4064e2e89bf1631d459205f85dc65b0a 100644
--- a/chrome/browser/extensions/extension_toolbar_model.cc
+++ b/chrome/browser/extensions/extension_toolbar_model.cc
@@ -256,7 +256,7 @@ size_t ExtensionToolbarModel::FindNewPositionFromLastKnownGood(
     }
   }
 
-  return -1;
+  return toolbar_items_.size();  // Is this correct?  Or maybe |new_index|?

[Peter Kasting, 2014/07/03 01:18:50]

This line needs review.

[Finnur, 2014/07/03 12:17:54]

It is definitely wrong to return |new_index| because if |last_known_positions_|
doesn't contain the |extension_id| for |extension| then you'd return 0 which
means front of the list. It is fine to return |toolbar_items_.size()|,
signifying beyond the end.

Maybe change the comment to:
// Position not found.

... or something.

[Peter Kasting, 2014/07/07 19:15:29]

Done.

 }
 
 void ExtensionToolbarModel::AddExtension(const Extension* extension) {
@@ -264,7 +264,7 @@ void ExtensionToolbarModel::AddExtension(const Extension* extension) {
   if (!ExtensionActionManager::Get(profile_)->GetBrowserAction(*extension))
     return;
 
-  size_t new_index = -1;
+  size_t new_index = toolbar_items_.size();
 
   // See if we have a last known good position for this extension.
   ExtensionIdList::iterator last_pos = std::find(last_known_positions_.begin(),
@@ -280,10 +280,9 @@ void ExtensionToolbarModel::AddExtension(const Extension* extension) {
     }
   } else {
     // This is a never before seen extension, that was added to the end. Make
-    // sure to reflect that.
+    // sure to reflect that. (|new_index| was set above.)
     toolbar_items_.push_back(make_scoped_refptr(extension));
     last_known_positions_.push_back(extension->id());
-    new_index = toolbar_items_.size() - 1;

[Finnur, 2014/07/03 12:17:54]

This removal is not correct, I believe. |new_index| is a zero-based index of
visible icons, so you'll tell the observers (line 294) that a never before seen
icon was just added to a position one off from where it was actually added (and
potentially out of bounds altogether). 

I think you should probably initialize new_index at the top to
|toolbar_items_.size() - 1|, then this line can be deleted.

I'd also drop the comment you added to line 283, as new_index isn't used in the
else clause.

[Peter Kasting, 2014/07/07 19:15:29]

I think you're misreading the code.

Note that in the original code here, we do this:

  toolbar_items_.push_back(...);
  ...
  new_index = toolbar_items_.size() - 1;

In my change, we instead do this:

  new_index = toolbar_items_.size();
  ...
  toolbar_items_.push_back(...);

These should be equivalent, because the push_back() call increases the size of
|toolbar_items_| by one.

If I were to init at the top in the way you suggest, I would be changing the
behavior from the existing code, and (I believe) introducing an off-by-one
error.
I added the comment because |new_index| is used unconditionally below, so I
assumed readers might wonder about whether we needed to touch it.  I'm not
wedded to it; if, after responding to my comment above, you think this should
still be nuked I'm happy to do so.

[Finnur, 2014/07/08 10:48:24]

Sure. But I must still be misreading the CL because I don't see where you change
the push_back call.
I'm fine either way. Maybe lose the braces?

[Peter Kasting, 2014/07/08 19:21:25]

Now I'm the one confused.  I'm not changing the push_back() call.  It already
exists, and is correct.
What braces can we lose?  This is a multiline block.

FINNUR I AM SO CONFUSE.  WHY YOU MAKE HEAD HURT.

[Finnur, 2014/07/08 23:31:20]

My apologies. I was trying to get a review in (to expedite your work) in between
dashing between popular tourist destinations here in Iceland (waterfalls,
glaciers, etc...) :) Looks like I should have waited until things settled
down...

So yes... the confusion was on my part. You weren't saying "I moved the
push_back call", which is what triggered me looking for that kind of change. I
get it now. I think your change is correct. 
Gaa... not braces. I meant parentheses! Sorry -- Ienlisch is nod my farst
langeech.  :)

LGTM either way.


On 2014/07/08 19:21:25, Peter Kasting wrote:

     UpdatePrefs();
   }