Skip to content

/ servo

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 the correct IDL setter for <font>.size #7898

Merged
merged 1 commit into from Oct 7, 2015
Merged

Use the correct IDL setter for <font>.size #7898

merged 1 commit into from Oct 7, 2015
+15 −2

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Oct 7, 2015

Previously, the IDL attribute would incorrectly set the size attribute
for <font> elements as AttrValue::String. Now it correctly sets it
as AttrValue::Length. Also included is a regression test.

Review on Reviewable
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 7, 2015

@nox r?
@frewsxcv frewsxcv mentioned this pull request Oct 7, 2015
Merged
@eefriedman
Copy link
Contributor

eefriedman commented Oct 7, 2015

Are you sure this is actually right?

The standard says "The color, face, and size IDL attributes of the font element must reflect the respective content attributes of the same name.", and there aren't any relevant special cases in the rules for reflection... so, for example, elem.size="foobar"; assert_equals(elem.size, "foobar"); should pass.

As far as I can tell, the parsing rules only apply to the presentational hint.
@eefriedman
Copy link
Contributor

eefriedman commented Oct 7, 2015

Nevermind, your commit looks right; I was misreading it.
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 7, 2015

I'm not sure I want a macro with a generic name such as make_length_setter to call parse_legacy_font_size. That seems like a footgun.
@Ms2ger Ms2ger self-assigned this Oct 7, 2015
@eefriedman
Copy link
Contributor

eefriedman commented Oct 7, 2015

I've been looking at the attribute stuff a bit more; it's not clear to me that there's a good reason for AttrValue::Length to exist. Having a field of type Option<Length> on HTMLFontElement, updated in HTMLFontElement::attribute_mutated, seems a lot more straightforward than stuffing an Option<Length> into AttrValue and performing an attribute lookup to retrieve the value in layout.
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 7, 2015

I'm not sure I want a macro with a generic name such as make_length_setter to call parse_legacy_font_size. That seems like a footgun.

Yes, you're absolutely right. I really need to stop writing code when I'm half awake.
@frewsxcv
Use the correct IDL setter for <font>.size
Loading status checks…
eabaf2c 
Previously, the IDL attribute would incorrectly set the `size` attribute
for `<font>` elements as `AttrValue::String`. Now it correctly sets it
as `AttrValue::Length`. Also included is a regression test.
@frewsxcv frewsxcv force-pushed the frewsxcv:htmlfontelement-size-attribute-setter branch from 5a092aa to eabaf2c Oct 7, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 7, 2015

Let me know how that latest force push looks
@nox
Copy link
Member

nox commented Oct 7, 2015

@bors-servo r+

Reviewed 2 of 2 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

📌 Commit eabaf2c has been approved by nox
@nox
Copy link
Member

nox commented Oct 7, 2015

Ah sorry @Ms2ger, didn't see you had assigned yourself.
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 7, 2015

@eefriedman With respect to your concerns, #7863 might be a better place to voice them
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

⌛ Testing commit eabaf2c with merge 60a77de...
bors-servo pushed a commit that referenced this pull request Oct 7, 2015
bors-servo
Auto merge of #7898 - frewsxcv:htmlfontelement-size-attribute-setter,…
Loading status checks…
60a77de 
… r=nox

Use the correct IDL setter for <font>.size

Previously, the IDL attribute would incorrectly set the `size` attribute
for `<font>` elements as `AttrValue::String`. Now it correctly sets it
as `AttrValue::Length`. Also included is a regression test.

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

bors-servo commented Oct 7, 2015

💔 Test failed - mac-rel-wpt
@nox
Copy link
Member

nox commented Oct 7, 2015

@bors-servo retry #7787
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit are reusable. Rebuilding only linux-rel, mac-rel-css, mac-rel-wpt...
@nox nox assigned nox and unassigned Ms2ger Oct 7, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit eabaf2c into servo:master Oct 7, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Oct 7, 2015
Closed
@frewsxcv frewsxcv deleted the frewsxcv:htmlfontelement-size-attribute-setter branch Oct 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@frewsxcv @eefriedman @Ms2ger @nox @bors-servo @highfive
You can’t perform that action at this time.