Bug 208424

Created attachment 392095 [details] Patch

Created attachment 392096 [details] Patch

Created attachment 392098 [details] Patch

Created attachment 392100 [details] Patch

Comment on attachment 392100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392100&action=review All these frame load request with all these default policies listed over and over again point to poor design patterns; we should figure out a way to construct these things where we don’t write so much boilerplate code. I’d say we should use a struct so we can set named properties, but FrameLoadRequest already is a bit like that. Seriously the number of places with these ridiculously long lists of arguments and policy settings is out of hand. > Source/WebCore/html/HTMLAnchorElement.cpp:497 > + ReferrerPolicy referrerPolicy = hasRel(Relation::NoReferrer) ? ReferrerPolicy::NoReferrer : this->referrerPolicy(); I’d use auto here for the variable type. No reason to repeat the name ReferrerPolicy extra times. > Source/WebCore/html/HTMLAnchorElement.cpp:622 > +void HTMLAnchorElement::setReferrerPolicyForBindings(const AtomString& value) > +{ > + setAttributeWithoutSynchronization(referrerpolicyAttr, value); > +} > + > +String HTMLAnchorElement::referrerPolicyForBindings() const > +{ > + return referrerPolicyToString(referrerPolicy()); > +} > + > +ReferrerPolicy HTMLAnchorElement::referrerPolicy() const > +{ > + if (RuntimeEnabledFeatures::sharedFeatures().referrerPolicyAttributeEnabled()) > + return parseReferrerPolicy(attributeWithoutSynchronization(referrerpolicyAttr), ReferrerPolicySource::ReferrerPolicyAttribute).valueOr(ReferrerPolicy::EmptyString); > + return ReferrerPolicy::EmptyString; > +} Would be nicer to share a little bit more code with image element, but not sure I know how to do that. > Source/WebCore/loader/FrameLoadRequest.cpp:65 > + , m_referrerPolicy { ReferrerPolicy::EmptyString } These defaults should be in the class definition instead of here. Unless there’s some reason not to do that. > Source/WebCore/loader/FrameLoadRequest.h:113 > + ReferrerPolicy m_referrerPolicy; Like here: ReferrerPolicy m_referrerPolicy { ReferrerPolicy::EmptyString }; > LayoutTests/http/tests/referrer-policy-anchor/no-referrer-when-downgrade/cross-origin-http-http-expected.txt:9 > +Tests the behavior of no-referrer-when-downgrade referrer policy when same origin. > + > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > + > + > +PASS referrer is expected > +PASS successfullyParsed is true > + > +TEST COMPLETE This is a frustrating style of test output. The entire text amounts to a single "PASS"; nothing about the test is visible. Even the title of the test has been copied and pasted and so does not make clear what is being tested and how this differs from other tests. The standard form writes out a value and what the expected value was. That gives the person running the test more insight into what is going on. We should put some thought into the design of the test output. Seems like the tests have pretty good coverage. Maybe a single test that covers multiple cases would be practical, too. These tests are OK, much better than not having test coverage, but the design here is not the intent of the test framework.

Created attachment 392117 [details] Patch

Comment on attachment 392100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392100&action=review >> Source/WebCore/html/HTMLAnchorElement.cpp:497 >> + ReferrerPolicy referrerPolicy = hasRel(Relation::NoReferrer) ? ReferrerPolicy::NoReferrer : this->referrerPolicy(); > > I’d use auto here for the variable type. No reason to repeat the name ReferrerPolicy extra times. Done. >> Source/WebCore/html/HTMLAnchorElement.cpp:622 >> +} > > Would be nicer to share a little bit more code with image element, but not sure I know how to do that. There are multiple places now with this code, but indeed hard to share. >> Source/WebCore/loader/FrameLoadRequest.cpp:65 >> + , m_referrerPolicy { ReferrerPolicy::EmptyString } > > These defaults should be in the class definition instead of here. Unless there’s some reason not to do that. Done. >> Source/WebCore/loader/FrameLoadRequest.h:113 >> + ReferrerPolicy m_referrerPolicy; > > Like here: > > ReferrerPolicy m_referrerPolicy { ReferrerPolicy::EmptyString }; Done. >> LayoutTests/http/tests/referrer-policy-anchor/no-referrer-when-downgrade/cross-origin-http-http-expected.txt:9 >> +TEST COMPLETE > > This is a frustrating style of test output. The entire text amounts to a single "PASS"; nothing about the test is visible. Even the title of the test has been copied and pasted and so does not make clear what is being tested and how this differs from other tests. > > The standard form writes out a value and what the expected value was. That gives the person running the test more insight into what is going on. > > We should put some thought into the design of the test output. Seems like the tests have pretty good coverage. > > Maybe a single test that covers multiple cases would be practical, too. > > These tests are OK, much better than not having test coverage, but the design here is not the intent of the test framework. Agreed, the output is not great, but at least it covers everything. Note that the real problem is that we can't import the WPT tests because of the problem illustrated here https://github.com/web-platform-tests/wpt/pull/19381.

(In reply to Darin Adler from comment #5) > Comment on attachment 392100 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392100&action=review > > All these frame load request with all these default policies listed over and > over again point to poor design patterns; we should figure out a way to > construct these things where we don’t write so much boilerplate code. I’d > say we should use a struct so we can set named properties, but > FrameLoadRequest already is a bit like that. Seriously the number of places > with these ridiculously long lists of arguments and policy settings is out > of hand. Agreed, it is not pretty. I am likely to touch FrameLoadRequest in the future so I will keep it in the back of my mind.

Comment on attachment 392117 [details] Patch Clearing flags on attachment: 392117 Committed r257707: <https://trac.webkit.org/changeset/257707>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59945018>

Reopening to attach new patch.

Created attachment 392132 [details] Patch

Wrong bug?

(In reply to Rob Buis from comment #14) > Wrong bug? Definitely....