style: Rewrite the restyle hints code to allow different kinds of element snapshots. #12469

emilio · 2016-07-16T00:46:02Z

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

These changes do not require tests because refactoring.

This is a rewrite for how style interfaces with its consumers in order to allow
different representations for an element snapshot.

This also changes the requirements of an element snapshot, requiring them to
only implement MatchAttr, instead of MatchAttrGeneric. This is important for
stylo since implementing MatchAttrGeneric is way more difficult for us given the
atom limitations. This also allows for more performant implementations in the
Gecko side of things.

I don't want to get this merged just yet, mainly because the stylo part is not
implemented, but I'd like early feedback from @bholley and/or @heycam: How do
you see this approach? I don't think we'll have much problem to implement
MatchAttr for our element snapshots, but... worth checking.

r? @heycam

This change is

nox · 2016-07-16T09:05:55Z

Do we really need yet another "Servo" prefix? :(

emilio · 2016-07-19T22:35:10Z

This is basically ready for review... r? @bholley or @heycam

Relevant gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1287951

emilio · 2016-07-19T22:37:54Z

@SimonSapin: This kind of conflicts with your current efforts to use conditional compilation in style. I don't expect this to be a lot of breakage (the only relevant changes for that are the restyle_hints.rs file, and the implementation added in geckolib), though could you please check?

I don't mind holding this off for a bit and rebase if your changes are landing soon :)

SimonSapin · 2016-07-19T23:08:44Z

I tried merging this PR into #12515 (which I just submitted) and got conflicts in:

ports/geckolib/wrapper.rs
ports/geckolib/glue.rs
components/style/selector_matching.rs
components/style/selector_impl.rs
components/style/restyle_hints.rs
components/script/dom/document.rs

I haven’t looked in more details than that.

emilio · 2016-07-19T23:30:21Z

@SimonSapin: Then don't bother about it, I'll do the rebase :)

bors-servo · 2016-07-20T03:02:20Z

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

bholley · 2016-07-21T19:06:34Z

Reviewed 13 of 14 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

components/style/selector_impl.rs, line 82 [r2] (raw file):

// NB: The `Clone` trait is here for convenience due to:
// https://github.com/rust-lang/rust/issues/26925

This text appears twice...

ports/geckolib/glue.rs, line 452 [r2] (raw file):

    let element = unsafe { GeckoElement::from_raw(element) };

    // NB: This involves an FFI call, we can take rid of it easily if needed.

Nit: "get rid of".

ports/geckolib/glue.rs, line 459 [r2] (raw file):

                                                 current_state);

    // NB: Binaries representation match.

Do we have an enforcement mechanism for this somewhere? If not, please file a bug so that we remember to do that.

ports/geckolib/glue.rs, line 460 [r2] (raw file):

    // NB: Binaries representation match.
    unsafe { transmute(hint.bits() as u32) }

Does this really need to be a transmute? I don't remember if |as| works for repr(C) enums.

ports/geckolib/snapshot.rs, line 16 [r2] (raw file):

// NB: This is sound, in some sense, because during computation of restyle hints
// the snapshot is kept alive by the modified elements table.

Hm, so these are basically only valid in stack scope, right? Can we either use a lifetime or a strong reference here to make sure these don't escape? Happy for that to happen in a followup if that makes sense.

ports/geckolib/snapshot.rs, line 31 [r2] (raw file):

    fn has_any(&self, flags: Flags) -> bool {
        unsafe { (*self.0).mContains as u8 & flags as u8 != 0 }

Can you parenthesize a bit more here make the precedence clearer?

ports/geckolib/snapshot.rs, line 62 [r2] (raw file):

                                               attr.select_name(self.is_html_element_in_html_document()),
                                               value.as_ptr(),
                                               /* ignoreCase = */ false)

Don't you want ignoreCase = true here?

ports/geckolib/snapshot.rs, line 129 [r2] (raw file):

        } else {
            Some(Atom::from(value))
        }

Can you use .as_ref().map here instead?

ports/geckolib/snapshot.rs, line 132 [r2] (raw file):

    }

    // TODO: share logic with Element::{has_class, each_class}?

Yeah, the logic here is complicated enough that I think we should share these and just pass a function argument to the helper which invokes the appropriate FFI method.

Comments from Reviewable

bholley · 2016-07-21T19:08:14Z

r=me with those addressed. @bors-servo delegate+

bors-servo · 2016-07-21T19:08:15Z

✌️ @emilio can now approve this pull request


        style: Rewrite the restyle hints code to allow different kinds of ele…

…ment snapshots, and use it for Gecko. This is a rewrite for how style interfaces with its consumers in order to allow different representations for an element snapshot. This also changes the requirements of an element snapshot, requiring them to only implement MatchAttr, instead of MatchAttrGeneric. This is important for stylo since implementing MatchAttrGeneric is way more difficult for us given the atom limitations. This also allows for more performant implementations in the Gecko side of things.


        stylo: Update generated bindings

emilio · 2016-07-21T21:25:28Z

Review status: all files reviewed at latest revision, 9 unresolved discussions.

components/style/selector_impl.rs, line 82 [r2] (raw file):