stylo: Remove a bunch of unneeded ool-calls. #13440

emilio · 2016-09-26T19:50:49Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors

These changes do not require tests because stylo

This change is

KiChjang · 2016-09-26T19:52:44Z

That's an interesting title.

emilio · 2016-09-26T20:17:40Z

@KiChjang Yeah, hub is fun.

r? @bholley

bholley

lgtm modulo those issues.

bholley · 2016-09-27T19:34:27Z

components/style/gecko/wrapper.rs

@@ -96,10 +92,36 @@ fn from_opaque_style_data(d: *mut OpaqueStyleData) -> *mut NonOpaqueStyleData {
 pub struct GeckoNode<'ln>(pub &'ln RawGeckoNode);

 impl<'ln> GeckoNode<'ln> {
+    fn from_content(content: &'ln nsIContent) -> Self {
+        use std::mem;
+        GeckoNode(unsafe { mem::transmute(content) })


Why do we need to do this instead of just pulling it out of content._base?

Whoops, nice catch.

bholley · 2016-09-27T19:34:27Z

components/style/gecko/wrapper.rs

+        GeckoNode(unsafe { mem::transmute(content) })
+    }
+
+    fn node_info(&self) -> *mut structs::NodeInfo {


Wouldn't it be nicer to return a &structs::NodeInfo so that callers can read it without an unsafe dereference? NodeInfo can never be null.

Yep, at first I thought we might want to mutate it and didn't want to discuss aliasing issues, but it didn't happen.

bholley · 2016-09-27T19:34:27Z

components/style/gecko/wrapper.rs

+    // FIXME: We can implement this without OOL calls, but we can't easily given
+    // GeckoNode is a raw reference.
+    //
+    // We can use a Cell<T>, but that's a bit of a pain.


I thought the idea as that we rewrite flags as an AtomicUsize (which has the same interior mutability semantics as Cell)? Seems like that would solve this problem.

Fine to do in a followup if you like though.

Hmm... I thought the idea was setting (some of) those flags atomically from Gecko (which is the equivalent to use the intrinsics::atomic_or operation in Rust, but that's unstable.

In any case, if we want to rely in atomics being raw types with atomic operations, that might not be true in all platforms, we'd need an AtomicUInt32, which is also unstable.

Hm, so what's our plan here? Do we really need to continue using FFI calls on the flags until AtomicU32 stablizes?

Pretty much, since the other alternative (implementing it ourselves with asm! for platforms we care about) is also unstable.

bholley · 2016-09-27T19:34:27Z

components/style/gecko/wrapper.rs

        unsafe {
-            Gecko_IsTextNode(self.0)


We'll presumably want a corresponding gecko bug to remove the unused APIs (followup is of course fine).

bholley · 2016-09-27T23:21:45Z

@bors-servo r+

bors-servo · 2016-09-27T23:21:46Z

📌 Commit 0aa9c24 has been approved by bholley

bors-servo · 2016-09-28T03:48:43Z

⌛ Testing commit 0aa9c24 with merge 017b326...


        Auto merge of #13440 - emilio:no-ool, r=bholley

stylo: Remove a bunch of unneeded ool-calls. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because stylo   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13440)

bors-servo · 2016-09-28T03:50:16Z

💔 Test failed - mac-rel-wpt

KiChjang · 2016-09-28T03:52:47Z

@bors-servo retry

bors-servo · 2016-09-28T03:53:50Z

⌛ Testing commit 0aa9c24 with merge 2ab517e...


        Auto merge of #13440 - emilio:no-ool, r=bholley

stylo: Remove a bunch of unneeded ool-calls. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because stylo   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13440)

bors-servo · 2016-09-28T03:55:47Z

💔 Test failed - mac-rel-wpt

KiChjang · 2016-09-28T04:03:04Z

@bors-servo retry

jdm · 2016-10-01T14:48:08Z

/Users/servo/buildbot/slave/mac-rel-css/build/components/style/gecko/wrapper.rs:591:27: 591:74 error: attempted access of field `mRawPtr` on type `gecko_bindings::structs::nsCOMPtr<gecko_bindings::structs::nsIAtom>`, but no field with that name was found
/Users/servo/buildbot/slave/mac-rel-css/build/components/style/gecko/wrapper.rs:591             WeakAtom::new(self.as_node().node_info().mInner.mName.mRawPtr)
                                                                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/servo/buildbot/slave/mac-rel-css/build/components/style/gecko/wrapper.rs:591:67: 591:74 help: did you mean `_base`?
/Users/servo/buildbot/slave/mac-rel-css/build/components/style/gecko/wrapper.rs:591             WeakAtom::new(self.as_node().node_info().mInner.mName.mRawPtr)
                                                                                                                                                      ^~~~~~~
error: aborting due to previous error
error: Could not compile `style`.

emilio · 2016-10-02T00:53:13Z

I'm pretty sure that field exists, just rebased and worked without any change. I guess some of the recent regenerations made this fail spuriously?

@bors-servo: r=bholley

bors-servo · 2016-10-02T00:53:15Z

📌 Commit 11de803 has been approved by bholley

bors-servo · 2016-10-02T00:53:17Z

⌛ Testing commit 11de803 with merge 778c39f...


        Auto merge of #13440 - emilio:no-ool, r=bholley

stylo: Remove a bunch of unneeded ool-calls. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because stylo   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13440)

bors-servo · 2016-10-02T01:38:51Z

💔 Test failed - mac-rel-css

emilio · 2016-10-02T01:49:17Z

Pfft, I'm stupid.

We inherit conditionally depending on the build type, for some reason
http://searchfox.org/mozilla-central/source/xpcom/glue/nsCOMPtr.h#350.

I'll make a proper fix for it when I find the time.

On 10/02/2016 03:39 AM, bors-servo wrote:

💔 Test failed - mac-rel-css
http://build.servo.org/builders/mac-rel-css/builds/3335

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#13440 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwumzv-wWwZ4vNZVI-IcEh-4K2lCApks5qvws3gaJpZM4KG7FV.


        stylo: Remove a bunch of out of now-unneeded out-of-line calls.

@bholley

r? @bholley

emilio · 2016-10-02T15:52:37Z

I added a new sugar module for nsCOMPtr that handles this properly depending on the build type.

@bors-servo: r=bholley

bors-servo · 2016-10-02T15:52:38Z

📌 Commit eb0c12a has been approved by bholley

bors-servo · 2016-10-02T15:59:26Z

⌛ Testing commit eb0c12a with merge c4021a6...


        Auto merge of #13440 - emilio:no-ool, r=bholley

stylo: Remove a bunch of unneeded ool-calls. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] These changes do not require tests because stylo   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13440)

bors-servo · 2016-10-02T17:00:33Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned asajeffrey Sep 26, 2016

highfive added the S-awaiting-review label Sep 26, 2016

emilio force-pushed the emilio:no-ool branch from 8327121 to 9789b93 Sep 26, 2016

emilio changed the title ~~~~ stylo: Remove a bunch of unneeded ool-calls. Sep 26, 2016

highfive assigned bholley and unassigned asajeffrey Sep 26, 2016

emilio force-pushed the emilio:no-ool branch from 9789b93 to 6702f65 Sep 27, 2016

bholley reviewed Sep 27, 2016

View changes

emilio force-pushed the emilio:no-ool branch 2 times, most recently from dcf4351 to 0aa9c24 Sep 27, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 27, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 28, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Sep 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 1, 2016

emilio force-pushed the emilio:no-ool branch from 0aa9c24 to 11de803 Oct 2, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Oct 2, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 2, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 2, 2016

emilio force-pushed the emilio:no-ool branch from 11de803 to 0e82fe4 Oct 2, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Oct 2, 2016

stylo: Remove a bunch of out of now-unneeded out-of-line calls.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

eb0c12a

r? @bholley

emilio force-pushed the emilio:no-ool branch from 0e82fe4 to eb0c12a Oct 2, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 2, 2016

bors-servo merged commit eb0c12a into servo:master Oct 2, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 2, 2016

servo / servo

stylo: Remove a bunch of unneeded ool-calls. #13440

stylo: Remove a bunch of unneeded ool-calls. #13440

emilio commented Sep 26, 2016 •

edited by larsbergstrom

KiChjang commented Sep 26, 2016

emilio commented Sep 26, 2016

bholley left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Sep 27, 2016

bors-servo commented Sep 27, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

KiChjang commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

KiChjang commented Sep 28, 2016

jdm commented Oct 1, 2016

emilio commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

emilio commented Oct 2, 2016

emilio commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

servo / servo

Join GitHub today

stylo: Remove a bunch of unneeded ool-calls. #13440

stylo: Remove a bunch of unneeded ool-calls. #13440

Conversation

emilio commented Sep 26, 2016 • edited by larsbergstrom

KiChjang commented Sep 26, 2016

emilio commented Sep 26, 2016

bholley left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Sep 27, 2016

bors-servo commented Sep 27, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

KiChjang commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

KiChjang commented Sep 28, 2016

jdm commented Oct 1, 2016

emilio commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

emilio commented Oct 2, 2016

emilio commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

emilio commented Sep 26, 2016 •

edited by larsbergstrom