Stop parsing style attributes during restyle in geckolib. #11787

heycam · 2016-06-18T19:27:57Z

This allows PropertyDeclarationBlocks parsed for style="" attributes to be stored on a Gecko node so that we don't have to re-parse it each time we compute style for an element. Works with Gecko bug 1280772.

r? @bholley
CC @emilio

./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

highfive · 2016-06-18T19:28:01Z

Warning

These commits modify unsafe code. Please review it carefully!

bors-servo · 2016-06-21T22:20:15Z

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

heycam · 2016-06-22T07:18:19Z

A few questions on this updated patch:

Should the new GeckoDeclarationBlock struct live elsewhere?
Is it right to use RefCells for the two members that I need to be able to modify, and if so, should I combine them into a single RefCell?
Do you think it's important to have the Servo_ClearDeclarationBlockCachePointer function? (If not, we can make GeckoDeclarationBlock::cache not a RefCell.) If the nsHTMLCSSStyleSheet is going away, then we're in document shutdown, so we shouldn't need to look at the GeckoDeclarationBlock again.
In TElement::style_attribute for GeckoElement, I'm using transmute to get around lifetime issues when returning arc.declarations. Is there something better I should be doing?

bholley · 2016-06-22T21:05:15Z

Should the new GeckoDeclarationBlock struct live elsewhere?

Per the bug I think we should get rid of the pointer and the RefCell. And maybe just put the bool inside PropertyDeclarationBlock and conditionally compile it for geckolib?

Is it right to use RefCells for the two members that I need to be able to modify, and if so, should I combine them into a single RefCell?

N/A given the discussion in the bug.

Do you think it's important to have the Servo_ClearDeclarationBlockCachePointer function? (If not, we can make GeckoDeclarationBlock::cache not a RefCell.) If the nsHTMLCSSStyleSheet is going away, then we're in document shutdown, so we shouldn't need to look at the GeckoDeclarationBlock again.

Same.

In TElement::style_attribute for GeckoElement, I'm using transmute to get around lifetime issues when returning arc.declarations. Is there something better I should be doing?

I think you should be able to just use .as_ref() on the the *PropertyDeclarationBlock and the borrow checker should be satisfied.

heycam · 2016-06-23T07:11:25Z

Per the IRC discussion we had, I've left the nsHTMLCSSStyleSheet pointer and the immutable bool, and left them on the GeckoDeclarationBlock rather than move them up to PropertyDeclarationBlock and make them conditionally compiled.

bholley · 2016-06-23T17:05:34Z

ports/geckolib/glue.rs

+#[no_mangle]
+pub extern "C" fn Servo_GetDeclarationBlockCache(declarations: *mut ServoDeclarationBlock)
+                                                 -> *mut nsHTMLCSSStyleSheet {
+    let declarations: &GeckoDeclarationBlock = unsafe { transmute(declarations) };


We should be able to remove these transmutes by mapping the types together and then using .as_ref(). We do this elsewhere IIRC.

bholley · 2016-06-23T17:06:14Z

ports/geckolib/wrapper.rs

+            if ptr.is_null() {
+                &NO_STYLE_ATTRIBUTE
+            } else {
+                let declarations: &GeckoDeclarationBlock = transmute(ptr);


We should be able to eliminate this as well.

And .as_ref() will map nicely to the NO_STYLE_ATTRIBUTE case.

In general transmute is a big hammer, and we should restrict our uses to the cases where we really need it.

bholley · 2016-06-23T17:07:32Z

@bors-servo delegate+ with the transmutes removed.

bors-servo · 2016-06-23T17:07:33Z

✌️ @heycam can now approve this pull request

heycam · 2016-06-24T02:15:11Z

@bors-servo r=bholley

bors-servo · 2016-06-24T02:15:12Z

📌 Commit 21e378b has been approved by bholley

bors-servo · 2016-06-24T02:15:14Z

⌛ Testing commit 21e378b with merge 62fad97...


        Auto merge of #11787 - heycam:style-attr, r=bholley

@bholley

Stop parsing style attributes during restyle in geckolib.  This allows `PropertyDeclarationBlock`s parsed for `style=""` attributes to be stored on a Gecko node so that we don't have to re-parse it each time we compute style for an element. Works with [Gecko bug 1280772](https://bugzilla.mozilla.org/show_bug.cgi?id=1280772). r? @bholley CC @emilio ---  - [ ] `./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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11787)

bors-servo · 2016-06-24T02:25:09Z

💔 Test failed - linux-dev


        Stop parsing style attributes during restyle in geckolib.

heycam · 2016-06-24T02:38:48Z

@bors-servo retry

jdm · 2016-06-24T02:46:22Z

@bors-servo: r=bholley

bors-servo · 2016-06-24T02:46:23Z

📌 Commit 98a32a7 has been approved by bholley

bors-servo · 2016-06-24T02:46:26Z

⌛ Testing commit 98a32a7 with merge 581c8ba...


        Auto merge of #11787 - heycam:style-attr, r=bholley

@bholley

Stop parsing style attributes during restyle in geckolib.  This allows `PropertyDeclarationBlock`s parsed for `style=""` attributes to be stored on a Gecko node so that we don't have to re-parse it each time we compute style for an element. Works with [Gecko bug 1280772](https://bugzilla.mozilla.org/show_bug.cgi?id=1280772). r? @bholley CC @emilio ---  - [ ] `./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).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11787)

bors-servo · 2016-06-24T03:37:03Z

💔 Test failed - mac-rel-wpt

heycam · 2016-06-24T04:19:45Z

@bors-servo retry

#11847

bors-servo · 2016-06-24T04:19:47Z

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

bors-servo · 2016-06-24T04:56:17Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned bholley Jun 18, 2016

highfive added the S-awaiting-review label Jun 18, 2016

highfive added the S-needs-rebase label Jun 21, 2016

heycam force-pushed the heycam:style-attr branch from 2497893 to 401b627 Jun 22, 2016

heycam force-pushed the heycam:style-attr branch from 401b627 to b04061b Jun 23, 2016

bholley reviewed Jun 23, 2016
View changes

heycam force-pushed the heycam:style-attr branch from b04061b to 21e378b Jun 24, 2016

highfive removed S-awaiting-review S-needs-rebase labels Jun 24, 2016

highfive added the S-awaiting-merge label Jun 24, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 24, 2016

heycam force-pushed the heycam:style-attr branch from 21e378b to 98a32a7 Jun 24, 2016

Stop parsing style attributes during restyle in geckolib.

Loading status checks…

98a32a7

highfive added S-awaiting-review and removed S-tests-failed labels Jun 24, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 24, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 24, 2016

bors-servo merged commit 98a32a7 into servo:master Jun 24, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Stop parsing style attributes during restyle in geckolib. #11787

Stop parsing style attributes during restyle in geckolib. #11787

heycam commented Jun 18, 2016 •

edited by larsbergstrom

highfive commented Jun 18, 2016

bors-servo commented Jun 21, 2016

heycam commented Jun 22, 2016

bholley commented Jun 22, 2016

heycam commented Jun 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Jun 23, 2016

bors-servo commented Jun 23, 2016

heycam commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

heycam commented Jun 24, 2016

jdm commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

heycam commented Jun 24, 2016 •

edited

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

servo / servo

Join GitHub today

Stop parsing style attributes during restyle in geckolib. #11787

Stop parsing style attributes during restyle in geckolib. #11787

Conversation

heycam commented Jun 18, 2016 • edited by larsbergstrom

highfive commented Jun 18, 2016

bors-servo commented Jun 21, 2016

heycam commented Jun 22, 2016

bholley commented Jun 22, 2016

heycam commented Jun 23, 2016

This comment has been minimized.

bholley Jun 23, 2016

This comment has been minimized.

bholley Jun 23, 2016

This comment has been minimized.

bholley Jun 23, 2016

bholley commented Jun 23, 2016

bors-servo commented Jun 23, 2016

heycam commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

heycam commented Jun 24, 2016

jdm commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

heycam commented Jun 24, 2016 • edited

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

heycam commented Jun 18, 2016 •

edited by larsbergstrom

heycam commented Jun 24, 2016 •

edited