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

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[priyansu pulak]

someone help please...
as i have just started exploring open source, i couldn't understand the system here.

Comment 2

[Eric Rahm [:erahm]]

(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

[manishjoshi394]

(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?

Comment 4

[Eric Rahm [:erahm]]

(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

Comment 5

[rahul0379]

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.

Comment 6

[rahul0379]

Do we have to build Firefox on our machines before working on this? Is Visual Studio 15.7 still not supported?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(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

[Mridul Agarwal]

Hello,
I too am a newbie. Can I start working on this if it is still available?

Comment 9

[rahul0379]

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?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(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 :)

Comment 11

[rahul0379]

Attachment: bug1458580.patch - contains patch for this bug. renaming the GetSMILOverrideStyle calls and declarations

Hi, please find attached the patch file I created for this bug. Please review.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

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);

Comment 13

[rahul0379]

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?

Comment 14

[rahul0379]

Attachment: bug1458580.patch - contains patch for this bug. renaming the GetSMILOverrideStyle calls and declarations

Updated the patch. Please review.

Comment 15

[rahul0379]

Attachment: bug1458580.patch - contains patch for this bug. renaming the GetSMILOverrideStyle calls and declarations

Please review.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 17

[Eric Rahm [:erahm]]

Emilio, do you want to land this?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4ca25a0f39
Rename GetSMILStyleOverride since it can't return null. r=emilio

Comment 19

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 20

[rahul0379]

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.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

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 :)

Comment 22

[Cristina Coroiu [:ccoroiu]]

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