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

chrome/browser/extensions/extension_toolbar_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 
 #include <string>
 
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
@@ skipping 238 matching lines @@
     for (ExtensionList::const_iterator iter_ext = toolbar_items_.begin();
          iter_ext < toolbar_items_.end(); ++iter_ext) {
       if ((*iter_ext)->id().compare(*iter_id) == 0) {
         // This extension is visible, update the index value.
         ++new_index;
         break;
       }
     }
   }
 
-  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) {
   // We only care about extensions with browser actions.
   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(),
                                                  last_known_positions_.end(),
                                                  extension->id());
   if (last_pos != last_known_positions_.end()) {
     new_index = FindNewPositionFromLastKnownGood(extension);
     if (new_index != toolbar_items_.size()) {
       toolbar_items_.insert(toolbar_items_.begin() + new_index,
                             make_scoped_refptr(extension));
     } else {
       toolbar_items_.push_back(make_scoped_refptr(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();
   }
 
   // If we're currently highlighting, then even though we add a browser action
   // to the full list (|toolbar_items_|, there won't be another *visible*
   // browser action, which was what the observers care about.
   if (!is_highlighting_) {
     FOR_EACH_OBSERVER(Observer, observers_,
                       BrowserActionAdded(extension, new_index));
   }
@@ skipping 285 matching lines @@
     is_highlighting_ = false;
     if (old_visible_icon_count_ != visible_icon_count_) {
       SetVisibleIconCount(old_visible_icon_count_);
       FOR_EACH_OBSERVER(Observer, observers_, VisibleCountChanged());
     }
     FOR_EACH_OBSERVER(Observer, observers_, HighlightModeChanged(false));
   }
 }
 
 }  // namespace extensions