Enable MSVC warning for unused locals.

Enable MSVC warning for unused locals.

There is seemingly a bug in the compiler where it occasionally claims a local is
unused when it isn't.  This forces a few places to either inline such locals or
mark them ALLOW_UNUSED_LOCAL.

BUG=81439
TEST=none
R=brettw@chromium.org, cpu@chromium.org, jamesr@chromium.org, rvargas@chromium.org, sievers@chromium.org, sky@chromium.org, vitalybuka@chromium.org, wolenetz@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/be940e9e152e528fc567c4eeeeb5930710873f98

[Peter Kasting, 2014-11-18 00:41:01]

pkasting@chromium.org changed reviewers:
+ cpu@chromium.org, jamesr@chromium.org, rvargas@chromium.org,
sievers@chromium.org, sky@chromium.org, vitalybuka@chromium.org

[Peter Kasting, 2014-11-18 00:41:03]

sky: ash/, chrome/, ui/ OWNERS
rvargas: base/, net/ OWNERS
vitalyuka: cloud_print/ OWNERS
sievers: content/, gpu/ OWNERS
jamesr: mojo/ OWNERS; if this change looks OK to you, could you land it in the
mojo repo for me and merge it back over?
cpu: win8/ OWNERS

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

https://codereview.chromium.org/731373002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/external_provider_impl.cc:468: #endif
Note: Every change other than the couple lines above this is simply to try to
unify the various ifdef blocks a bit more, e.g.:

#if A
...
#endif
#if !A && B
...
#endif
#if A
...
#endif

->

#if A
...
...
#else if B
...
#endif

There should be no functional change.

Technically this isn't necessary for this CL but I rearranged these while trying
to figure out what conditions used |bundled_extension_creation_flags| and
thought I'd just leave the rearrangements as they're slightly more sane.

https://codereview.chromium.org/731373002/diff/1/chrome/browser/history/in_me...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/chrome/browser/history/in_me...
chrome/browser/history/in_memory_url_index_unittest.cc:990:
GetPrivateData()->URLSchemeIsWhitelisted(url, whitelist));
This is really just a DCHECK + cheap accessor so it's not a problem to run it
inside the loop.

https://codereview.chromium.org/731373002/diff/1/cloud_print/service/win/clou...
File cloud_print/service/win/cloud_print_service.cc (right):

https://codereview.chromium.org/731373002/diff/1/cloud_print/service/win/clou...
cloud_print/service/win/cloud_print_service.cc:344: return ReportError(E_FAIL,
IDS_ERROR_FAILED_CREATE_CONFIG);
We never checked the value of |is_valid| before.  It seems like we should, so I
tentatively added this ReportError() call.  Not sure if it's really correct.

https://codereview.chromium.org/731373002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/731373002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_aura.cc:673: // ?? Should
this use host instead of host_?
This needs to be answered before landing.

I have no idea what the difference is.  Code in this file uses both frequently,
which confuses me to no end.

https://codereview.chromium.org/731373002/diff/1/content/renderer/render_font...
File content/renderer/render_font_warmup_win.cc (right):

https://codereview.chromium.org/731373002/diff/1/content/renderer/render_font...
content/renderer/render_font_warmup_win.cc:194:
GetCustomFontCollection(factory);
I assumed calling this was important to warm up something, even if we didn't
care about the return code.  If not, this line should be removed entirely.

https://codereview.chromium.org/731373002/diff/1/gpu/command_buffer/service/b...
File gpu/command_buffer/service/buffer_manager_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/gpu/command_buffer/service/b...
gpu/command_buffer/service/buffer_manager_unittest.cc:213: const GLsizeiptr
kDataSize = 10;
This is a guess on the correct way to rewrite this -- if |data| (and not just
its size) was actually supposed to be used below, then obviously some other
change is needed.

https://codereview.chromium.org/731373002/diff/1/net/base/file_stream_context...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/731373002/diff/1/net/base/file_stream_context...
net/base/file_stream_context_win.cc:72: return 0;  // Report EOF by returning 0
bytes read.
This change is to comply with the Chromium "no else after return" style rule.

https://codereview.chromium.org/731373002/diff/1/net/quic/quic_unacked_packet...
File net/quic/quic_unacked_packet_map_test.cc (right):

https://codereview.chromium.org/731373002/diff/1/net/quic/quic_unacked_packet...
net/quic/quic_unacked_packet_map_test.cc:188: VerifyUnackedPackets(unacked2,
arraysize(unacked2));
Technically this functions identically to the old code, since |unacked| starts
with 1, but I think this is what the author intended.

https://codereview.chromium.org/731373002/diff/1/net/socket/socks5_client_soc...
File net/socket/socks5_client_socket_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/net/socket/socks5_client_soc...
net/socket/socks5_client_socket_unittest.cc:261: MockWrite(ASYNC, partial1,
arraysize(partial1)),
I assume we meant to actually write |partial1|?

https://codereview.chromium.org/731373002/diff/1/net/socket/socks_client_sock...
File net/socket/socks_client_socket_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/net/socket/socks_client_sock...
net/socket/socks_client_socket_unittest.cc:290: MockWrite(ASYNC,
kSOCKSPartialRequest1, arraysize(kSOCKSPartialRequest1)),
I assume we meant to actually write |kSOCKSPartialRequest1|?

https://codereview.chromium.org/731373002/diff/1/ui/views/controls/menu/nativ...
File ui/views/controls/menu/native_menu_win.cc (left):

https://codereview.chromium.org/731373002/diff/1/ui/views/controls/menu/nativ...
ui/views/controls/menu/native_menu_win.cc:289: config.check_height) / 2;
The DrawToNativeContext() call below does some calculations kind of like this,
but not exactly -- were they supposed to be using these variables?

[jamesr, 2014-11-18 00:44:16]

https://codereview.chromium.org/731373002/diff/1/mojo/public/cpp/system/tests...
File mojo/public/cpp/system/tests/macros_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/mojo/public/cpp/system/tests...
mojo/public/cpp/system/tests/macros_unittest.cc:70:
MOJO_ALLOW_UNUSED_LOCAL(local_array);
this local seems to be used on the next line - no? is this a toolchain bug?

[Peter Kasting, 2014-11-18 00:50:30]

https://codereview.chromium.org/731373002/diff/1/mojo/public/cpp/system/tests...
File mojo/public/cpp/system/tests/macros_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/mojo/public/cpp/system/tests...
mojo/public/cpp/system/tests/macros_unittest.cc:70:
MOJO_ALLOW_UNUSED_LOCAL(local_array);
On 2014/11/18 00:44:16, jamesr wrote:
> this local seems to be used on the next line - no? is this a toolchain bug?

MSVC considers something unused if e.g. you only ever take sizeof(local), or
refer to it in typedefs, or the like, and not actually use its value.  I find
this arguable, but it turns out to be fairly rare that we actually want to use a
variable for only such purposes and nothing else, so it's easy enough to just
say this is ALLOW_UNUSED and move on.

The total number of places across the codebase that MSVC's quirks cause us to
have to do something otherwise-ugly, like this, is a small subset of the changes
in this CL -- less than half a dozen total -- and the process of enabling this
warning has turned up some questionable code and potential bugs, so I think the
tradeoff is worth it.

[rvargas (doing something else), 2014-11-18 02:11:48]

base and net lgtm

https://codereview.chromium.org/731373002/diff/1/net/socket/socks5_client_soc...
File net/socket/socks5_client_socket_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/net/socket/socks5_client_soc...
net/socket/socks5_client_socket_unittest.cc:261: MockWrite(ASYNC, partial1,
arraysize(partial1)),
On 2014/11/18 00:41:03, Peter Kasting wrote:
> I assume we meant to actually write |partial1|?

wow. I'm surprised it worked at all.

[Peter Kasting, 2014-11-18 02:15:17]

pkasting@chromium.org changed reviewers:
+ brettw@chromium.org, wolenetz@chromium.org

[Peter Kasting, 2014-11-18 02:15:18]

+brettw for .gn change and wolenetz for media/

https://codereview.chromium.org/731373002/diff/1/net/socket/socks5_client_soc...
File net/socket/socks5_client_socket_unittest.cc (right):

https://codereview.chromium.org/731373002/diff/1/net/socket/socks5_client_soc...
net/socket/socks5_client_socket_unittest.cc:261: MockWrite(ASYNC, partial1,
arraysize(partial1)),
On 2014/11/18 02:11:48, rvargas wrote:
> On 2014/11/18 00:41:03, Peter Kasting wrote:
> > I assume we meant to actually write |partial1|?
> 
> wow. I'm surprised it worked at all.

Could be a sign that the tests are not as exhaustive as they should be?  :)

[Peter Kasting, 2014-11-18 02:17:15]

https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
media/audio/win/audio_low_latency_input_win.cc:246: DLOG(WARNING) << "Failed to
set new input master volume.";
Note: There was nothing wrong with the old code except that in release mode,
|hr| appeared to be unused.  This is one possible fix; another is to add
"ALLOW_UNUSED_LOCAL(hr);" instead in these places; still another would be to do
something akin to:

#if DLOG_IS_ON
  HRESULT hr =
#endif
      ...;
  DLOG_IF...

I thought this was the least ugly choice of the three.

[sky, 2014-11-18 03:33:31]

LGTM

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
File chrome/browser/search/suggestions/suggestions_source.cc (left):

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
chrome/browser/search/suggestions/suggestions_source.cc:119:
SuggestionsServiceFactory* suggestions_service_factory =
Does MSVC really complain about this style?

[Peter Kasting, 2014-11-18 06:23:17]

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
File chrome/browser/search/suggestions/suggestions_source.cc (left):

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
chrome/browser/search/suggestions/suggestions_source.cc:119:
SuggestionsServiceFactory* suggestions_service_factory =
On 2014/11/18 03:33:31, sky wrote:
> Does MSVC really complain about this style?

Yes :(

I think this is some kind of bug.  I can't figure out what conditions trigger
it, since it complains rarely.

[jamesr, 2014-11-18 06:30:58]

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
File chrome/browser/search/suggestions/suggestions_source.cc (left):

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
chrome/browser/search/suggestions/suggestions_source.cc:119:
SuggestionsServiceFactory* suggestions_service_factory =
On 2014/11/18 06:23:17, Peter Kasting wrote:
> On 2014/11/18 03:33:31, sky wrote:
> > Does MSVC really complain about this style?
> 
> Yes :(
> 
> I think this is some kind of bug.  I can't figure out what conditions trigger
> it, since it complains rarely.

Have you filed a bug?  Transforms like this make the code less readable, imo,
especially if the expression is long enough to induce line wrapping where it
wouldn't exist otherwise.

[Peter Kasting, 2014-11-18 06:36:06]

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
File chrome/browser/search/suggestions/suggestions_source.cc (left):

https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
chrome/browser/search/suggestions/suggestions_source.cc:119:
SuggestionsServiceFactory* suggestions_service_factory =
On 2014/11/18 06:30:58, jamesr wrote:
> On 2014/11/18 06:23:17, Peter Kasting wrote:
> > On 2014/11/18 03:33:31, sky wrote:
> > > Does MSVC really complain about this style?
> > 
> > Yes :(
> > 
> > I think this is some kind of bug.  I can't figure out what conditions
trigger
> > it, since it complains rarely.
> 
> Have you filed a bug?  Transforms like this make the code less readable, imo,
> especially if the expression is long enough to induce line wrapping where it
> wouldn't exist otherwise.

I have not filed a bug, in part because, as I said, I can't figure out what
conditions trigger this, and in part because it affected a very small number of
cases across the entire codebase, and the transforms in question generally
improved readability rather than hindering it IMO (here, for example), so I
haven't had a lot of motivation to spend time trying to narrow things down.

[no sievers, 2014-11-18 19:27:27]

lgtm

https://codereview.chromium.org/731373002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/731373002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:673: // ?? Should
this use host instead of host_?
No I'm guessing GetHost() is just to check that the view is actually currently
attached and early-out if not. Maybe you'd hit the NOTIMPLEMENTED() below
otherwise.

[Vitaly Buka (NO REVIEWS), 2014-11-18 19:35:56]

https://codereview.chromium.org/731373002/diff/40001/cloud_print/service/win/...
File cloud_print/service/win/cloud_print_service.cc (right):

https://codereview.chromium.org/731373002/diff/40001/cloud_print/service/win/...
cloud_print/service/win/cloud_print_service.cc:343:
!service_state.FromString(contents)) {
please don't return error. that was just unused war.
it's OK if file is missing
code here just needs to get proxy_id if it's there from previous run, and
rewrite new settings.

[Peter Kasting, 2014-11-18 20:45:31]

https://codereview.chromium.org/731373002/diff/40001/cloud_print/service/win/...
File cloud_print/service/win/cloud_print_service.cc (right):

https://codereview.chromium.org/731373002/diff/40001/cloud_print/service/win/...
cloud_print/service/win/cloud_print_service.cc:343:
!service_state.FromString(contents)) {
On 2014/11/18 19:35:56, Vitaly Buka wrote:
> please don't return error. that was just unused war.
> it's OK if file is missing
> code here just needs to get proxy_id if it's there from previous run, and
> rewrite new settings.

OK, removed error handling.

https://codereview.chromium.org/731373002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/731373002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:673: // ?? Should
this use host instead of host_?
On 2014/11/18 19:27:27, sievers wrote:
> No I'm guessing GetHost() is just to check that the view is actually currently
> attached and early-out if not. Maybe you'd hit the NOTIMPLEMENTED() below
> otherwise.

OK.  Removed comment.

[Vitaly Buka (NO REVIEWS), 2014-11-19 01:15:13]

cloud_print/ lgtm

[Peter Kasting, 2014-11-19 01:53:00]

On 2014/11/18 06:30:58, jamesr wrote:
>
https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
> File chrome/browser/search/suggestions/suggestions_source.cc (left):
> 
>
https://codereview.chromium.org/731373002/diff/40001/chrome/browser/search/su...
> chrome/browser/search/suggestions/suggestions_source.cc:119:
> SuggestionsServiceFactory* suggestions_service_factory =
> On 2014/11/18 06:23:17, Peter Kasting wrote:
> > On 2014/11/18 03:33:31, sky wrote:
> > > Does MSVC really complain about this style?
> > 
> > Yes :(
> > 
> > I think this is some kind of bug.  I can't figure out what conditions
trigger
> > it, since it complains rarely.
> 
> Have you filed a bug?  Transforms like this make the code less readable, imo,
> especially if the expression is long enough to induce line wrapping where it
> wouldn't exist otherwise.

We figured this out offline.  It's actually triggered by invoking static methods
on a particular instance variable, i.e.:

class Test {
  static void Foo();
}

Test* t = ...;
t->Foo();  // If this is the only use of |t|, MSVC warns |t| is unused.

In other words, the warning is legit, and I was silencing it the wrong way. 
Instead of changing the above to "...->Foo();", I should have changed to
"Test::Foo();".  New patch uploaded that does precisely that.

[brettw, 2014-11-19 18:07:17]

build lgtm

[jamesr, 2014-11-20 20:27:48]

lgtm, but imo it'd be better to not enable this warning. it's too stupid.

[cpu_(ooo_6.6-7.5), 2014-11-20 22:19:13]

lgtm

[wolenetz, 2014-11-20 22:47:26]

media/ lgtm (see also my note, per chat:)

https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
media/audio/win/audio_low_latency_input_win.cc:246: DLOG(WARNING) << "Failed to
set new input master volume.";
On 2014/11/18 02:17:15, Peter Kasting wrote:
> Note: There was nothing wrong with the old code except that in release mode,
> |hr| appeared to be unused.  This is one possible fix; another is to add
> "ALLOW_UNUSED_LOCAL(hr);" instead in these places; still another would be to
do
> something akin to:
> 
> #if DLOG_IS_ON
>   HRESULT hr =
> #endif
>       ...;
>   DLOG_IF...
> 
> I thought this was the least ugly choice of the three.

Per chat, I would prefer instead DLOG_IF implicitly referenced the condition (on
MSVC), to prevent this warning on release mode, and to prevent DLOG_IF users
from explicitly referencing the condition. 

note: I'm fine with this immediate change followed later by a revert along with
a DLOG_IF implementation update to do the implicit referencing. (Also, please
include henrika@chromium.org on that revert CR, because I'm not sure that the
previous code really should proceed here to UpdateAgcVolume() if FAILED(hr).)

[Peter Kasting, 2014-11-20 22:49:42]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-11-20 22:50:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731373002/140001

[commit-bot: I haz the power, 2014-11-20 23:00:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 23:00:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-11-20 23:17:30]

Patchset 8 (id:??) landed as
https://crrev.com/be940e9e152e528fc567c4eeeeb5930710873f98
Cr-Commit-Position: refs/heads/master@{#305108}

[Peter Kasting, 2014-11-20 23:17:35]

Committed patchset #8 (id:140001) manually as
be940e9e152e528fc567c4eeeeb5930710873f98 (presubmit successful).

[wolenetz, 2014-11-20 23:33:37]

On 2014/11/20 22:47:26, wolenetz wrote:
> ...
>
https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
> File media/audio/win/audio_low_latency_input_win.cc (right):
> 
>
https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
> ...
> note: I'm fine with this immediate change followed later by a revert along
with
> a DLOG_IF implementation update to do the implicit referencing.

fyi - This subsequent revert + change of DLOG_IF behavior by pkasting@ is *not*
likely to occur, since DLOG_IF impl has prior comment that it does not reference
the condition in release mode (intentionally). This CL still l g t m.

[henrika (OOO until Aug 14), 2014-11-21 09:35:07]

henrika@chromium.org changed reviewers:
+ henrika@chromium.org

[henrika (OOO until Aug 14), 2014-11-21 09:35:09]

see comment inline

https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
media/audio/win/audio_low_latency_input_win.cc:246: DLOG(WARNING) << "Failed to
set new input master volume.";
Thanks for fixing Peter ;-)

wolenetz@: I agree that it would probably be "more correct" to avoid calling
UpdateAgcVolume()but it will actually not hurt to keep going either. If Set
fails, the Update call will do nothing and we are fine.

To keep things simple, I propose that we don't change the call-flow at this
stage if that is OK with you guys.

[wolenetz, 2014-11-21 18:29:23]

On 2014/11/21 09:35:09, henrika wrote:
> see comment inline
> 
>
https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
> File media/audio/win/audio_low_latency_input_win.cc (right):
> 
>
https://codereview.chromium.org/731373002/diff/20001/media/audio/win/audio_lo...
> media/audio/win/audio_low_latency_input_win.cc:246: DLOG(WARNING) << "Failed
to
> set new input master volume.";
> Thanks for fixing Peter ;-)
> 
> wolenetz@: I agree that it would probably be "more correct" to avoid calling
> UpdateAgcVolume()but it will actually not hurt to keep going either. If Set
> fails, the Update call will do nothing and we are fine.
> 
> To keep things simple, I propose that we don't change the call-flow at this
> stage if that is OK with you guys.

sgtm. Thanks,
Matt