Migrate WaitableEvent to enum-based constructor in chrome/

Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
Committed: https://crrev.com/d955d78dcaa5815b958f3baf078c7da16006d896
Cr-Commit-Position: refs/heads/master@{#397920}

[gab, 2016-06-01 18:59:05]

Description was changed from

==========
Migrate WaitableEvent to enum-based constructor in chrome/

BUG=612843
R=thakis@chromium.org
==========

to

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
==========

[gab, 2016-06-02 12:46:29]

Description was changed from

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
==========

to

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

Also fixed presubmit for static string usage in thread_watcher_unittest.cc

BUG=612843
==========

[gab, 2016-06-02 13:03:23]

Description was changed from

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

Also fixed presubmit for static string usage in thread_watcher_unittest.cc

BUG=612843
==========

to

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
==========

[gab, 2016-06-02 13:33:34]

@Nico when you have a chance, this is the last component pending review :-)

[gab, 2016-06-02 16:09:43]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 16:10:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021393004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2021393004/20001

[commit-bot: I haz the power, 2016-06-02 16:14:03]

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

[commit-bot: I haz the power, 2016-06-02 16:14:04]

Dry run: This issue passed the CQ dry run.

[gab, 2016-06-02 17:28:13]

Just did a manual pass of every automated change in this CL and it lgtm, CQ is
also happy @ Nico for rubberstamp :-)

[Nico, 2016-06-04 01:48:18]

lgtm

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:798: base::WaitableEvent::ResetPolicy::MANUAL,
if this must be an enum class i feel it shouldn't have been a nested one. then
these would be half as verbose.

[gab, 2016-06-04 11:45:20]

Thanks, reply below

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:798: base::WaitableEvent::ResetPolicy::MANUAL,
On 2016/06/04 01:48:18, Nico wrote:
> if this must be an enum class i feel it shouldn't have been a nested one. then
> these would be half as verbose.

We thought about that [1] but decided to go this route because every namespace
you remove you have to makeup for with a more specific enum name, i.e.
base::ResetPolicy is too generic and say base::WaitableEventResetPolicy is
merely saving a ::.

[1] https://crbug.com/612843#c11

[gab, 2016-06-04 11:45:58]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-06-04 11:46:10]

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

[commit-bot: I haz the power, 2016-06-04 13:15:52]

Description was changed from

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
==========

to

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
==========

[commit-bot: I haz the power, 2016-06-04 13:15:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-04 13:17:09]

Description was changed from

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843
==========

to

==========
Migrate WaitableEvent to enum-based constructor in chrome/

Change automated with clang-tidy (details @ https://crbug.com/612843#c13)

BUG=612843

Committed: https://crrev.com/d955d78dcaa5815b958f3baf078c7da16006d896
Cr-Commit-Position: refs/heads/master@{#397920}
==========

[commit-bot: I haz the power, 2016-06-04 13:17:11]

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

[Nico, 2016-06-04 16:33:56]

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:798: base::WaitableEvent::ResetPolicy::MANUAL,
On 2016/06/04 11:45:20, gab wrote:
> On 2016/06/04 01:48:18, Nico wrote:
> > if this must be an enum class i feel it shouldn't have been a nested one.
then
> > these would be half as verbose.
> 
> We thought about that [1] but decided to go this route because every namespace
> you remove you have to makeup for with a more specific enum name, i.e.
> base::ResetPolicy is too generic and say base::WaitableEventResetPolicy is
> merely saving a ::.

I submit that if every step along the way seems to have made sense but the end
state is base::WaitableEvent::InitialState::NOT_SIGNALED, then one of the steps
along the way wasn't the right step, even if it felt right at the time ;-)

[gab, 2016-06-04 19:03:23]

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/2021393004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:798: base::WaitableEvent::ResetPolicy::MANUAL,
On 2016/06/04 16:33:56, Nico wrote:
> On 2016/06/04 11:45:20, gab wrote:
> > On 2016/06/04 01:48:18, Nico wrote:
> > > if this must be an enum class i feel it shouldn't have been a nested one.
> then
> > > these would be half as verbose.
> > 
> > We thought about that [1] but decided to go this route because every
namespace
> > you remove you have to makeup for with a more specific enum name, i.e.
> > base::ResetPolicy is too generic and say base::WaitableEventResetPolicy is
> > merely saving a ::.
> 
> I submit that if every step along the way seems to have made sense but the end
> state is base::WaitableEvent::InitialState::NOT_SIGNALED, then one of the
steps
> along the way wasn't the right step, even if it felt right at the time ;-)

Hmm, do you have a specific counterproposal? I'm happy to change it if you have
something we all agree is better.