Franziskus Kiefer [:franziskus] Assignee
	Description • 9 years ago

The attribute to specify referrer policies has to be changed from referrer to referrerpolicy [1].

[1] https://github.com/w3c/webappsec/pull/435

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 9 years ago

[Tracking Requested - why for this release]: we should probably make sure not to ship anything that uses the old name; not sure how much of it is preffed off.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 2 • 9 years ago

It is completely preffed off so that shouldn't be a problem. But we should be able to do this soonish, as soon as it actually lands in the spec draft.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 3 • 9 years ago

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Franziskus, would this only affect 42, or will this need uplift to versions shipping sooner?

	Steve Workman [:sworkman] (INACTIVE)
	Comment 4 • 9 years ago

HI Liz - this should only affect 42. And it's all pref'd off for now.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 5 • 9 years ago

Great. Thanks Steve!
 
What is it that's preffed off? Is there a bug for it or some sort of meta bug?

	Steve Workman [:sworkman] (INACTIVE)
	Comment 6 • 9 years ago

The referrer attribute itself is currently disabled - it is undefined in JS until the pref is flipped.

The meta bug is bug 999754, which this bug currently blocks.

	Sylvestre Ledru [:Sylvestre]
	Comment 7 • 9 years ago

Steve, we are in the middle of the beta cycle, any news on this topic? Thanks

	Steve Workman [:sworkman] (INACTIVE)
	Comment 8 • 9 years ago

Hi Sylvestre - we're still waiting on the PR on github. Note that the attribute is still behind a pref - websites will not be able to use "<a href="http://example.com" referrer=...>" unless a user has specifically gone into about:config. So, nothing to worry about here and I think we can remove tracking status for this.

	Sylvestre Ledru [:Sylvestre]
	Comment 9 • 9 years ago

OK, thanks.

	Henri Sivonen (:hsivonen)
	Comment 12 • 9 years ago

Comment on attachment 8681004 [details] [diff] [review]
Generated code for renaming referrer to referrerpolicy in html parser.

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

This seems to remove an unrelated attribute "property". Maybe the patch for bug 1178484 never got landed in the Java repo?

Marking r- to get bug 1178484 sorted out instead of overwriting it.

	Henri Sivonen (:hsivonen)
	Comment 13 • 9 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> Maybe the patch for
> bug 1178484 never got landed in the Java repo?

Landed now.

	Henri Sivonen (:hsivonen)
	Comment 14 • 9 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #13)
> (In reply to Henri Sivonen (:hsivonen) from comment #12)
> > Maybe the patch for
> > bug 1178484 never got landed in the Java repo?
> 
> Landed now.

If you regenerate the C++ code after pulling from the Java repo, you can assume rs=hsivonen on the result.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 15 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 18 • 9 years ago

Since you reviewed the other patches already, would you mind doing this one as well? Should be quick.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 19 • 9 years ago

and here the test changes to use attribute referrerpolicy

	Henri Sivonen (:hsivonen)
	Comment 20 • 9 years ago

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #18)
>  Should be quick.

Turns out not to be quick because:
 1) It seems the spec hasn't been updated accordingly.
 2) The text in the git repo for the spec doesn't match what was proposed. (annevk said referrerPolicy for the DOM attribute, spec says referrerpolicy. I want to know which one will hold.)

	Franziskus Kiefer [:franziskus] Assignee
	Comment 21 • 9 years ago

Hi Henry, it will be merged in gh-pages when the other issues are fixed as well. But you're right the camelcase thing should be clarified. I'll follow up on the spec PR.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 22 • 9 years ago

The initial PR was missing the IDL. That'll be done in [1]. The attribute is referrerpolicy, and the IDL attribute is supposed to be referrerPolicy.

[1] https://github.com/w3c/webappsec-referrer-policy/issues/3

	Henri Sivonen (:hsivonen)
	Comment 23 • 9 years ago

The changes look good, except for the camelCase thing, which requires a respin of the patches.

	Henri Sivonen (:hsivonen)
	Comment 24 • 9 years ago

Comment on attachment 8682410 [details] [diff] [review]
renaming referrer policy attribute

Looks OK, except the patch is now wrong in terms of casing if we assume the spec will go with camelCase referrerPolicy for the DOM attribute.

	Henri Sivonen (:hsivonen)
	Comment 25 • 9 years ago

Comment on attachment 8682411 [details] [diff] [review]
renaming referrer policy attribute - tests

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

Looks good except for the casing of the DOM attribute.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 26 • 9 years ago

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

	Franziskus Kiefer [:franziskus] Assignee
	Comment 27 • 9 years ago

changed casing in IDL

	Franziskus Kiefer [:franziskus] Assignee
	Comment 28 • 9 years ago

test changes for IDL changes

	Henri Sivonen (:hsivonen)
	Comment 29 • 9 years ago

Comment on attachment 8686116 [details] [diff] [review]
renaming referrer policy attribute

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

I think with the change proposed below, this would be r+ once https://github.com/w3c/webappsec-referrer-policy/issues/3 clears up the existence of the IDL attribute.

::: dom/base/Element.cpp
@@ +3602,5 @@
>    return 1.0;
>  }
>  
>  net::ReferrerPolicy
> +Element::GetReferrerpolicy()

Is this method changing away from camelCase because it would otherwise conflict with the method whose name is dictated by IDL? If so, it seems confusing to distinguish the two just by the case of the letter 'p'. I suggest naming this GetReferrerPolicyAsEnum.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 30 • 9 years ago

(In reply to Henri Sivonen (:hsivonen) from comment #29)
> Comment on attachment 8686116 [details] [diff] [review]
> renaming referrer policy attribute
> 
> Review of attachment 8686116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think with the change proposed below, this would be r+ once
> https://github.com/w3c/webappsec-referrer-policy/issues/3 clears up the
> existence of the IDL attribute.

yeah, I'm waiting for Scott on this, but hope this gets resolved soon (don't expect any surprises here though).

> 
> ::: dom/base/Element.cpp
> @@ +3602,5 @@
> >    return 1.0;
> >  }
> >  
> >  net::ReferrerPolicy
> > +Element::GetReferrerpolicy()
> 
> Is this method changing away from camelCase because it would otherwise
> conflict with the method whose name is dictated by IDL? If so, it seems
> confusing to distinguish the two just by the case of the letter 'p'. I
> suggest naming this GetReferrerPolicyAsEnum.

yep, this collides with the GetReferrerPolicy from the IDL. I agree, it could have a better name, will rename it to GetReferrerPolicyAsEnum.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 31 • 9 years ago

renamed GetReferrepolicy to GetReferrerPolicyAsEnum

	Franziskus Kiefer [:franziskus] Assignee
	Comment 32 • 9 years ago

With [1] landed in the spec I think we can land this.

https://github.com/w3c/webappsec-referrer-policy/issues/3#issuecomment-158036349

	Carsten Book [:Tomcat]
	Comment 33 • 9 years ago

the last patch failed to apply:

1 out of 1 hunks FAILED -- saving rejects to file dom/html/HTMLAreaElement.h.rej
patching file dom/webidl/HTMLAreaElement.webidl
Hunk #1 FAILED at 23
1 out of 1 hunks FAILED -- saving rejects to file dom/webidl/HTMLAreaElement.webidl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh referrerPolicyRename.patch

could you take a look, thanks!

	Franziskus Kiefer [:franziskus] Assignee
	Comment 34 • 9 years ago

sorry, this was sitting here for too long. rebased it, r+ carries over

	Franziskus Kiefer [:franziskus] Assignee
	Comment 35 • 9 years ago

Henri or William, you might want to land the java bits as well

	Pulsebot
	Comment 36 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5ab235060e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7614d8b3082
https://hg.mozilla.org/integration/mozilla-inbound/rev/64bb552ce03a

	William Chen [:wchen]
	Comment 37 • 9 years ago

https://hg.mozilla.org/projects/htmlparser/rev/81d4f0c2ec07

	Carsten Book [:Tomcat]
	Comment 38 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7f5ab235060e
https://hg.mozilla.org/mozilla-central/rev/e7614d8b3082
https://hg.mozilla.org/mozilla-central/rev/64bb552ce03a

	Franziskus Kiefer [:franziskus] Assignee
	Comment 39 • 9 years ago

Thanks for noticing Gijs, totally missed this. That happens when there is insufficient test coverage (none). Can you review this? Is only string replacement of "referrer" to "referrerpolicy".

	Franziskus Kiefer [:franziskus] Assignee
	Comment 40 • 9 years ago

Those tests are far from ideal, but this gives us at least minimal test coverage for the frontend code.

	:Gijs (he/him)
	Comment 42 • 9 years ago

Comment on attachment 8694716 [details] [diff] [review]
renaming referrer policy attribute (frontend)

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

There's a second use of "referrer" in content.js on line 391. With that also updated, r=me

	Franziskus Kiefer [:franziskus] Assignee
	Comment 43 • 9 years ago

uh, not my day. changed the second one as well.

r+ carries over

	:Gijs (he/him)
	Comment 44 • 9 years ago

Comment on attachment 8694721 [details] [diff] [review]
renaming referrer policy attribute (frontend) - tests

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

Because of the wrong url, this isn't actually testing the new server. Can you fix up the patch, and check it works, and then rerequest review?

::: browser/base/content/test/referrer/file_referrer_policyserver_attr.sjs
@@ +1,5 @@
> +/**
> + * Renders a link with the provided referrer policy.
> + * Used in browser_referrer_*.js, bug 1113431.
> + * Arguments: ?scheme=http://&policy=origin&rel=noreferrer
> + */

Please use hg cp with the original file to preserve blame. It would have been useful for the diff as well, but oh well.

::: browser/base/content/test/referrer/head.js
@@ +207,5 @@
> + * same as referrerTestCaseLoaded but using referrerpolicy attribute
> + */
> +function referrerTestCaseLoadedAttribute(aTestNumber) {
> +  let test = getReferrerTest(aTestNumber);
> +  let url = test.fromScheme + REFERRER_POLICYSERVER_URL +

This should be using URL_ATTRIBUTE, surely?

@@ +243,5 @@
>      if (getReferrerTest(nextTestNumber)) {
> +      if (rounds === 0) {
> +        referrerTestCaseLoaded(nextTestNumber).then(function() {
> +          aStartTestCase(nextTestNumber);
> +        });

let loadMethod = rounds === 0 ? referrerTestCaseLoaded : referrerTestCaseLoadedAttribute;

loadMethod(nextTestNumber).then(function() {
  aStartTestCase(nextTestNumber);
});

	Franziskus Kiefer [:franziskus] Assignee
	Comment 45 • 9 years ago

well it was running, just not the correct tests :/ should be correct now

	:Gijs (he/him)
	Comment 46 • 9 years ago

Comment on attachment 8694751 [details] [diff] [review]
renaming referrer policy attribute (frontend) - tests

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

Thanks!

r=me with the below addressed one way or another.

::: browser/base/content/test/referrer/browser_referrer_middle_click.js
@@ +1,4 @@
>  // Tests referrer on middle-click navigation.
>  // Middle-clicks on the link, which opens it in a new tab.
>  
> +SpecialPowers.pushPrefEnv({"set": [['network.http.enablePerElementReferrer', true]]});

Is there a reason not to put this in head.js instead of the individual files?

::: browser/base/content/test/referrer/head.js
@@ +58,5 @@
>    //    and origin referrer for cross-origin requests.
>    {
>      fromScheme: "https://",
>      toScheme: "https://",
> +    policy: "no-referrer",

you might be able to use a getter to still use origin-when-cross-origin for the non-attr case?

get policy() {
  return rounds === 0 ? "origin-when-cross-origin" : "referrer";
},

or something. Same for the next one, and you'll need a getter for the result as well. If that's too confusing/yucky and bug 1178337 will happen soon, can also just do this. Either works!

	Franziskus Kiefer [:franziskus] Assignee
	Comment 47 • 9 years ago

r+ carries over

> Is there a reason not to put this in head.js instead of the individual files?
there is not... put it in head.js

> you might be able to use a getter to still use origin-when-cross-origin for the > non-attr case?
> 
> get policy() {
>   return rounds === 0 ? "origin-when-cross-origin" : "referrer";
> },
> 
> or something. Same for the next one, and you'll need a getter for the result as > well. If that's too confusing/yucky and bug 1178337 will happen soon, can also just do this. Either works!

I think that's only confusing. Bug 1178337 has a patch already that's waiting for review. Once that landed I'll update the test to use origin-when-cross-origin again.

	Franziskus Kiefer [:franziskus] Assignee
	Comment 48 • 9 years ago

renaming referrer policy attribute (frontend) and renaming referrer policy attribute (frontend) - tests need check-in then this can be closed again.

	Pulsebot
	Comment 49 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/19dce9ab4d97
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dbac1acaca7

	Carsten Book [:Tomcat]
	Comment 50 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/19dce9ab4d97
https://hg.mozilla.org/mozilla-central/rev/0dbac1acaca7

	Jean-Yves Perrier [:teoli]
	Comment 51 • 9 years ago

Updated:
https://developer.mozilla.org/en-US/Firefox/Releases/45#HTML
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAnchorElement/referrerPolicy
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/referrerPolicy
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/area
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLAreaElement/referrerPolicy
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement
and
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/referrerPolicy