Closed Bug 1187357 Opened 4 years ago Closed 4 years ago

Rename referrer attribute to referrerpolicy

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox40 --- unaffected
firefox41 --- unaffected
firefox42 + wontfix
firefox45 --- fixed

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 10 obsolete files)

9.01 KB, patch
wchen
: review+
Details | Diff | Splinter Review
55.45 KB, patch
wchen
: review+
Details | Diff | Splinter Review
7.73 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
14.35 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
3.25 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
6.70 KB, patch
franziskus
: review+
Details | Diff | Splinter Review
The attribute to specify referrer policies has to be changed from referrer to referrerpolicy [1].

[1] https://github.com/w3c/webappsec/pull/435
[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.
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.
[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?
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(Ms2ger)
HI Liz - this should only affect 42. And it's all pref'd off for now.
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(Ms2ger)
Great. Thanks Steve!
 
What is it that's preffed off? Is there a bug for it or some sort of meta bug?
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.
Steve, we are in the middle of the beta cycle, any news on this topic? Thanks
Flags: needinfo?(sworkman)
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.
Flags: needinfo?(sworkman)
Assignee: nobody → franziskuskiefer
Attachment #8681002 - Flags: review?(hsivonen) → review+
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.
Attachment #8681004 - Flags: review?(hsivonen) → review-
(In reply to Henri Sivonen (:hsivonen) from comment #12)
> Maybe the patch for
> bug 1178484 never got landed in the Java repo?

Landed now.
(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.
Since you reviewed the other patches already, would you mind doing this one as well? Should be quick.
Attachment #8682410 - Flags: review?(hsivonen)
and here the test changes to use attribute referrerpolicy
Attachment #8682411 - Flags: review?(hsivonen)
(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.)
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.
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
The changes look good, except for the camelCase thing, which requires a respin of the patches.
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.
Attachment #8682410 - Flags: review?(hsivonen) → review-
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.
Attachment #8682411 - Flags: review?(hsivonen) → review-
Blocks: 1223838
changed casing in IDL
Attachment #8682410 - Attachment is obsolete: true
Attachment #8686116 - Flags: review?(hsivonen)
test changes for IDL changes
Attachment #8682411 - Attachment is obsolete: true
Attachment #8686117 - Flags: review?(hsivonen)
Attachment #8686117 - Flags: review?(hsivonen) → review+
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.
(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.
renamed GetReferrepolicy to GetReferrerPolicyAsEnum
Attachment #8686116 - Attachment is obsolete: true
Attachment #8686116 - Flags: review?(hsivonen)
Attachment #8688399 - Flags: review?(hsivonen)
Attachment #8688399 - Flags: review?(hsivonen) → review+
With [1] landed in the spec I think we can land this.

https://github.com/w3c/webappsec-referrer-policy/issues/3#issuecomment-158036349
Keywords: checkin-needed
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!
Flags: needinfo?(franziskuskiefer)
Keywords: checkin-needed
sorry, this was sitting here for too long. rebased it, r+ carries over
Attachment #8688399 - Attachment is obsolete: true
Flags: needinfo?(franziskuskiefer)
Attachment #8691851 - Flags: review+
Keywords: checkin-needed
Henri or William, you might want to land the java bits as well
Flags: needinfo?(wchen)
Flags: needinfo?(hsivonen)
https://hg.mozilla.org/projects/htmlparser/rev/81d4f0c2ec07
Flags: needinfo?(wchen)
Flags: needinfo?(hsivonen)
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".
Attachment #8694716 - Flags: review?(gijskruitbosch+bugs)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Those tests are far from ideal, but this gives us at least minimal test coverage for the frontend code.
Attachment #8694719 - Flags: review?(gijskruitbosch+bugs)
Attachment #8694719 - Attachment is obsolete: true
Attachment #8694719 - Flags: review?(gijskruitbosch+bugs)
Attachment #8694721 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8694716 - Flags: review?(gijskruitbosch+bugs) → review+
uh, not my day. changed the second one as well.

r+ carries over
Attachment #8694716 - Attachment is obsolete: true
Attachment #8694725 - Flags: review+
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);
});
Attachment #8694721 - Flags: review?(gijskruitbosch+bugs)
well it was running, just not the correct tests :/ should be correct now
Attachment #8694721 - Attachment is obsolete: true
Attachment #8694751 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8694751 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8694751 - Attachment is obsolete: true
Attachment #8694780 - Flags: review+
renaming referrer policy attribute (frontend) and renaming referrer policy attribute (frontend) - tests need check-in then this can be closed again.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/19dce9ab4d97
https://hg.mozilla.org/mozilla-central/rev/0dbac1acaca7
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.