Fixes for re-enabling more MSVC level 4 warnings: Source/core/ edition

Fixes for re-enabling more MSVC level 4 warnings: Source/core/ edition

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

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

BUG=81439
TEST=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178391

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

Ping for Dominic.

[Peter Kasting, 2014-07-16 20:27:52]

Adam, could you review this?  Dominic doesn't seem to be responding.

[dmazzoni, 2014-07-16 20:43:18]

Sorry I missed this. Taking a look now, but it might be better for Adam to
review as well if there's anything nontrivial.

[abarth-chromium, 2014-07-16 20:44:17]

LGTM

[dmazzoni, 2014-07-16 21:21:17]

I'm slightly worried that this pattern was pretty uncommon in Google and Chrome
code, but it's quite prevalent in Blink.

I'm curious if there are other ways to silence the warning other than just never
doing assignment in a conditional.

https://codereview.chromium.org/383153007/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/383153007/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXObject.cpp:724: for (; scrollParent &&
!scrollableArea; scrollableArea = scrollParent ?
scrollParent->getScrollableAreaIfScrollable() : 0)
I find this change a little harder to read. How about this instead:

ScrollableArea* scrollableArea = 0;
while (scrollParent) {
    if (scrollableArea = scrollParent->getScrollableAreaIfScrollable())
        break;
    scrollParent = scrollParent->parentObject();
}

If it complains about equality in the conditional, I'd still prefer this:

ScrollableArea* scrollableArea = 0;
while (scrollParent) {
    scrollableArea = scrollParent->getScrollableAreaIfScrollable();
    if (scrollableArea)
        break;
    scrollParent = scrollParent->parentObject();
}

https://codereview.chromium.org/383153007/diff/20001/Source/core/editing/Text...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/383153007/diff/20001/Source/core/editing/Text...
Source/core/editing/TextIterator.cpp:932: for (Node* next =
NodeTraversal::nextSkippingChildren(node); next; next =
NodeTraversal::nextSkippingChildren(*next)) {
To me, duplicating NodeTraversal::nextSkippingChildren(node) in order to avoid
the compiler warning hurts readability. How about:

Node* next = &node;
do {
  next = NodeTraversal::nextSkippingChildren(*next);
  if (next && next->renderer())
      return true;
} while (next);

https://codereview.chromium.org/383153007/diff/20001/Source/core/page/FrameTr...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/383153007/diff/20001/Source/core/page/FrameTr...
Source/core/page/FrameTree.cpp:41: const unsigned invalidChildCount =
~static_cast<unsigned>(0);
No strong preference, but I wonder if UINT_MAX or 0xffffffff wouldn't be more
clear?

[Peter Kasting, 2014-07-16 23:06:58]

On 2014/07/16 21:21:17, dmazzoni wrote:
> I'm slightly worried that this pattern was pretty uncommon in Google and
Chrome
> code, but it's quite prevalent in Blink.

It actually wasn't very common -- and they're all gone now in any case (this was
the very last patch to be approved).

> I'm curious if there are other ways to silence the warning other than just
never
> doing assignment in a conditional.

You can actually silence the warning by wrapping the assignment in another
conditional test, like "while ((foo = bar()) != NULL)", but I think that's less
readable than any other alternative (especially when "foo" and/or "bar" are
longer expressions).  Especially when now almost no code anywhere in Chrome or
Blink will do assignments inside conditionals, having one happen very rarely
seems to me like courting danger.

PTAL.

https://codereview.chromium.org/383153007/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/383153007/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXObject.cpp:724: for (; scrollParent &&
!scrollableArea; scrollableArea = scrollParent ?
scrollParent->getScrollableAreaIfScrollable() : 0)
On 2014/07/16 21:21:16, dmazzoni wrote:
> I find this change a little harder to read. How about this instead:
> 
> ScrollableArea* scrollableArea = 0;
> while (scrollParent) {
>     if (scrollableArea = scrollParent->getScrollableAreaIfScrollable())
>         break;
>     scrollParent = scrollParent->parentObject();
> }
> 
> If it complains about equality in the conditional, I'd still prefer this:
> 
> ScrollableArea* scrollableArea = 0;
> while (scrollParent) {
>     scrollableArea = scrollParent->getScrollableAreaIfScrollable();
>     if (scrollableArea)
>         break;
>     scrollParent = scrollParent->parentObject();
> }

Your first form won't work, but the second will.  Changed.

https://codereview.chromium.org/383153007/diff/20001/Source/core/editing/Text...
File Source/core/editing/TextIterator.cpp (right):

https://codereview.chromium.org/383153007/diff/20001/Source/core/editing/Text...
Source/core/editing/TextIterator.cpp:932: for (Node* next =
NodeTraversal::nextSkippingChildren(node); next; next =
NodeTraversal::nextSkippingChildren(*next)) {
On 2014/07/16 21:21:16, dmazzoni wrote:
> To me, duplicating NodeTraversal::nextSkippingChildren(node) in order to avoid
> the compiler warning hurts readability. How about:
> 
> Node* next = &node;
> do {
>   next = NodeTraversal::nextSkippingChildren(*next);
>   if (next && next->renderer())
>       return true;
> } while (next);

I really find this significantly less readable than the form I had (while loops
in general and duplicated conditions between the middle and the end of the loop
fry my brain), but I'd prefer not to spend time arguing about it.  Done.

https://codereview.chromium.org/383153007/diff/20001/Source/core/page/FrameTr...
File Source/core/page/FrameTree.cpp (right):

https://codereview.chromium.org/383153007/diff/20001/Source/core/page/FrameTr...
Source/core/page/FrameTree.cpp:41: const unsigned invalidChildCount =
~static_cast<unsigned>(0);
On 2014/07/16 21:21:16, dmazzoni wrote:
> No strong preference, but I wonder if UINT_MAX or 0xffffffff wouldn't be more
> clear?

0xffffffff wouldn't be good in case int isn't 32 bits (maybe for a hypothetical
64-bit build or something).

UINT_MAX should work, and I discovered the other day that ~0U works.  I've
changed to the second.

[dmazzoni, 2014-07-17 16:27:22]

On 2014/07/16 23:06:58, Peter Kasting wrote:
> On 2014/07/16 21:21:17, dmazzoni wrote:
> > I'm slightly worried that this pattern was pretty uncommon in Google and
> Chrome
> > code, but it's quite prevalent in Blink.
> 
> It actually wasn't very common -- and they're all gone now in any case (this
was
> the very last patch to be approved).

I was thinking of cases like these:

https://code.google.com/p/chromium/codesearch#search/&q=if%5C%20%5C(.*%5C%20=...

Oh, I see - I guess you don't get a compile warning if you declare the variable
in the same conditional? I didn't realize at first that there was a distinction,
but I guess that makes sense. Do you think it's perfectly safe to do that, or
would you prefer those are changed too?

That pattern is a bit more common in Blink - but if it's safe and doesn't
generate any compile warnings then there's nothing wrong with that.

[dmazzoni, 2014-07-17 16:27:28]

lgtm

[Peter Kasting, 2014-07-17 17:53:00]

On 2014/07/17 16:27:22, dmazzoni wrote:
> I was thinking of cases like these:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=if%5C%20%5C(.*%5C%20=...
> 
> Oh, I see - I guess you don't get a compile warning if you declare the
variable
> in the same conditional? I didn't realize at first that there was a
distinction,
> but I guess that makes sense. Do you think it's perfectly safe to do that, or
> would you prefer those are changed too?

Personally, I'd prefer to not do that sort of thing, but I'm not going to try to
fix these, since they don't generate any warnings.  Unless the compiler warned
about them (and I don't think MSVC will ever warn about these, even under a
disabled-unless-/WAll warning), they'd just reappear anyway, unless everyone
else shared my view that this is a somewhat surprising and risky pattern. 
Besides, while I may not like this, it's certainly less dangerous than the types
of assignments I _am_ removing.

[Peter Kasting, 2014-07-17 17:53:03]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-17 17:53:57]

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

[commit-bot: I haz the power, 2014-07-17 19:12:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-07-17 19:32:11]

Change committed as 178391