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

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

This contains fixes for the following sorts of issues:
* Possibly-uninitialized local variable
* Signedness mismatch
* Assignment inside conditional

This also contains a small number of other cleanups/simplifications to nearby
 code.

BUG=81439
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283325

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

finnur: chrome/browser/extensions/, in particular the line I commented on.
darin: rest

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:259: return
toolbar_items_.size();  // Is this correct?  Or maybe |new_index|?
This line needs review.

https://codereview.chromium.org/368133005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:848: priority > 1.0) {
Note: These rather arbitrary-looking changes make the "possibly uninitialized
local: |priority|" warning in MSVC go away.

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

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/uma_policy.cc (right):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/uma_policy.cc:112: }
nit: None of these return line need the braces (lines 107, 111, 120, 143 and
friends) -- consistent with rest of file too.

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (left):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:286: new_index =
toolbar_items_.size() - 1;
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.

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:259: return
toolbar_items_.size();  // Is this correct?  Or maybe |new_index|?
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.

[Finnur, 2014-07-03 12:18:09]

And with that LGTM

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

Some replies to Finnur.

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/uma_policy.cc (right):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/uma_policy.cc:112: }
On 2014/07/03 12:17:54, Finnur (OOO - back Jul 14th) wrote:
> nit: None of these return line need the braces (lines 107, 111, 120, 143 and
> friends) -- consistent with rest of file too.

Sure, I can remove these.

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (left):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:286: new_index =
toolbar_items_.size() - 1;
On 2014/07/03 12:17:54, Finnur (OOO - back Jul 14th) wrote:
> 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 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'd also drop the comment you added to line 283, as new_index isn't used in
the
> else clause.

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.

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:259: return
toolbar_items_.size();  // Is this correct?  Or maybe |new_index|?
On 2014/07/03 12:17:54, Finnur (OOO - back Jul 14th) wrote:
> 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.

Done.

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

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (left):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:286: new_index =
toolbar_items_.size() - 1;
> These should be equivalent, because the push_back() call increases the size of
> |toolbar_items_| by one.

Sure. But I must still be misreading the CL because I don't see where you change
the push_back call.

> 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.

I'm fine either way. Maybe lose the braces?

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

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (left):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:286: new_index =
toolbar_items_.size() - 1;
On 2014/07/08 10:48:24, Finnur (OOO - back Jul 14th) wrote:
> > These should be equivalent, because the push_back() call increases the size
of
> > |toolbar_items_| by one.
> 
> Sure. But I must still be misreading the CL because I don't see where you
change
> the push_back call.

Now I'm the one confused.  I'm not changing the push_back() call.  It already
exists, and is correct.

> > 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.
> 
> I'm fine either way. Maybe lose the braces?

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:21]

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_toolbar_model.cc (left):

https://codereview.chromium.org/368133005/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_toolbar_model.cc:286: new_index =
toolbar_items_.size() - 1;
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. 

> What braces can we lose?  This is a multiline block.

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:
> On 2014/07/08 10:48:24, Finnur (OOO - back Jul 14th) wrote:
> > > These should be equivalent, because the push_back() call increases the
size
> of
> > > |toolbar_items_| by one.
> > 
> > Sure. But I must still be misreading the CL because I don't see where you
> change
> > the push_back call.
> 
> Now I'm the one confused.  I'm not changing the push_back() call.  It already
> exists, and is correct.
> 
> > > 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.
> > 
> > I'm fine either way. Maybe lose the braces?
> 
> What braces can we lose?  This is a multiline block.
> 
> FINNUR I AM SO CONFUSE.  WHY YOU MAKE HEAD HURT.

[Peter Kasting, 2014-07-08 23:35:43]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-08 23:41:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/368133005/40001

[Peter Kasting, 2014-07-09 00:04:15]

The CQ bit was unchecked by pkasting@chromium.org

[Peter Kasting, 2014-07-09 00:04:37]

On 2014/07/09 00:04:15, Peter Kasting wrote:
> The CQ bit was unchecked by mailto:pkasting@chromium.org

Forgot, I can't CQ+ until Darin reviews.

[Peter Kasting, 2014-07-11 17:21:55]

Friendly ping for Darin

[Peter Kasting, 2014-07-14 22:17:07]

Scott, no good deed goes unpunished, so I'm going to send you another two of
these, since Darin seems to be unresponsive as well.  Here's 2 of 2.

[sky, 2014-07-15 04:22:25]

LGTM

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/prerender...
chrome/browser/prerender/prerender_local_predictor.cc:842: if
(!d->GetString("url", &url) || !GURL(url).is_valid()) {
nit: no {}

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:971: FileHeader header
= {{0}};
tricky

[Peter Kasting, 2014-07-15 18:56:07]

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/prerender...
chrome/browser/prerender/prerender_local_predictor.cc:842: if
(!d->GetString("url", &url) || !GURL(url).is_valid()) {
On 2014/07/15 04:22:24, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right):

https://codereview.chromium.org/368133005/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_store_file.cc:971: FileHeader header
= {{0}};
On 2014/07/15 04:22:24, sky wrote:
> tricky

Yeah, I'm not a huge fan of this, but "= {0}" doesn't compile on all compilers,
and I didn't want to go so far as to add a constructor for the union -- that
feels pretty weird.

[Peter Kasting, 2014-07-15 18:56:16]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-15 19:01:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/368133005/60001

[commit-bot: I haz the power, 2014-07-16 04:07:59]

Change committed as 283325