Misc cleanup for DOM attributes #8404

eefriedman · 2015-11-08T08:48:51Z

No functional change; just cleanups related to parsing attributes.

highfive · 2015-11-08T08:48:55Z

Warning

These commits modify unsafe code. Please review it carefully!

nox · 2015-11-08T12:02:12Z

A minor nit, otherwise LGTM.

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

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

^{components/script/dom/htmlbodyelement.rs, line 100 [r3] (raw file):}
I'm pretty sure the & is superfluous.

^{components/script/dom/htmlbodyelement.rs, line 110 [r3] (raw file):}
Ditto.

^{components/script/dom/htmlbodyelement.rs, line 120 [r3] (raw file):}
Ditto.

^{components/script/dom/htmlfontelement.rs, line 94 [r3] (raw file):}
Ditto.

^{components/script/dom/htmlfontelement.rs, line 104 [r3] (raw file):}
Ditto.

^{components/script/dom/htmlfontelement.rs, line 114 [r3] (raw file):}
Ditto.

^{components/script/dom/htmliframeelement.rs, line 208 [r3] (raw file):}
Ditto.

^{components/script/dom/htmliframeelement.rs, line 210 [r3] (raw file):}
We should probably file a followup to make this a variant of AttrValue too.

^{components/script/dom/htmliframeelement.rs, line 218 [r3] (raw file):}
Ditto.

Comments from the review on Reviewable.io

eefriedman · 2015-11-08T18:59:57Z

Review status: 1 of 6 files reviewed at latest revision, 9 unresolved discussions.

^{components/script/dom/htmliframeelement.rs, line 210 [r3] (raw file):}
Sure; I'll file it.

Comments from the review on Reviewable.io

eefriedman · 2015-11-08T19:00:42Z

@bors-servo r=nox

bors-servo · 2015-11-08T19:00:42Z

📌 Commit c38856d has been approved by nox

bors-servo · 2015-11-08T19:21:04Z

⌛ Testing commit c38856d with merge 928aba0...


        Auto merge of #8404 - eefriedman:misc-attributes, r=nox

Misc cleanup for DOM attributes No functional change; just cleanups related to parsing attributes.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8404)

bors-servo · 2015-11-08T19:53:22Z

💔 Test failed - mac-rel-wpt

frewsxcv · 2015-11-08T19:54:46Z

Fail seems legit

  ▶ TIMEOUT [expected OK] /html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/document-color-02.html
  │ 
  │ thread 'LayoutWorker worker 1/6' panicked at 'Color not found', /Users/servo/buildbot/slave/mac-rel-wpt/build/components/script/dom/attr.rs:118
  │ stack backtrace:
  │    1:        0x10b7f9b48 - sys::backtrace::tracing::imp::write::h662793599f713c74T5s
  │    2:        0x10b7fb63f - panicking::log_panic::_<closure>::closure.38923
  │    3:        0x10b7fb0e9 - panicking::log_panic::h87af6096e0b6fa7c4Uw
  │    4:        0x10b7e9dd6 - sys_common::unwind::begin_unwind_inner::h1e338bc64cf56d9dr8r
  │    5:        0x10a4c0924 - sys_common::unwind::begin_unwind::begin_unwind::h11769312394538122404
  │    6:        0x10a7fe5fc - dom::htmlbodyelement::_<impl>::get_background_color::h96cce9c616bb54e5ktw
  │    7:        0x10a4ae39a - css::matching::_<impl>::match_element::h823c88ff0903b64c9Hz
  │    8:        0x10a493160 - traversal::_<impl>::process::h3858f81d9981cb5bvYx
  │    9:        0x10a492821 - parallel::_<impl>::run_parallel::h13d7b06c0342da40aet
  │   10:        0x10a493d7e - parallel::recalc_style::h5edbc18b903df6bcAet
  │   11:        0x10a43113e - boxed::_<impl>::call_box::call_box::h11765958047692383731
  │   12:        0x10a40d5c2 - sys_common::unwind::try::try_fn::try_fn::h6634675915793128526
  │   13:        0x10b7f9018 - __rust_try
  │   14:        0x10b7f64be - sys_common::unwind::try::inner_try::hc13d8e198528cd0fZ4r
  │   15:        0x10a40d767 - boxed::_<impl>::call_box::call_box::h5255149473090508000
  │   16:        0x10b7faa0d - sys::thread::_<impl>::new::thread_start::h906ddce3a93d4852ksw
  │   17:     0x7fff8416a059 - _pthread_body
  └   18:     0x7fff84169fd6 - _pthread_start

frewsxcv · 2015-11-08T20:23:50Z

components/script/dom/htmlbodyelement.rs

-        match name {
-            &atom!("text") => AttrValue::from_legacy_color(value),
+        match *name {
+            atom!("bgcolor") |


Should this be bgColor?

frewsxcv · 2015-11-08T20:24:44Z

components/script/dom/htmlbodyelement.rs

+    fn get_background_color(&self) -> Option<RGBA> {
+        unsafe {
+            (&*self.upcast::<Element>().unsafe_get())
+                .get_attr_for_layout(&ns!(""), &atom!("bgcolor"))


Should this be bgColor?

I actually don't know, just pointing out one of these might be capitalized

HTML attributes are all lower-case; bgcolor is correct.

(I tracked down the reason for the test failure; it actually isn't a mistake in htmlbodyelement.rs.)

eefriedman · 2015-11-08T20:38:38Z

@nox Updated with a fix for document.rs ; please double-check that the change makes sense.

bors-servo · 2015-11-09T00:49:10Z

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


        Move storage of bgcolor attribute on <body>.


        Move storage of color attribute on <font>.

nox · 2015-11-09T13:44:05Z

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 2 of 2 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

^{components/script/dom/htmlbodyelement.rs, line 100 [r3] (raw file):}
You forgot that one.

Comments from the review on Reviewable.io


        Move unsafe layout calls onto LayoutJS.


        Move more unsafe layout calls onto LayoutJS.

eefriedman · 2015-11-09T19:02:20Z

@bors-servo r=nox

bors-servo · 2015-11-09T19:02:22Z

📌 Commit af78173 has been approved by nox

bors-servo · 2015-11-09T19:02:24Z

⌛ Testing commit af78173 with merge d8df028...


        Auto merge of #8404 - eefriedman:misc-attributes, r=nox

Misc cleanup for DOM attributes No functional change; just cleanups related to parsing attributes.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8404)

bors-servo · 2015-11-09T19:05:45Z

💔 Test failed - gonk

eefriedman · 2015-11-09T19:07:53Z

@bors-servo retry #8416

bors-servo · 2015-11-09T19:21:10Z

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

bors-servo · 2015-11-09T19:41:22Z

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

highfive added the S-awaiting-review label Nov 8, 2015

nox self-assigned this Nov 8, 2015

nox added S-needs-code-changes A-content/dom I-cleanup and removed S-awaiting-review labels Nov 8, 2015

eefriedman mentioned this pull request Nov 8, 2015

Remove HTMLFontElement::color parsed attribute value #8411

Closed

eefriedman force-pushed the eefriedman:misc-attributes branch from 1a0819b to c38856d Nov 8, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 8, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 8, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 8, 2015

frewsxcv reviewed Nov 8, 2015
View changes

components/script/dom/htmlbodyelement.rs

match name {

&atom!("text") => AttrValue::from_legacy_color(value),

match *name {

atom!("bgcolor") |

This comment has been minimized.

Sign in to view

Copy link

frewsxcv Nov 8, 2015
Member

Should this be bgColor?

frewsxcv reviewed Nov 8, 2015
View changes

eefriedman force-pushed the eefriedman:misc-attributes branch from c38856d to 224fb63 Nov 8, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Nov 8, 2015

eefriedman added 2 commits Nov 8, 2015

Move storage of bgcolor attribute on <body>.

0901e5b

Move storage of color attribute on <font>.

ef52da7

eefriedman force-pushed the eefriedman:misc-attributes branch from 224fb63 to bee81d9 Nov 9, 2015

eefriedman removed the S-needs-rebase label Nov 9, 2015

nox added S-needs-code-changes S-needs-rebase and removed S-awaiting-review labels Nov 9, 2015

eefriedman added 2 commits Nov 8, 2015

Move unsafe layout calls onto LayoutJS.

5293afc

Move more unsafe layout calls onto LayoutJS.

Loading status checks…

af78173

eefriedman force-pushed the eefriedman:misc-attributes branch from bee81d9 to af78173 Nov 9, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 9, 2015

highfive removed S-awaiting-review S-needs-rebase labels Nov 9, 2015

highfive added the S-awaiting-merge label Nov 9, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 9, 2015

bors-servo mentioned this pull request Nov 9, 2015

Invoke specialized callback behaviour for OnErrorEventHandler. #8430

Merged

eefriedman mentioned this pull request Nov 9, 2015

Remove fields with parsed attribute values. #7863

Open

SimonSapin removed the S-tests-failed label Jan 27, 2016

servo / servo

Misc cleanup for DOM attributes #8404

Misc cleanup for DOM attributes #8404

eefriedman commented Nov 8, 2015

highfive commented Nov 8, 2015

nox commented Nov 8, 2015

eefriedman commented Nov 8, 2015

eefriedman commented Nov 8, 2015

bors-servo commented Nov 8, 2015

bors-servo commented Nov 8, 2015

bors-servo commented Nov 8, 2015

frewsxcv commented Nov 8, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eefriedman commented Nov 8, 2015

bors-servo commented Nov 9, 2015

nox commented Nov 9, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

servo / servo

Join GitHub today

Misc cleanup for DOM attributes #8404

Misc cleanup for DOM attributes #8404

Conversation

eefriedman commented Nov 8, 2015

highfive commented Nov 8, 2015

nox commented Nov 8, 2015

eefriedman commented Nov 8, 2015

eefriedman commented Nov 8, 2015

bors-servo commented Nov 8, 2015

bors-servo commented Nov 8, 2015

bors-servo commented Nov 8, 2015

frewsxcv commented Nov 8, 2015

This comment has been minimized.

frewsxcv Nov 8, 2015

This comment has been minimized.

frewsxcv Nov 8, 2015

This comment has been minimized.

eefriedman Nov 8, 2015

eefriedman commented Nov 8, 2015

bors-servo commented Nov 9, 2015

nox commented Nov 9, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015

eefriedman commented Nov 9, 2015

bors-servo commented Nov 9, 2015

bors-servo commented Nov 9, 2015