DNS: Support reprioritization of requests.

DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
Committed: https://crrev.com/c53b19a77794aec3c8d10ebd6552e7523d360a0d
Cr-Commit-Position: refs/heads/master@{#391797}

[Julia Tuttle, 2016-04-27 20:57:23]

juliatuttle@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Julia Tuttle, 2016-04-27 20:57:24]

PTAL, rdsmith.

[Julia Tuttle, 2016-04-27 20:58:14]

FYI, akalin. (Review if you want; CCing you because it was your TODO.)

[Julia Tuttle, 2016-04-27 20:58:48]

Description was changed from

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver classes hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

to

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

[Randy Smith (Not in Mondays), 2016-04-28 18:04:40]

Initial comments, but this looks basically fine.  Thanks!!

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver.cc#ne...
net/dns/host_resolver.cc:108: NOTIMPLEMENTED();
Why NOTIMPLEMENTED() rather than =0 in the declaration?

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
net/dns/host_resolver_impl_unittest.cc:1206: // run.
I'm failing to understand the test infrastructure despite 5-10m of poking.  Why
does req0 start immediately?  All the implementations of CreateRequest I could
find just create objects and queue things.

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo.cc
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo....
net/dns/host_resolver_mojo.cc:97: // Do nothing, since Resolve() discarded the
priority anyway.
Oh, for goodness sake.  What uses the DNS resolver through the mojo interface? 
(This is not a blocking comment on this CL, I'm just a bit miffed at mojoization
tossing priority.)

[Julia Tuttle, 2016-05-04 14:43:20]

PTAL.

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver.cc#ne...
net/dns/host_resolver.cc:108: NOTIMPLEMENTED();
On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> Why NOTIMPLEMENTED() rather than =0 in the declaration?

Didn't want to update the dozen unittest mocks that are just going to
NOTIMPLEMENTED() or {} it themselves.

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
net/dns/host_resolver_impl_unittest.cc:1206: // run.
On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> I'm failing to understand the test infrastructure despite 5-10m of poking. 
Why
> does req0 start immediately?  All the implementations of CreateRequest I could
> find just create objects and queue things.

They queue things, but a serial resolver is willing to run one request at a
time, so the first request will start and the rest will be queued. (There's no
reason *for* it to wait, so it starts immediately.)

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo.cc
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo....
net/dns/host_resolver_mojo.cc:97: // Do nothing, since Resolve() discarded the
priority anyway.
On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> Oh, for goodness sake.  What uses the DNS resolver through the mojo interface?

> (This is not a blocking comment on this CL, I'm just a bit miffed at
mojoization
> tossing priority.)

No idea!

[mmenke, 2016-05-04 17:33:15]

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

[mmenke, 2016-05-04 17:33:16]

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo.cc
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo....
net/dns/host_resolver_mojo.cc:97: // Do nothing, since Resolve() discarded the
priority anyway.
On 2016/05/04 14:43:20, Julia Tuttle wrote:
> On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> > Oh, for goodness sake.  What uses the DNS resolver through the mojo
interface?
> 
> > (This is not a blocking comment on this CL, I'm just a bit miffed at
> mojoization
> > tossing priority.)
> 
> No idea!

The out of process PAC stuff, I would assume.  I believe the PAC code has always
tossed priority, so this most likely inherited the behavior from that.

Could be completely wrong about all that, though.

[mmenke, 2016-05-04 17:33:25]

Description was changed from

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

to

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

[mmenke, 2016-05-04 17:33:25]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[Randy Smith (Not in Mondays), 2016-05-05 00:07:25]

LGTM; thanks again!  (Comments below up to you.)

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
net/dns/host_resolver_impl_unittest.cc:1206: // run.
On 2016/05/04 14:43:20, Julia Tuttle wrote:
> On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> > I'm failing to understand the test infrastructure despite 5-10m of poking. 
> Why
> > does req0 start immediately?  All the implementations of CreateRequest I
could
> > find just create objects and queue things.
> 
> They queue things, but a serial resolver is willing to run one request at a
> time, so the first request will start and the rest will be queued. (There's no
> reason *for* it to wait, so it starts immediately.)

I think I get it.  But wouldn't this comment be more appropriate down next to
the first Resolve call below?  Isn't that when the first request actually
starts?

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo.cc
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo....
net/dns/host_resolver_mojo.cc:97: // Do nothing, since Resolve() discarded the
priority anyway.
On 2016/05/04 14:43:20, Julia Tuttle wrote:
> On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> > Oh, for goodness sake.  What uses the DNS resolver through the mojo
interface?
> 
> > (This is not a blocking comment on this CL, I'm just a bit miffed at
> mojoization
> > tossing priority.)
> 
> No idea!

Matt's probably right.  Willing to put a TODO in the Mojo code near the
declaration of Resolve() to *not* drop priority on the floor?  If it's not
needed for the current use case, that's fine, but I don't want someone else
starting to rely on that implementation and not realize they don't get priority.

[Julia Tuttle, 2016-05-05 12:47:56]

Thanks, Randy!

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_impl_...
net/dns/host_resolver_impl_unittest.cc:1206: // run.
On 2016/05/05 00:07:25, Randy Smith - Not in Fridays wrote:
> On 2016/05/04 14:43:20, Julia Tuttle wrote:
> > On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> > > I'm failing to understand the test infrastructure despite 5-10m of poking.

> > Why
> > > does req0 start immediately?  All the implementations of CreateRequest I
> could
> > > find just create objects and queue things.
> > 
> > They queue things, but a serial resolver is willing to run one request at a
> > time, so the first request will start and the rest will be queued. (There's
no
> > reason *for* it to wait, so it starts immediately.)
> 
> I think I get it.  But wouldn't this comment be more appropriate down next to
> the first Resolve call below?  Isn't that when the first request actually
> starts?

Done.

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo.cc
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo....
net/dns/host_resolver_mojo.cc:97: // Do nothing, since Resolve() discarded the
priority anyway.
On 2016/05/05 00:07:25, Randy Smith - Not in Fridays wrote:
> On 2016/05/04 14:43:20, Julia Tuttle wrote:
> > On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> > > Oh, for goodness sake.  What uses the DNS resolver through the mojo
> interface?
> > 
> > > (This is not a blocking comment on this CL, I'm just a bit miffed at
> > mojoization
> > > tossing priority.)
> > 
> > No idea!
> 
> Matt's probably right.  Willing to put a TODO in the Mojo code near the
> declaration of Resolve() to *not* drop priority on the floor?  If it's not
> needed for the current use case, that's fine, but I don't want someone else
> starting to rely on that implementation and not realize they don't get
priority.

I put a note in the .h instead, where I think a user might be more likely to see
it.

[Julia Tuttle, 2016-05-05 12:48:04]

The CQ bit was checked by juliatuttle@chromium.org

[Julia Tuttle, 2016-05-05 12:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1928743003/#ps20001
(title: "Make a couple of comment changes")

[commit-bot: I haz the power, 2016-05-05 12:48:21]

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

[mmenke, 2016-05-05 12:51:11]

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

[mmenke, 2016-05-05 12:51:12]

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo.cc
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/1928743003/diff/1/net/dns/host_resolver_mojo....
net/dns/host_resolver_mojo.cc:97: // Do nothing, since Resolve() discarded the
priority anyway.
On 2016/05/05 00:07:25, Randy Smith - Not in Fridays wrote:
> On 2016/05/04 14:43:20, Julia Tuttle wrote:
> > On 2016/04/28 18:04:40, Randy Smith - Not in Fridays wrote:
> > > Oh, for goodness sake.  What uses the DNS resolver through the mojo
> interface?
> > 
> > > (This is not a blocking comment on this CL, I'm just a bit miffed at
> > mojoization
> > > tossing priority.)
> > 
> > No idea!
> 
> Matt's probably right.  Willing to put a TODO in the Mojo code near the
> declaration of Resolve() to *not* drop priority on the floor?  If it's not
> needed for the current use case, that's fine, but I don't want someone else
> starting to rely on that implementation and not realize they don't get
priority.

I think there's actually a bug filed against the proxy to correctly send the
priorities, too...Anyhow, agree that this interface should support it, too.

[commit-bot: I haz the power, 2016-05-05 13:51:40]

Description was changed from

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

to

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

[commit-bot: I haz the power, 2016-05-05 13:51:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-05 13:52:55]

Description was changed from

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296
==========

to

==========
DNS: Support reprioritization of requests.

Add HostResolver::ChangeRequestPriority, and make HostResolverImpl
update the priority of a request when it's called.

Also add an empty stub to HostResolverMojo, since it appears to ignore
priorities at the moment.

Let other HostResolver subclasses hit a NOTIMPLEMENTED(), since it's a
brand new method nobody's using in tests.

BUG=607296

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

[commit-bot: I haz the power, 2016-05-05 13:52:56]

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