Introduce an intra-priority level sorting value - Chromium Side

This CL adds an intra-priority level sorting value for finer grained control of resource loading. The blink side of this patch sorts visible images by estimated on screen real-estate, preferring to download images with more visual impact first.  Ie. On a page with a large banner, and dozens of small icons, the large banner will most likely be scheduled ahead of the others. 

In the future, we can also use this to experiment with other signals, ie. prioritize by distance to viewport.

This needs https://codereview.chromium.org/153983004/ to land first.

In our tests, this was a nice win on speed-index for desktop, with a gain of about ~2-3%.

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

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

[James Simonsen, 2014-02-07 03:45:09]

Have you considered how you're going to land this? You'll probably need three
check-ins.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:31: typedef
std::multiset<ScheduledResourceRequest*, ScheduledResourceSorter>
Is multiset guaranteed to be FIFO for equivalent elements? I don't even know
where to go to look.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:113: int intraPriorityValue() const
{ return intra_priority_value_; }
For simple accessors, you're supposed to use intra_priority_value() style.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:149: net::RequestPriority
priority_;
This is unused.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:337: DCHECK(new_priority !=
old_priority ||
willchan will complain about not having operator precedence memorized when he
sees this. You might want to add () to make it explicit.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:373: for
(RequestQueue::NetQueue::iterator request_iter =
Is this what clang format comes up with? I'm not really sure what I'd do. Maybe
move the declaration out of the for() so it flows more nicely?

[shatch, 2014-02-10 19:55:47]

New snapshot uploaded, ptal.

To land it, there's an old post by Darin on chromium-dev, following that
pattern:

"
1) Add the new WebKit interface, preserving the old.
2) Change Chrome to use the new interface.
3) Modify WebKit to remove the old interface.
"

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:31: typedef
std::multiset<ScheduledResourceRequest*, ScheduledResourceSorter>
On 2014/02/07 03:45:09, James Simonsen wrote:
> Is multiset guaranteed to be FIFO for equivalent elements? I don't even know
> where to go to look.

Hmm, searching for this it looks like it depends on the implementation until
c++11, so I added a fifo_ordering counter to enforce it.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:113: int intraPriorityValue() const
{ return intra_priority_value_; }
On 2014/02/07 03:45:09, James Simonsen wrote:
> For simple accessors, you're supposed to use intra_priority_value() style.

Done.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:149: net::RequestPriority
priority_;
On 2014/02/07 03:45:09, James Simonsen wrote:
> This is unused.

Done.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:337: DCHECK(new_priority !=
old_priority ||
On 2014/02/07 03:45:09, James Simonsen wrote:
> willchan will complain about not having operator precedence memorized when he
> sees this. You might want to add () to make it explicit.

Done.

https://codereview.chromium.org/146333004/diff/140001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:373: for
(RequestQueue::NetQueue::iterator request_iter =
On 2014/02/07 03:45:09, James Simonsen wrote:
> Is this what clang format comes up with? I'm not really sure what I'd do.
Maybe
> move the declaration out of the for() so it flows more nicely?

Done.

[James Simonsen, 2014-02-10 20:46:42]

lgtm

https://codereview.chromium.org/146333004/diff/330001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/330001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:398: for (;request_iter !=
client->pending_requests.End();) {
while()?

[shatch, 2014-02-21 21:42:34]

ptal

Should be mostly just getting the intra priority value from blink to the
resource scheduler.

content/
webkit/

darin


content/common/resource_messages.h

cevans

[Chris Evans, 2014-02-21 22:42:30]

resource_messages.h LGTM (with a question but I think we're ok)

https://codereview.chromium.org/146333004/diff/380001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/380001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:362: (old_intra_priority !=
new_intra_priority_value));
Note that a compromised renderer could cause the condition represented by this
DCHECK pretty easily.

I'm assuming that we handle it gracefully and simply have the DCHECK to detect
for a buggy renderer?

[shatch, 2014-02-25 18:48:33]

https://codereview.chromium.org/146333004/diff/380001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/380001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:362: (old_intra_priority !=
new_intra_priority_value));
On 2014/02/21 22:42:30, Chris Evans wrote:
> Note that a compromised renderer could cause the condition represented by this
> DCHECK pretty easily.
> 
> I'm assuming that we handle it gracefully and simply have the DCHECK to detect
> for a buggy renderer?

Yeah it's just to check that we're not spamming the same value over and over
from blink. If the same value came through, the request would just be requeued,
worst I can think of is that the resource's fifo order changes. I added a check
for that, but I'm not sure it matters either way.

[shatch, 2014-02-27 19:40:57]

ping darin

On 2014/02/25 18:48:33, shatch wrote:
>
https://codereview.chromium.org/146333004/diff/380001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/146333004/diff/380001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:362: (old_intra_priority !=
> new_intra_priority_value));
> On 2014/02/21 22:42:30, Chris Evans wrote:
> > Note that a compromised renderer could cause the condition represented by
this
> > DCHECK pretty easily.
> > 
> > I'm assuming that we handle it gracefully and simply have the DCHECK to
detect
> > for a buggy renderer?
> 
> Yeah it's just to check that we're not spamming the same value over and over
> from blink. If the same value came through, the request would just be
requeued,
> worst I can think of is that the resource's fifo order changes. I added a
check
> for that, but I'm not sure it matters either way.

[darin (slow to review), 2014-02-28 05:14:11]

Apologies for the delay in providing review feedback. In general, this approach
seems rational, but I'm concerned only about how we represent priority. It seems
we would be better off having a single type to represent priority as very few
parts of the code need to see a distinction between major and minor priority
values.

https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:173: if (a->priority() !=
b->priority())
it seems like you want to treat priority as a tuple with two values: major and
minor. that suggests that RequestPriority should become a struct with major and
minor fields, no?

since the network stack only needs to the major field, perhaps this implies that
net::RequestPriority shouldn't change but rather that we should invent a new
content level struct to represent this new priority representation.

https://codereview.chromium.org/146333004/diff/440001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/146333004/diff/440001/content/common/resource...
content/common/resource_messages.h:329: net::RequestPriority,
it is confusing to have two priority fields. this intra_priority_value needs to
be defined somewhere.

[shatch, 2014-02-28 20:40:52]

On 2014/02/28 05:14:11, darin wrote:
> Apologies for the delay in providing review feedback. In general, this
approach
> seems rational, but I'm concerned only about how we represent priority. It
seems
> we would be better off having a single type to represent priority as very few
> parts of the code need to see a distinction between major and minor priority
> values.
> 
>
https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:173: if (a->priority() !=
> b->priority())
> it seems like you want to treat priority as a tuple with two values: major and
> minor. that suggests that RequestPriority should become a struct with major
and
> minor fields, no?
> 
> since the network stack only needs to the major field, perhaps this implies
that
> net::RequestPriority shouldn't change but rather that we should invent a new
> content level struct to represent this new priority representation.
> 

Sure, a single type sounds nice and clean. I'll wrap the values up as a struct.

>
https://codereview.chromium.org/146333004/diff/440001/content/common/resource...
> File content/common/resource_messages.h (right):
> 
>
https://codereview.chromium.org/146333004/diff/440001/content/common/resource...
> content/common/resource_messages.h:329: net::RequestPriority,
> it is confusing to have two priority fields. this intra_priority_value needs
to
> be defined somewhere.

Sorry, not totally getting this suggestion. Could you please elaborate? The
message could be modified to just take a single priority field, the new tuple
struct (as suggested above).

[darin (slow to review), 2014-02-28 21:17:23]

On Fri, Feb 28, 2014 at 12:40 PM, <simonhatch@chromium.org> wrote:

> On 2014/02/28 05:14:11, darin wrote:
>
>> Apologies for the delay in providing review feedback. In general, this
>>
> approach
>
>> seems rational, but I'm concerned only about how we represent priority. It
>>
> seems
>
>> we would be better off having a single type to represent priority as very
>> few
>> parts of the code need to see a distinction between major and minor
>> priority
>> values.
>>
>
>
> https://codereview.chromium.org/146333004/diff/440001/
> content/browser/loader/resource_scheduler.cc
>
>> File content/browser/loader/resource_scheduler.cc (right):
>>
>
>
> https://codereview.chromium.org/146333004/diff/440001/
> content/browser/loader/resource_scheduler.cc#newcode173
>
>> content/browser/loader/resource_scheduler.cc:173: if (a->priority() !=
>> b->priority())
>> it seems like you want to treat priority as a tuple with two values:
>> major and
>> minor. that suggests that RequestPriority should become a struct with
>> major
>>
> and
>
>> minor fields, no?
>>
>
>  since the network stack only needs to the major field, perhaps this
>> implies
>>
> that
>
>> net::RequestPriority shouldn't change but rather that we should invent a
>> new
>> content level struct to represent this new priority representation.
>>
>
>
> Sure, a single type sounds nice and clean. I'll wrap the values up as a
> struct.
>
>
>
> https://codereview.chromium.org/146333004/diff/440001/
> content/common/resource_messages.h
>
>> File content/common/resource_messages.h (right):
>>
>
>
> https://codereview.chromium.org/146333004/diff/440001/
> content/common/resource_messages.h#newcode329
>
>> content/common/resource_messages.h:329: net::RequestPriority,
>> it is confusing to have two priority fields. this intra_priority_value
>> needs
>>
> to
>
>> be defined somewhere.
>>
>
> Sorry, not totally getting this suggestion. Could you please elaborate? The
> message could be modified to just take a single priority field, the new
> tuple
> struct (as suggested above).
>

Sorry, you can ignore this comment. I wrote it prior to suggesting the
single priority struct idea.

-Darin


>
> https://codereview.chromium.org/146333004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shatch, 2014-03-03 18:57:15]

New snapshot uploaded, ptal.

https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:173: if (a->priority() !=
b->priority())
On 2014/02/28 05:14:12, darin wrote:
> it seems like you want to treat priority as a tuple with two values: major and
> minor. that suggests that RequestPriority should become a struct with major
and
> minor fields, no?
> 
> since the network stack only needs to the major field, perhaps this implies
that
> net::RequestPriority shouldn't change but rather that we should invent a new
> content level struct to represent this new priority representation.

Done.

[shatch, 2014-03-10 18:25:58]

ping darin ptal

On 2014/03/03 18:57:15, shatch wrote:
> New snapshot uploaded, ptal.
> 
>
https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
> File content/browser/loader/resource_scheduler.cc (right):
> 
>
https://codereview.chromium.org/146333004/diff/440001/content/browser/loader/...
> content/browser/loader/resource_scheduler.cc:173: if (a->priority() !=
> b->priority())
> On 2014/02/28 05:14:12, darin wrote:
> > it seems like you want to treat priority as a tuple with two values: major
and
> > minor. that suggests that RequestPriority should become a struct with major
> and
> > minor fields, no?
> > 
> > since the network stack only needs to the major field, perhaps this implies
> that
> > net::RequestPriority shouldn't change but rather that we should invent a new
> > content level struct to represent this new priority representation.
> 
> Done.

[darin (slow to review), 2014-03-24 18:23:33]

LGTM w/ nits fixed.

I think it would be worthwhile to invent a combined priority struct for use
throughout content, so that you don't have to pass around net::RequestPriority
and another int. You can just pass a single struct around whereever you need to
pass a "request priority".

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:32: intra_priority(0)
nit: opening "{" on previous line

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:47: return (priority !=
other.priority) ||
nit: Implement in terms of operator== ?  e.g.: return !(*this == other);

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:51: bool GetSortingValue(const
RequestPriorityParams& other) const {
nit: maybe call this function GreaterThan (or invert and call it LessThan)? I
don't think it is uncommon to invent functions like this and call them LessThan.

[shatch, 2014-03-24 21:08:44]

Thanks, will follow up with struct suggestion once this lands.

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:32: intra_priority(0)
On 2014/03/24 18:23:33, darin wrote:
> nit: opening "{" on previous line

Done.

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:47: return (priority !=
other.priority) ||
On 2014/03/24 18:23:33, darin wrote:
> nit: Implement in terms of operator== ?  e.g.: return !(*this == other);

Done.

https://codereview.chromium.org/146333004/diff/460002/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:51: bool GetSortingValue(const
RequestPriorityParams& other) const {
On 2014/03/24 18:23:33, darin wrote:
> nit: maybe call this function GreaterThan (or invert and call it LessThan)? I
> don't think it is uncommon to invent functions like this and call them
LessThan.

Done.

[shatch, 2014-03-28 16:35:12]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:35:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/146333004/490001

[commit-bot: I haz the power, 2014-03-28 16:36:26]

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

[commit-bot: I haz the power, 2014-03-28 16:36:27]

Failed to apply patch for webkit/child/weburlloader_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: webkit/child/weburlloader_impl.cc
  |diff --git a/webkit/child/weburlloader_impl.cc
b/webkit/child/weburlloader_impl.cc
  |index
08857219febad376f6cc332f665f9fcf1a6d9711..e81466f94c22b0af52f276cdcf02cf8ab9786ff7
100644
  |--- a/webkit/child/weburlloader_impl.cc
  |+++ b/webkit/child/weburlloader_impl.cc
  --------------------------
  No file to patch.  Skipping patch.
  3 out of 3 hunks ignored

Patch:       webkit/child/weburlloader_impl.cc
Index: webkit/child/weburlloader_impl.cc
diff --git a/webkit/child/weburlloader_impl.cc
b/webkit/child/weburlloader_impl.cc
index
08857219febad376f6cc332f665f9fcf1a6d9711..e81466f94c22b0af52f276cdcf02cf8ab9786ff7
100644
--- a/webkit/child/weburlloader_impl.cc
+++ b/webkit/child/weburlloader_impl.cc
@@ -223,7 +223,8 @@ class WebURLLoaderImpl::Context : public
base::RefCounted<Context>,
 
   void Cancel();
   void SetDefersLoading(bool value);
-  void DidChangePriority(WebURLRequest::Priority new_priority);
+  void DidChangePriority(WebURLRequest::Priority new_priority,
+                         int intra_priority_value);
   void Start(
       const WebURLRequest& request,
       ResourceLoaderBridge::SyncLoadResponse* sync_load_response,
@@ -294,10 +295,10 @@ void WebURLLoaderImpl::Context::SetDefersLoading(bool
value) {
 }
 
 void WebURLLoaderImpl::Context::DidChangePriority(
-    WebURLRequest::Priority new_priority) {
+    WebURLRequest::Priority new_priority, int intra_priority_value) {
   if (bridge_)
     bridge_->DidChangePriority(
-        ConvertWebKitPriorityToNetPriority(new_priority));
+        ConvertWebKitPriorityToNetPriority(new_priority),
intra_priority_value);
 }
 
 void WebURLLoaderImpl::Context::Start(
@@ -852,8 +853,9 @@ void WebURLLoaderImpl::setDefersLoading(bool value) {
   context_->SetDefersLoading(value);
 }
 
-void WebURLLoaderImpl::didChangePriority(WebURLRequest::Priority new_priority)
{
-  context_->DidChangePriority(new_priority);
+void WebURLLoaderImpl::didChangePriority(WebURLRequest::Priority new_priority,
+                                         int intra_priority_value) {
+  context_->DidChangePriority(new_priority, intra_priority_value);
 }
 
 }  // namespace webkit_glue

[shatch, 2014-03-31 15:12:48]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-03-31 15:12:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/146333004/510001

[commit-bot: I haz the power, 2014-03-31 19:56:52]

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

[commit-bot: I haz the power, 2014-03-31 19:56:53]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[shatch, 2014-03-31 20:08:56]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:09:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/146333004/510001

[commit-bot: I haz the power, 2014-03-31 21:59:53]

Change committed as 260665

[tkent, 2014-04-01 04:09:35]

A revert of this CL has been created in
https://codereview.chromium.org/220553002/ by tkent@chromium.org.

The reason for reverting is: Broke a layout test,
http/tests/loading/promote-img-in-viewport-priority.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40....

[shatch, 2014-04-07 18:01:15]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-04-07 18:01:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/146333004/540001

[commit-bot: I haz the power, 2014-04-07 22:02:41]

Change committed as 262204

[mmenke, 2016-01-26 17:52:21]

On 2014/04/07 22:02:41, commit-bot: I haz the power wrote:
> Change committed as 262204

Hrm...I think this CL really should have gone through a content/browser/loader
owner.

[mmenke, 2016-01-26 17:57:28]

On 2016/01/26 17:52:21, mmenke wrote:
> On 2014/04/07 22:02:41, commit-bot: I haz the power wrote:
> > Change committed as 262204
> 
> Hrm...I think this CL really should have gone through a content/browser/loader
> owner.

Also no bug was filed for this feature, so there's no context here.

[shatch, 2016-01-26 18:03:02]

Description was changed from

==========
This CL adds an intra-priority level sorting value for finer grained control of
resource loading. The blink side of this patch sorts visible images by estimated
on screen real-estate, preferring to download images with more visual impact
first.  Ie. On a page with a large banner, and dozens of small icons, the large
banner will most likely be scheduled ahead of the others. 

In the future, we can also use this to experiment with other signals, ie.
prioritize by distance to viewport.

This needs https://codereview.chromium.org/153983004/ to land first.

In our tests, this was a nice win on speed-index for desktop, with a gain of
about ~2-3%.

BUG=

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

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

to

==========
This CL adds an intra-priority level sorting value for finer grained control of
resource loading. The blink side of this patch sorts visible images by estimated
on screen real-estate, preferring to download images with more visual impact
first.  Ie. On a page with a large banner, and dozens of small icons, the large
banner will most likely be scheduled ahead of the others. 

In the future, we can also use this to experiment with other signals, ie.
prioritize by distance to viewport.

This needs https://codereview.chromium.org/153983004/ to land first.

In our tests, this was a nice win on speed-index for desktop, with a gain of
about ~2-3%.

BUG=319073

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

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

[shatch, 2016-01-26 18:04:08]

On 2016/01/26 17:57:28, mmenke wrote:
> On 2016/01/26 17:52:21, mmenke wrote:
> > On 2014/04/07 22:02:41, commit-bot: I haz the power wrote:
> > > Change committed as 262204
> > 
> > Hrm...I think this CL really should have gone through a
content/browser/loader
> > owner.
> 
> Also no bug was filed for this feature, so there's no context here.

Apologies for the lack of bug, I believe this was a followup to
crbug.com/319073. I'll update the description.

About the lack of owner for content/browser/loader, I believe simonjam was an
owner at the time, although digging through git logs it looks like he was
removed from the OWNERS file shortly after his lgtm and I never noticed
(https://codereview.chromium.org/146313008).

[mmenke, 2016-01-26 18:26:18]

On 2016/01/26 18:04:08, shatch wrote:
> On 2016/01/26 17:57:28, mmenke wrote:
> > On 2016/01/26 17:52:21, mmenke wrote:
> > > On 2014/04/07 22:02:41, commit-bot: I haz the power wrote:
> > > > Change committed as 262204
> > > 
> > > Hrm...I think this CL really should have gone through a
> content/browser/loader
> > > owner.
> > 
> > Also no bug was filed for this feature, so there's no context here.
> 
> Apologies for the lack of bug, I believe this was a followup to
> crbug.com/319073. I'll update the description.
> 
> About the lack of owner for content/browser/loader, I believe simonjam was an
> owner at the time, although digging through git logs it looks like he was
> removed from the OWNERS file shortly after his lgtm and I never noticed
> (https://codereview.chromium.org/146313008).

Thanks!  I had forgotten he was the owner before david and I (I always think of
jam as being the former owner, forgot it went to simonjam before us).