Skip to content
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

Implement encoding determination for external scripts. #9185

Closed
Ms2ger opened this issue Jan 7, 2016 · 6 comments
Closed

Implement encoding determination for external scripts. #9185

Ms2ger opened this issue Jan 7, 2016 · 6 comments
Labels
A-content/dom Interacting with the DOM from web content C-assigned There is someone working on resolving the issue C-needs-test E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss

Comments

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 7, 2016

@Ms2ger Ms2ger added A-content/dom Interacting with the DOM from web content E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss C-needs-test labels Jan 7, 2016
@njskalski
Copy link
Contributor

I attempted to do something here, but I am not sure if I get steps 3 and 4 right.

master...askalski:issue9185

@jdm jdm added the C-assigned There is someone working on resolving the issue label Jan 9, 2016
@njskalski
Copy link
Contributor

I was unable to locate "script block's fallback character encoding", and I don't know which one is "character encoding"

should any of these mimic this:
https://developer.mozilla.org/en-US/docs/Web/Guide/Localizations_and_character_encodings#Specifying_the_fallback_encoding
?

@njskalski
Copy link
Contributor

From what I understand fromhttps://html.spec.whatwg.org/multipage/scripting.html#establish-script-block-source , I should deduce "script block's fallback character encoding" from document's one.

I added two "HELPME" comments, with specific questions. I would appreciate some suggestions.

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 11, 2016

First, in Servo right now, "the script block's fallback character encoding" will always be UTF-8, so feel free to read "UTF-8" where that is written.

Second, I think we'll need to change the type of block_character_encoding. Rather than having it be DOMRefCell<EncodingRef> and initializing to DOMRefCell::new(UTF_8 as EncodingRef), I think we should make it a DOMRefCell<Option<EncodingRef>> and initialize it to DOMRefCell::new(None). This will allow you to implement the "If the algorithm above set the script block's character encoding" check.

@njskalski
Copy link
Contributor

I implemented suggested changes. I open PR now to see Travis CI output #9248

I also have a question what tests should look like.
Should I create wpt tests with simple pages, in which I have assertions that loaded external js files have some property set? If so, I need a suggestion. I have never used charset properties from JS.

Or should I write some unit tests in Rust?

@njskalski njskalski mentioned this issue Jan 11, 2016
bors-servo pushed a commit that referenced this issue Jan 11, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 11, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 11, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 14, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 14, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 14, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 14, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 15, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 15, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 16, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Feb 19, 2016
Issue9185

I have to write tests yet, but I wanted to see full Travis CI output.

Fixes #9185

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9248)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Mar 21, 2016

Fixed by #10079.

@jdm jdm closed this as completed Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/dom Interacting with the DOM from web content C-assigned There is someone working on resolving the issue C-needs-test E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss
Projects
None yet
Development

No branches or pull requests

3 participants