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

Implementing the referrer attribute for the image tag to specify a referrer policy for images.

http://w3c.github.io/webappsec/specs/referrer-policy/#referrer-policy-delivery-referrer-attribute

	Christoph Kerschbaumer [:ckerschb]
	Comment 4 • 9 years ago

Comment on attachment 8616801 [details] [diff] [review]
referrer attribute for img tag (r=ckerschb)

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

Hi Franziskus, I took a first look at this. Looks pretty good to me. Please fix all my nits and please split the patch, so that the test is in it's own patch. Once cleaned up, you can flag me again and I will also take a look at the logic. Cool Stuff - Excited to see this happening!

::: dom/base/nsImageLoadingContent.cpp
@@ +960,5 @@
> +  mozilla::net::ReferrerPolicy imgReferrerPolicy;
> +  GetImageReferrerPolicy(imgReferrerPolicy);
> +  // replace referrerPolicy with the one define in the img tag
> +  if (imgReferrerPolicy != mozilla::net::RP_Unset)
> +    referrerPolicy = imgReferrerPolicy;

You don't need to query both policies, right?
Query the document wide one only if the image does not provide it's own.

mozilla::net::ReferrerPolicy imgReferrerPolicy;
rv = GetImageReferrerPolicy(imgReferrerPolicy);
NS_ENSURE_SUCCESS(rv, rv);
// if the image does not provide it's own referrer, fall back to the documents one
if (imgReferrerPolicy == mozilla::net::RP_Unset) {
  ...
}

Please make sure do add braces everywhere.

@@ +1044,5 @@
> +//     *aReferrerPolicy = mozilla::net::RP_Default;
> +//   }
> +
> +//   return NS_OK;
> +// }

If the code is unused, please remove.

@@ +1613,5 @@
> +//   // FIXME: make this something nully fall back to document wide referrer when not implemented
> +//   aReferrerPolicy = mozilla::net::RP_No_Referrer;
> +  
> +//   return NS_OK;
> +// }

Same here, please remove if not used.

::: dom/base/nsImageLoadingContent.h
@@ +24,5 @@
>  #include "nsAutoPtr.h"
>  #include "nsIContentPolicy.h"
>  #include "mozilla/dom/BindingDeclarations.h"
>  
> +#include "mozilla/net/ReferrerPolicy.h"

nit: please remove the empty line above.

@@ +204,5 @@
> +  {
> +    aReferrerPolicy = mozilla::net::RP_Unset;
> +
> +    return NS_OK; 
> +  };

I am not sure if we inline virtual functions, please move to the *.cpp; remove the empty line and the whitespace after NS_OK;

::: dom/html/HTMLImageElement.cpp
@@ +305,5 @@
>    SetWidth(aWidth, rv);
>    return rv.StealNSResult();
>  }
>  
> +

please remove that empty line.

::: dom/html/HTMLImageElement.h
@@ +202,5 @@
> +  GetImageReferrerPolicy(mozilla::net::ReferrerPolicy &aReferrerPolicy)
> +  {
> +    GetReferrerPolicy(aReferrerPolicy);
> +
> +    return NS_OK;  

Nit: please remove the empty line and also the whitespace after NS_OK;

::: dom/html/nsGenericHTMLElement.cpp
@@ +998,5 @@
>      }
>  
> +    if (aAttribute == nsGkAtoms::referrer) {
> +      ParseReferrerAttribute(aValue, aResult);
> +      return true;

return ParseReferrerAttribute(aValue, aResult);

@@ +1270,5 @@
>  }
>  
>  bool
> +nsGenericHTMLElement::ParseReferrerAttribute(const nsAString& aString,
> +                                          nsAttrValue& aResult)

nit: please align arguments

::: dom/html/nsGenericHTMLElement.h
@@ +240,5 @@
> +    GetEnumAttr(nsGkAtoms::referrer, nullptr, aPolicyString);
> +    if (!aPolicyString.IsEmpty())
> +      aReferrerPolicy = mozilla::net::ReferrerPolicyFromString(aPolicyString);
> +    else
> +      aReferrerPolicy = mozilla::net::RP_Unset;

please use early returns, e.g.
if (aPolicy.IsEmpty()) {
   aReferrerPolicy = mozilla::net::RP_Unset;
   return NS_OK;
}
aReferrerPolicy = mozilla::net::ReferrerPolicyFromString(aPolicyString);
return NS_OK;

@@ +242,5 @@
> +      aReferrerPolicy = mozilla::net::ReferrerPolicyFromString(aPolicyString);
> +    else
> +      aReferrerPolicy = mozilla::net::RP_Unset;
> +
> +    return NS_OK;  

nit: trailing whitespace

@@ +725,5 @@
>                                      const nsAString& aString,
>                                      nsAttrValue& aResult);
> +
> +  static bool ParseReferrerAttribute(const nsAString& aString,
> +                                    nsAttrValue& aResult);

nit: please align arguments.

::: dom/interfaces/html/nsIDOMHTMLImageElement.idl
@@ +23,5 @@
>             attribute DOMString        src;
>             attribute DOMString        srcset;
>             attribute DOMString        sizes;
>             attribute DOMString        useMap;
> +           attribute DOMString        referrer;

you have to ref the uuid if you add an attribute, you can use:
./mach uuid

::: image/imgLoader.cpp
@@ +669,5 @@
>                       int32_t corsmode, nsIPrincipal* loadingPrincipal,
>                       nsISupports* aCX, ReferrerPolicy referrerPolicy)
>  {
>    // If the entry's Referrer Policy doesn't match, we can't use this request.
> +  // XXX: this will return false if an attribute uses a different policy than the document 

what does that mean? how do we fix this?

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

fixed all points and some more.

Referrer attribute is now behind preference network.http.useReferrerAttributes (false by default).

	Christoph Kerschbaumer [:ckerschb]
	Comment 7 • 9 years ago

Comment on attachment 8620696 [details] [diff] [review]
referrer attribute for img tag, src

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

Looks good; getting there. Would like to see it again with my comments addressed.

::: dom/base/nsImageLoadingContent.cpp
@@ +73,5 @@
>    uri->GetSpec(spec);
>    printf("spec='%s'\n", spec.get());
>  }
>  #endif /* DEBUG_chb */
>  

please revert that change!

@@ +97,5 @@
>  {
>    if (!nsContentUtils::GetImgLoaderForChannel(nullptr, nullptr)) {
>      mLoadingEnabled = false;
>    }
> +

same here, please revert that change.

@@ +957,5 @@
>    }
>  
> +  nsresult rv;
> +  // get img referrer policy if there is one
> +  mozilla::net::ReferrerPolicy imgReferrerPolicy;

don't call this imgReferrerPolicy, might also be the policy of the document.

@@ +958,5 @@
>  
> +  nsresult rv;
> +  // get img referrer policy if there is one
> +  mozilla::net::ReferrerPolicy imgReferrerPolicy;
> +  rv = GetImageReferrerPolicy(imgReferrerPolicy);

you don't have to define nsresult rv above, just use it here and initialize it right away.

@@ +966,5 @@
> +  bool referrerAttributeEnabled = false;
> +  referrerAttributeEnabled = Preferences::GetBool("network.http.useReferrerAttributes", referrerAttributeEnabled);
> +  if (imgReferrerPolicy == mozilla::net::RP_Unset || !referrerAttributeEnabled) {
> +    imgReferrerPolicy = aDocument->GetReferrerPolicy();
> +  }

so, here is how I would do it, since this also lives behind a pref. From a semantic point of view we have a document referrer policy, right? 
(1) init a variable with the document wide referrer
(2) check the bool pref
(3) if enabled, then query the img referrer and overrule the document wide one within that if-statement.

@@ +1604,5 @@
>    MOZ_COUNT_DTOR(ImageObserver);
>    NS_CONTENT_DELETE_LIST_MEMBER(ImageObserver, this, mNext);
>  }
> +
> +nsresult 

nit, trailing whitespace.

@@ +1607,5 @@
> +
> +nsresult 
> +nsImageLoadingContent::GetImageReferrerPolicy(mozilla::net::ReferrerPolicy& aReferrerPolicy)
> +{
> +  aReferrerPolicy = mozilla::net::RP_Unset;

We should add a comment on top and define who falls back to call this base class and who overrides it. As I see it, at the moment only HTMLImageElement overrides this function, right? But Firefox supports loading SVGs within an image tag, right? So probably also SVGImageElement should override it and not fall back to the base class.

There are also this other classes that inherit nsImageLoadingContent:
/dom/base/nsGenConImageContent.cpp
/dom/base/nsObjectLoadingContent.h
/dom/html/HTMLInputElement.h
/dom/svg/SVGFEImageElement.h

Not sure what an nsGenConImage is, but we should make sure. Definitely need a testcase for loading an SVG using an img tag.

::: dom/html/HTMLImageElement.cpp
@@ +238,5 @@
> +NS_IMETHODIMP
> +HTMLImageElement::GetReferrer(nsAString& aReferrer)
> +{
> +  GetEnumAttr(nsGkAtoms::referrer, nullptr, aReferrer);
> +

nit, remove empty line.

::: dom/html/HTMLImageElement.h
@@ +194,5 @@
> +  NS_IMETHODIMP
> +  GetImageReferrerPolicy(mozilla::net::ReferrerPolicy &aReferrerPolicy)
> +  {
> +    GetReferrerPolicy(aReferrerPolicy);
> +    return NS_OK;

return GetReferrerPolicy

::: image/imgLoader.cpp
@@ +671,5 @@
>  {
>    // If the entry's Referrer Policy doesn't match, we can't use this request.
> +  // XXX: this will return false if an image has different referrer attributes,
> +  // i.e. we currently don't use the cached image but reload the image with
> +  // the new referrer policy

comment ok, but we still need to determine if we want to do that. Dan Veditz should help us make a decision.

::: modules/libpref/init/all.js
@@ +1260,5 @@
>  // Controls whether we send HTTPS referres to other HTTPS sites.
>  // By default this is enabled for compatibility (see bug 141641)
>  pref("network.http.sendSecureXSiteReferrer", true);
>  
> +// Controls whether referrer attributes in a, img, area, and iframe are honoured

nit, please write the tage like this
<a>, <area>, <img> or <iframe>
otherwise the <a> might get lost

	Christoph Kerschbaumer [:ckerschb]
	Comment 8 • 9 years ago

Comment on attachment 8620697 [details] [diff] [review]
referrer attribute for img tag, tests

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

Looks good, I like that the *.sjs is generating all the html instead of loading external files. Let me see it one more time before we can hand it off to some other reviewer.

::: dom/base/test/bug1166910.sjs
@@ +6,5 @@
> +         'policy=' + policy + '&' +
> +         'name=' + name;
> +}
> +
> +function createTestPage(head, imgPolicy, name) {

please also prefix with a, otherwise the head unerneath looks like you forgot to add the <> around it.

@@ +9,5 @@
> +
> +function createTestPage(head, imgPolicy, name) {
> +  var _createTestUrl = createTestUrl.bind(null, imgPolicy, 'test', name);
> +
> +  return '<!DOCTYPE HTML>\n\

instead of using \n you could use the same as you do above, closing the string literal using " and using +
but that's up to you.

@@ +33,5 @@
> +//   <img> with referrer
> +function createTest(policy, imgPolicy, name) {
> +  var headString = '<head>';
> +  if (policy)
> +    headString += '<meta name="referrer" content="' + policy + '">';

please brace your ifs

@@ +103,5 @@
> +    var state = getSharedState(sharedKey);
> +    state = {};
> +    setSharedState(sharedKey, JSON.stringify(state));
> +    response.write("");
> +  } else if (action === 'test') {

can you rewrite all this if-else statements and use early returns instead?

@@ +192,5 @@
> +    var otherPolicy = unescape(params[2].split('=')[1]);
> +    var name = unescape(params[3].split('=')[1]);
> +
> +    response.write(createReadOnlyTest(imgPolicy, otherPolicy, name));
> +  }

maybe then also send some error message back in case no if (early return is taken).

::: dom/base/test/mochitest.ini
@@ +55,5 @@
>    bug696301-script-2.js
>    bug704320.sjs
>    bug704320_counter.sjs
>    bug819051.sjs
> +  bug1166910.sjs

Nit, please rename to img_referrer_testserver.sjs or something like that. I think in general we would like to get away from using bugnumbers as file names.

@@ +680,5 @@
>  [test_bug1163743.html]
>  support-files = referrerHelper.js
>  [test_bug1165501.html]
>  support-files = referrerHelper.js
> +[test_bug1166910.html]

same nit here, please rename.

::: dom/base/test/test_bug1166910.html
@@ +2,5 @@
> +<html>
> +<!--
> +Spot test to see if newer meta-referrer policy is used
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1165501
> +-->

Maybe add a small description what the test is doing.
Loading 3 images using 3 different policies, etc, so people get an idea of what we are testing here.

@@ +9,5 @@
> +  <meta charset="utf-8">
> +  <title>Test policies for Bug 1165501</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +

does all that need to go into the head? or is otherwise the spec-parser not triggered? If so, please also add a small comment so in case people are going to change/extend the test, they know about that.

@@ +43,5 @@
> +      onSuccess(xhr);
> +    } else {
> +      onFail(xhr);
> +    }
> +  };

I think you can use onload and onerror instead of quering the status codes.

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

added regression tests that meta is still working if referrer attributes are enabled

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

fixed windows compilation bug

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

	Christoph Kerschbaumer [:ckerschb]
	Comment 12 • 9 years ago

Comment on attachment 8621714 [details] [diff] [review]
referrer attribute for img tag, src

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

Great - really excited to see that happening.

@Sid: I don't know whether you would like to review those patches as well? if you are fine with my r+, we could hand it off to a dom peer for review. Any suggestions?

::: modules/libpref/init/all.js
@@ +1268,5 @@
>  pref("network.http.sendSecureXSiteReferrer", true);
>  
> +// Controls whether referrer attributes in <a>, <img>, <area>, and <iframe> are honoured
> +pref("network.http.useReferrerAttributes", false);
> +

Sorry, I haven't pointed that out in the previous review, maybe you can change that to:
> network.http.enablePerElementReferrer

	Christoph Kerschbaumer [:ckerschb]
	Comment 13 • 9 years ago

Comment on attachment 8621655 [details] [diff] [review]
referrer attribute for img tag, tests

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

::: dom/base/test/test_img_referrer.html
@@ +34,5 @@
> +    }
> +    else if (event.data.contains("childLoadComplete")) {
> +      mTestResult = event.data.split(",")[1];
> +      advance();
> +    }

nit: sometimes you use 4 spaces and sometimes only 2 spaces for indentation.

	Sid Stamm [:geekboy or :sstamm]
	Comment 14 • 9 years ago

Comment on attachment 8621714 [details] [diff] [review]
referrer attribute for img tag, src

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

> @Sid: I don't know whether you would like to review those patches as well?

I think your review is sufficient from the DOM::Security perspective, but I took a quick look at the changes in ReferrerPolicy.h and nsIHttpChannel.idl and will give some comments.

> if you are fine with my r+, we could hand it off to a dom peer for review. 
> Any suggestions?

Works for me.  You probably want someone from necko to approve the interface changes in netwerk.

::: netwerk/base/ReferrerPolicy.h
@@ +27,5 @@
>    /* spec tokens: always unsafe-url */
> +  RP_Unsafe_URL                  = nsIHttpChannel::REFERRER_POLICY_UNSAFE_URL,
> +
> +  /* referrer policy is not set */
> +  RP_Unset                       = nsIHttpChannel::REFERRER_POLICY_NOT_SET

RP_Unset gives me pause since it's not absolutely clear how this is different than RP_Default.  Isn't that the same?  If it is, we should remove either RP_Default or RP_Unset and make them effectively be the same thing.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +71,5 @@
>      const unsigned long REFERRER_POLICY_ORIGIN_WHEN_XORIGIN        = 3;
>      /*   always sends the referrer, even on downgrade.                  */
>      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> +    /*   no referrer attribute set                                      */
> +    const unsigned long REFERRER_POLICY_NOT_SET                    = 5;

I am uneasy about using "NOT_SET" because the rest of the tokens are valid policies and this one is not.  (see my comment in ReferrerPolicy.h)

	Sid Stamm [:geekboy or :sstamm]
	Comment 15 • 9 years ago

Comment on attachment 8621655 [details] [diff] [review]
referrer attribute for img tag, tests

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

I think r=ckerschb is sufficient for tests of this feature.

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

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> ::: netwerk/base/ReferrerPolicy.h
> @@ +27,5 @@
> >    /* spec tokens: always unsafe-url */
> > +  RP_Unsafe_URL                  = nsIHttpChannel::REFERRER_POLICY_UNSAFE_URL,
> > +
> > +  /* referrer policy is not set */
> > +  RP_Unset                       = nsIHttpChannel::REFERRER_POLICY_NOT_SET
> 
> RP_Unset gives me pause since it's not absolutely clear how this is
> different than RP_Default.  Isn't that the same?  If it is, we should remove
> either RP_Default or RP_Unset and make them effectively be the same thing.
> 

RP_Unset is not an actually value but is returned when a tag has no referrer attribute. Without changing additional code we always have to hand over a ReferrerPolicy Enum. That's why I added it to the enum. We could actually reuse RP_Default because that's not a valid referrer attribute, but I'd prefer this new value to easier distinguish between these two cases in the code.

> ::: netwerk/protocol/http/nsIHttpChannel.idl
> @@ +71,5 @@
> >      const unsigned long REFERRER_POLICY_ORIGIN_WHEN_XORIGIN        = 3;
> >      /*   always sends the referrer, even on downgrade.                  */
> >      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> > +    /*   no referrer attribute set                                      */
> > +    const unsigned long REFERRER_POLICY_NOT_SET                    = 5;
> 
> I am uneasy about using "NOT_SET" because the rest of the tokens are valid
> policies and this one is not.  (see my comment in ReferrerPolicy.h)

Ok, we could move it out of there or rename it, but if we want to keep the RP_Unset enum value, we should define it somewhere in the idl. Something that should solve your points is reusing REFERRER_POLICY_DEFAULT for RP_Unset.

	Sid Stamm [:geekboy or :sstamm]
	Comment 17 • 9 years ago

> Something that should solve your points is reusing 
> REFERRER_POLICY_DEFAULT for RP_Unset.

Yeah, this might be best.

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

Comment on attachment 8624481 [details] [diff] [review]
referrer attribute for img tag, src

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

r+ if the two comments are addressed.

::: dom/base/nsImageLoadingContent.h
@@ +198,5 @@
>     * default implementation returns CORS_NONE unconditionally.
>     */
>    virtual mozilla::CORSMode GetCORSMode();
>  
> +  virtual nsresult GetImageReferrerPolicy(mozilla::net::ReferrerPolicy& aReferrerPolicy);

The nsresult return type seems useless, since it always returns NS_OK. Please make this take zero arguments and return mozilla::net::ReferrerPolicy instead.

::: parser/html/nsHtml5SpeculativeLoad.h
@@ +180,5 @@
>    private:
>      eHtml5SpeculativeLoad mOpCode;
>      nsString mUrl;
>      nsString mMetaReferrerPolicy;
> +    nsString mImageReferrerPolicy;

In order to avoid uselessly bloating the size of speculative load objects, please don't add new members here when you don't need a larger number of members than are already there. Instead, please rename mMetaReferrerPolicy to mReferrerPolicy and reuse that.

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

Comment on attachment 8624485 [details] [diff] [review]
referrer attribute for img tag, tests

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

r+ with the below nits fixed.

::: dom/base/test/test_img_referrer.html
@@ +7,5 @@
> +* loading a single image multiple times with different policies (generate-img-policy-test3)
> +* testing setAttribute and .referrer (generate-setAttribute-test)
> +* regression tests that meta referrer is still working even if attribute referrers are enabled
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1166910
> +-->

Please move the comment after <meta charset>. Having comments before <meta charset> risks the <meta> prescan failing. (I didn't count the bytes here, but it's a good idea to never have any comments before <meta charset>.)

@@ +41,5 @@
> +/**
> + * helper to perform an XHR.
> + */
> +function doXHR(aUrl, onSuccess, onFail) {
> +  var xhr = new XMLHttpRequest();

Please set xhr.responseType = "json"; here.

@@ +59,5 @@
> +function checkIndividualResults(aTestname, aExpectedImg, aName) {
> +  doXHR('/tests/dom/base/test/img_referrer_testserver.sjs?action=get-test-results',
> +        function(xhr) {
> +          var results = JSON.parse(xhr.responseText);
> +          info(xhr.responseText);

Please use xhr.response here (thanks to responseType = "json") and omit the JSON.parse call.

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

r+ carries over

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

r+ carries over

	Pulsebot
	Comment 24 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f3827ffcf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/981a1dbe042b

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

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=11248314&repo=mozilla-inbound

	Pulsebot
	Comment 26 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b492dc1934f
https://hg.mozilla.org/integration/mozilla-inbound/rev/74795c76fadc

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

It seems to me that https://bug1166910.bugzilla.mozilla.org/attachment.cgi?id=8616800 was not checked in, which defines 'ATTR_REFERRER'.

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

a try run (looks all good): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd5d9e37941d
the following patches need checkin (in this order)
* Teach parser about referrer (generated bits)
* referrer attribute for img tag, src 
* referrer attribute for img tag, tests

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 9 years ago

webidl changes need DOM peer review

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

Henri is DOM peer, do you want another one?

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 9 years ago

Please don't change nsIDOMHTMLImageElement at all.  Just add this to the webidl interface and implement only the webidl version.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 9 years ago

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #30)
> Henri is DOM peer, do you want another one?

Odd, he's listed on the hg hook:
https://hg.mozilla.org/hgcustom/version-control-tools/file/62dfcd2ca7bf/hghooks/mozhghooks/prevent_webidl_changes.py

But it rejected my push :\. Maybe my spelling sucked. Anyway, sounds like bz wants you to change the patch now anyway.

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

(clean up) r+ carries over

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 9 years ago

Comment on attachment 8628548 [details] [diff] [review]
referrer attribute for img tag, src

r=me on just the webidl and HTMLImageElement.h bits.

I'm not sure why the referrer attr parsing is being done generically for all HTML elements, and it wasn't that way in the earlier reviewed patches on this bug, but I'm going to assume someone has reviewed that part and is OK with it.  If not, let me know...

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

The referrer attribute is used in other HTML elements as well (see bug 1174913 and bug 1175736) and therefore done generically for all elements.

	Pulsebot
	Comment 37 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a48456e8eaa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d7963ccfb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fae1257ad4e

	Daniel Holbert [:dholbert]
	Comment 38 • 9 years ago

The second commit here was missing an 'override' annotation on GetImageReferrerPolicy() (declared in nsImageLoadingContent and overridden in HTMLImageElement). The missing 'override' causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I'm pushing a followup (posted here via pulsebot shortly) to add that keyword, with blanket r+ that ehsan
granted me for fixes of this sort over in bug 1126447 comment 2.

	Pulsebot
	Comment 39 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8a4fd19f56

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 40 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/a48456e8eaa5
https://hg.mozilla.org/mozilla-central/rev/78d7963ccfb0
https://hg.mozilla.org/mozilla-central/rev/3fae1257ad4e

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

https://hg.mozilla.org/mozilla-central/rev/8c8a4fd19f56

	Boris Zbarsky [:bzbarsky]
	Comment 42 • 9 years ago

Was there an intent to implement/ship sent for this?  If not, why not?  (Yes, I should have asked this before granting review on the IDL...)

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

This is still disabled behind a pref. We currently prepare the intent to implement/ship.

	Boris Zbarsky [:bzbarsky]
	Comment 44 • 9 years ago

> This is still disabled behind a pref.

It is?  The IDL changes were not disabled behind a pref!

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

The pref only disables applying of the referrer policy, not parsing (in order to avoid unnecessary code). If you strongly prefer disabling the parsing as well, we could add that (same goes for bug 1174913 and bug 1175736 implementing referrer policies for iframe, a, and anchor).

	Boris Zbarsky [:bzbarsky]
	Comment 46 • 9 years ago

The parsing is not a big deal, since it's not script-visible, right?

But having the IDL property present without actually applying the policy seems pretty broken to me.  In particular, it breaks the ability to do detection of whether referrer policy is supported, no?

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

right, we should put the attribute behind the pref as well.

	Boris Zbarsky [:bzbarsky]
	Comment 48 • 9 years ago

Comment on attachment 8630765 [details] [diff] [review]
put attribute behind pref in webidl, r=bz

r=me

	Pulsebot
	Comment 49 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/674efa62ef17

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

https://hg.mozilla.org/mozilla-central/rev/674efa62ef17

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

:fkiefer, I am in the progress of documenting this. I don't find a mention HTMLImageElement.referrer (as well as about HTML{Anchor,Area,IFrame}Element.referrer in the spec [1]. Is this defined elsewhere or should we file a spec bug?

In the latter case, how?


[1] http://w3c.github.io/webappsec/specs/referrer-policy/

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

I'll rename the relevant pages, when it will be renamed (dev-doc-needed is set on the relevant bug)

Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img
https://developer.mozilla.org/en-US/Firefox/Releases/42#HTML

New page created:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/referrer

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

The spec is not there yet. [1] mentions it and should be extended when the attribute is renamed (and then probably should also be added to the html spec, but I guess that should wait til the referrer spec is more stable).

[1] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery-referrer-attribute