Bug 32167 – Extend ResourceRequest::TargetType to be more specific

• Component: Page Loading
• Status: RESOLVED FIXED

[Mike Belshe]

We'd like to pass more information about a ResourceRequest through to the network layer.

The Chromium ResourceRequest::RequestType currently defines all subresources as "TargetIsSubResource".  Expand this to specify CSS, Script, etc, so that the network layer can more accurately assign priority to these resources.

[Mike Belshe]

Created attachment 44336 [details]
First patch.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44336 [details] without any errors.

[Eric Seidel (no email)]

Entertainingly someone recently tried this in a different bug, but their excuse was to enable ad-blocking at the network layer.  I think that this proposal has much more merit.  I'll see if I can dig up the other bug.

[Eric Seidel (no email)]

Bug 27787 was the bug I was thinking of.

[Mike Belshe]

Thanks for taking a look Eric - I hadn't seen that other (very similar!) bug. Fortunately, my patch does not have the layering violation which Adam pointed out.  It keeps the two types separate (CachedResource::Type and ResourceRequest::Type).  Although it is tempting to merge them, it doesn't seem like a good thing (due to layering).

[Darin Fisher (:fishd, Google)]

Comment on attachment 44336 [details]
First patch.

> Index: WebCore/ChangeLog

> +        Bug 32167 - Update the ResourceRequest::RequestType.  This previously
> +        was specific to Chromium.  Moved into ResourceRequestBase, enabling
> +        more specificity about the type (which is otherwise only known to the
> +        loader), and also making this information available to all platforms.
> +        Any platform with a network layer which can utilize this information
> +        may want to use it for prioritization.

nit: Bug XXX should be a https://bugs.webkit.org/ link so that it is clickable
from trac.webkit.org.


> Index: WebCore/loader/loader.cpp
...
> +ResourceRequestBase::TargetType CachedResourceTypeToTargetType(CachedResource::Type type)

nit: webkit style has functions starting with a lowercase letter.

nit: i think it should be okay to refer to ResourceRequest::TargetType.
we generally avoid referring to the *Base class since it is actually
just an implementation detail (to allow for code sharing).


> +        return ResourceRequest::TargetIsStyleSheet;

^^^ here you just use ResourceRequest::, which is nice.


> Index: WebCore/platform/network/ResourceRequestBase.h

> +        enum TargetType {
> +            TargetIsMainFrame,     // Resource is a main frame.
> +            TargetIsSubFrame,      // Resource is a sub frame.
> +            TargetIsSubResource,   // Resource is a generic subresource.  (Generally a specific type should be specified)
> +            TargetIsStyleSheet,    // A stylesheet subresource.
> +            TargetIsScript,        // A script subresource.
> +            TargetIsFontResource,  // A font subresource.
> +            TargetIsImage,         // An image subresource.
> +            TargetIsObject,        // An object resource.
> +            TargetIsMedia          // A media resource.
> +        };

nit: The comments seems a bit redundant.  WebKit style is typically to
avoid redundant comments.  The parenthetical comment is helpful though.


LGTM with these nits addressed.

[Darin Adler]

This additional information is something that won’t survive the round trip through NSURLRequest on Mac OS X.

[Darin Fisher (:fishd, Google)]

> This additional information is something that won’t survive the round trip
> through NSURLRequest on Mac OS X.

Is that a big problem?  Is there a WebKit API where that survival is important?  webView:resource:willSendRequest:redirectResponse:fromDataSource:?

[Darin Adler]

(In reply to comment #8)
> > This additional information is something that won’t survive the round trip
> > through NSURLRequest on Mac OS X.
> 
> Is that a big problem?  Is there a WebKit API where that survival is important?
>  webView:resource:willSendRequest:redirectResponse:fromDataSource:?

I don't know.

[Adam Barth]

This restriction keeps coming up.  Can we fix things so that WebCore types don't depend on implementation details of the Mac port?

[Mike Belshe]

Created attachment 44415 [details]
Patch addressing Darin's comments.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44415 [details] without any errors.

[Darin Adler]

It would be nice if someone completely eliminated the issue of ResourceRequest fields that don't round trip in NSURLRequest with some architectural change to the Mac OS X version of WebKit.

But even before that we just need to answer the simpler question: Does that fact cause any problems with this patch? My hope is that the answer is no, and this won't hold up landing the patch. But it would be good to know that someone has considered this and decided the answer is no.

[Darin Fisher (:fishd, Google)]

> But even before that we just need to answer the simpler question: Does that
> fact cause any problems with this patch? My hope is that the answer is no, and
> this won't hold up landing the patch. But it would be good to know that someone
> has considered this and decided the answer is no.

I think the answer is no.  The caller of willSendRequest can just copy fields from the original ResourceRequest to the resultant ResourceRequest that correspond to fields that cannot be expressed via NSURLReqeuest.  That way the fields are not lost.  Exposing the fields to the embedder would require API changes, but that's another matter.

[Darin Fisher (:fishd, Google)]

One more thing:  willSendRequest is the only round-trip case I could find.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44415 [details]
Patch addressing Darin's comments.

> Index: WebCore/platform/network/ResourceRequestBase.h
...
> +        enum TargetType {
> +            TargetIsMainFrame,
> +            TargetIsSubFrame,
> +            TargetIsSubResource,  // Resource is a generic subresource.  (Generally a specific type should be specified)

Sorry to over nit, but just for completeness my suggestion was to only keep the
comment in parenthesis since the "Resource is a generic subresource" is a bit
redundant with TargetIsSubResource.  So, just this:

  TargetIsSubResource,  // Generally a specific type should be specified instead.

R=me

[Darin Adler]

Oops, just noticed something.

Normally "subframe" and "subresource" are treated as single words, not "sub-frame" and "sub-resource", hence capitalization should be "Subframe" and "Subresource" not "SubFrame" and "SubResource".

See the spelling of the class name SubresourceLoader and the function name subframeIsLoading.

I see a few cases where this is wrong, but all but a few are in this Chromium code. Lets stay consistent.

[Alexey Proskuryakov]

It's not entirely obvious that preserving the field is the desired behavior though. A client can replace the request with an entirely different one.

This is probably OK for TargetType, but auditing every future addition to ResourceRequest from this point of view can be difficult.

[Mike Belshe]

Created attachment 44432 [details]
Rename SubResource to Subresource as per DarinAdler.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44432 [details] without any errors.

[Darin Adler]

Comment on attachment 44432 [details]
Rename SubResource to Subresource as per DarinAdler.

> +            TargetIsSubFrame,

As long as you are taking my renaming suggestion, what about SubFrame here?

[Mike Belshe]

Created attachment 44444 [details]
Also rename SubFrame to Subframe

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44444 [details] without any errors.

[Darin Adler]

Comment on attachment 44444 [details]
Also rename SubFrame to Subframe

> +        No new tests. (OOPS!)

Commit queue won't be able to land the patch with this in the ChangeLog.

[WebKit Commit Bot]

Comment on attachment 44444 [details]
Also rename SubFrame to Subframe

Rejecting patch 44444 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
re/loader/loader.cpp
	M	WebCore/platform/network/ResourceRequestBase.h
	M	WebCore/platform/network/chromium/ResourceRequest.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 558

[Mike Belshe]

Created attachment 44474 [details]
oops the oops.

[WebKit Commit Bot]

Comment on attachment 44474 [details]
oops the oops.

Rejecting patch 44474 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/modules/committers.py by adding yourself to the file (no review needed) and then set the reviewer flag again.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44474 [details] without any errors.

[Adam Barth]

Comment on attachment 44474 [details]
oops the oops.

Forwarding darin's r+

[WebKit Commit Bot]

Comment on attachment 44474 [details]
oops the oops.

Clearing flags on attachment: 44474

Committed r51859: <http://trac.webkit.org/changeset/51859>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

Note that this patch broke the build and required http://trac.webkit.org/changeset/51861 as a followup build fix.

Is it expected that the commit bot can break the build?  

That's one human task we probably shouldn't have replaced by a script...

[Adam Barth]

(In reply to comment #32)
> Note that this patch broke the build and required
> http://trac.webkit.org/changeset/51861 as a followup build fix.

Sorry, this was entirely my fault.  I clicked the button and got distracted.  :(

[Eric Seidel (no email)]

The commit-queue will never break Mac Leopard Release (that platform it's currently being run from), but can certainly break other platforms.  In general this has not been a problem in the past.  The commit queue has landed nearly 900 patches and this is the second break I've heard of (although there have likely been a few others).  Still way less often than any human. :)

Sorry for the trouble.

[Brady Eidson]

Humans build-breaking problem will never be fixed 100%.  But at least when a human breaks the build, there's concrete blame and a specific individual whose responsibility it is to fix it.

If there's an anonymous tool checking in and it has known shortcomings, shouldn't they be fixed...?

[Adam Barth]

> Humans build-breaking problem will never be fixed 100%.  But at least when a
> human breaks the build, there's concrete blame and a specific individual whose
> responsibility it is to fix it.

There is a specific individual to blame: the one who set the commit-queue+ flag.  In this case, that's me.  We can surface that information move visibly (e.g., in the ChangeLog) if that would be helpful.  Morally, there isn't really a difference between typing "svn ci" on the command line and marking commit-queue+ in bugzilla.

[Brady Eidson]

This is a serious question, I don't know the answer to it:  By design, should the commit-bot be able to check something in that breaks the build?

IMO, setting "commit-queue+" and typing "svn ci" can only be morally equivalent if the commit-bot is designed to be capable of breaking the build.

[Adam Barth]

> This is a serious question, I don't know the answer to it:  By design, should
> the commit-bot be able to check something in that breaks the build?

Yes.  It's not possible to design a system that is guaranteed not to break the build on all platforms.  For example, we don't even have the ability to build all the ports.  The commit-queue is a filter to stop bad patches from hitting the tree (just like the tree is a filter to stop bad patches from hitting releases).  It has false positives and false negatives like all filters.

> IMO, setting "commit-queue+" and typing "svn ci" can only be morally equivalent
> if the commit-bot is designed to be capable of breaking the build.

The social contract for setting commit-queue+ has always been that it's morally equivalent to typing svn ci.  I wrote that in my first email on the subject.  If it's not on the web site, we can clarify that point.

Apparently the idea of surfacing who marked commit-queue+ in the ChangeLog has been discussed before:

https://bugs.webkit.org/show_bug.cgi?id=29274

If you have thoughts on the matter, that might be a good place to mention them.

[Adam Barth]

> If it's not on the web site, we can clarify that point.

This was missing from the commit queue FAQ, but I've added it.

https://trac.webkit.org/wiki/CommitQueue

[Mike Belshe]

Sorry about the build pain.   I thought I had done all of the procedures right
for commit.

[Darin Adler]

(In reply to comment #38)
> The social contract for setting commit-queue+ has always been that it's morally
> equivalent to typing svn ci.  I wrote that in my first email on the subject. 
> If it's not on the web site, we can clarify that point.

I don’t understand this. It’s wrong for me to “svn ci” without first testing the build and tests on at least one platform. But I thought it was right for me to set commit-queue+ without doing that.

Maybe this bug is not the right place to discuss it.

[Adam Barth]

> I don’t understand this. It’s wrong for me to “svn ci” without first testing
> the build and tests on at least one platform. But I thought it was right for me
> to set commit-queue+ without doing that.

The commit-queue does build and test on the Mac platform.  I believe this patch broke a different platform.

[Brady Eidson]

(In reply to comment #42)
> > I don’t understand this. It’s wrong for me to “svn ci” without first testing
> > the build and tests on at least one platform. But I thought it was right for me
> > to set commit-queue+ without doing that.
> 
> The commit-queue does build and test on the Mac platform.  I believe this patch
> broke a different platform.

It builds and tests on Leopard.  It broke SnowLeopard.

[Adam Barth]

> It builds and tests on Leopard.  It broke SnowLeopard.

We could probably upgrade the box to SnowLeopard, but that wouldn't solve the reverse problem.  In any case, chasing all the platforms is a fools errand.  The early warning system we're working on will help with platform coverage and is more scalable, but that's a webkit-dev thread for another day.

The current false negative rate seems to be low enough that responsible actions by committers should be sufficient.  The problem in this case is that I acted irresponsibly by not watching the tree.