Closed Bug 1307357 Opened 8 years ago Closed 8 years ago

stylo: Implement access to CSSStyleRule

Categories

(Core :: DOM: CSS Object Model, defect)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(7 files)

Bug 1307357 part 1 - Make css::Rule hold StyleSheet rather than CSSStyleSheet.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.
58 bytes, text/x-review-board-request
manishearth
: review+
heycam
: review+
Details
Bug 1307357 part 2 - Fix issues appear after adding file to unified source.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1307357 part 4 - Add impl class of CSSStyleRule for stylo.
58 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.
58 bytes, text/x-review-board-request
SimonSapin
: review+
heycam
: review+
Details
Bug 1307357 part 6 - Implement CSSStyleRule.style.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1307357 part 7 - Implement ServoStyleRule::List.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
I decide to separate this work from bug 1292432.
Assignee: nobody → xidorn+moz
Depends on: 1309109
Let's split the task further. CSSStyleRule apparently is the most important rule. And I think the next one would be @media, then @keyframes.
Summary: stylo: Implement interface for CSS rules → stylo: Implement access to CSSStyleRule
This bug would need the nsDOMCSSDeclaration code added in bug 1294299.
Depends on: 1294299
Blocks: 1313293
Blocks: 1315601
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review90834

::: layout/style/ServoStyleRule.cpp:111
(Diff revision 1)
>  ServoStyleRule::SetSelectorText(const nsAString& aSelectorText)
>  {
> -  return NS_ERROR_NOT_IMPLEMENTED;
> +  // XXX We need to implement this... But Gecko doesn't have this either
> +  //     so it's probably okay to leave it unimplemented currently?
> +  //     See bug 37468 and mozilla::css::StyleRule::SetSelectorText.
> +  return NS_OK;

If it’s not implemented, shouldn’t it still return NS_ERROR_NOT_IMPLEMENTED?
Comment on attachment 8808053 [details]
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.

https://reviewboard.mozilla.org/r/90972/#review90870

::: layout/style/ServoBindings.h:69
(Diff revision 1)
>    void Gecko_Release##name_##ArbitraryThread(ThreadSafe##name_##Holder* aPtr) \
>    { NS_RELEASE(aPtr); }                                                       \
>  
> +
> +#define DEFINE_ARRAY_TYPE_FOR(type_)                        \
> +  struct nsTArray_##type_ {                                 \

There should be a Borrowed in the name.

::: servo/components/style/stylesheets.rs:78
(Diff revision 1)
>  }
>  
>  impl CSSRule {
> +    pub fn rule_type(&self) -> u16 {
> +        match *self {
> +            CSSRule::Style(_)     => 1,

can these magic numbers use gecko_bindings consts?
Attachment #8808053 - Flags: review?(manishearth) → review+
Comment on attachment 8808055 [details]
Bug 1307357 part 4 - Add impl class of CSSStyleRule for stylo.

https://reviewboard.mozilla.org/r/90976/#review90884
Attachment #8808055 - Flags: review?(manishearth) → review+
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review90834

> If it’s not implemented, shouldn’t it still return NS_ERROR_NOT_IMPLEMENTED?

I suppose returning NOT_IMPLEMENTED would throw exception in JS. I prefer to match Gecko's current behavior here for now.
Comment on attachment 8808053 [details]
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.

https://reviewboard.mozilla.org/r/90972/#review90870

> There should be a Borrowed in the name.

nsTArrayBorrowed_##type_?

> can these magic numbers use gecko_bindings consts?

These magic numbers are specced in CSSOM, so they should be shared between Gecko and Servo. Probably better write the name of the constants down...
> nsTArrayBorrowed_##type_?

That's fine.


> Probably better write the name of the constants down...

Make an enum with assigned discriminants somewhere.
(In reply to Manish Goregaokar [:manishearth] from comment #15)
> 
> > Probably better write the name of the constants down...
> 
> Make an enum with assigned discriminants somewhere.

Servo would eventually want to expose those constants in its JS interface, see https://drafts.csswg.org/cssom/#the-cssrule-interface (that probably would need to be declaraed in .webidl file anyway, though...) I'm not sure whether making an enum is more useful than separate constants. Also enum may need an additional explicit conversion to number I suppose?
> Also enum may need an additional explicit conversion to number I suppose?

Cast via `as`

> Servo would eventually want to expose those constants in its JS interface,

Yes, but style can't refer to DOM.
Attachment #8808053 - Flags: review+ → review?(manishearth)
Comment on attachment 8808053 [details]
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.

https://reviewboard.mozilla.org/r/90972/#review91282
Attachment #8808053 - Flags: review?(manishearth) → review+
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review90834

> I suppose returning NOT_IMPLEMENTED would throw exception in JS. I prefer to match Gecko's current behavior here for now.

I’m not a fan of having unimplemented things be there and silently do nothing, but ok.
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review91346
Attachment #8808056 - Flags: review?(simon.sapin) → review+
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review92646

::: servo/components/style/stylesheets.rs:181
(Diff revision 2)
> +    // https://drafts.csswg.org/cssom/#serialize-a-css-rule CSSStyleRule
> +    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
> +        // Step 1
> +        try!(self.selectors_to_css(dest));
> +        // Step 2
> +        try!(write!(dest, "{{ "));

nit: there should be a space before this
Comment on attachment 8808053 [details]
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.

I guess this patch need rework base on servo/servo#14190.
Attachment #8808053 - Flags: review?(cam)
Comment on attachment 8808052 [details]
Bug 1307357 part 1 - Make css::Rule hold StyleSheet rather than CSSStyleSheet.

https://reviewboard.mozilla.org/r/90970/#review94488

::: layout/style/nsCSSRules.cpp:129
(Diff revision 2)
>  CSSStyleSheet*
>  GroupRuleRuleList::GetParentObject()
>  {
>    if (!mGroupRule) {
>      return nullptr;
>    }
> -
> -  return mGroupRule->GetStyleSheet();
> +  StyleSheet* sheet = mGroupRule->GetStyleSheet();
> +  return sheet ? sheet->AsGecko() : nullptr;
>  }

Is there any reason not to make GroupRuleList::GetParentObject return StyleSheet?

::: layout/style/nsCSSRules.cpp:455
(Diff revision 2)
> -  if (sheet) {
> -    sheet->SetModifiedByChildRule();
> +  if (sheet && sheet->IsGecko()) {
> +    // XXX We need to implement it for Servo as well.
> +    sheet->AsGecko()->SetModifiedByChildRule();
>    }

Do an NS_ERROR("stylo: appending style rules not supported") if sheet->IsServo()?

::: layout/style/nsCSSRules.cpp:548
(Diff revision 2)
> +    // XXX We need to implement it for Servo as well.
> +    return NS_ERROR_NOT_IMPLEMENTED;

Here too.

(Probably the assertion thrown will be sufficient to catch this in existing tests, but we have been using the NS_ERROR(...) pattern to capture cases like this.)

::: layout/style/nsCSSRules.cpp:567
(Diff revision 2)
> +    // XXX We need to implement it for Servo as well.
> +    return NS_ERROR_NOT_IMPLEMENTED;

And here.

::: layout/style/nsCSSRules.cpp:632
(Diff revision 2)
> -    mMedia->SetStyleSheet(aSheet);
> +    if (aSheet) {
> +      mMedia->SetStyleSheet(aSheet->AsGecko());
> +    }

If you like, you could add GetAsGecko / GetAsServo methods to MOZ_DECL_STYLO_METHODS, which would return null if the wrong type (like we have on StyleSetHandle) to avoid the if statement.
Attachment #8808052 - Flags: review?(cam) → review+
Comment on attachment 8808054 [details]
Bug 1307357 part 2 - Fix issues appear after adding file to unified source.

https://reviewboard.mozilla.org/r/90974/#review94496
Attachment #8808054 - Flags: review?(cam) → review+
Comment on attachment 8808053 [details]
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.

https://reviewboard.mozilla.org/r/90972/#review94498

::: layout/style/ServoCSSRuleList.h:38
(Diff revision 3)
> +  void DropReference();
> +
> +private:
> +  virtual ~ServoCSSRuleList();
> +
> +  // XXX Is it possible to have an address lower than or equal to 255?

I guess in theory.  (On x86_64 I don't think so.)

::: layout/style/ServoCSSRuleList.cpp:56
(Diff revision 3)
> +  return CastToPtr(rule)->GetDOMRule();
> +}
> +
> +template<typename Func>
> +void
> +ServoCSSRuleList::EnumerateRules(Func aCallback)

Maybe call this EnumerateInstantiatedRules or something like that, to indicate that it's not working on all the rules the RuleList object represents?

::: servo/components/style/stylesheets.rs:86
(Diff revision 3)
> +pub enum CssRuleType {
> +    // https://drafts.csswg.org/cssom/#the-cssrule-interface

Any way we can assert these numbers match up with the Gecko constants?  (Though they're likely to be right, since they come from specs...)
Attachment #8808053 - Flags: review?(cam) → review+
Comment on attachment 8808052 [details]
Bug 1307357 part 1 - Make css::Rule hold StyleSheet rather than CSSStyleSheet.

https://reviewboard.mozilla.org/r/90970/#review94488

> Is there any reason not to make GroupRuleList::GetParentObject return StyleSheet?

Because I don't think we will use it for stylo, so it doesn't make much sense to have it return StyleSheet and fix all its callers (if any).

> Do an NS_ERROR("stylo: appending style rules not supported") if sheet->IsServo()?

I guess I should just do AsGecko() directly. After revisiting GroupRule, I guess we don't want to use it at all for stylo. It doesn't seem to have anything useful for reusing.

> Here too.
> 
> (Probably the assertion thrown will be sufficient to catch this in existing tests, but we have been using the NS_ERROR(...) pattern to capture cases like this.)

Like above, I think it should just AsGecko() directly. I'm not going to use GroupRule for stylo.

> If you like, you could add GetAsGecko / GetAsServo methods to MOZ_DECL_STYLO_METHODS, which would return null if the wrong type (like we have on StyleSetHandle) to avoid the if statement.

I think it is intentionally using AsGecko() directly, since it is not expected to use the existing MediaRule for stylo.
Comment on attachment 8808053 [details]
Bug 1307357 part 3 - Implement CSSRuleList interface for stylo.

https://reviewboard.mozilla.org/r/90972/#review94498

> Any way we can assert these numbers match up with the Gecko constants?  (Though they're likely to be right, since they come from specs...)

We need to include nsIDOMCSSRule in binding headers... I guess it is doable, but I don't think it's really worth doing...
Comment on attachment 8808052 [details]
Bug 1307357 part 1 - Make css::Rule hold StyleSheet rather than CSSStyleSheet.

https://reviewboard.mozilla.org/r/90970/#review94488

> I think it is intentionally using AsGecko() directly, since it is not expected to use the existing MediaRule for stylo.

The additional null-check here is because we cannot call AsGecko() on a null pointer.
Comment on attachment 8808055 [details]
Bug 1307357 part 4 - Add impl class of CSSStyleRule for stylo.

https://reviewboard.mozilla.org/r/90976/#review94836
Attachment #8808055 - Flags: review?(cam) → review+
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review94840

::: layout/style/ServoStyleRule.cpp:109
(Diff revision 3)
>  
>  NS_IMETHODIMP
>  ServoStyleRule::SetSelectorText(const nsAString& aSelectorText)
>  {
> -  return NS_ERROR_NOT_IMPLEMENTED;
> +  // XXX We need to implement this... But Gecko doesn't have this either
> +  //     so it's probably okay to leave it unimplemented currently?

Yes, sounds fine.
Attachment #8808056 - Flags: review+
Comment on attachment 8808056 [details]
Bug 1307357 part 5 - Implement css text getters for ServoStyleRule.

https://reviewboard.mozilla.org/r/90978/#review94842
Comment on attachment 8808057 [details]
Bug 1307357 part 6 - Implement CSSStyleRule.style.

https://reviewboard.mozilla.org/r/90980/#review94854
Attachment #8808057 - Flags: review?(cam) → review+
Comment on attachment 8808058 [details]
Bug 1307357 part 7 - Implement ServoStyleRule::List.

https://reviewboard.mozilla.org/r/90982/#review94856
Attachment #8808058 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/067fb417bb7b
part 1 - Make css::Rule hold StyleSheet rather than CSSStyleSheet. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9d6a3cdf1a
part 2 - Fix issues appear after adding file to unified source. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba29077a23e
part 3 - Implement CSSRuleList interface for stylo. r=heycam,manishearth
https://hg.mozilla.org/integration/mozilla-inbound/rev/31fdda2f930d
part 4 - Add impl class of CSSStyleRule for stylo. r=heycam,manishearth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6a77d3e4bb8
part 5 - Implement css text getters for ServoStyleRule. r=SimonSapin,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d831f568dd47
part 6 - Implement CSSStyleRule.style. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/29b1e2eb707e
part 7 - Implement ServoStyleRule::List. r=heycam
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/902bf2829601
followup 2 - Add AddRef/Release bindings to ServoBindingList.
Depends on: 1319994
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: