stylo: Pass in CSS loader for insertRule

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

This is necessary for landing bug 1352025 because otherwise we would crash in test_rule_insertion.html.
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1102402d9056c11572e79621432253324d0db1b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc501c31e67fb2301710f94d697047a8f1ae990c

Comment 6

9 months ago
mozreview-review
Comment on attachment 8853858 [details]
Bug 1352763 part 1 - Constify RawServoStyleSheet.

https://reviewboard.mozilla.org/r/125884/#review128396
Attachment #8853858 - Flags: review?(cam) → review+

Comment 7

9 months ago
mozreview-review
Comment on attachment 8853859 [details]
Bug 1352763 part 2 - Pass borrowed child stylesheet to Gecko for loading rather than the import rule.

https://reviewboard.mozilla.org/r/125886/#review128402
Attachment #8853859 - Flags: review?(emilio+bugs) → review+

Comment 8

9 months ago
mozreview-review
Comment on attachment 8853860 [details]
Bug 1352763 part 3 - Have ServoStyleSheet also implement nsICSSLoaderObserver.

https://reviewboard.mozilla.org/r/125888/#review128400

::: layout/style/ServoStyleSheet.cpp:117
(Diff revision 1)
> -  Inner()->mSheet = Servo_StyleSheet_Empty(mParsingMode).Consume();
> +    Inner()->mSheet = Servo_StyleSheet_Empty(mParsingMode).Consume();
> -}
> +  }

May as well `return NS_OK;` early here.

::: layout/style/ServoStyleSheet.cpp:190
(Diff revision 1)
>    aRv = mRuleList->InsertRule(aRule, aIndex);
>    if (aRv.Failed()) {
>      return 0;
>    }
> -  if (mDocument) {
> -    // XXX When we support @import rules, we should not notify here,
> +  uint16_t type = mRuleList->GetRuleType(aIndex);
> +  if (mDocument && type != nsIDOMCSSRule::IMPORT_RULE) {

Did you check that we don't need to do something similar to CSSStyleSheet with its RuleHasPendingChildSheet call?

::: layout/style/StyleSheetInlines.h:92
(Diff revision 1)
>  StyleSheet::GetParentObject() const
>  {
>    if (mOwningNode) {
>      return dom::ParentObject(mOwningNode);
>    }
> -  return dom::ParentObject(GetParentSheet());
> +  return dom::ParentObject(static_cast<nsIDOMCSSStyleSheet*>(mParent), mParent);

Why is this needed?  Is the call to the ParentObject templated constructor ambiguous?
Attachment #8853860 - Flags: review?(cam) → review+

Comment 9

9 months ago
mozreview-review
Comment on attachment 8853861 [details]
Bug 1352763 part 4 - Pass CSS loader to Servo for insertRule.

https://reviewboard.mozilla.org/r/125890/#review128404

::: servo/components/script/dom/cssrulelist.rs:95
(Diff revision 1)
>          let index = idx as usize;
>  
>          let parent_stylesheet = self.parent_stylesheet.style_stylesheet();
>          let new_rule = {
>              let mut guard = parent_stylesheet.shared_lock.write();
> -            css_rules.write_with(&mut guard).insert_rule(rule, parent_stylesheet, index, nested)?
> +            // FIXME We should probably pass in a proper StylesheetLoader.

Could you file a Servo issue and reference it here?

::: servo/components/style/stylesheets.rs:139
(Diff revision 1)
>          })
>      }
>  
>      /// https://drafts.csswg.org/cssom/#insert-a-css-rule
> -    pub fn insert_rule(&mut self, rule: &str, parent_stylesheet: &Stylesheet, index: usize, nested: bool)
> +    pub fn insert_rule(&mut self, rule: &str, parent_stylesheet: &Stylesheet,
> +                       index: usize, nested: bool, loader: Option<&StylesheetLoader>)

move each argument to its own line?

::: servo/ports/geckolib/glue.rs:523
(Diff revision 1)
>      })
>  }
>  
>  #[no_mangle]
>  pub extern "C" fn Servo_CssRules_InsertRule(rules: ServoCssRulesBorrowed, sheet: RawServoStyleSheetBorrowed,
>                                              rule: *const nsACString, index: u32, nested: bool,

Same re. the arguments, though it's pre-existing in this case.
Attachment #8853861 - Flags: review?(emilio+bugs) → review+
Looks like I cannot merge LoadFailed into StyleSheetLoaded.
(Assignee)

Comment 11

9 months ago
mozreview-review-reply
Comment on attachment 8853860 [details]
Bug 1352763 part 3 - Have ServoStyleSheet also implement nsICSSLoaderObserver.

https://reviewboard.mozilla.org/r/125888/#review128400

> Did you check that we don't need to do something similar to CSSStyleSheet with its RuleHasPendingChildSheet call?

Oh, actually I guess we really need that.

> Why is this needed?  Is the call to the ParentObject templated constructor ambiguous?

Yeah, the constructor of ParentObject wants to cast it to `nsISupports` which is ambiguous here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8853860 [details]
Bug 1352763 part 3 - Have ServoStyleSheet also implement nsICSSLoaderObserver.

Change the code that:
1. no longer skip import rule for now (I'll probably fix it in bug 1352968 with the correct skip rule)
2. LoadFailed is no longer merged into StyleSheetLoaded, since it seems there is something expects the raw sheet pointer to be available before StyleSheetLoaded is invoked.
Attachment #8853860 - Flags: review+ → review?(cam)

Comment 17

9 months ago
mozreview-review
Comment on attachment 8853860 [details]
Bug 1352763 part 3 - Have ServoStyleSheet also implement nsICSSLoaderObserver.

https://reviewboard.mozilla.org/r/125888/#review128458
Attachment #8853860 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://github.com/servo/servo/pull/16241

Comment 23

9 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2143aae5e80
part 1 - Constify RawServoStyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/baffb5defe2f
part 2 - Pass borrowed child stylesheet to Gecko for loading rather than the import rule. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3de3d7d4725e
part 3 - Have ServoStyleSheet also implement nsICSSLoaderObserver. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4487ef34d110
part 4 - Pass CSS loader to Servo for insertRule. r=emilio

Comment 24

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2143aae5e80
https://hg.mozilla.org/mozilla-central/rev/baffb5defe2f
https://hg.mozilla.org/mozilla-central/rev/3de3d7d4725e
https://hg.mozilla.org/mozilla-central/rev/4487ef34d110
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.