stylo: Use atoms as the pseudo-element back-end. #12815

emilio · 2016-08-11T06:23:18Z

A bit of work left, and we can uber-optimize this (left comments, will fill follow-ups), but this is a decent refactor so I thought I'd rather get some feedback on it.

r? @bholley (not formally yet, maybe, but some feedback is appreciated).

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

These changes do not require tests because geckolib only...

This change is

highfive · 2016-08-11T06:23:22Z

Heads up! This PR modifies the following files:

@bholley: components/style/gecko_selector_impl.rs, components/style/generated/gecko_pseudo_element_helper.rs

highfive · 2016-08-11T06:23:23Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style code, but no tests are modified. Please consider adding a test!

bholley · 2016-08-12T23:42:18Z

ports/geckolib/string_cache/lib.rs

+    }
+
+    #[inline]
+    pub unsafe fn from_static(ptr: *mut nsIAtom) -> Self {


Please document this.

emilio · 2016-08-12T23:45:06Z

ports/geckolib/binding_tools/regen.py

+        else:
+            print("OK")
+
+        return 0


whoops, return ret.

emilio · 2016-08-12T23:45:36Z

ports/geckolib/binding_tools/regen_atoms.py

+    CLASS = "nsGKAtoms"
+    TYPE = "nsICSSAnonBoxPseudo"
+
+    @staticmethod


This is not needed anymore.

bholley · 2016-08-12T23:45:51Z

ports/geckolib/string_cache/lib.rs

@@ -214,7 +223,7 @@ impl fmt::Debug for Atom {

 impl fmt::Display for Atom {
    fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
-        for c in char::decode_utf16(self.as_slice().iter().cloned()) {
+        for c in self.chars() {


I actually wanted @SimonSapin to remove this in #12571, because it makes it too easy to accidentally trigger a slow stringification. Can you revert this line and remove |pub fn chars|? I'd also be ok with chars() being non-public with a comment indicating that it's slow and shouldn't be exposed.

I’ve removed PartialEq<str> for Atom because, I agree, it’s easy to accidentally use it instead of fast atom comparison.

But consuming Atom::chars is O(n) like str::chars, and calling it has to be explicit, so I don’t see what the problem is. When possible, it’s better than using with_str which allocates.

bholley · 2016-08-12T23:50:32Z

components/style/gecko_selector_impl.rs

-    MozSVGForeignContent,
-    MozSVGText,
+// TODO: automatize this list from
+// http://searchfox.org/mozilla-central/source/layout/style/nsCSSPseudoElements.h and


This gets fixed up in future patches, right?

bholley · 2016-08-12T23:54:17Z

components/style/gecko_selector_impl.rs

+            match Self::from_atom(&*atom, true) {
+                Some(pseudo) => {
+                    assert_eq!(pseudo.is_anon_box(), is_anon_box);
+                    return pseudo;


Why do we need this return?

bholley · 2016-08-13T00:05:31Z

components/style/gecko_selector_impl.rs

+        #[derive(Clone)]
+        struct Utf16toASCIIIter<'a>(Iter<'a, u16>);
+
+        impl<'a> Iterator for Utf16toASCIIIter<'a> {


I don't understand why we need to do all this junk, though it looks like it goes away in subsequent patches? In the mean time, wouldn't we get the same effect by just comparing against atom!(":before") like from_bytes_iter does? Seems preferable to all that nasty unicode munging (which I didn't really look at because it goes away).

(Would still prefer to not introduce the nasty stuff at all though, even for a temporary patch)

bholley · 2016-08-13T00:16:50Z

components/style/gecko_selector_impl.rs

+        }
+
+        parse!("before", atom!(":before"));
+        parse!("after", atom!(":after"));


Do I understand correctly that this temporarily regresses our support for anon boxes during this patch? I don't see us passing |true| to parse! anywhere. I guess I'm ok with that, though generally would prefer to review less intermediate code...

bholley · 2016-08-13T00:17:03Z

components/style/gecko_selector_impl.rs

+                                                     in_ua_stylesheet: bool,
+                                                     byte_len: usize) -> Option<Self> {
+        use std::ascii::AsciiExt;
+        macro_rules! parse {


Per IRC discussion this macro should go away.

bholley · 2016-08-13T01:02:24Z

ports/geckolib/binding_tools/regen_atoms.py

+        self.value = value
+        self.cpp_class = source.CLASS
+        # FIXME(emilio): This is crap, and we really could/should just store a
+        # reference to the source, but...


Why not do that? :-)

bholley · 2016-08-13T01:03:52Z

ports/geckolib/binding_tools/regen_atoms.py

+ * Expected usage is as follows:
+ *
+ * Also, it guarantees the property that normal pseudo-elements are processed
+ * before anonymous boxes.


Nit: put this above the 'expected usage' part.

bholley · 2016-08-13T01:23:20Z

ports/geckolib/glue.rs

-            return ptr::null_mut();
-        }
-    };
+    let atom = unsafe { Atom::from_static(pseudo_tag) };


This is a nice optimization, but a bit scary, since it would lead to an unpaired Release() if we ever got it wrong (this only works because ReleaseAtom() is a no-op for static atoms).

Can we make the gecko Atom struct carry an extra bit that indicates whether the atom was created with from_static, and skip the ReleaseAtom() call in that case? That would reduce the risk here, and also save an FFI call.

It's probably better if we generate accessors for the atom bitfield, with the extra benefit that gives us the length (which means in some cases we don't need to make an ffi call to make some comparisons).

Oh, also, note that from_atom_unchecked still checks the atom in debug builds.

bholley · 2016-08-13T01:25:34Z

ports/geckolib/string_cache/lib.rs

-            Gecko_AtomEqualsUTF8IgnoreCase(self.as_ptr(), s.as_ptr() as *const _, s.len() as u32)
+            Gecko_AtomEqualsUTF8IgnoreCase(self.as_ptr(),
+                                           bytes.as_ptr() as *const _,
+                                           bytes.len() as u32)


yikes. @Manishearth, any ideas on tooling to prevent us from making errors like this in the future?

I think this change is a no-op: str::len returns a number of UTF-8 bytes, not of code points.

That's true, I removed it. I'll also fill follow-ups to remove the FFI
functions for string comparison and a few other things.

On 08/13/2016 09:47 AM, Simon Sapin wrote:

In ports/geckolib/string_cache/lib.rs
#12815 (comment):

unsafe {

Gecko_AtomEqualsUTF8IgnoreCase(self.as_ptr(), s.as_ptr() as *const _, s.len() as u32)

Gecko_AtomEqualsUTF8IgnoreCase(self.as_ptr(),

bytes.as_ptr() as *const _,

bytes.len() as u32)

I think this change is a no-op: |str::len| returns a number of UTF-8
bytes, not of code points.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/servo/servo/pull/12815/files/93e6fb1a6569a2b6934cb28c8d4889122d0b2897..22a7892934b829e32c75a8e408b5569535f13414#r74689195,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwurP0WCuwUHsCBBfOmvp2EkqOgZ76ks5qffUtgaJpZM4Jh0-a.

bholley · 2016-08-13T01:48:03Z

components/style/properties/gecko.mako.rs

@@ -1339,6 +1339,80 @@ fn static_assert() {
    ${impl_coord_copy('column_width', 'mColumnWidth')}
 </%self:impl_trait>

+<%self:impl_trait style_struct_name="Counters"


Nit: The comment message should say "the content property"

Actually, could you split this commit out into a separate PR? I've got to run pretty soon and may not have time to review the whole thing, and it's pretty separate from the atom stuff (and also presumably needs some gecko-side changes).

bholley · 2016-08-13T01:50:51Z

r=me with comments addressed on everything but the |content| patch, which I'd like to see in a separate PR.

@bors-servo delegate+

bors-servo · 2016-08-13T01:50:52Z

✌️ @emilio can now approve this pull request

SimonSapin · 2016-08-13T15:04:34Z

components/style/gecko_selector_impl.rs

-
-    AnonBox(AnonBoxPseudoElement),
-}
+pub struct PseudoElement(Atom, bool);


Maybe use struct syntax with named fields? I particular to say what this bool is.

It's documented above (though the diff makes it difficult to find), but
yeah, it's fair, I'll do it.

On 08/13/2016 08:04 AM, Simon Sapin wrote:

In components/style/gecko_selector_impl.rs
#12815 (comment):

MozNumberWrapper,

MozNumberText,

MozNumberSpinBox,

MozNumberSpinUp,

MozNumberSpinDown,

MozProgressBar,

MozRangeTrack,

MozRangeProgress,

MozRangeThumb,

MozMeterBar,

MozPlaceholder,

- MozColorSwatch,

AnonBox(AnonBoxPseudoElement),
-}
+pub struct PseudoElement(Atom, bool);

Maybe use struct syntax with named fields? I particular to say what this
|bool| is.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/servo/servo/pull/12815/files/8a92235c7c372b7d884a6928608a7477e2af7c70#r74687932,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwull1-KtkBA4749IbfIn2nU6g_1AUks5qfd0PgaJpZM4Jh0-a.


        string_cache nits.


        stylo: Move gecko_bindins/tools to binding_tools

emilio · 2016-08-16T06:04:04Z

ports/geckolib/binding_tools/regen_atoms.py

+
+
+def msvc32_symbolify(source, ident):
+    return "?" + ident + "@" + source.CLASS + "@@2PAV" + source.TYPE + "@@A"


cc @upsuper, hopefully I got the msvc manglings fine.

TBH, I have no idea about msvc manglings. Their mangling rule looks complicated, so I disassembled the object file and copied the pattern to here directly.

If the type is just pointers, I guess reusing this is probably fine, but I'm unconfident about that anyway.

emilio · 2016-08-16T06:08:25Z

Left just the atom patches, will submit the content patches after this lands.

@bors-servo: r=bholley

bors-servo · 2016-08-16T06:08:26Z

📌 Commit 09cc240 has been approved by bholley

bors-servo · 2016-08-16T06:08:28Z

⌛ Testing commit 09cc240 with merge d9adca6...


        Auto merge of #12815 - emilio:stylo-atoms, r=bholley

@bholley

stylo: Use atoms as the pseudo-element back-end.  A bit of work left, and we can uber-optimize this (left comments, will fill follow-ups), but this is a decent refactor so I thought I'd rather get some feedback on it. r? @bholley (not formally yet, maybe, but some feedback is appreciated). ---  - [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 geckolib only...   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12815)

bors-servo · 2016-08-16T06:54:55Z

💔 Test failed - linux-rel

highfive · 2016-08-16T06:54:57Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/perspective-origin-006.htm
  └   → /css-transforms-1_dev/html/perspective-origin-006.htm d2f9f49eb190646b004d0ea13de938dfa265561d
/css-transforms-1_dev/html/reference/ref-filled-green-100px-square.htm d4aa213e3e31e41d0d8e84aec79e94f860f27178
Testing d2f9f49eb190646b004d0ea13de938dfa265561d == d4aa213e3e31e41d0d8e84aec79e94f860f27178

emilio · 2016-08-16T07:57:26Z

@bors-servo: retry #9087

bors-servo · 2016-08-16T07:57:27Z

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

bors-servo · 2016-08-16T08:43:29Z

💔 Test failed - linux-rel

highfive · 2016-08-16T08:43:31Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/perspective-origin-006.htm
  └   → /css-transforms-1_dev/html/perspective-origin-006.htm d2f9f49eb190646b004d0ea13de938dfa265561d
/css-transforms-1_dev/html/reference/ref-filled-green-100px-square.htm d4aa213e3e31e41d0d8e84aec79e94f860f27178
Testing d2f9f49eb190646b004d0ea13de938dfa265561d == d4aa213e3e31e41d0d8e84aec79e94f860f27178

emilio · 2016-08-16T17:42:57Z

@bors-servo: retry

#9087 again

bors-servo · 2016-08-16T17:50:30Z

⌛ Testing commit 09cc240 with merge c386a3c...


        Auto merge of #12815 - emilio:stylo-atoms, r=bholley

@bholley

stylo: Use atoms as the pseudo-element back-end.  A bit of work left, and we can uber-optimize this (left comments, will fill follow-ups), but this is a decent refactor so I thought I'd rather get some feedback on it. r? @bholley (not formally yet, maybe, but some feedback is appreciated). ---  - [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 geckolib only...   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12815)

bors-servo · 2016-08-16T18:48:18Z

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

upsuper · 2016-08-19T11:50:24Z

Looks like it works fine with MSVC.

highfive assigned bholley Aug 11, 2016

highfive added the S-awaiting-review label Aug 11, 2016

bholley reviewed Aug 12, 2016
View changes

ports/geckolib/string_cache/lib.rs Outdated

}

#[inline]

pub unsafe fn from_static(ptr: *mut nsIAtom) -> Self {

This comment has been minimized.

Sign in to view

bholley Aug 12, 2016
Contributor

Please document this.

emilio reviewed Aug 12, 2016
View changes

ports/geckolib/binding_tools/regen.py Outdated

else:

print("OK")

return 0

This comment has been minimized.

Sign in to view

emilio Aug 12, 2016
Author Member

whoops, return ret.

emilio reviewed Aug 12, 2016
View changes

ports/geckolib/binding_tools/regen_atoms.py Outdated

CLASS = "nsGKAtoms"

TYPE = "nsICSSAnonBoxPseudo"

@staticmethod

This comment has been minimized.

Sign in to view

emilio Aug 12, 2016
Author Member

This is not needed anymore.

bholley reviewed Aug 12, 2016
View changes

components/style/gecko_selector_impl.rs Outdated

match Self::from_atom(&*atom, true) {

Some(pseudo) => {

assert_eq!(pseudo.is_anon_box(), is_anon_box);

return pseudo;

This comment has been minimized.

Sign in to view

bholley Aug 12, 2016
Contributor

Why do we need this return?

bholley reviewed Aug 13, 2016
View changes

components/style/gecko_selector_impl.rs Outdated

in_ua_stylesheet: bool,

byte_len: usize) -> Option<Self> {

use std::ascii::AsciiExt;

macro_rules! parse {

This comment has been minimized.

Sign in to view

bholley Aug 13, 2016
Contributor

Per IRC discussion this macro should go away.

emilio force-pushed the emilio:stylo-atoms branch 2 times, most recently from 8dd76af to 01f981a Aug 13, 2016

bholley reviewed Aug 13, 2016
View changes

emilio force-pushed the emilio:stylo-atoms branch 2 times, most recently from dd159cc to 8a92235 Aug 13, 2016

SimonSapin reviewed Aug 13, 2016
View changes

emilio force-pushed the emilio:stylo-atoms branch 3 times, most recently from 2a71630 to 15f8390 Aug 13, 2016

emilio added 2 commits Aug 10, 2016

string_cache nits.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

f9f347e

stylo: Move gecko_bindins/tools to binding_tools

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

54b9201

emilio force-pushed the emilio:stylo-atoms branch from 4827a24 to f783caf Aug 16, 2016

emilio reviewed Aug 16, 2016
View changes

emilio force-pushed the emilio:stylo-atoms branch from f783caf to 09cc240 Aug 16, 2016

highfive removed the S-awaiting-review label Aug 16, 2016

highfive added the S-awaiting-merge label Aug 16, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 16, 2016

highfive added the S-tests-failed label Aug 16, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 16, 2016

bors-servo merged commit 09cc240 into servo:master Aug 16, 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 Aug 16, 2016

This was referenced Aug 16, 2016

Add safer bindings for dealing with owned Arcs over FFI #12826

Merged

stylo: Support clip-path #12878

Merged

emilio deleted the emilio:stylo-atoms branch Aug 16, 2016



		def msvc32_symbolify(source, ident):
		return "?" + ident + "@" + source.CLASS + "@@2PAV" + source.TYPE + "@@A"

servo / servo

Join GitHub today

stylo: Use atoms as the pseudo-element back-end. #12815

stylo: Use atoms as the pseudo-element back-end. #12815

Conversation

emilio commented Aug 11, 2016 • edited by larsbergstrom

highfive commented Aug 11, 2016

highfive commented Aug 11, 2016

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.

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 Aug 13, 2016

bors-servo commented Aug 13, 2016

This comment has been minimized.

This comment has been minimized.

- MozColorSwatch,

This comment has been minimized.

This comment has been minimized.

upsuper Aug 17, 2016 • edited

emilio commented Aug 16, 2016

bors-servo commented Aug 16, 2016

bors-servo commented Aug 16, 2016

bors-servo commented Aug 16, 2016

highfive commented Aug 16, 2016

emilio commented Aug 16, 2016

bors-servo commented Aug 16, 2016

bors-servo commented Aug 16, 2016

highfive commented Aug 16, 2016

emilio commented Aug 16, 2016

bors-servo commented Aug 16, 2016

bors-servo commented Aug 16, 2016

upsuper commented Aug 19, 2016

emilio commented Aug 11, 2016 •

edited by larsbergstrom

upsuper Aug 17, 2016 •

edited