Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 7 years ago

      No description provided.

	Hiroyuki Ikezoe (:hiro)
	Comment 1 • 7 years ago

There are two issue (bug 1337678 and bug 1337680) on current gecko that we should fix for stylo as well (or only fix for stylo?).

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 9 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c64d94a1623f24acb9d123d02d20255081e19551

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 17 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dfd4d195fe56a7a28d0a852064b45d1c6be4353

	Cameron McCormack (:heycam)
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8868499 [details]
Bug 1345697 part 1 - Various fix for adding new files.

https://reviewboard.mozilla.org/r/140116/#review143884

	Cameron McCormack (:heycam)
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8868500 [details]
Bug 1345697 part 2 - Add CSSKeyframesRule and CSSKeyframeRule base class.

https://reviewboard.mozilla.org/r/140118/#review143886

	Cameron McCormack (:heycam)
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8868501 [details]
Impl to_css for KeyframeSelector.

https://reviewboard.mozilla.org/r/140120/#review143888

	Cameron McCormack (:heycam)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8868502 [details]
Record source location for keyframes rule.

https://reviewboard.mozilla.org/r/140122/#review143890

	Cameron McCormack (:heycam)
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8868503 [details]
Align serialization of keyframes rule with Gecko.

https://reviewboard.mozilla.org/r/140124/#review143892

	Cameron McCormack (:heycam)
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8868504 [details]
Bug 1345697 part 3 - Implement CSSKeyframesRule and CSSKeyframeRule for stylo.

https://reviewboard.mozilla.org/r/140126/#review143900

Does it make sense to try to re-use some code between ServoCSSRuleList and ServoKeyframeList?  (Maybe you already tried?)

::: layout/style/ServoKeyframesRule.cpp:331
(Diff revision 2)
> +    // Construct mKeyframeList but ignore the result.
> +    CssRules();
> +    return mKeyframeList->GetRule(index);

Why not just:

  return CssRules()->GetRule(index);

::: servo/components/style/stylesheets.rs:590
(Diff revision 2)
>          dest.write_str("\n}")
>      }
>  }
>  
> +impl KeyframesRule {
> +    /// Returns the index of last keyframe whose matches the given selector.

"Returns the index of the last keyframe that matches the given selector."

::: servo/components/style/stylesheets.rs:595
(Diff revision 2)
> +    /// Returns the index of last keyframe whose matches the given selector.
> +    /// If the selector is not valid, or no keyframe is found, returns None.
> +    ///
> +    /// Related spec:
> +    /// https://drafts.csswg.org/css-animations-1/#interface-csskeyframesrule-findrule
> +    pub fn find_rule(&self, guard: &SharedRwLockReadGuard, select: &str) -> Option<usize> {

"selector" instead of "select"?

	Cameron McCormack (:heycam)
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8868505 [details]
Bug 1345697 part 4 - Update test expectation.

https://reviewboard.mozilla.org/r/140128/#review143924

::: layout/style/test/stylo-failures.md:39
(Diff revision 2)
> -  * test_animations.html [1]
> +  * test_animations.html [3]
>    * test_animations_dynamic_changes.html [1]
> -  * test_bug716226.html [1]
> +  * test_bug716226.html [3]

;_;

	Hiroyuki Ikezoe (:hiro)
	Comment 25 • 7 years ago

(In reply to Cameron McCormack (:heycam) from comment #24)
> Comment on attachment 8868505 [details]
> Bug 1345697 part 4 - Update test expectation.
> 
> https://reviewboard.mozilla.org/r/140128/#review143924
> 
> ::: layout/style/test/stylo-failures.md:39
> (Diff revision 2)
> > -  * test_animations.html [1]
> > +  * test_animations.html [3]
> >    * test_animations_dynamic_changes.html [1]
> > -  * test_bug716226.html [1]
> > +  * test_bug716226.html [3]
> 
> ;_;

It's because of bug 1344581 or bug 1364799. A test case that had not been running starts running. It's a good sign. Great Xidorn! Thank you!

I am now trying to fix both of bugs.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 26 • 7 years ago
mozreview-review-reply

Comment on attachment 8868504 [details]
Bug 1345697 part 3 - Implement CSSKeyframesRule and CSSKeyframeRule for stylo.

https://reviewboard.mozilla.org/r/140126/#review143900

Actually, it doesn't seem to me there is much code shareable between the two classes :/

> Why not just:
> 
>   return CssRules()->GetRule(index);

This is exactly what I tried first :)

Ths issue is that, `CssRules()` returns a `dom::CSSRuleList*`, which doesn't have a `GetRule` which returns a `ServoKeyframeRule`.

If we want to make `CssRules()` return `ServoKeyframeList*`, we would need to move the declaration of the class to the header file, otherwise the compiler would complain that `CssRules` doesn't have a compatible signature with the method it overrides. But the declaration is not needed otherwise... so I think it isn't worth.

	Mozilla Autoland
	Comment 31 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 407e3aa3f1b8 -d 55010a7ef404: rebasing 396794:407e3aa3f1b8 "Bug 1345697 part 1 - Various fix for adding new files. r=heycam"
merging layout/style/ServoBindings.cpp
rebasing 396795:a97464201a12 "Bug 1345697 part 2 - Add CSSKeyframesRule and CSSKeyframeRule base class. r=heycam"
rebasing 396796:41460384de01 "Bug 1345697 part 3 - Implement CSSKeyframesRule and CSSKeyframeRule for stylo. r=heycam"
rebasing 396797:a967a1284448 "Bug 1345697 part 4 - Update test expectation. r=heycam" (tip)
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 32 • 7 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96c55b0b6a8c
part 1 - Various fix for adding new files. r=heycam
https://hg.mozilla.org/integration/autoland/rev/723d9907e9a9
part 2 - Add CSSKeyframesRule and CSSKeyframeRule base class. r=heycam
https://hg.mozilla.org/integration/autoland/rev/52f5f0e30e27
part 3 - Implement CSSKeyframesRule and CSSKeyframeRule for stylo. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c5796625b535
part 4 - Update test expectation. r=heycam

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/96c55b0b6a8c
https://hg.mozilla.org/mozilla-central/rev/723d9907e9a9
https://hg.mozilla.org/mozilla-central/rev/52f5f0e30e27
https://hg.mozilla.org/mozilla-central/rev/c5796625b535