Fix "unreachable code" warnings (MSVC warning 4702) in content/.

Fix "unreachable code" warnings (MSVC warning 4702) in content/.

BUG=346399
TEST=none
R=jam@chromium.org

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

[Peter Kasting, 2014-03-18 02:55:54]

https://codereview.chromium.org/202863004/diff/1/content/common/indexed_db/in...
File content/common/indexed_db/indexed_db_key.cc (right):

https://codereview.chromium.org/202863004/diff/1/content/common/indexed_db/in...
content/common/indexed_db/indexed_db_key.cc:124: return (array_.size() >
other.array_.size()) ? 1 : 0;
(Was trying for a consistent style in this switch -- "if (...) return -1; return
(...) ? 1 : 0;")

https://codereview.chromium.org/202863004/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/202863004/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.cc:465: return
capturers_.empty() ? NULL : capturers_.back();
(Discussed this with xians to ensure this was really the correct thing for this
code to be doing.)

[jam, 2014-03-18 23:55:19]

has there been consensus that we will turn on this warning?

[Peter Kasting, 2014-03-19 00:00:59]

On 2014/03/18 23:55:19, jam wrote:
> has there been consensus that we will turn on this warning?

It certainly seemed so from both the thread and from Scott/Nico/my conversations
about it.  Doing these fixes has caught a half-dozen real bugs so there's no
question in my mind that it's worth doing.

[jam, 2014-03-19 16:44:36]

On 2014/03/19 00:00:59, Peter Kasting wrote:
> On 2014/03/18 23:55:19, jam wrote:
> > has there been consensus that we will turn on this warning?
> 
> It certainly seemed so from both the thread and from Scott/Nico/my
conversations
> about it.  Doing these fixes has caught a half-dozen real bugs so there's no
> question in my mind that it's worth doing.

I see, it wasn't as obvious to me from the thread that we want to do that. Note
I don't care either way :) This change is fine on its own, so I'll let others
debate things when the flag is actually turned on.

lgtm with the comments

https://codereview.chromium.org/202863004/diff/20001/content/browser/gpu/comp...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/browser/gpu/comp...
content/browser/gpu/compositor_util.cc:210: #if defined(OS_WIN)
nit: take out the OS_WIN section now since USE_AURA == OS_WIN

https://codereview.chromium.org/202863004/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:322: 
nit: the style guide doesn't show blank lines between case statements, so leave
as is

https://codereview.chromium.org/202863004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_queue.cc:87: 
ditto

https://codereview.chromium.org/202863004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_queue.cc:97: return
!scrolling_in_progress_;
leave the code as before; I think it's more readable before as well. but
orthogonality, it seems that when making any changes to code, it's not good
practice to change code from one valid style to another valid style just because
one prefers a different style. otherwise we'd get a lot of hysteresis in the
code.

https://codereview.chromium.org/202863004/diff/20001/content/child/indexed_db...
File content/child/indexed_db/indexed_db_key_builders.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/child/indexed_db...
content/child/indexed_db/indexed_db_key_builders.cc:48: 
ditto

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_key.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_key.cc:124: return (array_.size() >
other.array_.size()) ? 1 : 0;
here, and in line 133 and 138, leave the code as is

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_key.cc:125: 
ditto

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_param_traits.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_param_traits.cc:39: 
ditto

[jsbell, 2014-03-19 16:52:05]

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_key.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_key.cc:124: return (array_.size() >
other.array_.size()) ? 1 : 0;
On 2014/03/19 16:44:36, jam wrote:
> here, and in line 133 and 138, leave the code as is

I wouldn't object to a follow-up patch that introduced a helper:

namespace {
template<typename T>
Compare(const T& a, const T& b) {
  if (a < b) return -1;
  return (b < a) ? 1 : 0;
}
}

[Peter Kasting, 2014-03-19 17:50:41]

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_key.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_key.cc:124: return (array_.size() >
other.array_.size()) ? 1 : 0;
On 2014/03/19 16:44:36, jam wrote:
> here, and in line 133 and 138, leave the code as is

133 and 138 nest ternaries, which has always been informally banned because
people don't find it readable.  (I know this because the ban was instituted
because I tried to do this a few times in the early days.)  The code on these
lines was fine, but if it's going to do the same thing as multiple other cases,
shouldn't it look the same as those other cases?

I'm fine with doing it as a separate change, but I think this really should be
cleaned up.  Do you like jsbell's helper solution?  To me it's kind of overkill
and I'd prefer doing things as I did them here, but I'd like to know what you
prefer since you own the code.

[Peter Kasting, 2014-03-19 21:10:56]

https://codereview.chromium.org/202863004/diff/20001/content/browser/gpu/comp...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/browser/gpu/comp...
content/browser/gpu/compositor_util.cc:210: #if defined(OS_WIN)
On 2014/03/19 16:44:36, jam wrote:
> nit: take out the OS_WIN section now since USE_AURA == OS_WIN

Done.

https://codereview.chromium.org/202863004/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:322: 
On 2014/03/19 16:44:36, jam wrote:
> nit: the style guide doesn't show blank lines between case statements, so
leave
> as is

Done.

https://codereview.chromium.org/202863004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_queue.cc:87: 
On 2014/03/19 16:44:36, jam wrote:
> ditto

Done.

https://codereview.chromium.org/202863004/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_queue.cc:97: return
!scrolling_in_progress_;
On 2014/03/19 16:44:36, jam wrote:
> leave the code as before; I think it's more readable before as well. but
> orthogonality, it seems that when making any changes to code, it's not good
> practice to change code from one valid style to another valid style just
because
> one prefers a different style. otherwise we'd get a lot of hysteresis in the
> code.

Done.  Sorry, I didn't view this as a "style change" (the way the newlines
between cases are), just thought there was an opportunity to make the code
simpler.

https://codereview.chromium.org/202863004/diff/20001/content/child/indexed_db...
File content/child/indexed_db/indexed_db_key_builders.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/child/indexed_db...
content/child/indexed_db/indexed_db_key_builders.cc:48: 
On 2014/03/19 16:44:36, jam wrote:
> ditto

Done.

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_key.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_key.cc:124: return (array_.size() >
other.array_.size()) ? 1 : 0;
On 2014/03/19 17:50:42, Peter Kasting wrote:
> On 2014/03/19 16:44:36, jam wrote:
> > here, and in line 133 and 138, leave the code as is
> 
> 133 and 138 nest ternaries, which has always been informally banned because
> people don't find it readable.  (I know this because the ban was instituted
> because I tried to do this a few times in the early days.)  The code on these
> lines was fine, but if it's going to do the same thing as multiple other
cases,
> shouldn't it look the same as those other cases?
> 
> I'm fine with doing it as a separate change, but I think this really should be
> cleaned up.  Do you like jsbell's helper solution?  To me it's kind of
overkill
> and I'd prefer doing things as I did them here, but I'd like to know what you
> prefer since you own the code.

(Splitting this to a separate CL so this CL doesn't make changes in this area)

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_key.cc:125: 
On 2014/03/19 16:44:36, jam wrote:
> ditto

Done.

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
File content/common/indexed_db/indexed_db_param_traits.cc (right):

https://codereview.chromium.org/202863004/diff/20001/content/common/indexed_d...
content/common/indexed_db/indexed_db_param_traits.cc:39: 
On 2014/03/19 16:44:36, jam wrote:
> ditto

Done.

[Peter Kasting, 2014-03-19 21:16:02]

Committed patchset #3 manually as r258096 (presubmit successful).