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

Add support for more @font-face descriptors #15356

Merged
merged 12 commits into from Feb 3, 2017

Conversation

Projects
None yet
5 participants
@SimonSapin
Member

SimonSapin commented Feb 2, 2017

Part of https://bugzilla.mozilla.org/show_bug.cgi?id=1290237. I’ll add conversions to nsCSSValue separately because that requires new C++ functions in the stylo repository.

r? @bholley


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin
Member

SimonSapin commented Feb 2, 2017

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Feb 2, 2017

Contributor

⌛️ Trying commit 4701850 with merge 8603674...

Contributor

bors-servo commented Feb 2, 2017

⌛️ Trying commit 4701850 with merge 8603674...

bors-servo added a commit that referenced this pull request Feb 2, 2017

Auto merge of #15356 - servo:font-face-descriptors, r=<try>
Add support for more @font-face descriptors

<!-- Please describe your changes on the following line: -->

Part of https://bugzilla.mozilla.org/show_bug.cgi?id=1290237. I’ll add conversions to `nsCSSValue` separately because that requires new C++ functions in the stylo repository.

r? @bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/15356)
<!-- Reviewable:end -->
@bholley

This comment has been minimized.

Show comment
Hide comment
@bholley

bholley Feb 2, 2017

Contributor

Nice! I think Manish is probably the best person to review this.

r? @Manishearth

Contributor

bholley commented Feb 2, 2017

Nice! I think Manish is probably the best person to review this.

r? @Manishearth

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Feb 2, 2017

Contributor

💔 Test failed - mac-dev-unit

Contributor

bors-servo commented Feb 2, 2017

💔 Test failed - mac-dev-unit

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Feb 3, 2017

Member
	thread 'fetch::test_fetch_redirect_count_ceiling' panicked at 'assertion failed: !fetch_response.is_network_error()', /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/net/fetch.rs:642

Seems unrelated…

@bors-servo retry

Member

SimonSapin commented Feb 3, 2017

	thread 'fetch::test_fetch_redirect_count_ceiling' panicked at 'assertion failed: !fetch_response.is_network_error()', /Users/servo/buildbot/slave/mac-dev-unit/build/tests/unit/net/fetch.rs:642

Seems unrelated…

@bors-servo retry

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Feb 3, 2017

Contributor

⌛️ Trying commit 4701850 with merge b46b9b2...

Contributor

bors-servo commented Feb 3, 2017

⌛️ Trying commit 4701850 with merge b46b9b2...

bors-servo added a commit that referenced this pull request Feb 3, 2017

Auto merge of #15356 - servo:font-face-descriptors, r=<try>
Add support for more @font-face descriptors

<!-- Please describe your changes on the following line: -->

Part of https://bugzilla.mozilla.org/show_bug.cgi?id=1290237. I’ll add conversions to `nsCSSValue` separately because that requires new C++ functions in the stylo repository.

r? @bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/15356)
<!-- Reviewable:end -->
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 3, 2017

Member

:lgtm:


Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 3 of 3 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, 2 of 2 files at r7, 2 of 2 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/style/parser.rs, line 109 at r11 (raw file):

impl<T> Parse for Vec<T> where T: Parse + CommaSeparated {
    fn parse(context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
        input.parse_comma_separated(|input| T::parse(context, input))

Slight issue is that we need to differentiate between vecs that are allowed to be empty and those that aren't. I guess we can always special case in the use cases.


Comments from Reviewable

Member

Manishearth commented Feb 3, 2017

:lgtm:


Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 3 of 3 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, 2 of 2 files at r7, 2 of 2 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/style/parser.rs, line 109 at r11 (raw file):

impl<T> Parse for Vec<T> where T: Parse + CommaSeparated {
    fn parse(context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
        input.parse_comma_separated(|input| T::parse(context, input))

Slight issue is that we need to differentiate between vecs that are allowed to be empty and those that aren't. I guess we can always special case in the use cases.


Comments from Reviewable

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo
Contributor

bors-servo commented Feb 3, 2017

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Feb 3, 2017

Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/style/parser.rs, line 109 at r11 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

Slight issue is that we need to differentiate between vecs that are allowed to be empty and those that aren't. I guess we can always special case in the use cases.

What it work for you if I rename the CommaSeparated marker to OneOrMoreCommaSeparated? If needed we can also add ZeroOrMoreCommaSeparated.


Comments from Reviewable

Member

SimonSapin commented Feb 3, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/style/parser.rs, line 109 at r11 (raw file):

Previously, Manishearth (Manish Goregaokar) wrote…

Slight issue is that we need to differentiate between vecs that are allowed to be empty and those that aren't. I guess we can always special case in the use cases.

What it work for you if I rename the CommaSeparated marker to OneOrMoreCommaSeparated? If needed we can also add ZeroOrMoreCommaSeparated.


Comments from Reviewable

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Feb 3, 2017

Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/style/parser.rs, line 109 at r11 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

What it work for you if I rename the CommaSeparated marker to OneOrMoreCommaSeparated? If needed we can also add ZeroOrMoreCommaSeparated.

Although, is there anything with zero or more components? CSS usually doesn’t like empty things, they tend to be replaced with a keyword like normal.


Comments from Reviewable

Member

SimonSapin commented Feb 3, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/style/parser.rs, line 109 at r11 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

What it work for you if I rename the CommaSeparated marker to OneOrMoreCommaSeparated? If needed we can also add ZeroOrMoreCommaSeparated.

Although, is there anything with zero or more components? CSS usually doesn’t like empty things, they tend to be replaced with a keyword like normal.


Comments from Reviewable

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 3, 2017

Member

There are a couple, see the allow_empty param for vector longhands.

OneOrMore... works for me. r=me

Member

Manishearth commented Feb 3, 2017

There are a couple, see the allow_empty param for vector longhands.

OneOrMore... works for me. r=me

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Feb 3, 2017

Member

@bors-servo r=Manishearth

Member

SimonSapin commented Feb 3, 2017

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Feb 3, 2017

Contributor

📌 Commit 9ec8418 has been approved by Manishearth

Contributor

bors-servo commented Feb 3, 2017

📌 Commit 9ec8418 has been approved by Manishearth

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Feb 3, 2017

Contributor

⌛️ Testing commit 9ec8418 with merge 48f3cc8...

Contributor

bors-servo commented Feb 3, 2017

⌛️ Testing commit 9ec8418 with merge 48f3cc8...

bors-servo added a commit that referenced this pull request Feb 3, 2017

Auto merge of #15356 - servo:font-face-descriptors, r=Manishearth
Add support for more @font-face descriptors

<!-- Please describe your changes on the following line: -->

Part of https://bugzilla.mozilla.org/show_bug.cgi?id=1290237. I’ll add conversions to `nsCSSValue` separately because that requires new C++ functions in the stylo repository.

r? @bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/15356)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Feb 3, 2017

Contributor

☀️ 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
Approved by: Manishearth
Pushing 48f3cc8 to master...

Contributor

bors-servo commented Feb 3, 2017

☀️ 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
Approved by: Manishearth
Pushing 48f3cc8 to master...

@bors-servo bors-servo merged commit 9ec8418 into master Feb 3, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details

@SimonSapin SimonSapin deleted the font-face-descriptors branch Feb 7, 2017

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