Handle crossorigin in Link #14940

nmvk · 2017-01-10T11:12:21Z

Implemented Step three and handled step four of obtain the resource part
of 4.2.4 The link element.
Link to spec : https://html.spec.whatwg.org/multipage/semantics.html#concept-link-obtain

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes OR

This change is

highfive · 2017-01-10T11:12:25Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmllinkelement.rs, components/script/dom/webidls/HTMLLinkElement.webidl, components/script/stylesheet_loader.rs
@KiChjang: components/script/dom/htmllinkelement.rs, components/script/dom/webidls/HTMLLinkElement.webidl, components/script/stylesheet_loader.rs

jdm

I'm glad to see more tests passing. I've made a few suggestions for ways to avoid duplicating code.

jdm · 2017-01-10T16:51:26Z

components/script/dom/htmllinkelement.rs

@@ -377,6 +387,32 @@ impl HTMLLinkElementMethods for HTMLLinkElement {
    // https://html.spec.whatwg.org/multipage/#dom-link-target
    make_setter!(SetTarget, "target");

+    // https://html.spec.whatwg.org/multipage/#dom-link-crossorigin


This should be using an enumerated getter instead: https://dxr.mozilla.org/servo/rev/124301cf484818a87ab0b3e0c4ef34645be66174/components/script/dom/htmlimageelement.rs#296

Using enumerated_getter always returns default crossorigin value as anonymous. This value is returned even if request is not a valid crossorigin request. This breaks many test case.

You're right. Could we share this code with htmlscriptelement.rs instead of duplicating it, in that case?

I have fixed this.

jdm · 2017-01-10T16:51:26Z

components/script/dom/htmllinkelement.rs

+    }
+
+    // https://html.spec.whatwg.org/multipage/#dom-link-crossorigin
+    fn SetCrossOrigin(&self, value: Option<DOMString>) {


This can use make_setter: https://dxr.mozilla.org/servo/rev/124301cf484818a87ab0b3e0c4ef34645be66174/components/script/dom/htmlimageelement.rs#298

My mistake; it can't. Let's add another set_cross_origin_attribute helper we can call from both HTMLLinkElement and HTMLScriptElement.

jdm · 2017-01-10T16:51:26Z

components/script/dom/htmllinkelement.rs

@@ -235,6 +237,14 @@ impl HTMLLinkElement {
            }
        };

+        // Step 3
+        let cors_setting = match self.GetCrossOrigin() {


Let's make a helper function for this and share it with htmlscriptelement.rs.

I have added a method in request.rs which given a &str returns CorsSettings. I am not sure if this is good refactoring. Other option could be return CorsSettings given DomString but I dont know what is the right location for such utility.

I propose the following in element.rs:

fn reflect_cross_origin_attribute(element: &Element) -> Option<DOMString> { // body of HTMLScriptElement::GetCrossOrigin } fn cors_setting_for_element(element: &Element) -> Option<CorsSetting> { reflect_cross_origin_attribute(element).map(|attr| { // body of CorsSetting::from_str_ref }) }

Then these can be called by both HTMLLinkElement and HTMLScriptElement as necessary.

jdm · 2017-01-10T16:51:26Z

components/script/stylesheet_loader.rs

@@ -244,6 +255,6 @@ impl<'a> StylesheetLoader<'a> {

 impl<'a> StyleStylesheetLoader for StylesheetLoader<'a> {
    fn request_stylesheet(&self, import: &Arc<RwLock<ImportRule>>) {
-        self.load(StylesheetContextSource::Import(import.clone()), "".to_owned())
+        self.load(StylesheetContextSource::Import(import.clone()), None, "".to_owned())


Let's add a TODO comment about whether we should use the original loader's CORS setting. I don't find any direction from the specification about it.

bors-servo · 2017-01-11T19:32:17Z

☔ The latest upstream changes (presumably #14911) made this pull request unmergeable. Please resolve the merge conflicts.


        Handle crossorigin in link and refactor crossorigin handling

Implemented Step three and handled step four of obtain the resource part of 4.2.4 The link element. Link to spec : https://html.spec.whatwg.org/multipage/semantics.html#concept-link-obtain Refactored crossOrigin handling in HTMLScriptElement, HTMLImageElement

nmvk · 2017-01-13T18:07:00Z

@jdm Thanks, I have addressed review comments. Additionally I refactored HTMLImageElement to use this updated change.

jdm · 2017-01-13T18:22:07Z

@bors-servo: r+

bors-servo · 2017-01-13T18:22:08Z

📌 Commit 3d9e44a has been approved by jdm

bors-servo · 2017-01-13T18:22:12Z

⌛ Testing commit 3d9e44a with merge 9afd960...


        Auto merge of #14940 - mrnayak:link-cross, r=jdm

Handle crossorigin in Link Implemented Step three and handled step four of obtain the resource part of 4.2.4 The link element. Link to spec : https://html.spec.whatwg.org/multipage/semantics.html#concept-link-obtain  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14940)

bors-servo · 2017-01-13T19:13:00Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned emilio Jan 10, 2017

highfive added the S-awaiting-review label Jan 10, 2017

jdm assigned jdm and unassigned emilio Jan 10, 2017

jdm requested changes Jan 10, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 10, 2017

nmvk force-pushed the nmvk:link-cross branch from b234bd6 to 8449a0b Jan 11, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 11, 2017

highfive added the S-needs-rebase label Jan 11, 2017

nmvk force-pushed the nmvk:link-cross branch from 8449a0b to 31c19b0 Jan 11, 2017

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Jan 11, 2017

nmvk force-pushed the nmvk:link-cross branch from 31c19b0 to 9853df0 Jan 13, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 13, 2017

nmvk force-pushed the nmvk:link-cross branch from 9853df0 to 3d9e44a Jan 13, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 13, 2017

bors-servo merged commit 3d9e44a into servo:master Jan 13, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 13, 2017

nmvk deleted the nmvk:link-cross branch Jan 13, 2017

servo / servo

Handle crossorigin in Link #14940

Handle crossorigin in Link #14940

nmvk commented Jan 10, 2017 •

edited

highfive commented Jan 10, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jan 11, 2017

nmvk commented Jan 13, 2017

jdm commented Jan 13, 2017

bors-servo commented Jan 13, 2017

bors-servo commented Jan 13, 2017

bors-servo commented Jan 13, 2017

servo / servo

Join GitHub today

Handle crossorigin in Link #14940

Handle crossorigin in Link #14940

Conversation

nmvk commented Jan 10, 2017 • edited

highfive commented Jan 10, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jan 11, 2017

nmvk commented Jan 13, 2017

jdm commented Jan 13, 2017

bors-servo commented Jan 13, 2017

bors-servo commented Jan 13, 2017

bors-servo commented Jan 13, 2017

nmvk commented Jan 10, 2017 •

edited