Implement <font> 'face' attribute

[frewsxcv]

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[nox]

./components/script/dom/element.rs:65: use statement is not in alphabetical order
    expected: style::properties::longhands::font_family
    found: style::properties::longhands::{self, background_image, border_spacing}
./components/script/dom/element.rs:66: use statement is not in alphabetical order
    expected: style::properties::longhands::{self, background_image, border_spacing}
    found: style::properties::longhands::font_family

[frewsxcv]

Ah, thanks. Missed that

[nox]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r1, 7 of 8 files at r2.
Review status: 7 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

^{components/script/dom/element.rs, line 66 [r2] (raw file):}
Just do only one import from longhands to keep tidy happy.

---

^{components/script/dom/htmlfontelement.rs, line 26 [r2] (raw file):}
DOMRefCell<Option<DOMString>> would be better here, IMO.

---

^{components/script/dom/htmlfontelement.rs, line 86 [r2] (raw file):}
Should do value.as_string().clone(), there is no reason this attribute should be something else than AttrValue::String.

---

^{components/script/dom/htmlfontelement.rs, line 108 [r2] (raw file):}
Store the string only if non-empty.

---

Comments from the review on Reviewable.io

[nox]

Reviewed 1 of 8 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[frewsxcv]

^{components/script/dom/htmlfontelement.rs, line 86 [r2] (raw file):}
There is no as_string method on AttrValue. Should I add one?

---

Comments from the review on Reviewable.io

[nox]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

^{components/script/dom/htmlfontelement.rs, line 86 [r2] (raw file):}
Yes. I guess there are none because you can just Deref to &str, but I would rather have it panic if it's not a string. These things aren't supposed to change their variant after they are set for the first time.

---

Comments from the review on Reviewable.io

[frewsxcv]

How do those changes look?

---

Comments from the review on Reviewable.io

[nox]

-S-awaiting-review -S-fails-tidy +S-needs-code-changes

One last remark.

---

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{components/script/dom/htmlfontelement.rs, line 86 [r3] (raw file):}
No need for the deref here, AFAICT.

---

Comments from the review on Reviewable.io

[frewsxcv]

^{components/script/dom/htmlfontelement.rs, line 86 [r3] (raw file):}
You are right. Fixed in the latest force push

---

Comments from the review on Reviewable.io

[nox]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{components/script/dom/htmlfontelement.rs, line 86 [r4] (raw file):}
Mmmh, actually, I'm sorry, I didn't notice the attribute value was ultimately converted to an Atom when pushing the presentational hint. Could you implement the virtual method parse_plain_attribute(), make that return an AttrValue::Atom for the font attribute, and then clone that atom into the face field here? If you need more information on how to do this, feel free to ask me questions; and if you don't, that can also be filed as a follow-up.

Sorry for not noticing this earlier.

---

Comments from the review on Reviewable.io

[frewsxcv]

^{components/script/dom/htmlfontelement.rs, line 86 [r4] (raw file):}

Sorry for not noticing this earlier.

No reason to be sorry :)

Yeah, I think I understand all of that. I'm on a train right now and my laptop is about to die, but I'll try to get to it tonight

---

Comments from the review on Reviewable.io

[nox]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{components/script/dom/htmlfontelement.rs, line 86 [r4] (raw file):}
Cool!

I'll have a thought for your laptop. Poor laptop. RIP.

---

Comments from the review on Reviewable.io

[frewsxcv]

^{components/script/dom/htmlfontelement.rs, line 86 [r4] (raw file):}
Should the face field on the HTMLFontElement struct continue to be a String or should it be changed to an Atom?

---

Comments from the review on Reviewable.io

[nox]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{components/script/dom/htmlfontelement.rs, line 86 [r4] (raw file):}
Atom too.

---

Comments from the review on Reviewable.io

[nox]

@bors-servo r+

---

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 3ae76f4 has been approved by nox

[bors-servo]

⌛ Testing commit 3ae76f4 with merge 25abe64...

[bors-servo]

💔 Test failed - mac-rel-wpt

[nox]

@bors-servo retry

#7422

[bors-servo]

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit are reusable. Rebuilding only linux-rel, mac-rel-css, mac-rel-wpt...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt