stylo: Expose computed style list to dev tools

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
2 months ago
9 days ago

People

(Reporter: bradwerth, Assigned: bradwerth)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

2 months ago
We need a Servo mechanism to expose computed styles to the dev tools. A method is proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=1337305#c13.
(Assignee)

Updated

2 months ago
Assignee: nobody → bwerth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 months ago
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

Emilio, I am struggling with the Servo types for this case. I would greatly appreciate it if you would review my non-working Servo code. I've annotated the code with comments to show my thought process.
Attachment #8845956 - Flags: feedback?(emilio+bugs)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review121050

::: servo/ports/geckolib/glue.rs:1227
(Diff revision 2)
> +// it's possible, or what the bindings should look like.
> +pub extern "C" fn Servo_Element_GetStyleRule(element: RawGeckoElementBorrowed) -> RawServoStyleRuleStrong
> +{
> +    let element = GeckoElement(element);
> +    let style_source_option = element.borrow_data().unwrap().styles().primary.rules.style_source();
> +    

nit: Trailing whitespace should go before landing.

::: servo/ports/geckolib/glue.rs:1230
(Diff revision 2)
> +    let element = GeckoElement(element);
> +    let style_source_option = element.borrow_data().unwrap().styles().primary.rules.style_source();
> +    
> +    // This is clearly unsafe, but I'm not sure how to choose a sensible return
> +    // value if style_source_option is None.
> +    let style_source = style_source_option.unwrap();

If there's no style source, it means the element doesn't match any rules, otherwise this should be ok.

::: servo/ports/geckolib/glue.rs:1237
(Diff revision 2)
> +    // Similar problem here, that I want to return a real style rule, and not
> +    // an Option. The "unreachable!" code is clearly reachable.
> +    let style_rule = match style_source {
> +        &StyleSource::Style(ref rule) => rule.clone().into_strong(),
> +        _ => {
> +            unreachable!("Servo_Element_GetStyleRule should only be called on an element with style rule.");

This is wrong. Presumably you want to return either a StyleRule or a ServoDeclarationBlock. Gecko uses `nsIStyleRule` to represent the equivalent of both.

We can coalesce Servo to represent style attributes, etc. as style rules, but I'd prefer not.

If for now you only want to return the last CSS rule matched, you could do something like:

```
let mut rules = element.borrow_data().unwrap().styles().primary.rules.clone();

loop {
    match rules.style_source() {
        Some(&StyleSource::Style(ref rule)) => return rule.clone().into_strong(),
        None => return RawServoStyleRuleStrong::null(),
        _ => {},
    }
    
    rules = rules.get().parent.unwrap()
}
```

Or something like that (haven't really tested).
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

It's close :). I think you definitely should be able to return null from this function, since you may inspect an element which doesn't match any rules for example.

I think something like my last comment should be fine for now, we can care about the style attribute and animation rules later.
Attachment #8845956 - Flags: feedback?(emilio+bugs)
Priority: -- → P1
Comment hidden (mozreview-request)
Created attachment 8849426 [details] [diff] [review]
wip patch

I think you would want something like this in Servo side.

If you also need style elements, there are several other functions you would need to add, but this should be the basic bit of things you may need.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> If you also need style elements, there are several other functions you would
> need to add,

I meant... if you also need style of pseudo-elements...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8845956 - Flags: review?(cam)
Attachment #8850152 - Flags: review?(cam)
Comment on attachment 8850152 [details]
Bug 1346256 Part 4: Change GetCSSStyleRules to pull servo style data directly from the element.

https://reviewboard.mozilla.org/r/122842/#review125140

::: layout/inspector/inDOMUtils.cpp:270
(Diff revision 1)
> +  #ifdef MOZ_STYLO
> +  else {

I don't think you need `#ifdef MOZ_STYLO` here. You should probably:
1. early return if `source.IsNull()`, then
2. just check `if (source.IsGeckoRuleNodeOrNull())` and its `else` without any condition compile.

`source.IsGeckoRuleNodeOrNull()` always returns true if stylo is not compiled.

::: layout/inspector/inDOMUtils.cpp:282
(Diff revision 1)
> +      RefPtr<css::Rule> ruleObj = new ServoStyleRule(
> +        Servo_StyleSourceList_GetStyleRuleAt(styleSourceList, i).Consume());

A style source can be either a rule object or a property declaration block. In the latter case, `Servo_StyleSourceList_GetStyleRuleAt` would return nullptr. You should probably check that and avoid creating `ServoStyleRule` for that.

And I wonder whether you want declaration block as well, which you should probably create a `ServoDeclarationBlock` object instead.

Comment 12

a month ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review125192

Generally OK.  Two things to address.

::: layout/style/ServoBindingList.h:322
(Diff revision 4)
> +SERVO_BINDING_FUNC(Servo_Element_GetStyleSourceList, ServoStyleSourceListOwnedOrNull,
> +                   RawGeckoElementBorrowed element);

I think we need a way to drop the StyleSourceList when we're done with it, since that won't happen automatically.  Please add a Servo_StyleSourceList_Drop function that does this.

::: servo/ports/geckolib/glue.rs:1436
(Diff revision 4)
> +fn collect_style_sources(computed: &ComputedStyle) -> Vec<StyleSource> {
> +    let mut result = vec![];
> +    for rule_node in computed.rules.self_and_ancestors() {
> +        if let Some(source) = rule_node.style_source() {
> +            result.push(source.clone());
> +        }
> +    }
> +    result
> +}

Why don't we include StyleSource::Declarations in here too?  The fact that we have a Servo_StyleSourceList_GetDeclarationBlockAt function below hints that we should be including it.
Attachment #8845956 - Flags: review?(cam) → review-

Comment 13

a month ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review125202

::: servo/ports/geckolib/glue.rs:1460
(Diff revision 4)
> +        ServoStyleSourceListOwnedOrNull::null()
> +    }
> +}
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_StyleSourceList_Length(sources: ServoStyleSourceListBorrowed) -> usize {

This usize should be a u32, to match the C++ function prototype.

::: servo/ports/geckolib/glue.rs:1466
(Diff revision 4)
> +    Vec::<StyleSource>::from_ffi(sources).len()
> +}
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_StyleSourceList_GetStyleRuleAt(sources: ServoStyleSourceListBorrowed,
> +                                                       index: usize) -> RawServoStyleRuleStrong {

Here too.

::: servo/ports/geckolib/glue.rs:1476
(Diff revision 4)
> +    }
> +}
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_StyleSourceList_GetDeclarationBlockAt(sources: ServoStyleSourceListBorrowed,
> +                                                              index: usize) -> RawServoDeclarationBlockStrong

And here.
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review125192

> I think we need a way to drop the StyleSourceList when we're done with it, since that won't happen automatically.  Please add a Servo_StyleSourceList_Drop function that does this.

Oh, I forgot... I think we should have some static check to ensure that... Something related to UniquePtr I guess...

> Why don't we include StyleSource::Declarations in here too?  The fact that we have a Servo_StyleSourceList_GetDeclarationBlockAt function below hints that we should be including it.

What do you mean? I think both `StyleSource::Declarations` and `StyleSource::Style` gets collected, no?
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review125202

> This usize should be a u32, to match the C++ function prototype.

This is the length of a Rust Vec, and Rust uses `usize` for the length, so I'd prefer this to match that.

Comment 16

a month ago
mozreview-review-reply
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review125192

> What do you mean? I think both `StyleSource::Declarations` and `StyleSource::Style` gets collected, no?

You are right, I misread the code.

Comment 17

a month ago
mozreview-review
Comment on attachment 8850152 [details]
Bug 1346256 Part 4: Change GetCSSStyleRules to pull servo style data directly from the element.

https://reviewboard.mozilla.org/r/122842/#review125204

r=me with these and Xidorn's comments addressed.

::: layout/inspector/inDOMUtils.cpp:280
(Diff revision 1)
> +    uint16_t length = Servo_StyleSourceList_Length(styleSourceList);
> +    for (uint16_t i = 0; i < length; i++) {

uint32_t (or size_t, if the FFI function signature changes to use that)

::: layout/inspector/inDOMUtils.cpp:282
(Diff revision 1)
> +      RefPtr<css::Rule> ruleObj = new ServoStyleRule(
> +        Servo_StyleSourceList_GetStyleRuleAt(styleSourceList, i).Consume());

I suppose it doesn't really matter for the current consumers of the GetCSSStyleRules API, but this will cause us to have a different ServoStyleRule object than the one that would be returned from CSSOM methods.  Assuming this is not a problem (Xidorn?) please mention this in the comment in inIDOMUtils.idl.

::: layout/inspector/inDOMUtils.cpp:282
(Diff revision 1)
> +      RefPtr<css::Rule> ruleObj = new ServoStyleRule(
> +        Servo_StyleSourceList_GetStyleRuleAt(styleSourceList, i).Consume());

GetCSSStyleRules filters out Declaration objects that don't have an owning rule, which I think would include the kind of things represented by StyleSource::Declarations.  If that's right, then we should be safe just to skip anything that Servo_StyleSourceList_GetStyleRuleAt returns null for.
Attachment #8850152 - Flags: review?(cam) → review+
Comment on attachment 8850152 [details]
Bug 1346256 Part 4: Change GetCSSStyleRules to pull servo style data directly from the element.

https://reviewboard.mozilla.org/r/122842/#review125204

> I suppose it doesn't really matter for the current consumers of the GetCSSStyleRules API, but this will cause us to have a different ServoStyleRule object than the one that would be returned from CSSOM methods.  Assuming this is not a problem (Xidorn?) please mention this in the comment in inIDOMUtils.idl.

I think it wouldn't be too problematic given that it is just a wrapper of the Servo data struct.

But we probably need to call `SetStyleSheet` properly to make it work correctly with dynamic change. Oh, actually this could make it more tricky... I don't have a good idea how we should handle that.
How difficult would it be to call into your existing CSSOM code on rule lists to get the ServoStyleRule object?
(Maybe difficult, since we can't easily go from the Servo StyleRule object back up to the rule list object it is in...)
You would need to build the whole CSSOM tree and search the given Servo pointer from all of them... which sounds pretty expensive.
We can traverse the whole CSSOM tree, and insert all style rules into a hashtable to do the matching. Not sure whether traversing that would make it unacceptably slow.
After thinking a bit further, I realized that my original proposal doesn't work because there is no way to get the corresponding stylesheet of the rule, which means the ServoStyleRule we create would be basically immutable. I suppose that is not what devtools wants.

My new proposal would be
* create a new method in ServoStyleSheet which traverses (and triggers creation if necessary) the whole CSSOM tree, and collect all style rules into a hashmap mapping RawServoStyleRule* to ServoStyleRule*.
* make the Servo function return pointer of RawServoStyleRule, and search the hashmap to find the corresponding CSSOM object rather than create a new one.

There are two open questions:
1. how often would we need to call getCSSStyleRules? would it be often enough that we may want to cache the hashmap somehow so that we don't need to do the traversal every time even if nothing changes?
2. do we need to take care of the declaration blocks from style source as well? I hope we don't, but I'm not sure.
I discussed this with heycam in the IRC [1] and he thought it is probably fine to traverse the CSSOM tree.

[1] http://logs.glob.uno/?c=mozilla%23servo&s=23+Mar+2017&e=23+Mar+2017#c637538
(Assignee)

Comment 25

a month ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review126476

::: servo/ports/geckolib/glue.rs:1476
(Diff revision 4)
> +    }
> +}
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_StyleSourceList_GetDeclarationBlockAt(sources: ServoStyleSourceListBorrowed,
> +                                                              index: usize) -> RawServoDeclarationBlockStrong

Left these as usize and changed the ServoBindingList.h to use size_t.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a month ago
Somehow, the servo changes in attachment 8845956 [details] cause this reftest to fail: file:///Users/brad/repos/stylo/layout/reftests/w3c-css/received/css-writing-modes-3/text-orientation-upright-srl-018.xht.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8851796 - Flags: review?(cam)
Attachment #8851797 - Flags: review?(cam)
(Assignee)

Comment 33

a month ago
(In reply to Brad Werth [:bradwerth] from comment #28)
> Somehow, the servo changes in attachment 8845956 [details] cause this
> reftest to fail:
> file:///Users/brad/repos/stylo/layout/reftests/w3c-css/received/css-writing-
> modes-3/text-orientation-upright-srl-018.xht.

Specifically, the change to add a strong null value to OwnedOrNull causes this test failure. Attachment 8845956 [details] isolates the change. Xidorn, what are the possible side effects of this change?
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 34

a month ago
(In reply to Brad Werth [:bradwerth] from comment #33)

> Specifically, the change to add a strong null value to OwnedOrNull causes
> this test failure. Attachment 8845956 [details] isolates the change. Xidorn,
> what are the possible side effects of this change?

Sorry, this is a false alarm. I get this test failure intermittently, and it doesn't appear to be connected to this patch.
Flags: needinfo?(xidorn+moz)
(Assignee)

Updated

a month ago
Attachment #8851796 - Flags: review?(cam)
Attachment #8851797 - Flags: review?(cam)

Comment 35

a month ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review126574
Attachment #8845956 - Flags: review?(cam) → review+
Created attachment 8852319 [details] [diff] [review]
servo-side patch

If we only need style rules, then the functions can be simplified a lot.

With this patch, you can pass in an nsTArray<const RawServoStyleRule*> and get a list of unowned RawServoStyleRule pointers.
Attachment #8849426 - Attachment is obsolete: true
(Assignee)

Comment 37

28 days ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #23)
> My new proposal would be
> * create a new method in ServoStyleSheet which traverses (and triggers
> creation if necessary) the whole CSSOM tree, and collect all style rules
> into a hashmap mapping RawServoStyleRule* to ServoStyleRule*.

If I understand this correctly, I believe this map would be easier to create in ServoCSSRuleList, since it actually creates the ServoStyleRule from the RawServoStyleRule. I'm going to create it there and add a public accessor function for entries in the map, and add a function to force the creation of css::Rule objects for all raw rules (which will fully populate the map).
I think you just need to create ServoStyleRules and subclasses of GroupRule (which we only have ServoMediaRule at present).

You can see this function: https://dxr.mozilla.org/mozilla-central/rev/7a3f514cf8490d271ee373a1d2999e4ea4dee2d7/layout/style/ServoCSSRuleList.cpp#74-110 for how they should be created.
Probably just build up the whole CSSOM tree is also fine, given that majority of rules should be style rule.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

25 days ago
Attachment #8850152 - Attachment is obsolete: true
(Assignee)

Comment 43

25 days ago
Comment on attachment 8851797 [details]
Bug 1346256 Part 3: Modify GetCSSStyleRules to collect ServoStyleRules.

As proposed in comment 23, these patches supply a hash from RawServoStyleRule to ServoStyleRule, accessible from ServoStyleSheet. However, I'm not clear on how I can get a ServoStyleSheet from the element or the style context within GetCSSStyleRules() to complete the patch.
Attachment #8851797 - Flags: feedback?(xidorn+moz)
(Assignee)

Comment 44

24 days ago
Bobby, you said there was a way to get StyleSheet pointers from the rule tree held by Servo?
Flags: needinfo?(bobbyholley)
(In reply to Brad Werth [:bradwerth] from comment #44)
> Bobby, you said there was a way to get StyleSheet pointers from the rule
> tree held by Servo?

It looks like the servo rule true doesn't have pointers to the StyleSheets. I'm not too familiar with the CSSOM stuff, so I'll let xidorn advise here.
Flags: needinfo?(bobbyholley)
Comment on attachment 8851797 [details]
Bug 1346256 Part 3: Modify GetCSSStyleRules to collect ServoStyleRules.

You can get the owner doc of the element, then enumerate stylesheets of the document via nsDocument::GetStyleSheetAt.
Attachment #8851797 - Flags: feedback?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

24 days ago
Attachment #8851796 - Flags: review?(xidorn+moz)
Attachment #8851797 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

24 days ago
Try server shows memory leaks in the crashtest https://treeherder.mozilla.org/#/jobs?repo=try&revision=362004849179&selectedJob=88696537. I'm picking through the changes to try to spot the error with a cycle or a pointer, but I wanted to call it out during Xidorn's review of the code.
Flags: needinfo?(xidorn+moz)
Attachment #8845956 - Flags: review?(manishearth)
Attachment #8845956 - Flags: review?(manishearth)
(Assignee)

Updated

24 days ago
Flags: needinfo?(xidorn+moz) → needinfo?(manishearth)
Comment on attachment 8851796 [details]
Bug 1346256 Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule.

https://reviewboard.mozilla.org/r/124014/#review129134

I think the idea of this implementation is basically wrong, because it doesn't handle nested rules, e.g. `@media`, `@supports`.

The idea in my mind is, we create a in-stack hashtable in `inDOMUtils::GetCSSStyleRules`, and we pass it as reference into a function in `CSSRuleList` to collect style rule recursively, that when a style rule is found, collect it, and when a media rule is found, recursively call the same function on the list of the media rule. `@supports` and `@-moz-document` may also need to be traversed, but CSSOM support of them hasn't been implemented at the moment, so probably ignore them for now. Alternatively you can check whether the rule is a `GroupRule`, and if so, do the recursion. Not sure whether it is doable, though.

::: layout/style/ServoCSSRuleList.h:83
(Diff revision 4)
> +  typedef nsRefPtrHashtable<nsPtrHashKey<const RawServoStyleRule>, ServoStyleRule>
> +    StyleRuleHashtable;
> +  StyleRuleHashtable mStyleRuleMap;

This is wrong. Why do you need to own this hash table inside CSSRuleList? The CSSOM is a tree, and the list can have children which have CSSRuleList as well (@media, @support, etc.), so embedding this in CSSRuleList doesn't look like a sensible thing to do.

In addition, by having it hold a RefPtr to ServoStyleRule without reporting this edge to the cycle collector, you would leak when there is a cycle reference from the rule, as shown in your try run.
Attachment #8851796 - Flags: review?(xidorn+moz) → review-
Comment on attachment 8851797 [details]
Bug 1346256 Part 3: Modify GetCSSStyleRules to collect ServoStyleRules.

https://reviewboard.mozilla.org/r/124016/#review129138
Attachment #8851797 - Flags: review?(xidorn+moz) → review-
Not clear on the cssom ownership story, xidorn's comment makes sense though.
Flags: needinfo?(manishearth)
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

This is a rework of the original patch by me, so still need a review from someone else.
Attachment #8845956 - Flags: review?(cam)

Comment 57

23 days ago
mozreview-review
Comment on attachment 8845956 [details]
Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/119046/#review129288

::: commit-message-800ba:1
(Diff revision 8)
> +Bug 1346256 Part 1: Add FFI interfaces for exposing style sources.
> +
> +changeset: 399627:b3c03e489ec8
> +tag: tip
> +mylocal: bug1346256
> +mylocal: bug1346256p1
> +parent: 399304:a5edc0021f59
> +user: Xidorn Quan <me@upsuper.org>
> +date: Tue Mar 21 17:11:26 2017 +1100
> +files: layout/style/ServoBindingList.h layout/style/ServoBindingTypes.h servo/components/style/build_gecko.rs servo/components/style/gecko/arc_types.rs servo/ports/geckolib/glue.rs
> +description:
> +
> +MozReview-Commit-ID: FOQCgXD68E9

Something strange happened here...

::: servo/ports/geckolib/glue.rs:1420
(Diff revision 8)
>  
>  #[no_mangle]
> +pub extern "C" fn Servo_Element_GetStyleRuleList(element: RawGeckoElementBorrowed,
> +                                                 rules: RawGeckoServoStyleRuleListBorrowedMut) {
> +    let element = GeckoElement(element);
> +    let data = unsafe { element.ensure_data() }.borrow();

Instead of calling ensure_data(), and then returning early when we find it doesn't have any styles, let's do |let data = element.borrow_data();| and then return if |data| is null.
Attachment #8845956 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8851796 [details]
Bug 1346256 Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule.

https://reviewboard.mozilla.org/r/124014/#review129704

Much better. Please address the comments below.

::: layout/style/ServoCSSRuleList.h:18
(Diff revision 6)
>  class ServoStyleSheet;
> +class ServoStyleRule;

Consider moving `ServoStyleRule` to be above `ServoStyleSheet` so that they are listed in order.

::: layout/style/ServoCSSRuleList.h:49
(Diff revision 6)
> +  typedef nsRefPtrHashtable<nsPtrHashKey<const RawServoStyleRule>,
> +    ServoStyleRule> StyleRuleHashtable;

I don't really think this needs to be a `nsRefPtrHashtable`. As far as the hashtable is hold in-stack, a `nsDataHashtable<..., ServoStyleRule*>` should probably be enough.

::: layout/style/ServoCSSRuleList.h:50
(Diff revision 6)
>    nsresult DeleteRule(uint32_t aIndex);
>  
>    uint16_t GetRuleType(uint32_t aIndex) const;
>  
> +  typedef nsRefPtrHashtable<nsPtrHashKey<const RawServoStyleRule>,
> +    ServoStyleRule> StyleRuleHashtable;

The second line should be aligned with that on the same level before, like this:
```c++
  typedef nsRefPtrHashtable<nsPtrHashKey<const RawServoStyleRule>,
                            ServoStyleRule> StyleRuleHashtable;
```

::: layout/style/ServoCSSRuleList.h:51
(Diff revision 6)
>  
>    uint16_t GetRuleType(uint32_t aIndex) const;
>  
> +  typedef nsRefPtrHashtable<nsPtrHashKey<const RawServoStyleRule>,
> +    ServoStyleRule> StyleRuleHashtable;
> +  void FillStyleRuleHashtable(StyleRuleHashtable &table);

The reference mark should be attached to the type rather than the parameter name:
```c++
void FillStyleRuleHashtable(StyleRuleHashtable& table);
```

::: layout/style/ServoCSSRuleList.cpp:206
(Diff revision 6)
>    }
>    return CastToPtr(rule)->Type();
>  }
>  
> +void
> +ServoCSSRuleList::FillStyleRuleHashtable(StyleRuleHashtable &table)

ditto.

::: layout/style/ServoCSSRuleList.cpp:209
(Diff revision 6)
> +    uint16_t type = GetRuleType(i);
> +    if (type == nsIDOMCSSRule::STYLE_RULE) {

You may want to use
```c++
switch (GetRuleType(i)) {
}
```
but I guess either way is fine.

::: layout/style/ServoCSSRuleList.cpp:211
(Diff revision 6)
> +ServoCSSRuleList::FillStyleRuleHashtable(StyleRuleHashtable &table)
> +{
> +  for (uint32_t i = 0; i < mRules.Length(); i++) {
> +    uint16_t type = GetRuleType(i);
> +    if (type == nsIDOMCSSRule::STYLE_RULE) {
> +      ServoStyleRule* castedRule = (ServoStyleRule*)(GetRule(i));

Please use `static_cast`:
```c++
auto castedRule = static_cast<ServoStyleRule*>(GetRule(i));
```

::: layout/style/ServoCSSRuleList.cpp:215
(Diff revision 6)
> +    if (type == nsIDOMCSSRule::STYLE_RULE) {
> +      ServoStyleRule* castedRule = (ServoStyleRule*)(GetRule(i));
> +      RawServoStyleRule* rawRule = castedRule->Raw();
> +      table.Put(rawRule, castedRule);
> +    } else if (type == nsIDOMCSSRule::MEDIA_RULE) {
> +      ServoMediaRule* castedRule = (ServoMediaRule*)(GetRule(i));

ditto.

::: layout/style/ServoCSSRuleList.cpp:218
(Diff revision 6)
> +      nsCOMPtr<nsIDOMCSSRuleList> ruleList;
> +      castedRule->GetCssRules(getter_AddRefs(ruleList));

Probably easier by just
```c++
CSSRuleList* ruleList = castedRule->CssRules();
```

::: layout/style/ServoCSSRuleList.cpp:221
(Diff revision 6)
> +
> +      // Call this method recursively on the ServoCSSRuleList in the rule.
> +      nsCOMPtr<nsIDOMCSSRuleList> ruleList;
> +      castedRule->GetCssRules(getter_AddRefs(ruleList));
> +
> +      ServoCSSRuleList* castedRuleList = (ServoCSSRuleList*)(ruleList.get());

`static_cast`.
Attachment #8851796 - Flags: review?(xidorn+moz)
Comment on attachment 8851797 [details]
Bug 1346256 Part 3: Modify GetCSSStyleRules to collect ServoStyleRules.

https://reviewboard.mozilla.org/r/124016/#review129712

r=me with the comments addressed.

::: layout/inspector/inDOMUtils.cpp:278
(Diff revision 6)
> +    Servo_Element_GetStyleRuleList(element, &rawRuleList);
> +    size_t rawRuleCount = rawRuleList.Length();
> +
> +    // We have RawServoStyleRules, and now we'll map them to ServoStyleRules
> +    // by looking them up in the ServoStyleSheets owned by this document.
> +    StyleRuleHashtable rawRulesToRules;

I guess you would need `ServoCSSRuleList::StyleRuleHashtable` here, no?

::: layout/inspector/inDOMUtils.cpp:285
(Diff revision 6)
> +    nsIDocument* document = element->GetOwnerDocument();
> +    int32_t sheetCount = document->GetNumberOfStyleSheets();
> +
> +    for (int32_t i = 0; i < sheetCount; i++) {
> +      StyleSheet* sheet = document->GetStyleSheetAt(i);
> +      if (sheet->IsServo()) {

Could a document have both Servo and Gecko stylesheet? Can we just assert `sheet->IsServo()` here?

::: layout/inspector/inDOMUtils.cpp:287
(Diff revision 6)
> +
> +    for (int32_t i = 0; i < sheetCount; i++) {
> +      StyleSheet* sheet = document->GetStyleSheetAt(i);
> +      if (sheet->IsServo()) {
> +        ErrorResult ignored;
> +        ServoCSSRuleList* ruleList = (ServoCSSRuleList*)(

`static_cast`.

::: layout/inspector/inDOMUtils.cpp:288
(Diff revision 6)
> +    for (int32_t i = 0; i < sheetCount; i++) {
> +      StyleSheet* sheet = document->GetStyleSheetAt(i);
> +      if (sheet->IsServo()) {
> +        ErrorResult ignored;
> +        ServoCSSRuleList* ruleList = (ServoCSSRuleList*)(
> +          sheet->AsServo()->GetCssRules(*nsContentUtils::SubjectPrincipal(),

You can simply `sheet->GetCssRules` without `AsServo()` here.

::: layout/inspector/inDOMUtils.cpp:301
(Diff revision 6)
>  
> -    rules.forget(_retval);
> +    // Find matching rules in the table.
> +    for (size_t j = 0; j < rawRuleCount; j++) {
> +      const RawServoStyleRule* rawRule = rawRuleList.ElementAt(j);
> +      ServoStyleRule* rule = rawRulesToRules.GetWeak(rawRule);
> +      if (rule) {

I guess you can assert here that there should always exist such rule. If you think it is possible that there isn't a rule, it's probably worth a comment on when that could happen.
Attachment #8851797 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

22 days ago
Attachment #8851796 - Flags: review?(cam)
Comment on attachment 8851796 [details]
Bug 1346256 Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule.

https://reviewboard.mozilla.org/r/124014/#review130126

r=me with the issue addressed.

::: layout/style/ServoCSSRuleList.h:51
(Diff revision 7)
>  
>    uint16_t GetRuleType(uint32_t aIndex) const;
>  
> +  typedef nsDataHashtable<nsPtrHashKey<const RawServoStyleRule>,
> +                          ServoStyleRule*> StyleRuleHashtable;
> +  void FillStyleRuleHashtable(StyleRuleHashtable& table);

`aTable` rather than `table`.

::: layout/style/ServoCSSRuleList.cpp:206
(Diff revision 7)
>    }
>    return CastToPtr(rule)->Type();
>  }
>  
> +void
> +ServoCSSRuleList::FillStyleRuleHashtable(StyleRuleHashtable& table)

ditto.

::: layout/style/ServoCSSRuleList.cpp:211
(Diff revision 7)
> +ServoCSSRuleList::FillStyleRuleHashtable(StyleRuleHashtable& table)
> +{
> +  for (uint32_t i = 0; i < mRules.Length(); i++) {
> +    uint16_t type = GetRuleType(i);
> +    if (type == nsIDOMCSSRule::STYLE_RULE) {
> +      ServoStyleRule* castedRule = static_cast<ServoStyleRule*>(GetRule(i));

You can use `auto` here as far as the `static_cast` already indicates the type. It can help you shorten the line length.

::: layout/style/ServoCSSRuleList.cpp:215
(Diff revision 7)
> +    if (type == nsIDOMCSSRule::STYLE_RULE) {
> +      ServoStyleRule* castedRule = static_cast<ServoStyleRule*>(GetRule(i));
> +      RawServoStyleRule* rawRule = castedRule->Raw();
> +      table.Put(rawRule, castedRule);
> +    } else if (type == nsIDOMCSSRule::MEDIA_RULE) {
> +      ServoMediaRule* castedRule = static_cast<ServoMediaRule*>(GetRule(i));

Maybe you can just cast it to `GroupRule` here, so that in the future we can directly add more types in the condition. But this can be done latter when that happens as well.

::: layout/style/ServoCSSRuleList.cpp:218
(Diff revision 7)
> +      ServoCSSRuleList* castedRuleList = static_cast<ServoCSSRuleList*>(
> +        castedRule->CssRules());

ditto, hopefully that can make it use only one line.
Attachment #8851796 - Flags: review?(xidorn+moz) → review+

Comment 70

21 days ago
mozreview-review
Comment on attachment 8851796 [details]
Bug 1346256 Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule.

https://reviewboard.mozilla.org/r/124014/#review130248
Attachment #8851796 - Flags: review?(cam) → review+
(Assignee)

Comment 71

21 days ago
mozreview-review-reply
Comment on attachment 8851796 [details]
Bug 1346256 Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule.

https://reviewboard.mozilla.org/r/124014/#review130126

> Maybe you can just cast it to `GroupRule` here, so that in the future we can directly add more types in the condition. But this can be done latter when that happens as well.

I chose not to cast to GroupRule here, since the implementation doesn't (yet) uplift FillStyleRuleHashtable to GroupRule.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 78

21 days ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8cfaab9ac09
Part 1: Add FFI interfaces for exposing style sources. r=heycam
https://hg.mozilla.org/integration/autoland/rev/7fbc0a154eaa
Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule. r=heycam,xidorn
https://hg.mozilla.org/integration/autoland/rev/20bd44c62c89
Part 3: Modify GetCSSStyleRules to collect ServoStyleRules. r=xidorn

Comment 79

20 days ago
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b22e6093befb
Backout changesets 20bd44c62c89, 7fbc0a154eaa, and b8cfaab9ac09
Blocks: 1243581
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

18 days ago
Attachment #8856324 - Flags: review?(manishearth)
(Assignee)

Updated

18 days ago
Attachment #8856324 - Flags: review?(manishearth)

Comment 84

18 days ago
mozreview-review
Comment on attachment 8856324 [details]
Bug 1346256 Part 1a: (Servo) Add FFI interfaces for exposing style sources.

https://reviewboard.mozilla.org/r/128222/#review130730
Attachment #8856324 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

18 days ago
Attachment #8856324 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

18 days ago
Attachment #8856359 - Attachment is obsolete: true

Comment 95

18 days ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a05030ee369
Part 1b: (Gecko) Add FFI interfaces for exposing style sources. r=heycam
https://hg.mozilla.org/integration/autoland/rev/49e7c55c11c3
Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule. r=heycam,xidorn
https://hg.mozilla.org/integration/autoland/rev/22a33c416f11
Part 3: Modify GetCSSStyleRules to collect ServoStyleRules. r=xidorn

Comment 96

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a05030ee369
https://hg.mozilla.org/mozilla-central/rev/49e7c55c11c3
https://hg.mozilla.org/mozilla-central/rev/22a33c416f11
Status: NEW → RESOLVED
Last Resolved: 18 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1322657
You need to log in before you can comment on or make changes to this bug.