Split GetSMILOverrideStyle into a function that returns the existing one, and a function that creates it if needed.

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: emilio, Assigned: rahul0379, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

GetSMILOverrideStyle is a bit of a confusing name. We generally use the convention of Get* to mark things that can return null, but in this case it really cannot, and it creates the declaration.

It should be renamed to SMILOverrideStyle, and probably GetSMILOverrideStyleDeclaration should be renamed to GetSMILOverrideStyle, updating the callers.
Whiteboard: [lang=c++]
Priority: -- → P3

Comment 1

a year ago
someone help please...
as i have just started exploring open source, i couldn't understand the system here.
(In reply to priyansu pulak from comment #1)
> someone help please...
> as i have just started exploring open source, i couldn't understand the
> system here.

Emilio would like to rename `GetSMILOverrideStyle` to `SMILOverrideStyle` [1]. Once renamed you will need to update all callers and definitions to use the new name [2].

For more help getting started with contributing please take a look at our introduction guide [3].

[1] https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/dom/base/Element.h#364
[2] https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom7Element20GetSMILOverrideStyleEv&redirect=false
[3] https://developer.mozilla.org/docs/Mozilla/Developer_guide/Introduction

Comment 3

a year ago
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #2)
> (In reply to priyansu pulak from comment #1)
> > someone help please...
> > as i have just started exploring open source, i couldn't understand the
> > system here.
> 
> Emilio would like to rename `GetSMILOverrideStyle` to `SMILOverrideStyle`
> [1]. Once renamed you will need to update all callers and definitions to use
> the new name [2].
> 
> For more help getting started with contributing please take a look at our
> introduction guide [3].
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> f30847c12e5fb13791401ed4330ced3e1b9c8d81/dom/base/Element.h#364
> [2]
> https://searchfox.org/mozilla-central/search?q=symbol:
> _ZN7mozilla3dom7Element20GetSMILOverrideStyleEv&redirect=false
> [3] https://developer.mozilla.org/docs/Mozilla/Developer_guide/Introduction

Is anybody working on this thing ?
I am a newbie here and i am wondering whether i just have to rename the function name everywhere in the code ?
Or is it something else ?

If it is really just about successfully renaming the function, i can do it very well !
So please, i want to be assigned this task (If the task is all about what i just said)! 
Where can i get the source code and how do i submit the whole thing after i'm done?
(In reply to manishjoshi394 from comment #3)
> Is anybody working on this thing ?
> I am a newbie here and i am wondering whether i just have to rename the
> function name everywhere in the code ?
> Or is it something else ?

This bug is currently available for work, you are correct it's just a straightforward renaming.

> If it is really just about successfully renaming the function, i can do it
> very well !
> So please, i want to be assigned this task (If the task is all about what i
> just said)!

Once you have a patch uploaded I'll be happy to assign it to you. Please let us know if you need any assistance.

> Where can i get the source code and how do i submit the whole thing after
> i'm done?

Our introduction guide [1] gives full details, particularly step 1 and step 3. Specifically for getting the source and build take a look at our 'Building Firefox' [2] page and for submitting a patch see our 'How to submit a patch' page [3]. If you run into any issues you can ask questions in #introduction [4].

[1] https://developer.mozilla.org/docs/Mozilla/Developer_guide/Introduction
[2] https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[3] https://developer.mozilla.org/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
[4] https://client01.chat.mibbit.com/?url=irc%3A%2F%2Firc.mozilla.org%2F%23introduction
(Assignee)

Comment 5

11 months ago
Hi,

I am new to open source contribution. I can work on this bug as my first one if no one else has already done it. I couldn't find where are the source files which need to be patched in this page. Please direct me to them.
(Assignee)

Comment 6

11 months ago
Do we have to build Firefox on our machines before working on this? Is Visual Studio 15.7 still not supported?
(Reporter)

Comment 7

11 months ago
(In reply to rahul0379 from comment #5)
> Hi,
> 
> I am new to open source contribution. I can work on this bug as my first one
> if no one else has already done it. I couldn't find where are the source
> files which need to be patched in this page. Please direct me to them.

You're more than welcome to submit a patch, yeah!

The files are dom/base/Element.h / Element.cpp and then its caller which is in dom/smil/nsSMILCSSProperty.cpp. Then there's GetSMILOverrideDeclaration in a few other places, which you can find with:

https://searchfox.org/mozilla-central/search?q=getSmilOverrideStyle&path=

(In reply to rahul0379 from comment #6)
> Do we have to build Firefox on our machines before working on this? Is
> Visual Studio 15.7 still not supported?

Yes, you need to build Firefox on your machine. Apparently that VS version has a bug and can't be used at the moment:

  https://groups.google.com/forum/#!topic/mozilla.dev.platform/y6fqkz7JZaw

Comment 8

11 months ago
Hello,
I too am a newbie. Can I start working on this if it is still available?
(Assignee)

Comment 9

11 months ago
Hi,

I have built Firefox on my machine(it took forever).
I have made the necessary changes in the required files. How do I go ahead and test/commit/submit the patch?
(Reporter)

Comment 10

11 months ago
(In reply to rahul0379 from comment #9)
> Hi,
> 
> I have built Firefox on my machine(it took forever).

That's great! Yeah, it takes a bit, but builds afterwards should be fast.

> I have made the necessary changes in the required files. How do I go ahead
> and test/commit/submit the patch?

If it builds it's probably fine, you can submit the patch following:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

I'll push it to the CI server since you probably don't have access yet, and if it looks good we can land it. Thank you! :)

(In reply to Mridul Agarwal from comment #8)
> Hello,
> I too am a newbie. Can I start working on this if it is still available?

Yes, though it looks like rahul is working on it, so probably not worth stepping on his toes just yet. If you need help finding another bug let me know, I can think of something :)
(Assignee)

Comment 11

11 months ago
Hi, please find attached the patch file I created for this bug. Please review.
(Assignee)

Updated

11 months ago
Attachment #8979028 - Flags: review?(emilio)
(Reporter)

Comment 12

11 months ago
Comment on attachment 8979028 [details] [diff] [review]
bug1458580.patch - contains patch for this bug. renaming the GetSMILOverrideStyle calls and declarations

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

Thanks for working on this! Let's undo the GetSMILOverrideStyleDeclaration renaming, since that can return null. Other than that this looks good, just need to remove an unnecessary null-check :)

::: dom/base/Element.h
@@ +332,5 @@
>    /**
>     * Get the SMIL override style declaration for this element. If the
>     * rule hasn't been created, this method simply returns null.
>     */
> +  DeclarationBlock* SMILOverrideStyleDeclaration();

This shouldn't be renamed, since this one can return null. Since there are no callers that just want to poke at the existing nsDOMCSSAttributeDeclaration object, just renaming GetSMILOverrideStyle and removing the null-checks should be enough.

::: dom/smil/nsSMILCSSProperty.cpp
@@ +113,5 @@
>  void
>  nsSMILCSSProperty::ClearAnimValue()
>  {
>    // Put empty string in override style for our property
> +  nsDOMCSSAttributeDeclaration* overrideDecl = mElement->SMILOverrideStyle();

This can't return null, so let's remove the null-check as well, and just change this function to be:

  mElement->SmilOverrideStyle()->SetPropertyValue(mPropId, EmptyString(), nullptr);
Attachment #8979028 - Flags: review?(emilio) → feedback+
(Reporter)

Updated

11 months ago
Assignee: nobody → rahul0379
(Assignee)

Comment 13

11 months ago
I see! I've made the changes as follows:

In Element.h, I added Get in front of 'GetSMILOverrideStyleDeclaration'.
Same for it's definition in Element.cpp

::: dom/smil/nsSMILCSSProperty.cpp
The function is now just as you said.

Also in 
::: layout/style/nsDOMCSSAttrDeclaration.cpp
@@ line 99
Added Get in front of 'GetSMILOverrideStyleDeclaration' call.

I wanted to ask how I can use hg to commit to review.

`hg status` returns this:

    $ hg status
    M dom/base/Element.cpp
    M dom/base/Element.h
    M dom/smil/nsSMILCSSProperty.cpp
    M layout/style/nsDOMCSSAttrDeclaration.cpp
    ? .vs/ProjectSettings.json
    ? .vs/VSWorkspaceState.json
    ? .vs/mozilla-central/v15/.suo
    ? .vs/mozilla-central/v15/Browse.VC.db
    ? .vs/slnx.sqlite
    ? bug1458580.patch

After `hg commit -m "Bug 1458580 - GetSMILStyleOverride definition modified and renamed with callers. r=emilio`,
should I just `hg pull`, `hg merge`, `hg push review`?

Or should I do it another way?
(Assignee)

Comment 14

11 months ago
Updated the patch. Please review.
Attachment #8979028 - Attachment is obsolete: true
Attachment #8979109 - Flags: review?(emilio)
(Assignee)

Comment 15

11 months ago
Please review.
Attachment #8979109 - Attachment is obsolete: true
Attachment #8979109 - Flags: review?(emilio)
Attachment #8979112 - Flags: review?(emilio)
(Reporter)

Comment 16

11 months ago
Comment on attachment 8979112 [details] [diff] [review]
bug1458580.patch - contains patch for this bug. renaming the GetSMILOverrideStyle calls and declarations

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

Looks great, thank you!
Attachment #8979112 - Flags: review?(emilio) → review+

Comment 17

11 months ago
Emilio, do you want to land this?
Flags: needinfo?(emilio)

Comment 18

11 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4ca25a0f39
Rename GetSMILStyleOverride since it can't return null. r=emilio
(Reporter)

Comment 19

11 months ago
Yup, thanks for the reminder Eric. Rahul, for next time, you can also put the checkin-needed keyword on the bug if I forget.

Thanks for working on this!
Flags: needinfo?(emilio)
(Assignee)

Comment 20

11 months ago
Alright! I landed my first ever open source patch!

Emilio, can you help me get something to work on? I'm good with Python and C++. I also know JS, HTML, CSS. I'm also willing to work on something new to learn.
Flags: needinfo?(emilio)
(Reporter)

Comment 21

11 months ago
I have something in mind, though not sure if you'd be willing to work on it. I was working on bug 
1464428, would you be interested in bug 1464433?

It's a pre-requisite to optimize getElementById in shadow trees. Afterwards you can work on optimizing that if you want, shouldn't be too hard :)
Flags: needinfo?(emilio)

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc4ca25a0f39
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.