Followup to https://codereview.chromium.org/2902753003/

Followup to https://codereview.chromium.org/2902753003/

BUG=720058
Review-Url: https://codereview.chromium.org/2917433002
Cr-Commit-Position: refs/heads/master@{#475804}
Committed: https://chromium.googlesource.com/chromium/src/+/47cd98bbaa7eb5ffffee7469182446f5e2548b12

[robliao, 2017-05-30 20:00:15]

robliao@chromium.org changed reviewers:
+ fdoray@chromium.org, gab@chromium.org

[gab, 2017-05-30 21:27:38]

lgtm % discussion below

On 2017/05/30 20:01:06, robliao (OOO 25 and 26) wrote:
> On 2017/05/25 18:47:00, gab wrote:
> > ** is more common and less surprising than *& IMO, especially when non-const
> (or
> > maybe it should be const? didn't check usage yet)
> 
> I wanted to enforce the non-nullable property for
> SchedulerWorker*& worker =
>       thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED
>           ? dedicated_worker
>           : GetSharedSchedulerWorkerForTraits<DelegateType>(traits);
> 
> This was the main way to make it work with a template.

"enforce the non-nullable property" ? I don't see how this is different from
passing something by pointer in many other cases?

[robliao, 2017-05-30 21:28:36]

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

[commit-bot: I haz the power, 2017-05-30 21:29:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2017-05-30 21:30:54]

On 2017/05/30 21:27:38, gab wrote:
> lgtm % discussion below
> 
> On 2017/05/30 20:01:06, robliao (OOO 25 and 26) wrote:
> > On 2017/05/25 18:47:00, gab wrote:
> > > ** is more common and less surprising than *& IMO, especially when
non-const
> > (or
> > > maybe it should be const? didn't check usage yet)
> > 
> > I wanted to enforce the non-nullable property for
> > SchedulerWorker*& worker =
> >       thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED
> >           ? dedicated_worker
> >           : GetSharedSchedulerWorkerForTraits<DelegateType>(traits);
> > 
> > This was the main way to make it work with a template.
> 
> "enforce the non-nullable property" ? I don't see how this is different from
> passing something by pointer in many other cases?

With SchedulerWorker**, the variable is permitted to be nullptr. With
SchedulerWorker*&, you must assign a non-null reference to it. It guarantees
there will be something to receive the SchedulerWorker*.

This is similar to the difference between my_func(Foo& baz) and my_func(Foo*
baz). One version can receive a nullptr. The other version isn't without some
recasting fun.

[gab, 2017-05-30 21:35:27]

On 2017/05/30 21:30:54, robliao (OOO 25 and 26) wrote:
> On 2017/05/30 21:27:38, gab wrote:
> > lgtm % discussion below
> > 
> > On 2017/05/30 20:01:06, robliao (OOO 25 and 26) wrote:
> > > On 2017/05/25 18:47:00, gab wrote:
> > > > ** is more common and less surprising than *& IMO, especially when
> non-const
> > > (or
> > > > maybe it should be const? didn't check usage yet)
> > > 
> > > I wanted to enforce the non-nullable property for
> > > SchedulerWorker*& worker =
> > >       thread_mode == SingleThreadTaskRunnerThreadMode::DEDICATED
> > >           ? dedicated_worker
> > >           : GetSharedSchedulerWorkerForTraits<DelegateType>(traits);
> > > 
> > > This was the main way to make it work with a template.
> > 
> > "enforce the non-nullable property" ? I don't see how this is different from
> > passing something by pointer in many other cases?
> 
> With SchedulerWorker**, the variable is permitted to be nullptr. With
> SchedulerWorker*&, you must assign a non-null reference to it. It guarantees
> there will be something to receive the SchedulerWorker*.
> 
> This is similar to the difference between my_func(Foo& baz) and my_func(Foo*
> baz). One version can receive a nullptr. The other version isn't without some
> recasting fun.

OKay sure, I don't care strongly :), lgtm

[commit-bot: I haz the power, 2017-05-30 22:59:18]

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

[commit-bot: I haz the power, 2017-05-30 22:59:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[robliao, 2017-05-31 01:45:45]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2017-05-31 01:46:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 04:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496195145883820, "parent_rev":
"528541268e541b20e632380fd22aba8aeac53521", "commit_rev":
"47cd98bbaa7eb5ffffee7469182446f5e2548b12"}

[commit-bot: I haz the power, 2017-05-31 04:38:03]

Description was changed from

==========
Followup to https://codereview.chromium.org/2902753003/

BUG=720058
==========

to

==========
Followup to https://codereview.chromium.org/2902753003/

BUG=720058

Review-Url: https://codereview.chromium.org/2917433002
Cr-Commit-Position: refs/heads/master@{#475804}
Committed:
https://chromium.googlesource.com/chromium/src/+/47cd98bbaa7eb5ffffee74691824...
==========

[commit-bot: I haz the power, 2017-05-31 04:38:04]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/47cd98bbaa7eb5ffffee74691824...