Unify NonOwningAnimationTarget and OwningElementRef

RESOLVED FIXED in Firefox 55

Status

()

P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: christopher.bathgate, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Comment 1

2 years ago
I think Boris is familiar with this.
Mentor: boris.chiou
Keywords: good-first-bug
(Assignee)

Comment 2

2 years ago
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++.
Flags: needinfo?(boris.chiou)
(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!
Flags: needinfo?(boris.chiou)
Assignee: nobody → christopher.bathgate
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
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
Flags: needinfo?(boris.chiou)
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.
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
Flags: needinfo?(boris.chiou)
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
(Assignee)

Comment 8

2 years ago
Ok, will do.
Flags: needinfo?(boris.chiou)
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 9

2 years ago
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?
Flags: needinfo?(boris.chiou)
(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
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 11

2 years ago
I have a patch prepaired. How do I share it with you guys before I submit it?
Flags: needinfo?(boris.chiou)
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
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
Flags: needinfo?(boris.chiou)
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
(Assignee)

Comment 15

2 years ago
Posted file Path File (obsolete) —
Sorry for the long wait. Have been busy. Heres the path for the bug. Let me know if its what you want
Flags: needinfo?(boris.chiou)
Attachment #8848620 - Flags: review+
(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. :)
Flags: needinfo?(boris.chiou)
Comment on attachment 8848620 [details]
Path File

BTW, you can also write an operator==() in NonOwningAnimationTarget, which may make the code conciser. :)
Attachment #8848620 - Flags: review+
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.
(Assignee)

Comment 19

2 years ago
Updates will be pushed by tomorrow.
(Assignee)

Updated

2 years ago
Attachment #8848620 - Attachment is obsolete: true
(Assignee)

Comment 20

2 years ago
Flags: needinfo?(boris.chiou)
Attachment #8851289 - Flags: review?(hikezoe)
Attachment #8851289 - Flags: review?(bbirtles)
(Reporter)

Comment 21

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

I think Boris knows about this better than me.
Attachment #8851289 - Flags: review?(hikezoe) → review?(boris.chiou)
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.
Attachment #8851289 - Flags: review?(boris.chiou)
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 23

2 years ago
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 :).
Attachment #8851289 - Attachment is obsolete: true
Attachment #8851289 - Flags: review?(bbirtles)
Attachment #8851348 - Flags: review?(boris.chiou)
Attachment #8851348 - Flags: review?(bbirtles)
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.
Attachment #8851348 - Flags: review?(boris.chiou) → review+
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
{
...
}
Attachment #8851348 - Flags: review+
(Assignee)

Comment 27

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

Updated

2 years ago
Attachment #8851348 - Attachment is obsolete: true
Attachment #8851348 - Flags: review?(bbirtles)
(Assignee)

Comment 28

2 years ago
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.
Attachment #8851359 - Flags: review?(boris.chiou)
Attachment #8851359 - Flags: review?(bbirtles)
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.
Attachment #8851359 - Flags: review?(boris.chiou) → review+
(Assignee)

Comment 31

2 years ago
Hopefully this irons out the style issues and the pass by reference
Attachment #8851359 - Attachment is obsolete: true
Attachment #8851359 - Flags: review?(bbirtles)
Attachment #8851364 - Flags: review?(boris.chiou)
Attachment #8851364 - Flags: review?(bbirtles)
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
Attachment #8851359 - Attachment is obsolete: false
Attachment #8851359 - Flags: review+
Attachment #8851359 - Attachment is obsolete: true
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.
Attachment #8851364 - Flags: review?(boris.chiou)
(Assignee)

Comment 34

2 years ago
Added explicit and const to OwningElementRef constructor. Fingers crossed thats it. Thanks for the time and the help :)
Attachment #8851364 - Attachment is obsolete: true
Attachment #8851364 - Flags: review?(bbirtles)
Attachment #8851366 - Flags: review?(boris.chiou)
Attachment #8851366 - Flags: review?(bbirtles)
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.
Attachment #8851366 - Flags: review?(boris.chiou) → review+
(Assignee)

Comment 37

2 years ago
I promise to be more competent with my code style next bug.
Attachment #8851366 - Attachment is obsolete: true
Attachment #8851366 - Flags: review?(bbirtles)
Attachment #8851378 - Flags: review?(boris.chiou)
Attachment #8851378 - Flags: review?(bbirtles)
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.
Attachment #8851378 - Flags: review?(bbirtles)
(Assignee)

Comment 39

2 years ago
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.
Attachment #8851378 - Attachment is obsolete: true
Attachment #8851378 - Flags: review?(boris.chiou)
Attachment #8851425 - Flags: review?(boris.chiou)
Attachment #8851425 - Flags: review?(bbirtles)
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:'
Attachment #8851425 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 41

2 years ago
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.
Attachment #8851425 - Attachment is obsolete: true
Attachment #8851425 - Flags: review?(boris.chiou)
Attachment #8851436 - Flags: review?(boris.chiou)
Attachment #8851436 - Flags: review?(bbirtles)
Attachment #8851436 - Flags: review?(bbirtles) → review+
Thanks for doing this!
(Assignee)

Comment 43

2 years ago
(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 on attachment 8851436 [details] [diff] [review]
Bug 1318223 - Use NonOwningAnimationTarget in OwningElementRef

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

Look good to me.
Attachment #8851436 - Flags: review?(boris.chiou) → review+
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!
Keywords: checkin-needed
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

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1c6516372e
Use NonOwningAnimationTarget in OwningElementRef. r=boris, r=birtles
Keywords: checkin-needed

Comment 49

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa1c6516372e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.