Closed Bug 1315601 Opened 7 years ago Closed 7 years ago

stylo: Implement access to CSSMediaRule

Categories

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

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

Tracking

()

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

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(17 files)

Bug 1315601 part 1 - Move type_traits include down in nsCSSValue.h.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 2 - Replace macros with template function with lambda in nsMediaList.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 3 - Add base class MediaList and move part of nsMediaList to it.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 4 - Add raw Servo types for MediaList and MediaRule.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1315601 part 5 - Implement MediaList for Stylo.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 6 - Move GroupRule-related code into a separate source file.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 7 - Simplify ConditionRule.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 8 - Constify Rule::GetCssText.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 10 - Make ServoCSSRuleList support being nested.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 11 - Add Servo support to GroupRule.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 12 - Remove useless retval out param from InsertRuleIntoGroup.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 13 - Move common code of DeleteRuleFromGroup/InsertRuleIntoGroup from CSSStyleSheet to StyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 14 - Add InsertRuleIntoGroup support to ServoStyleSheet.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 15 - Move some common methods to a new CSSMediaRule binding class.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 16 - Implement ServoMediaRule.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1315601 part 17 - Update test expectation.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
      No description provided.
Priority: -- → P1
This bug would add more Arc types across the FFI boundary, so I'd like to make it depend on bug 1319614.
Depends on: 1319614
Taking this as I have been working on this.
Assignee: nobody → xidorn+moz
Component: DOM: CSS Object Model → CSS Parsing and Computation
Blocks: 1345698
Blocks: 1324702
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=914caccaf8d17d8d39cc44d71ad4dd66ded66881

I'm so sad that the numbers in stylo-failures.md increase a lot... Probably because those tests no longer terminates after accessing a null media rule...

We'll probably need to triage the new failures... I guess most of them are media rule parsing issues in servo.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #20)
> try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=914caccaf8d17d8d39cc44d71ad4dd66ded66881
> 
> I'm so sad that the numbers in stylo-failures.md increase a lot... Probably
> because those tests no longer terminates after accessing a null media rule...
> 
> We'll probably need to triage the new failures... I guess most of them are
> media rule parsing issues in servo.

That's ok - the big upside is that we've reduced uncertainty and uncovered a chunk of work we didn't know about before, which is great. Parsing issues are also nice because we have so many volunteers interested in working on them. :-)
Comment on attachment 8845346 [details]
Bug 1315601 part 4 - Add raw Servo types for MediaList and MediaRule.

https://reviewboard.mozilla.org/r/118536/#review120744
Attachment #8845346 - Flags: review?(manishearth) → review+
Comment on attachment 8845343 [details]
Bug 1315601 part 1 - Move type_traits include down in nsCSSValue.h.

https://reviewboard.mozilla.org/r/118530/#review120808
Attachment #8845343 - Flags: review?(cam) → review+
Comment on attachment 8845344 [details]
Bug 1315601 part 2 - Replace macros with template function with lambda in nsMediaList.

https://reviewboard.mozilla.org/r/118532/#review120836

::: layout/style/nsMediaList.h:309
(Diff revision 1)
> +  template<typename Func>
> +  nsresult nsMediaList::WrapMediaChange(Func aCallback);

Hmm, why does "nsMediaList::" have to be mentioned here?

Anyway, looks fine, though I would prefer a different name.  "Wrap" sounds like something that is being done to some objects or something.  How about just "DoMediaChange"?  And a quick comment here above the declaration would be good.
Attachment #8845344 - Flags: review?(cam) → review+
Comment on attachment 8845345 [details]
Bug 1315601 part 3 - Add base class MediaList and move part of nsMediaList to it.

https://reviewboard.mozilla.org/r/118534/#review120838

::: layout/style/MediaList.h:23
(Diff revision 1)
> +// XXX This class doesn't use the branch dispatch approach that we use
> +//     elsewhere for stylo, but instead just relies on virtual call.
> +//     That's because this class should not be critical to performance,
> +//     and using branch dispatch would make it much more complicated.
> +//     Performance critical path should hold a subclass of this class
> +//     directly. We may want to determine in the future whether the
> +//     above is correct.

This sounds right to me.

::: layout/style/MediaList.h:48
(Diff revision 1)
> +  NS_IMETHOD GetMediaText(nsAString& aMediaText) final;
> +  NS_IMETHOD SetMediaText(const nsAString& aMediaText) final;
> +  NS_IMETHOD GetLength(uint32_t* aLength) final;
> +  NS_IMETHOD Item(uint32_t aIndex, nsAString& aReturn) final;
> +  NS_IMETHOD DeleteMedium(const nsAString& aOldMedium) final;
> +  NS_IMETHOD AppendMedium(const nsAString& aNewMedium) final;

Why expand out NS_DECL_NSIDOMMEDIALIST like this?
Attachment #8845345 - Flags: review?(cam) → review+
Comment on attachment 8845345 [details]
Bug 1315601 part 3 - Add base class MediaList and move part of nsMediaList to it.

https://reviewboard.mozilla.org/r/118534/#review120838

> Why expand out NS_DECL_NSIDOMMEDIALIST like this?

Mainly for adding the `final`s I think... Not sure whether it's worth.
Blocks: 1339656
Comment on attachment 8845347 [details]
Bug 1315601 part 5 - Implement MediaList for Stylo.

https://reviewboard.mozilla.org/r/118538/#review120844

::: layout/style/ServoMediaList.h:33
(Diff revision 1)
> +  nsresult Delete(const nsAString & aOldMedium) final;
> +  nsresult Append(const nsAString & aNewMedium) final;

Nit: no space before "&"s.

::: servo/ports/geckolib/glue.rs:913
(Diff revision 1)
>                                                              property: nsCSSPropertyID) {
>      remove_property(declarations, get_property_id_from_nscsspropertyid!(property, ()))
>  }
>  
> +#[no_mangle]
> +pub extern "C" fn Servo_MediaList_GetText(list: RawServoMediaListBorrowed, result: *mut nsAString) -> () {

No need for the "-> ()" here and elsewhere.  (I know some other functions do this in this file.)
Attachment #8845347 - Flags: review?(cam) → review+
Comment on attachment 8845348 [details]
Bug 1315601 part 6 - Move GroupRule-related code into a separate source file.

https://reviewboard.mozilla.org/r/118540/#review120856
Attachment #8845348 - Flags: review?(cam) → review+
Comment on attachment 8845349 [details]
Bug 1315601 part 7 - Simplify ConditionRule.

https://reviewboard.mozilla.org/r/118542/#review120858

::: layout/style/GroupRule.h:104
(Diff revision 1)
>  
>  // Implementation of WebIDL CSSConditionRule.
>  class ConditionRule : public GroupRule
>  {
>  protected:
> -  ConditionRule(uint32_t aLineNumber, uint32_t aColumnNumber);
> +  using GroupRule::GroupRule;

Thank you for teaching me this. :-)
Attachment #8845349 - Flags: review?(cam) → review+
Comment on attachment 8845350 [details]
Bug 1315601 part 8 - Constify Rule::GetCssText.

https://reviewboard.mozilla.org/r/118544/#review120860

(Not really sure why this is needed but I guess I might found out in later patches.)
Attachment #8845350 - Flags: review?(cam) → review+
Comment on attachment 8845351 [details]
Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct.

https://reviewboard.mozilla.org/r/118546/#review120862

::: layout/style/GroupRule.h:115
(Diff revision 1)
>  
>  public:
>    void AppendStyleRule(Rule* aRule);
>  
> -  int32_t StyleRuleCount() const { return mRules.Count(); }
> -  Rule* GetStyleRuleAt(int32_t aIndex) const;
> +  DEFINE_REDIRECT_TO_INNER0(int32_t, StyleRuleCount, const)
> +  DEFINE_REDIRECT_TO_INNER(Rule*, GetStyleRuleAt, const, (int32_t, aIndex))

I find these DEFINE_* macros a bit hard to read.  Are you sure it wouldn't be clearer to (a) include the forwarding code inline here, so we don't need to use a macro, and (b) make the inner objects use inheritance and virtual functions, to skip the Variant-based dispatch?  Like the media list stuff, this is probably not performance critical to require non-virtual-function-based dispatch.  WDYT?
Comment on attachment 8845352 [details]
Bug 1315601 part 10 - Make ServoCSSRuleList support being nested.

https://reviewboard.mozilla.org/r/118548/#review120866
Attachment #8845352 - Flags: review?(cam) → review+
Comment on attachment 8845354 [details]
Bug 1315601 part 12 - Remove useless retval out param from InsertRuleIntoGroup.

https://reviewboard.mozilla.org/r/118552/#review120884
Attachment #8845354 - Flags: review?(cam) → review+
Comment on attachment 8845351 [details]
Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct.

https://reviewboard.mozilla.org/r/118546/#review120862

> I find these DEFINE_* macros a bit hard to read.  Are you sure it wouldn't be clearer to (a) include the forwarding code inline here, so we don't need to use a macro, and (b) make the inner objects use inheritance and virtual functions, to skip the Variant-based dispatch?  Like the media list stuff, this is probably not performance critical to require non-virtual-function-based dispatch.  WDYT?

IRC discussion convinced me that we can keep the Variant-based approach.  But I think it would be nice to write out the declarations a bit more, like:

  Rule* GetStyleRuleAt(int32_t aIndex) const {
    return CALL_INNER(GetStyleRuleAt, aIndex);
  }

It's a bit longer, but I think makes it clearer (and more readable) when reading the code for the declarations.
Comment on attachment 8845351 [details]
Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct.

https://reviewboard.mozilla.org/r/118546/#review120888

r=me with the change to the forwarding functions.
Attachment #8845351 - Flags: review?(cam) → review+
Comment on attachment 8845353 [details]
Bug 1315601 part 11 - Add Servo support to GroupRule.

https://reviewboard.mozilla.org/r/118550/#review120890

::: layout/style/GroupRule.h:84
(Diff revision 1)
> +    // Do we ever clone Servo rules?
> +    MOZ_ASSERT_UNREACHABLE("stylo: Cloning GroupRule not implemented");

We don't at the moment, but I think we'll have to to support the copy-on-write style sheet stuff.
Attachment #8845353 - Flags: review?(cam) → review+
Comment on attachment 8845355 [details]
Bug 1315601 part 13 - Move common code of DeleteRuleFromGroup/InsertRuleIntoGroup from CSSStyleSheet to StyleSheet.

https://reviewboard.mozilla.org/r/118554/#review120892
Attachment #8845355 - Flags: review?(cam) → review+
Comment on attachment 8845356 [details]
Bug 1315601 part 14 - Add InsertRuleIntoGroup support to ServoStyleSheet.

https://reviewboard.mozilla.org/r/118556/#review120894
Attachment #8845356 - Flags: review?(cam) → review+
Comment on attachment 8845357 [details]
Bug 1315601 part 15 - Move some common methods to a new CSSMediaRule binding class.

https://reviewboard.mozilla.org/r/118558/#review120906
Attachment #8845357 - Flags: review?(cam) → review+
Comment on attachment 8845358 [details]
Bug 1315601 part 16 - Implement ServoMediaRule.

https://reviewboard.mozilla.org/r/118560/#review120908

::: commit-message-d01e4:1
(Diff revision 1)
> +Bug 1315601 part 16 - Implemnet ServoMediaRule. r?heycam

*Implement
Attachment #8845358 - Flags: review?(cam) → review+
Comment on attachment 8845359 [details]
Bug 1315601 part 17 - Update test expectation.

https://reviewboard.mozilla.org/r/118562/#review120926
Attachment #8845359 - Flags: review?(cam) → review+
Comment on attachment 8845351 [details]
Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct.

It should be more readable?
Attachment #8845351 - Flags: review+ → review?(cam)
Comment on attachment 8845351 [details]
Bug 1315601 part 9 - Split Gecko-specific GroupRule logic into a separate struct.

Yes, I think that looks better, thanks!
Attachment #8845351 - Flags: review?(cam) → review+
Attachment #8845347 - Flags: review?(simon.sapin) → review?(manishearth)
Comment on attachment 8845347 [details]
Bug 1315601 part 5 - Implement MediaList for Stylo.

https://reviewboard.mozilla.org/r/118538/#review121378
Attachment #8845347 - Flags: review?(manishearth) → review+
Attachment #8845347 - Flags: review?(simon.sapin)
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/edd8f422534c
part 1 - Move type_traits include down in nsCSSValue.h. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bbf15d220ee4
part 2 - Replace macros with template function with lambda in nsMediaList. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ddb17c50f4f0
part 3 - Add base class MediaList and move part of nsMediaList to it. r=heycam
https://hg.mozilla.org/integration/autoland/rev/067410e1cc20
part 4 - Add raw Servo types for MediaList and MediaRule. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/f89b8ee3008c
part 5 - Implement MediaList for Stylo. r=heycam,manishearth
https://hg.mozilla.org/integration/autoland/rev/1b0514e7d9ab
part 6 - Move GroupRule-related code into a separate source file. r=heycam
https://hg.mozilla.org/integration/autoland/rev/915ca91769c9
part 7 - Simplify ConditionRule. r=heycam
https://hg.mozilla.org/integration/autoland/rev/dd3258136cd2
part 8 - Constify Rule::GetCssText. r=heycam
https://hg.mozilla.org/integration/autoland/rev/962cf3fafa83
part 9 - Split Gecko-specific GroupRule logic into a separate struct. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3514cd5db5ba
part 10 - Make ServoCSSRuleList support being nested. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d0d3a86720b2
part 11 - Add Servo support to GroupRule. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ca7be213dd48
part 12 - Remove useless retval out param from InsertRuleIntoGroup. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6de9f799a27c
part 13 - Move common code of DeleteRuleFromGroup/InsertRuleIntoGroup from CSSStyleSheet to StyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ff1725928cbe
part 14 - Add InsertRuleIntoGroup support to ServoStyleSheet. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d45f19e7fa28
part 15 - Move some common methods to a new CSSMediaRule binding class. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cdee044cb563
part 16 - Implement ServoMediaRule. r=heycam
https://hg.mozilla.org/integration/autoland/rev/cc79715e83a5
part 17 - Update test expectation. r=heycam
Depends on: 1346734
No longer depends on: 1346734
You need to log in before you can comment on or make changes to this bug.