Closed
Bug 1166910
Opened 10 years ago
Closed 10 years ago
Implement <img> referrer attribute
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: franziskus, Assigned: franziskus)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(5 files, 10 obsolete files)
3.93 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
52.21 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
23.44 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
16.46 KB,
patch
|
franziskus
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8616799 -
Flags: review?(hsivonen)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8616800 -
Flags: review?(hsivonen)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8616801 -
Flags: review?(mozilla)
Comment 4•10 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?
Attachment #8616801 -
Flags: review?(mozilla)
Updated•10 years ago
|
Attachment #8616799 -
Flags: review?(hsivonen) → review+
Updated•10 years ago
|
Attachment #8616800 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 5•10 years ago
|
||
fixed all points and some more.
Referrer attribute is now behind preference network.http.useReferrerAttributes (false by default).
Attachment #8616801 -
Attachment is obsolete: true
Attachment #8620696 -
Flags: review?(mozilla)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8620697 -
Flags: review?(mozilla)
Comment 7•10 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
Attachment #8620696 -
Flags: review?(mozilla)
Comment 8•10 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.
Attachment #8620697 -
Flags: review?(mozilla)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8620696 -
Attachment is obsolete: true
Attachment #8621654 -
Flags: review?(mozilla)
Assignee | ||
Comment 10•10 years ago
|
||
added regression tests that meta is still working if referrer attributes are enabled
Attachment #8620697 -
Attachment is obsolete: true
Attachment #8621655 -
Flags: review?(mozilla)
Assignee | ||
Comment 11•10 years ago
|
||
fixed windows compilation bug
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c896c81326
Attachment #8621654 -
Attachment is obsolete: true
Attachment #8621654 -
Flags: review?(mozilla)
Attachment #8621714 -
Flags: review?(mozilla)
Comment 12•10 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
Attachment #8621714 -
Flags: review?(sstamm)
Attachment #8621714 -
Flags: review?(mozilla)
Attachment #8621714 -
Flags: review+
Comment 13•10 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.
Attachment #8621655 -
Flags: review?(sstamm)
Attachment #8621655 -
Flags: review?(mozilla)
Attachment #8621655 -
Flags: review+
Comment 14•10 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)
Attachment #8621714 -
Flags: review?(sstamm)
Comment 15•10 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.
Attachment #8621655 -
Flags: review?(sstamm)
Assignee | ||
Comment 16•10 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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(sstamm)
Comment 17•10 years ago
|
||
> Something that should solve your points is reusing
> REFERRER_POLICY_DEFAULT for RP_Unset.
Yeah, this might be best.
Flags: needinfo?(sstamm)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8621714 -
Attachment is obsolete: true
Attachment #8624481 -
Flags: review?(hsivonen)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8621655 -
Attachment is obsolete: true
Attachment #8624485 -
Flags: review?(hsivonen)
Comment 20•10 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.
Attachment #8624481 -
Flags: review?(hsivonen) → review+
Comment 21•10 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.
Attachment #8624485 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 22•10 years ago
|
||
r+ carries over
Attachment #8624481 -
Attachment is obsolete: true
Attachment #8627709 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
r+ carries over
Attachment #8624485 -
Attachment is obsolete: true
Attachment #8627710 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f3827ffcf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/981a1dbe042b
Keywords: checkin-needed
Comment 25•10 years ago
|
||
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=11248314&repo=mozilla-inbound
Flags: needinfo?(franziskuskiefer)
Comment 26•10 years ago
|
||
Assignee | ||
Comment 27•10 years ago
|
||
It seems to me that https://bug1166910.bugzilla.mozilla.org/attachment.cgi?id=8616800 was not checked in, which defines 'ATTR_REFERRER'.
Flags: needinfo?(franziskuskiefer) → needinfo?(cbook)
Assignee | ||
Updated•10 years ago
|
Attachment #8616799 -
Attachment description: Teach parser about referrer (generated bits) → Teach parser about referrer (java)
Assignee | ||
Comment 28•10 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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 29•10 years ago
|
||
webidl changes need DOM peer review
Flags: needinfo?(cbook) → needinfo?(franziskuskiefer)
Keywords: checkin-needed
Assignee | ||
Comment 30•10 years ago
|
||
Henri is DOM peer, do you want another one?
Flags: needinfo?(franziskuskiefer) → needinfo?(ryanvm)
Comment 31•10 years ago
|
||
Please don't change nsIDOMHTMLImageElement at all. Just add this to the webidl interface and implement only the webidl version.
Comment 32•10 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.
Flags: needinfo?(ryanvm)
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8627709 -
Attachment is obsolete: true
Attachment #8628548 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 34•10 years ago
|
||
(clean up) r+ carries over
Attachment #8627710 -
Attachment is obsolete: true
Attachment #8628549 -
Flags: review+
Comment 35•10 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...
Attachment #8628548 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 36•10 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.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 37•10 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
Keywords: checkin-needed
Comment 38•10 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.
Comment 39•10 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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 41•10 years ago
|
||
Comment 42•10 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...)
Flags: needinfo?(franziskuskiefer)
Assignee | ||
Comment 43•10 years ago
|
||
This is still disabled behind a pref. We currently prepare the intent to implement/ship.
Flags: needinfo?(franziskuskiefer)
Comment 44•10 years ago
|
||
> This is still disabled behind a pref.
It is? The IDL changes were not disabled behind a pref!
Flags: needinfo?(franziskuskiefer)
Assignee | ||
Comment 45•10 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).
Flags: needinfo?(franziskuskiefer) → needinfo?(bzbarsky)
Comment 46•10 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?
Flags: needinfo?(bzbarsky)
Updated•10 years ago
|
Flags: needinfo?(franziskuskiefer)
Assignee | ||
Comment 47•10 years ago
|
||
right, we should put the attribute behind the pref as well.
Flags: needinfo?(franziskuskiefer)
Attachment #8630765 -
Flags: review?(bzbarsky)
Comment 48•10 years ago
|
||
Comment on attachment 8630765 [details] [diff] [review]
put attribute behind pref in webidl, r=bz
r=me
Attachment #8630765 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 49•10 years ago
|
||
Keywords: checkin-needed
Comment 50•10 years ago
|
||
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/
Flags: needinfo?(franziskuskiefer)
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
Keywords: dev-doc-needed → dev-doc-complete
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
Flags: needinfo?(franziskuskiefer)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•