[webkit-reviews] review denied: [Bug 28986] Add support for noreferrer link relation : [Attachment 39091] First pass

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Nate Chapin
<japhet at chromium.org>'s request for review:
Bug 28986: Add support for noreferrer link relation
https://bugs.webkit.org/show_bug.cgi?id=28986

Attachment 39091: First pass
https://bugs.webkit.org/attachment.cgi?id=39091&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 * loader/FrameLoaderTypes.h: Add enum for referrer policy
+	 (WebCore::):

It's best to edit or just remove the second line - prepare-Changelog is not
perfect, so its output is just a starting point or a human, and this is just
garbage.

+    , m_linkRelations()

A default constructor will be called anyway, there is no need to invoke it
explicitly.

+    ClassNames m_linkRelations;

This data is already stored in attribute map, I don't think you need any data
members added to HTMLAnchorElement class.

Instead, it should expose a shouldSendReferrer() method that will in turn call
getAttribute().

+    if (referrerPolicy == NoReferrer || !argsReferrer.isEmpty())
	 referrer = argsReferrer;

If I'm reading it correctly, this code depends on argsReferrer being empty if
referrerPolicy is NoReferrer. Please add an assertion to this effect.

Also, can this result in an empty Referer header being sent with some or all
network back-ends? Do we want this, or do we want to omit it completely?
Formally, servers should not make any distinction between those, so we probably
want to save a few bytes sent over the wire.

+    , m_suppressOpenerInNewFrame(false)

The ChangeLog should promptly mention this (probably before per-function logs),
because this is not entirely expected.

Could you add a test case for normal navigation that just replaces top frame
content, and another for navigation in a subframe? Also, what about navigating
an existing subframe? The spec seems to be silent about this, which is strange:


<iframe name="a" src="a.html"></iframe>
<a href="b.html" target="a" rel="noreferrer">link</a>

+    if (referrerPolicy == NoReferrer) {
+	 m_suppressOpenerInNewFrame = true;

Does it need to be reset to false otherwise? I'm not 100% sure about the role
of FrameLoader, but I think it is re-used for future navigations.

I think that this (two consecutive navigations with different noreferrer
values) should be tested, too.

HTML5 says that AREA elements also support noreferrer. Will this be added in a
followup patch?

+    newEvent.initMouseEvent("click", false, false, window, 0, 10, 10, 10, 10,
false, false, false, false, 0, target);

Will a simple click() work? I have some doubts whether a synthetic mouse event
should be able to do this - we already block synthetic keyboard events in
default handlers.

+    // Prevent from being cached.
+    header("Cache-Control: no-cache, private, max-age=0");

Counter-intuitively, the directive to prevent caching is "no-store", not
"no-cache". The latter only affects intermediate caches.