Franziskus Kiefer [:franziskus] Reporter
	Description • 8 years ago

      No description provided.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 3 • 8 years ago

Hi Franziskus,
Could you please take a look and let me know if I am in incorrect way of missing anything?
Thanks

	Franziskus Kiefer [:franziskus] Reporter
	Comment 4 • 8 years ago

Comment on attachment 8755815 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8755815 [details] [diff] [review]:
-----------------------------------------------------------------

I think most of this looks pretty good already. See comments inline.
Before giving this into review we should probably clarify the reload case.

::: dom/base/nsStyleLinkElement.cpp
@@ +434,5 @@
>        MOZ_LOG(SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
>                ("nsStyleLinkElement::DoUpdateStyleSheet, integrity=%s",
>                 NS_ConvertUTF16toUTF8(integrity).get()));
>      }
> +    // if referrer attributes are enabled in preferences, load link's referrer

the link's referrer ...

@@ +436,5 @@
>                 NS_ConvertUTF16toUTF8(integrity).get()));
>      }
> +    // if referrer attributes are enabled in preferences, load link's referrer
> +    // attribute. If the link does not provide a referrer attribute, ignore this
> +    // and use document's referrer policy

the document's referrer

@@ +439,5 @@
> +    // attribute. If the link does not provide a referrer attribute, ignore this
> +    // and use document's referrer policy
> +
> +    net::ReferrerPolicy referrerPolicy = doc->GetReferrerPolicy();
> +    net::ReferrerPolicy linkReferrerPolicy = net::RP_Unset;

you don't really need linkReferrerPolicy here. simply use referrerPolicy and check that for RP_Unset.

::: dom/webidl/HTMLLinkElement.webidl
@@ +29,5 @@
>             attribute DOMString hreflang;
>    [SetterThrows, Pure]
>             attribute DOMString type;
> +  [SetterThrows, Pure, Pref="network.http.enablePerElementReferrer"]
> +    attribute DOMString referrerPolicy;

indentation looks a bit off

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +139,5 @@
>      // configure HTTP specific stuff
>      nsCOMPtr<nsIHttpChannel> httpChannel =
>          do_QueryInterface(mChannel);
>      if (httpChannel) {
> +        httpChannel->SetReferrerWithPolicy(mReferrerURI, mReferrerPolicy);

so this didn't handle referrer policies before at all?

@@ +608,5 @@
> +    nsCOMPtr<nsINode> node(do_QueryInterface(aSource, &rv));
> +    if (NS_FAILED(rv)) return rv;
> +
> +    net::ReferrerPolicy referrerPolicy = node->OwnerDoc()->GetReferrerPolicy();
> +    net::ReferrerPolicy linkReferrerPolicy = net::RP_Unset;

same here

@@ +631,5 @@
>                       "document\n"));
>                  mCurrentNodes[i]->mSources.AppendElement(source);
> +                // The node is being prefetched, re-fetch with new referrer
> +                // policy
> +                if (linkReferrerPolicy != mCurrentNodes[i]->GetReferrerPolicy()) {

hm, not sure if want to do this. I think we shouldn't as it doesn't offer anything. But this should be checked with the spec (together with the img case).

@@ +883,5 @@
>      return NS_OK;
>  }
>  
>  // vim: ts=4 sw=4 expandtab
> +

?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 5 • 8 years ago

> ::: uriloader/prefetch/nsPrefetchService.cpp
> @@ +139,5 @@
> >      // configure HTTP specific stuff
> >      nsCOMPtr<nsIHttpChannel> httpChannel =
> >          do_QueryInterface(mChannel);
> >      if (httpChannel) {
> > +        httpChannel->SetReferrerWithPolicy(mReferrerURI, mReferrerPolicy);
> 
> so this didn't handle referrer policies before at all?
It used default referrer policy before, and I think using doc's referrer policy should be more reasonable. For example, if we have a referrer policy set in meta tag, link element should use that to fetch resource

	Thomas Nguyen (:tnguyen) Assignee
	Comment 6 • 8 years ago

Comment on attachment 8755816 [details] [diff] [review]
Update and add test

Would like to add more tests

	Franziskus Kiefer [:franziskus] Reporter
	Comment 8 • 8 years ago

Comment on attachment 8755816 [details] [diff] [review]
Update and add test

Review of attachment 8755816 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't actually check the tests yet because I think you can do this with significantly less code. That would also make it easier to read. You can for example have a look at the other referrer attribute test cases (e.g. test_anchor_area_referrer.html,  referrer_testserver.sjs, and referrerHelper.js).

	Thomas Nguyen (:tnguyen) Assignee
	Comment 9 • 8 years ago

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #4)
> Comment on attachment 8755815 [details] [diff] [review]
> Implement link referrerpolicy attribute
> 
> Review of attachment 8755815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think most of this looks pretty good already. See comments inline.
> Before giving this into review we should probably clarify the reload case.

I think it makes sense to do nothing if the request is sent out because it's too late.
I filed an issue in github and and I think we all agree that reloading the resource does not help.
I updated and noted some comments in the patch
https://github.com/w3c/resource-hints/issues/61#issuecomment-222177151

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 8 years ago

Comment on attachment 8757882 [details] [diff] [review]
Implement link referrerpolicy attribute

jdm volunteered to review this (I have a bit high review load)

	Josh Matthews [:jdm]
	Comment 14 • 8 years ago

Comment on attachment 8757882 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8757882 [details] [diff] [review]:
-----------------------------------------------------------------

These changes look like they do the job, but I think we can avoid some duplication. I'd like to see the next version of this patch.

::: dom/base/nsStyleLinkElement.cpp
@@ +434,5 @@
>        MOZ_LOG(SRILogHelper::GetSriLog(), mozilla::LogLevel::Debug,
>                ("nsStyleLinkElement::DoUpdateStyleSheet, integrity=%s",
>                 NS_ConvertUTF16toUTF8(integrity).get()));
>      }
> +    // if referrer attributes are enabled in preferences, load the link's referrer

nit: add a newline above this.

@@ +451,5 @@
>      NS_ENSURE_TRUE(clonedURI, NS_ERROR_OUT_OF_MEMORY);
>      rv = doc->CSSLoader()->
>        LoadStyleLink(thisContent, clonedURI, title, media, isAlternate,
> +                    GetCORSMode(), referrerPolicy == net::RP_Unset ?
> +                    doc->GetReferrerPolicy() : referrerPolicy , integrity,

Let's move this into a separate conditional check instead of including it inline in the arguments here.

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +611,5 @@
> +    net::ReferrerPolicy referrerPolicy = net::RP_Unset;
> +    if (node->IsHTMLElement(nsGkAtoms::link)) {
> +      referrerPolicy =
> +        static_cast<dom::HTMLLinkElement*>(node.get())->GetLinkReferrerPolicy();
> +    }

It seems like overkill to do this check here, rather than as part of OpenChannel where we already have code to deal with the CORS attributes. Getting the referrer policy there would mean that we wouldn't need any special logic for dealing with nodes that are in the queue but haven't been fetched yet, either.

::: uriloader/prefetch/nsPrefetchService.h
@@ +119,5 @@
>      RefPtr<nsPrefetchService>   mService;
>      nsCOMPtr<nsIChannel>        mChannel;
>      nsCOMPtr<nsIChannel>        mRedirectChannel;
>      int64_t                     mBytesRead;
> +    net::ReferrerPolicy         mReferrerPolicy;

The changes in this file shouldn't be necessary if we modify OpenChannel instead.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 15 • 8 years ago

(In reply to Josh Matthews [:jdm] from comment #14)
> 
> ::: uriloader/prefetch/nsPrefetchService.cpp
> @@ +611,5 @@
> > +    net::ReferrerPolicy referrerPolicy = net::RP_Unset;
> > +    if (node->IsHTMLElement(nsGkAtoms::link)) {
> > +      referrerPolicy =
> > +        static_cast<dom::HTMLLinkElement*>(node.get())->GetLinkReferrerPolicy();
> > +    }
> 
> It seems like overkill to do this check here, rather than as part of
> OpenChannel where we already have code to deal with the CORS attributes.
> Getting the referrer policy there would mean that we wouldn't need any
> special logic for dealing with nodes that are in the queue but haven't been
> fetched yet, either.

Nice. Thanks you for your review. This will be useful if we handle reloading (we decide not to reload), so should be moved to OpenChannel. I updated the patch.

	Franziskus Kiefer [:franziskus] Reporter
	Comment 18 • 8 years ago

Comment on attachment 8757885 [details] [diff] [review]
Update and add test

Review of attachment 8757885 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. But we should keep an eye on bug 1268962 and if this gives us errors.

::: dom/base/test/referrer_testserver.sjs
@@ +178,5 @@
> +
> +  var onEvent = '';
> +  // XXX Bug 1268962 - Fire load/error events on <link rel="prefetch">
> +  // Currently, we dont actually know when prefetching finishes.
> +  // Fire event after a delay 2 secs

hm, this sounds like a possible intermittent error

	Josh Matthews [:jdm]
	Comment 19 • 8 years ago

Comment on attachment 8758093 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8758093 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsStyleLinkElement.cpp
@@ +442,5 @@
> +
> +    net::ReferrerPolicy referrerPolicy = net::RP_Unset;
> +    if (thisContent->IsHTMLElement(nsGkAtoms::link)) {
> +      referrerPolicy =
> +        static_cast<dom::HTMLLinkElement*>(thisContent.get())->GetLinkReferrerPolicy();

Looking more closely at this, can't we call nsStyleLinkElement's GetLinkReferrerPolicy directly without casting to a particular concrete type? If that's true, we shouldn't need the `if` check either.

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +113,4 @@
>      if (source->IsHTMLElement(nsGkAtoms::link)) {
>        corsMode = static_cast<dom::HTMLLinkElement*>(source.get())->GetCORSMode();
> +      referrerPolicy =
> +        static_cast<dom::HTMLLinkElement*>(source.get())->GetLinkReferrerPolicy();

Let's make a local variable instead of casting for both of these method calls.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 20 • 8 years ago

Thanks a lot Franziskus. Josh

	Josh Matthews [:jdm]
	Comment 22 • 8 years ago

Comment on attachment 8758517 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8758517 [details] [diff] [review]:
-----------------------------------------------------------------

Delightful! Thanks for making the changes I requested.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 23 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=982b2ea903f4

	Thomas Nguyen (:tnguyen) Assignee
	Comment 24 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97404473773

	Thomas Nguyen (:tnguyen) Assignee
	Comment 25 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9724712000

	Thomas Nguyen (:tnguyen) Assignee
	Comment 26 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29139a40cf46

	Thomas Nguyen (:tnguyen) Assignee
	Comment 27 • 8 years ago

Hmm, there's weird failure on try
https://treeherder.mozilla.org/logviewer.html#?job_id=21859042&repo=try#L1922
referrerPolicy is not treated as attribute on Android 4.3

	Thomas Nguyen (:tnguyen) Assignee
	Comment 28 • 8 years ago

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #18)
> Comment on attachment 8757885 [details] [diff] [review]
> Update and add test
> 
> Review of attachment 8757885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm. But we should keep an eye on bug 1268962 and if this gives us errors.
> 
> ::: dom/base/test/referrer_testserver.sjs
> @@ +178,5 @@
> > +
> > +  var onEvent = '';
> > +  // XXX Bug 1268962 - Fire load/error events on <link rel="prefetch">
> > +  // Currently, we dont actually know when prefetching finishes.
> > +  // Fire event after a delay 2 secs
> 
> hm, this sounds like a possible intermittent error

Hmm, I ran try and it has intermittent error exactly for prefetching case. But I am not sure how and when to to send event back.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 29 • 8 years ago

Use "prefetch_load_completed" event observer for current implementation

	Thomas Nguyen (:tnguyen) Assignee
	Comment 30 • 8 years ago

test timeout on non-e10s opt test machine

	Thomas Nguyen (:tnguyen) Assignee
	Comment 31 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b762572fe86

	Thomas Nguyen (:tnguyen) Assignee
	Comment 35 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b561a353479&selectedJob=22718927

	Thomas Nguyen (:tnguyen) Assignee
	Comment 36 • 8 years ago

Comment on attachment 8764200 [details] [diff] [review]
Update and add test

Could you please review the patch?
The patch only changes as below compared to the last one you reviewed:
- Separate prefetch and stylesheet test
- Using prefetch-load-completed listener (could remove after bug 1268962 is fixed)
- Skip test failed (Bug 1281415, could enable after the bug is fixed)
Thanks

	Franziskus Kiefer [:franziskus] Reporter
	Comment 37 • 8 years ago

Comment on attachment 8764200 [details] [diff] [review]
Update and add test

Review of attachment 8764200 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. But why exactly do we need two different test cases test_link_prefetch.html and test_link_stylesheet.html?

	Thomas Nguyen (:tnguyen) Assignee
	Comment 38 • 8 years ago

Thanks for looking at. 

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #37)
> 
> lgtm. But why exactly do we need two different test cases
> test_link_prefetch.html and test_link_stylesheet.html?
Just separate the prefetch test cases, we could track bug 1268962 and bug 1281415 (these are only related to prefetch) and update later.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 39 • 8 years ago

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b561a353479&selectedJob=22718927

	Carsten Book [:Tomcat]
	Comment 40 • 8 years ago

need dom peer review 
got from hg:
WebIDL file dom/webidl/HTMLLinkElement.webidl altered without DOM peer review

thanks!

	Thomas Nguyen (:tnguyen) Assignee
	Comment 41 • 8 years ago

Thanks you.
(In reply to Carsten Book [:Tomcat] from comment #40)
> need dom peer review 
> got from hg:
> WebIDL file dom/webidl/HTMLLinkElement.webidl altered without DOM peer review
> 
> thanks!

Hi Kyle,
Could you please take a look at that?
Thanks

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 42 • 8 years ago

Comment on attachment 8758517 [details] [diff] [review]
Implement link referrerpolicy attribute

Review of attachment 8758517 [details] [diff] [review]:
-----------------------------------------------------------------

The WebIDL changes look fine.

	Thomas Nguyen (:tnguyen) Assignee
	Comment 43 • 8 years ago

Nice! Thanks Kyle.

Updated commit's summary

	Thomas Nguyen (:tnguyen) Assignee
	Comment 44 • 8 years ago

Comment on attachment 8764800 [details] [diff] [review]
Implement link referrerpolicy attribute  (dom-reviewed)

Carry r+

	Pulsebot
	Comment 45 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e75889b87f3a
Implement link referrerPolicy attribute. r=josh, r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9f5b5b4286
Update and add test cases. r=franziskus

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 46 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e75889b87f3a
https://hg.mozilla.org/mozilla-central/rev/4e9f5b5b4286

	Florian Scholz (Open Web Docs)
	Comment 47 • 8 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/50#HTML
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement/referrerPolicy