Closed Bug 1346256 Opened 8 years ago Closed 8 years ago

stylo: Expose computed style list to dev tools

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1346256 Part 1b: (Gecko) Add FFI interfaces for exposing style sources.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1346256 Part 2: Define methods in ServoCSSRuleList to fill a hash of RawServoStyleRule to ServoStyleRule.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1346256 Part 3: Modify GetCSSStyleRules to collect ServoStyleRules.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
servo-side patch
12.56 KB, patch
Details | Diff | Splinter Review
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: nobody → bwerth
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 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
Attached patch wip patch (obsolete) — — Splinter Review
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...
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 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 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 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 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
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.
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.
Attachment #8851796 - Flags: review?(cam)
Attachment #8851797 - Flags: review?(cam)
(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)
(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)
Attachment #8851796 - Flags: review?(cam)
Attachment #8851797 - Flags: review?(cam)
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+
Attached patch servo-side patch — — Splinter Review
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
(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.
Attachment #8850152 - Attachment is obsolete: true
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)
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)
Attachment #8851796 - Flags: review?(xidorn+moz)
Attachment #8851797 - Flags: review?(xidorn+moz)
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)
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 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 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+
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 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+
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.
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
Backout by gszorc@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b22e6093befb Backout changesets 20bd44c62c89, 7fbc0a154eaa, and b8cfaab9ac09
Attachment #8856324 - Flags: review?(manishearth)
Attachment #8856324 - Flags: review?(manishearth)
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+
Attachment #8856324 - Attachment is obsolete: true
Attachment #8856359 - Attachment is obsolete: true
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: