Closed Bug 1352763 Opened 7 years ago Closed 7 years ago

stylo: Pass in CSS loader for insertRule

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(4 files)

Bug 1352763 part 1 - Constify RawServoStyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1352763 part 2 - Pass borrowed child stylesheet to Gecko for loading rather than the import rule.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1352763 part 3 - Have ServoStyleSheet also implement nsICSSLoaderObserver.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1352763 part 4 - Pass CSS loader to Servo for insertRule.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
This is necessary for landing bug 1352025 because otherwise we would crash in test_rule_insertion.html.
Assignee: nobody → xidorn+moz
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 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 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 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.
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 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 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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: