1318223 - Unify NonOwningAnimationTarget and OwningElementRef

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P5)

Description

[Hiroyuki Ikezoe (:hiro)]

NonOwningAnimationTarget and OwningElementRef are pretty similar, we can unify them.

[1] https://hg.mozilla.org/mozilla-central/file/c27117f67fa3/dom/animation/AnimationTarget.h#l41
[2] https://hg.mozilla.org/mozilla-central/file/c27117f67fa3/layout/style/AnimationCommon.h#l89

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I think Boris is familiar with this.

Comment 2

[christopher.bathgate]

I would like to work on this bug. I am taking a software engineering course where one the assignment is to fix a firefox bug. I would like a mentor to help me out with this as I am new to the Mozilla repository and C++.

Comment 3

[Boris Chiou [:boris]]

(In reply to christopher.bathgate from comment #2)
> I would like to work on this bug. I am taking a software engineering course
> where one the assignment is to fix a firefox bug. I would like a mentor to
> help me out with this as I am new to the Mozilla repository and C++.

Hi, christopher.bathgate, here are some steps you can follow:

1. Let's setup your environment first. You can follow the steps in the document (based on your platform):
  * https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
  * In short:
    a) We use mercurial as the version control tool, so you have to install "Mercurial" first (which is an equivalent of git).
    b) Install Python 2
    c) Get the code:
      * `hg clone http://hg.mozilla.org/mozilla-central/`
    d) After downloading:
      * `cd mozilla-central`
      * `./mach bootstrap`  <-- This will download and install all you need for building Firefox
    e) Build the code
      * `./mach build`
    f) Run it
      * `./mach run`
2. If you want to know more about how to run the code, just type `./mach help`
3. You can also use this website to navigate firefox code:
  * https://dxr.mozilla.org/mozilla-central/source/
4. If you need more details about how to build/run/debug Firefox, you can read this:
  * https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Please feel free to ask me or hiro about how to setup your environment or anything about C++. Enjoy it!

Comment 4

[christopher.bathgate]

So Im curious how I should arrange the replacement class for the two. NonOwningTarget seems like a more relivant and semantic name with the fact that it is indeed "non-owned" while OwningElementRef's name is missleading. I was thinking of changing the Class OwningElementRef to class NonOwningTarget and delete the previous struct. Though I'm worried that seperating NonOwningTarget from OwningTarget could make things less cohearent. Originally I was thinking of including the features of OwningElementRef in the NonOwningTarget struct but Im not familliar with the build / depedency system enough to make that work. Is there any additional context that I'm missing that could make a replacement fit in better? 

Thanks for assigning me!
Cheers

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I don't know if we should merge these. It might be better use (Non)OwningAnimationTarget as a member of OwningElementRef and possibly add a ctor to OwningElementRef that takes a (Non)OwningAnimationTarget.

Comment 6

[Boris Chiou [:boris]]

Currently, we use |OwningElementRef| in CSS Animations and CSS Transitions for some special cases [1], and use |NonOwningAnimationTarget| for the animation backend API (e.g. KeyframeEffectReadOnly). Just replace |OwningElementRef| with |NonOwningAnimationTarget| may not break the system, but as Brian mentioned, it may not be necessary, so I think we can follow Brian's idea: We don't really need to remove |OwningElementRef|. Let |OwningElementRef| store a |NonOwningAnimationTarget| object, and revise its methods to use it, so |OwningElementRef| will looks like a wrapper of |NonOwningAnimationTarget|, for CSS Animations/Transitions. This way may be better and easier for us to unify these two classes.

[1] https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3AOwningElementRef

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

My understanding is that OwningElementRef is a CSS-specific thing. See CSS Animations 2 which defines the concept of an "owning element".[1] That's why I think it's ok to keep OwningElementRef and keep it as something defined in layout/style. Some of the methods like LessThan and GetRenderedPresContext are also a little bit CSS-specific.

So, I think just replacing the members of OwningElementRef with a NonOwningAnimationTarget would be enough to highlight the difference. Also, adding a constructor to OwningElementRef that takes a NonOwningAnimationTarget might be useful depending on how it is used (and bug 1340322 will probably change that).

[1] https://drafts.csswg.org/css-animations-2/#owning-element-section

Comment 8

[christopher.bathgate]

Ok, will do.

Comment 9

[christopher.bathgate]

I'm trying to include AnimationTarget.h into AnimationCommon.h but I am unfamiliar with the build system so I'm getting a file not found error.

In AnimationCommon.h I have included the latter line thats resulting in the error. 

#include "mozilla/dom/Animation.h
#include "mozilla/dom/AnimationTarget.h

In the layout/style/moz.build file would updating LOCAL_INCLUDES solve this?

Comment 10

[Boris Chiou [:boris]]

(In reply to christopher.bathgate from comment #9)
> I'm trying to include AnimationTarget.h into AnimationCommon.h but I am
> unfamiliar with the build system so I'm getting a file not found error.
> 
> In AnimationCommon.h I have included the latter line thats resulting in the
> error. 
> 
> #include "mozilla/dom/Animation.h
> #include "mozilla/dom/AnimationTarget.h
> 
> In the layout/style/moz.build file would updating LOCAL_INCLUDES solve this?

Oh, you should use:

#include "mozilla/AnimationTarget.h"  // AnimaitonTarget.h is in mozilla namespace, but not in dom namespace.

LOCAL_INCLUDES in moz.build is used for some special headers from other modules, and I think we don't need to revise it in this bug. If you want to know the specific namespace path of AnimationTarget.h, you can check dom/animation/moz.build [1].

Thanks.

[1] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/dom/animation/moz.build#29

Comment 11

[christopher.bathgate]

I have a patch prepaired. How do I share it with you guys before I submit it?

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hi Christopher, you can just attach the patch to the bug. If you have it as a patch file already you can use the "Attach file" link at the top of this page. You need to upload the file, write a description, and tick the box that says "patch".

There is some documentation on the wiki about making patches[1] but unfortunately it focuses on using MozReview which can be difficult to setup (it's probably worth doing after you have written 2 or 3 patches).

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 13

[Boris Chiou [:boris]]

Thanks, Brian. Yes, if you use hg, I think you can use `hg bzexport`, just as the document mentioned. This is the easiest way to attach few patches to Bugzilla. Besides, you can also follow the workflow in the document [1] to setup Mozreview. I think mozreview is more convenient for reviewers and committers if you have lots of patches in a bug. (I'm not sure whether `./mach mercurial-setup` helps you setup all the things correctly for Mozreview, but it is worth to try it)

For the newcomer, my suggestion is the same as Brian's: just attach the patch to the bug by `hg bzexport` or `hg export > change.patch` with `Attach File` button manually.

[1] https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html

Comment 14

[Boris Chiou [:boris]]

By the way. I'm a git user, so I can use the following ways to send my patches to Bugzilla.
1. Use Mozreview (by git-cinnnabar [1])
  * Mozreview is not friendly to git users, so I suggest that you use mercurial for Mozreview.

2. Use moz-git-tool [2]: (This is the equivalent way of `hg bzexport`)
  * After cloning moz-git-tool and setting the bash $PATH, I just type 'git bz attach [bug number] [commit] -e', which will show an interactive console to push your changes to Bugzilla by the bug number you provide (e.g. 1318223).

[1] https://github.com/glandium/git-cinnabar
[2] https://github.com/mozilla/moz-git-tools

Comment 15

[christopher.bathgate]

Attachment: Path File

Sorry for the long wait. Have been busy. Heres the path for the bug. Let me know if its what you want

Comment 16

[Boris Chiou [:boris]]

(In reply to christopher.bathgate from comment #15)
> Created attachment 8848620 [details]
> Path File
> 
> Sorry for the long wait. Have been busy. Heres the path for the bug. Let me
> know if its what you want

Thanks for the patch, but could you please re-upload this patch with the following checkbox:

1. Check "patch" in Content Type.
2. Use auto-detect for the file format
3. Mark r? to :hiro or :birtles (because they know more about the usage of OwningElementRef)
4. Obsolete the old patch to avoid confusion.

So the attachment will become "patch format", and reviewers would be easier to review it.


Thanks for the contribution. :)

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8848620 [details]
Path File

BTW, you can also write an operator==() in NonOwningAnimationTarget, which may make the code conciser. :)

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8848620 [details]
Path File

And the commit message could be something like:

Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef.

or others you like. Just need to put the bug number as the prefix.

Comment 19

[christopher.bathgate]

Updates will be pushed by tomorrow.

Comment 20

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8851289 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

I think Boris knows about this better than me.

Comment 22

[Boris Chiou [:boris]]

Comment on attachment 8851289 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851289 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/AnimationCommon.h
@@ +113,4 @@
>  {
>  public:
>    OwningElementRef()
> +    : mTarget(nullptr,CSSPseudoElementType::NotPseudo)

nit:
One space between the comma and CSSPseudoElementType.

e.g.
"nullptr, CSSPseudoElementType::NotPseudo"

@@ +145,3 @@
>    }
>  
> +  bool IsSet() const { return !!mTarget.mElement; }

One day, we may want to add the `operator bool() const` for NonOwningAnimationTarget. However, this is fine now.

@@ +148,4 @@
>  
>    void GetElement(dom::Element*& aElement,
>                    CSSPseudoElementType& aPseudoType) const {
> +    mTarget = NonOwningAnimationTarget(aElement,aPseudoType);

This is not "Set", so I think you should write something like this?
`
aElement = mTarget.mElement;
aPseudoType = mTarget.mPseudoType;
`

@@ +156,3 @@
>    dom::Element* MOZ_NON_OWNING_REF mElement;
>    CSSPseudoElementType             mPseudoType;
> +  */

Please remove the comment part. I think using NonOwningAnimationTarget is enough.

@@ +156,4 @@
>    dom::Element* MOZ_NON_OWNING_REF mElement;
>    CSSPseudoElementType             mPseudoType;
> +  */
> +  mutable NonOwningAnimationTarget         mTarget;

According to the above comment, we don't need "mutable" keyword.

Comment 23

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Let me know if anything more needs to change. Misread the the GetElement function for some reason last patch so thanks for pointing that out. I appreciate collab :).

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 8851348 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851348 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationTarget.h
@@ +40,4 @@
>  
>  struct NonOwningAnimationTarget
>  {
> +  // Default construtor if no arguments are provided

nit: add period.

Comment 25

[Boris Chiou [:boris]]

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

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8851348 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851348 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationTarget.h
@@ +51,5 @@
>  
>    explicit NonOwningAnimationTarget(const OwningAnimationTarget& aOther)
>      : mElement(aOther.mElement), mPseudoType(aOther.mPseudoType) { }
>  
> +  bool operator==(const NonOwningAnimationTarget& aOther){

Oops. This should be a const method! (Sorry didn't notice this in the previous review.)
e.g.
bool operator==(const NonOwningAnimationTarget& aOther) const
{
...
}

Comment 27

[christopher.bathgate]

Yep, I figured that out. Fixed it. I'm new to C++ so don't mind me :).

Comment 28

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Fixed Const issue. Send me another link to your CI if there is another bug. I ran mach test on my machine and it seemed to pass.

Comment 29

[Boris Chiou [:boris]]

Comment on attachment 8851359 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851359 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/AnimationTarget.h
@@ +51,5 @@
>  
>    explicit NonOwningAnimationTarget(const OwningAnimationTarget& aOther)
>      : mElement(aOther.mElement), mPseudoType(aOther.mPseudoType) { }
>  
> +  bool operator==(const NonOwningAnimationTarget& aOther) const{

nit:
Different projects have different coding styles, and here is the reference of mozilla coding style [1]. We use different lines for C++ method/function signature and curly brackets if one line is not enough to contain all things.

`
bool operator==(const NonOwningAnimationTarget& aOther) const
{
  return ...;
}
`
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

::: layout/style/AnimationCommon.h
@@ +115,5 @@
>    OwningElementRef()
> +    : mTarget(nullptr, CSSPseudoElementType::NotPseudo)
> +  { }
> +
> +  OwningElementRef(NonOwningAnimationTarget aTarget)

NonOwningAnimationTarget contains one pointer and an uint32_t enum, so using copy is fine for now. However, I prefer to pass by reference.
e.g. 
OwningElementRef(const NonOwningAnimationTarget& aTarget)

This will reduce the large memory copy if NonOwningAnimationTarget grows.

Comment 30

[Boris Chiou [:boris]]

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

Comment 31

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Hopefully this irons out the style issues and the pass by reference

Comment 32

[Boris Chiou [:boris]]

Comment on attachment 8851359 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851359 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, the try is failed, so we still need to update this patch.

::: layout/style/AnimationCommon.h
@@ +115,5 @@
>    OwningElementRef()
> +    : mTarget(nullptr, CSSPseudoElementType::NotPseudo)
> +  { }
> +
> +  OwningElementRef(NonOwningAnimationTarget aTarget)

This causes a compilation error on try server, we should use 'explicit' keyword, I think. So you can change this to something like:

explicit OwningElementRef(const NonOwningAnimationTarget& aTarget)

We use "explicit" to avoid bad implicit conversion if this constructor only has one argument. If we don't add 'explicit', this becomes a converting constructor [1] which is not what we want, and some unwanted implicit conversions might happen.

[1] http://en.cppreference.com/w/cpp/language/converting_constructor

Comment 33

[Boris Chiou [:boris]]

Comment on attachment 8851364 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851364 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/AnimationCommon.h
@@ +115,5 @@
>    OwningElementRef()
> +    : mTarget(nullptr, CSSPseudoElementType::NotPseudo)
> +  { }
> +
> +  OwningElementRef(NonOwningAnimationTarget& aTarget)

Use "const NonOwningAnimationTarget&", so the caller can pass a const or a non-const target to this constructor.

Comment 34

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Added explicit and const to OwningElementRef constructor. Fingers crossed thats it. Thanks for the time and the help :)

Comment 35

[Boris Chiou [:boris]]

Comment on attachment 8851366 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851366 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/AnimationCommon.h
@@ +115,5 @@
>    OwningElementRef()
> +    : mTarget(nullptr, CSSPseudoElementType::NotPseudo)
> +  { }
> +
> +  explicit OwningElementRef (const NonOwningAnimationTarget& aTarget)

nit: remove the space between method name and left parenthese.

Comment 36

[Boris Chiou [:boris]]

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

Comment 37

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

I promise to be more competent with my code style next bug.

Comment 38

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8851378 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851378 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I'd like to take another look with the following comments addressed.

Also, please update the commit message to explain what the patch is doing.

::: dom/animation/AnimationTarget.h
@@ +43,5 @@
> +  // Default constructor if no arguments are provided.
> +  NonOwningAnimationTarget()
> +    : mElement(nullptr)
> +    , mPseudoType(CSSPseudoElementType::NotPseudo)
> +  { }

I'm not sure we need this since the changes to AnimationCommon.h don't see to require it. However, perhaps it's best to keep this but make it:

  NonOwningAnimationTarget() = default;

(i.e. don't specify the members since they're specified by default initializers below.)

And then in AnimationCommon.h, make the default constructor just:

  OwningElementRef() = default;

@@ +53,5 @@
>      : mElement(aOther.mElement), mPseudoType(aOther.mPseudoType) { }
>  
> +  bool operator==(const NonOwningAnimationTarget& aOther) const
> +  {
> +    return mElement == aOther.mElement && mPseudoType == aOther.mPseudoType;

For consistency with OwningAnimationTarget, let's wrap the line after &&.

::: layout/style/AnimationCommon.h
@@ +136,4 @@
>                 "Elements to compare should not be null");
>  
> +    if (mTarget.mElement != aOther.mTarget.mElement) {
> +      return nsContentUtils::PositionIsBefore(mTarget.mElement, aOther.mTarget.mElement);

Please wrap this line to 80 characters.

@@ +154,4 @@
>    }
>  
>  private:
> +  NonOwningAnimationTarget         mTarget;

We can probably drop the extra space between NonOwningAnimationTarget and mTarget.

Comment 39

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Substituted OwningElementRef properties with a NonOwningAnimationTarget struct. Adjusted methods in OwningElementRef accordingly. Added default constructors to both OwningElementRef and NonOwningAnimationTarget. Added an equality operator method to NonOwningAnimationTarget to streamline code in OwningElementRef. Minor style additions.

Comment 40

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8851425 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851425 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with the following two comments addressed.

::: dom/animation/AnimationTarget.h
@@ +40,4 @@
>  
>  struct NonOwningAnimationTarget
>  {
> +  // Default constructor if no arguments are provided.

Let's drop this comment since it doesn't seem necessary now.

::: layout/style/AnimationCommon.h
@@ +112,5 @@
>  class OwningElementRef final
>  {
>  public:
> +
> +  OwningElementRef() = default;

Let's drop the blank line after 'public:'

Comment 41

[christopher.bathgate]

Attachment: Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Substituted OwningElementRef properties with a NonOwningAnimationTarget struct. Adjusted methods in OwningElementRef accordingly. Added default constructors to both OwningElementRef and NonOwningAnimationTarget. Added an equality operator method to NonOwningAnimationTarget to streamline code in OwningElementRef. Minor style additions.

Comment 42

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks for doing this!

Comment 43

[christopher.bathgate]

(In reply to Brian Birtles (:birtles) from comment #42)
> Thanks for doing this!

No thank you and Boris too for giving me all those pointers. This is my first contribution to an open source project and now that I've gotten a taste, I'm excited to continue to contribute to firefox and other projects in the future.

Anything more let me know.

Cheers!

Comment 44

[Boris Chiou [:boris]]

Comment on attachment 8851436 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

Review of attachment 8851436 [details] [diff] [review]:
-----------------------------------------------------------------

Look good to me.

Comment 45

[Boris Chiou [:boris]]

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

Comment 46

[Boris Chiou [:boris]]

After all try test cases are passed, I will mark this "checkin-needed", and our Sheriffs will cherry-pick your patch into mozilla-inbound. And wait for few hours, all patches in mozilla-inbound will be merged into mozilla-central, so you will see your name in firefox code base. Good job!

Comment 47

[Ryan VanderMeulen [:RyanVM]]

In the future, please ensure that patches include appropriate commit information when requesting checkin.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1c6516372e
Use NonOwningAnimationTarget in OwningElementRef. r=boris, r=birtles

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa1c6516372e