Closed Bug 1354349 Opened 7 years ago Closed 7 years ago

Add a channel.isDocument() method

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: valentin)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type
59 bytes, text/x-review-board-request
mcmanus
: review+
nika
: review+
Details
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor
59 bytes, text/x-review-board-request
nika
: review+
Details 
We are often asked how to determine if a channel is for a top-level document load, and our answer has always been "see if the LOAD_DOCUMENT_URI loadFlag is set".  It turns out that's not enough. See bug 1331680 comment 28 and patch 9 of bug 1337056. We should add a method (to either nsNetUtil.h or perhaps to nsIChannel?) that extracts the code in patch 9 in ContentParent::TransmitPermsFor() so we don't duplicate this code all over the tree.

Let's make sure to

1) replace the code in TransmitPermsFor() to use the new method

2) Change the docs in nsIChannel for LOAD_DOCUMENT_URI to note that while setting this flag is sufficient to mark a channel as a document load, /checking/ whether the channel is a document load requires the use of the new method.

3) a code audit of the tree for uses of LOAD_DOCUMENT_URI would be a good idea--there's a good chance that some of them should be using this new method instead.  

We're likely going to want this for bug 1331680, so we might as well do it now.
Assignee: nobody → valentin.gosu
Whiteboard: [necko-next] → [necko-active]
When this is landed it's probably worth a short note on dev.platform to train people.  There are a lot of folks out there who are used to just using LOAD_DOCUMENT_URI.
Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

https://reviewboard.mozilla.org/r/130040/#review132588
Attachment #8857993 - Flags: review?(mcmanus) → review+
Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

https://reviewboard.mozilla.org/r/130040/#review132590

::: dom/jsurl/nsJSProtocolHandler.cpp
(Diff revision 1)
>      NS_DECL_NSISCRIPTCHANNEL
>      NS_FORWARD_SAFE_NSIPROPERTYBAG(mPropertyBag)
>      NS_FORWARD_SAFE_NSIPROPERTYBAG2(mPropertyBag)
>  
>      nsresult Init(nsIURI *aURI, nsILoadInfo* aLoadInfo);
> -

nit: Please remove this unnecessary line deletion.

::: dom/jsurl/nsJSProtocolHandler.cpp:445
(Diff revision 1)
>  
>      return rv;
>  }
>  
> +
> +NS_IMETHODIMP

nit: Other methods only have one line of whitespace around them in this file.

::: netwerk/base/nsIChannel.idl:356
(Diff revision 1)
> +     * Returns true if the channel is used to create a document.
> +     * It returns true if the loadFlags have LOAD_DOCUMENT_URI set, or if
> +     * LOAD_HTML_OBJECT_DATA is set and the channel has the appropriate
> +     * MIME type.
> +     */
> +    readonly attribute bool isDocument;

We should probably note here that calling this function may return the wrong value before OnStartRequest (as we may not know the MIME type yet).

::: netwerk/base/nsIChannel.idl:359
(Diff revision 1)
> +     * MIME type.
> +     */
> +    readonly attribute bool isDocument;
> +
>  %{ C++
> +  inline bool IsDocument()

Why did you choose to write the wrapper this way instead of marking isDocument as [infallible] and handling errors by returning false in the implementation?

::: netwerk/base/nsNetUtil.cpp:372
(Diff revision 1)
> +  // document, depending on its MIME type.
> +
> +  if (!aChannel || !aIsDocument) {
> +      return NS_ERROR_NULL_POINTER;
> +  }
> +  nsLoadFlags loadFlags;

nit: I would prefer it if we set aIsDocument to false here, before we return any errors, just in case someone forgets to check the return value.
Attachment #8857993 - Flags: review?(michael) → review+
Comment on attachment 8857994 [details]
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

https://reviewboard.mozilla.org/r/130042/#review132602

::: dom/ipc/ContentParent.cpp:5036
(Diff revision 1)
> -  nsLoadFlags loadFlags;
> -  nsresult rv = aChannel->GetLoadFlags(&loadFlags);
> -  NS_ENSURE_SUCCESS(rv, rv);
>  
> -  if (!(loadFlags & nsIChannel::LOAD_DOCUMENT_URI)) {
> -    if (loadFlags & nsIRequest::LOAD_HTML_OBJECT_DATA) {
> +  nsresult rv;
> +  if (!aChannel->IsDocument()) {

Thanks for making this much nicer :)

::: dom/jsurl/nsJSProtocolHandler.cpp:621
(Diff revision 1)
>          // LOAD_DOCUMENT_URI flag, so a docloader we're loading in as the
>          // document channel will claim to not be busy, and our parent's onload
>          // could fire too early.
>          nsLoadFlags loadFlags;
>          mStreamChannel->GetLoadFlags(&loadFlags);
> -        if (loadFlags & LOAD_DOCUMENT_URI) {
> +        if (mStreamChannel->IsDocument()) {

Do we know our MIME type to take advantage of IsDocument() yet at this point?

::: dom/jsurl/nsJSProtocolHandler.cpp:739
(Diff revision 1)
>      mStreamChannel->GetLoadFlags(&loadFlags);
>  
>      uint32_t disposition;
>      if (NS_FAILED(mStreamChannel->GetContentDisposition(&disposition)))
>          disposition = nsIChannel::DISPOSITION_INLINE;
> -    if (loadFlags & LOAD_DOCUMENT_URI && disposition != nsIChannel::DISPOSITION_ATTACHMENT) {
> +    if (mStreamChannel->IsDocument() && disposition != nsIChannel::DISPOSITION_ATTACHMENT) {

nit: flip the arguments to && here - IsDocument is more expensive than the disposition check so it seems like we should do it second.
Attachment #8857994 - Flags: review?(michael) → review+
(In reply to Michael Layzell [:mystor] from comment #7)

> > +    readonly attribute bool isDocument;
> > +
> >  %{ C++
> > +  inline bool IsDocument()
> 
> Why did you choose to write the wrapper this way instead of marking
> isDocument as [infallible] and handling errors by returning false in the
> implementation?

[infallible] only works on builtinclass types.

(In reply to Michael Layzell [:mystor] from comment #8)
> > -        if (loadFlags & LOAD_DOCUMENT_URI) {
> > +        if (mStreamChannel->IsDocument()) {
> 
> Do we know our MIME type to take advantage of IsDocument() yet at this point?

We do, but I missed that mStreamChannel will always have text/html as a content type, so I don't really need to call isDocument here.
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec9fb39f7133
Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type r=mcmanus,mystor
https://hg.mozilla.org/integration/autoland/rev/a28aa86d1ffe
Use channel.isDocument in ContentParent::TransmitPermissionsFor r=mystor
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/185945b7ea63
Backed out changeset a28aa86d1ffe 
https://hg.mozilla.org/integration/autoland/rev/9c7ae09cb1cb
Backed out changeset ec9fb39f7133 for bustage
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92245557&repo=autoland
Flags: needinfo?(valentin.gosu)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b01181410a8a
Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type r=mcmanus,mystor
https://hg.mozilla.org/integration/autoland/rev/8a42c7e1f799
Use channel.isDocument in ContentParent::TransmitPermissionsFor r=mystor
Flags: needinfo?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/b01181410a8a
https://hg.mozilla.org/mozilla-central/rev/8a42c7e1f799
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: