New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use fetch infrastructure to load external scripts #12472

Merged
merged 8 commits into from Sep 22, 2016

Conversation

Projects
None yet
6 participants
@KiChjang
Member

KiChjang commented Jul 16, 2016

Fixes #9186.


This change is Reviewable

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Jul 16, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/document.rs, components/net/resource_thread.rs, components/script/dom/webidls/HTMLScriptElement.webidl, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/htmlscriptelement.rs, components/net_traits/request.rs, components/net_traits/request.rs

highfive commented Jul 16, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/document.rs, components/net/resource_thread.rs, components/script/dom/webidls/HTMLScriptElement.webidl, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/htmlscriptelement.rs, components/net_traits/request.rs, components/net_traits/request.rs
@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Jul 16, 2016

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!

highfive commented Jul 16, 2016

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!
@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Jul 16, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

⌛️ Trying commit b6a5ac6 with merge f4639fd...

Contributor

bors-servo commented Jul 16, 2016

⌛️ Trying commit b6a5ac6 with merge f4639fd...

bors-servo added a commit that referenced this pull request Jul 16, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=<try>
Make text decorations have the same color as the text if no shadows are present

Fixes  #9186.

r? @Ms2ger

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Jul 16, 2016

Member

That's a weird PR title.

Member

jdm commented Jul 16, 2016

That's a weird PR title.

@KiChjang KiChjang changed the title from Make text decorations have the same color as the text if no shadows are present to Use fetch infrastructure to load external scripts Jul 16, 2016

@KiChjang KiChjang changed the title from Use fetch infrastructure to load external scripts to Use fetch infrastructure to load external scripts Jul 16, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

💔 Test failed - mac-rel-css

Contributor

bors-servo commented Jul 16, 2016

💔 Test failed - mac-rel-css

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Jul 16, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

⌛️ Trying commit de36d2c with merge f33b18b...

Contributor

bors-servo commented Jul 16, 2016

⌛️ Trying commit de36d2c with merge f33b18b...

bors-servo added a commit that referenced this pull request Jul 16, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=<try>
Use fetch infrastructure to load external scripts

Fixes  #9186.

r? @Ms2ger

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

💔 Test failed - mac-rel-css

Contributor

bors-servo commented Jul 16, 2016

💔 Test failed - mac-rel-css

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Jul 16, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

⌛️ Trying commit a51bb00 with merge 6291e1b...

Contributor

bors-servo commented Jul 16, 2016

⌛️ Trying commit a51bb00 with merge 6291e1b...

bors-servo added a commit that referenced this pull request Jul 16, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=<try>
Use fetch infrastructure to load external scripts

Fixes  #9186.

r? @Ms2ger

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

💔 Test failed - mac-rel-wpt

Contributor

bors-servo commented Jul 16, 2016

💔 Test failed - mac-rel-wpt

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Jul 16, 2016

Member

Huh, looks like there's a lot more infrastructure to work on.

Member

KiChjang commented Jul 16, 2016

Huh, looks like there's a lot more infrastructure to work on.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Jul 16, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

⌛️ Trying commit 9208353 with merge cf372a4...

Contributor

bors-servo commented Jul 16, 2016

⌛️ Trying commit 9208353 with merge cf372a4...

bors-servo added a commit that referenced this pull request Jul 16, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=<try>
Use fetch infrastructure to load external scripts

Fixes  #9186.

r? @Ms2ger

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 16, 2016

Contributor

💔 Test failed - linux-dev

Contributor

bors-servo commented Jul 16, 2016

💔 Test failed - linux-dev

Show outdated Hide outdated components/script/dom/htmlscriptelement.rs
@@ -358,6 +365,7 @@ impl HTMLScriptElement {
fetch_a_classic_script(self, url, encoding);
true
},
// TODO: Step 19.

This comment has been minimized.

@Ms2ger

Ms2ger Jul 18, 2016

Contributor

What about step 19? I think that's all handled in the if-else chain below.

@Ms2ger

Ms2ger Jul 18, 2016

Contributor

What about step 19? I think that's all handled in the if-else chain below.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Jul 18, 2016

Contributor

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7.
Review status: 2 of 8 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


components/net_traits/request.rs, line 179 [r2] (raw file):

    pub integrity_metadata: RefCell<String>,
    pub cryptographic_metadata: RefCell<String>,
    pub parser_metadata: Cell<ParserMetadata>,

Don't add code that doesn't do anything.


components/net_traits/request.rs, line 144 [r8] (raw file):

#[derive(Serialize, Deserialize, Clone)]
pub struct PotentialCORSRequestInit {

Spec link


components/net_traits/request.rs, line 276 [r8] (raw file):

    }

    pub fn potential_cors_init(init: PotentialCORSRequestInit) -> Request {

Spec link


components/script/dom/htmlscriptelement.rs, line 119 [r3] (raw file):

#[derive(Clone, Copy, HeapSizeOf, JSTraceable, PartialEq)]
pub enum ScriptType {

You're not going to add support for modules, so revert this.


components/script/dom/htmlscriptelement.rs, line 287 [r4] (raw file):

        // Step 6.
        let type_attribute = element.get_attribute(&ns!(), &atom!("type"));

Revert this, it makes prepare() more cluttered for no good reason.


components/script/dom/htmlscriptelement.rs, line 346 [r5] (raw file):

        let event_attribute = element.get_attribute(&ns!(), &atom!("event"));
        match (for_attribute.r(), event_attribute.r(), self.type_.get()) {
            (Some(for_attribute), Some(event_attribute), ScriptType::Classic) => {

Revert


components/script/dom/htmlscriptelement.rs, line 368 [r6] (raw file):

        // Step 14.
        let cors_setting = match &*self.CrossOrigin() {

If you're going to keep this, you'd better have a slew of tests for it. If not, revert.


components/script/dom/htmlscriptelement.rs, line 217 [r8] (raw file):

        let document = document_from_node(elem.r());
        document.finish_load(LoadType::Script(self.url.clone()));

Where did this go?


components/script/dom/htmlscriptelement.rs, line 170 [r9] (raw file):

impl FetchResponseListener for ScriptContext {
    fn process_request_body(&mut self) {} // TODO(KiChjang): Perhaps add custom steps to perform fetch here?

What does that mean? Either way, if you think there's something to be done, file an issue.


components/script/dom/htmlscriptelement.rs, line 247 [r9] (raw file):

        cryptographic_metadata: String::from(cryptographic_nonce),
        parser_metadata: parser_state,
        origin: doc.url().clone(),

Looks wrong. Spec says "client", which comes to mean "request's client's origin". Client is "settings object"; which is "element's node document's Window object", which defines its origin to be "the origin of the Document with which window is currently associated", which may not be doc. In either case, it says "origin", not "url".


Comments from Reviewable

Contributor

Ms2ger commented Jul 18, 2016

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7.
Review status: 2 of 8 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


components/net_traits/request.rs, line 179 [r2] (raw file):

    pub integrity_metadata: RefCell<String>,
    pub cryptographic_metadata: RefCell<String>,
    pub parser_metadata: Cell<ParserMetadata>,

Don't add code that doesn't do anything.


components/net_traits/request.rs, line 144 [r8] (raw file):

#[derive(Serialize, Deserialize, Clone)]
pub struct PotentialCORSRequestInit {

Spec link


components/net_traits/request.rs, line 276 [r8] (raw file):

    }

    pub fn potential_cors_init(init: PotentialCORSRequestInit) -> Request {

Spec link


components/script/dom/htmlscriptelement.rs, line 119 [r3] (raw file):

#[derive(Clone, Copy, HeapSizeOf, JSTraceable, PartialEq)]
pub enum ScriptType {

You're not going to add support for modules, so revert this.


components/script/dom/htmlscriptelement.rs, line 287 [r4] (raw file):

        // Step 6.
        let type_attribute = element.get_attribute(&ns!(), &atom!("type"));

Revert this, it makes prepare() more cluttered for no good reason.


components/script/dom/htmlscriptelement.rs, line 346 [r5] (raw file):

        let event_attribute = element.get_attribute(&ns!(), &atom!("event"));
        match (for_attribute.r(), event_attribute.r(), self.type_.get()) {
            (Some(for_attribute), Some(event_attribute), ScriptType::Classic) => {

Revert


components/script/dom/htmlscriptelement.rs, line 368 [r6] (raw file):

        // Step 14.
        let cors_setting = match &*self.CrossOrigin() {

If you're going to keep this, you'd better have a slew of tests for it. If not, revert.


components/script/dom/htmlscriptelement.rs, line 217 [r8] (raw file):

        let document = document_from_node(elem.r());
        document.finish_load(LoadType::Script(self.url.clone()));

Where did this go?


components/script/dom/htmlscriptelement.rs, line 170 [r9] (raw file):

impl FetchResponseListener for ScriptContext {
    fn process_request_body(&mut self) {} // TODO(KiChjang): Perhaps add custom steps to perform fetch here?

What does that mean? Either way, if you think there's something to be done, file an issue.


components/script/dom/htmlscriptelement.rs, line 247 [r9] (raw file):

        cryptographic_metadata: String::from(cryptographic_nonce),
        parser_metadata: parser_state,
        origin: doc.url().clone(),

Looks wrong. Spec says "client", which comes to mean "request's client's origin". Client is "settings object"; which is "element's node document's Window object", which defines its origin to be "the origin of the Document with which window is currently associated", which may not be doc. In either case, it says "origin", not "url".


Comments from Reviewable

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Jul 18, 2016

Member

Review status: 2 of 8 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


components/script/dom/htmlscriptelement.rs, line 368 [r2] (raw file):

Previously, Ms2ger wrote…

What about step 19? I think that's all handled in the if-else chain below.

It doesn't look like so to me, the spec says to create a classic script if the `<script>` element does not have a src attribute, and it's missing here.

components/script/dom/htmlscriptelement.rs, line 217 [r8] (raw file):

Previously, Ms2ger wrote…

Where did this go?

It's there, on a later revision.

components/script/dom/htmlscriptelement.rs, line 170 [r9] (raw file):

Previously, Ms2ger wrote…

What does that mean? Either way, if you think there's something to be done, file an issue.

This is referring to step 3 of https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script.

components/script/dom/htmlscriptelement.rs, line 247 [r9] (raw file):

Previously, Ms2ger wrote…

Looks wrong. Spec says "client", which comes to mean "request's client's origin". Client is "settings object"; which is "element's node document's Window object", which defines its origin to be "the origin of the Document with which window is currently associated", which may not be doc. In either case, it says "origin", not "url".

Unfortunate as it is, fetch would hit an unimplemented!() code path if the origin is set to client (which is the default) since it doesn't know about the settings object yet, so we'll have to manually set the origin here.

Comments from Reviewable

Member

KiChjang commented Jul 18, 2016

Review status: 2 of 8 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.


components/script/dom/htmlscriptelement.rs, line 368 [r2] (raw file):

Previously, Ms2ger wrote…

What about step 19? I think that's all handled in the if-else chain below.

It doesn't look like so to me, the spec says to create a classic script if the `<script>` element does not have a src attribute, and it's missing here.

components/script/dom/htmlscriptelement.rs, line 217 [r8] (raw file):

Previously, Ms2ger wrote…

Where did this go?

It's there, on a later revision.

components/script/dom/htmlscriptelement.rs, line 170 [r9] (raw file):

Previously, Ms2ger wrote…

What does that mean? Either way, if you think there's something to be done, file an issue.

This is referring to step 3 of https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script.

components/script/dom/htmlscriptelement.rs, line 247 [r9] (raw file):

Previously, Ms2ger wrote…

Looks wrong. Spec says "client", which comes to mean "request's client's origin". Client is "settings object"; which is "element's node document's Window object", which defines its origin to be "the origin of the Document with which window is currently associated", which may not be doc. In either case, it says "origin", not "url".

Unfortunate as it is, fetch would hit an unimplemented!() code path if the origin is set to client (which is the default) since it doesn't know about the settings object yet, so we'll have to manually set the origin here.

Comments from Reviewable

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 18, 2016

Contributor

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

Contributor

bors-servo commented Jul 18, 2016

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

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Jul 21, 2016

Member

Review status: 0 of 8 files reviewed at latest revision, 11 unresolved discussions.


components/net_traits/request.rs, line 179 [r2] (raw file):

Previously, Ms2ger wrote…

Don't add code that doesn't do anything.

Even when both are mentioned in https://fetch.spec.whatwg.org/#concept-request-nonce-metadata?

Comments from Reviewable

Member

KiChjang commented Jul 21, 2016

Review status: 0 of 8 files reviewed at latest revision, 11 unresolved discussions.


components/net_traits/request.rs, line 179 [r2] (raw file):

Previously, Ms2ger wrote…

Don't add code that doesn't do anything.

Even when both are mentioned in https://fetch.spec.whatwg.org/#concept-request-nonce-metadata?

Comments from Reviewable

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Jul 21, 2016

Member

Still needs tests for crossOrigin.

Member

KiChjang commented Jul 21, 2016

Still needs tests for crossOrigin.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Sep 21, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2016

Contributor

⌛️ Trying commit f700e5e with merge 9c83bf0...

Contributor

bors-servo commented Sep 21, 2016

⌛️ Trying commit f700e5e with merge 9c83bf0...

bors-servo added a commit that referenced this pull request Sep 21, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=<try>
Use fetch infrastructure to load external scripts

Fixes  #9186.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2016

Contributor

💔 Test failed - linux-rel

Contributor

bors-servo commented Sep 21, 2016

💔 Test failed - linux-rel

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2016

Contributor

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

Contributor

bors-servo commented Sep 21, 2016

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

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Sep 21, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2016

Contributor

⌛️ Trying commit 31e51a9 with merge 2ef1856...

Contributor

bors-servo commented Sep 21, 2016

⌛️ Trying commit 31e51a9 with merge 2ef1856...

bors-servo added a commit that referenced this pull request Sep 21, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=<try>
Use fetch infrastructure to load external scripts

Fixes  #9186.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo
Contributor

bors-servo commented Sep 21, 2016

Show outdated Hide outdated components/net_traits/response.rs
};
if self.is_network_error() {
return Err(NetworkError::Internal("Cannot extract metadata from network error".to_string()));
if let Some(ref mut m) = metadata {

This comment has been minimized.

@jdm

jdm Sep 21, 2016

Member

Let's move the initialization into the appropriate if let block earlier.

@jdm

jdm Sep 21, 2016

Member

Let's move the initialization into the appropriate if let block earlier.

Show outdated Hide outdated components/net_traits/response.rs
Metadata::default(url.clone())
Some(Metadata::default(url.clone()))
} else if self.is_network_error() {
return Err(NetworkError::Internal("Cannot extract metadata from network error".to_owned()));

This comment has been minimized.

@jdm

jdm Sep 21, 2016

Member

Let's have a separate if block for this check before we check self.url.

@jdm

jdm Sep 21, 2016

Member

Let's have a separate if block for this check before we check self.url.

Show outdated Hide outdated components/net_traits/response.rs
} else {
match metadata {
Some(m) => Ok(FetchMetadata::Unfiltered(m)),
None => Err(NetworkError::Internal("No url found in response".to_owned()))

This comment has been minimized.

@jdm

jdm Sep 21, 2016

Member

I would rather make this an unreachable!(), because we shouldn't be able to have a URL-less response with no internal response.

@jdm

jdm Sep 21, 2016

Member

I would rather make this an unreachable!(), because we shouldn't be able to have a URL-less response with no internal response.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang

KiChjang Sep 21, 2016

Member

Comments addressed.

Member

KiChjang commented Sep 21, 2016

Comments addressed.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm
Member

jdm commented Sep 21, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2016

Contributor

📌 Commit 6fbd2aa has been approved by jdm

Contributor

bors-servo commented Sep 21, 2016

📌 Commit 6fbd2aa has been approved by jdm

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2016

Contributor

⌛️ Testing commit 6fbd2aa with merge f357afc...

Contributor

bors-servo commented Sep 21, 2016

⌛️ Testing commit 6fbd2aa with merge f357afc...

bors-servo added a commit that referenced this pull request Sep 21, 2016

Auto merge of #12472 - KiChjang:use-fetch-in-script, r=jdm
Use fetch infrastructure to load external scripts

Fixes  #9186.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12472)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo
Contributor

bors-servo commented Sep 22, 2016

@bors-servo bors-servo merged commit 6fbd2aa into servo:master Sep 22, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@KiChjang KiChjang deleted the KiChjang:use-fetch-in-script branch Sep 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment