1519185 - Remove the class attribute preparsing code.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Emilio Cobos Álvarez [:emilio]]

It's not used for stylo, we should move it to ParseAttribute instead, and remove the aNewValue attribute for AttributeWillChange, which is unused.

Relevant code is:

https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/dom/base/Element.cpp#2383

Comment 1

[Marty Eugen]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

It's not used for stylo, we should move it to ParseAttribute instead, and remove the aNewValue attribute for AttributeWillChange, which is unused.

Relevant code is:

https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/dom/base/Element.cpp#2383

Hello! I am looking for a first bug, this one doesn't seem to be occupied. May I take it?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Please go ahead! Let me know if you have any questions :)

Comment 3

[Harsh Soni]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

It's not used for stylo, we should move it to ParseAttribute instead, and remove the aNewValue attribute for AttributeWillChange, which is unused.

Relevant code is:

https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/dom/base/Element.cpp#2383

Hi Sir,

I'm very new to open source contributions and am trying to start with this bug. Can you please guide me through it and help me understand it.

Thank you.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Looks like Marty was also looking at it, but you're welcome to take a look as well I suppose.

In any case, the code in the link should be removed, and then the last argument passed to AttributeWillChange should be changed to be nullptr.

Once that's done and it compiles, that's a good first commit. The rest of the work would be to go through all places that call AttributeWillChange and remove the last argument, which should be unused.

Comment 5

[Harsh Soni]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Looks like Marty was also looking at it, but you're welcome to take a look as well I suppose.

In any case, the code in the link should be removed, and then the last argument passed to AttributeWillChange should be changed to be nullptr.

Once that's done and it compiles, that's a good first commit. The rest of the work would be to go through all places that call AttributeWillChange and remove the last argument, which should be unused.

Thanks for the reply.

I'm very new to the concept of open source contribution, so, if you don't find it annoying, I would like to ask where can I clone the code from and then compile and fix the bug(no problem if it is already fixed, I just want to get my hands on it, and start learning).

Thank you.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

See http://developer.mozilla.org/en/Introduction. There's also the #introduction channel on irc.mozilla.org where people are more than keen to ask questions you may have :)

Comment 7

[Marty Eugen]

I am going to leave this bug to Harsh and find a new one. Because I am probably not going to be very fast, I want to find a bug no one wants to fix. Maybe the old one. Any suggestions?

I have successfully built, btw. Your bootstraping script rocks :)

Comment 8

[Emilio Cobos Álvarez [:emilio]]

I don't know any off-hand, there could be some in https://codetribute.mozilla.org/projects/webplatform?tag%3Dgood-first-bug.

In any case this bug is not urgent, so even if you're not the fastest you're welcome to work on it!

Comment 9

[Marty Eugen]

What should I do to get this issue assigned to me? Or do I just start working?

Comment 10

[Emilio Cobos Álvarez [:emilio]]

We tend to assign the bug to someone once they submit their first patch. I'm not quite sure why tbf, I guess it minimizes the risk of it getting dropped on the floor.

I'll ask just in case I can assign it to you directly, but yeah, please feel free to work on it!

Comment 11

[Marty Eugen]

I get it, thanks for your reply.
May I email you if I have any issues?

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Sure, either email me, ask in the bug, or ask on IRC (I'm emilio there as well). I hang out on #developers, #introduction and a bunch of other channels.

Comment 13

[chandanaakriti9]

hi, i am really interested in solving this as my first bug in mozilla organisation, if you don't mind then can you please elaborate what changes exactly you want with necessary links so that it will be easy for me to solve in quick time. thanks.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

(In reply to chandanaakriti9 from comment #13)

hi, i am really interested in solving this as my first bug in mozilla organisation, if you don't mind then can you please elaborate what changes exactly you want with necessary links so that it will be easy for me to solve in quick time. thanks.

You need to remove the if / else block in:

https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/dom/base/Element.cpp#2381

And move the ParseAtomArray stuff here:

https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/dom/base/Element.cpp#2606

So that it looks like:

if (aAttribute == nsGkAtoms::_class) {
  aResult.ParseAtomArray(aValue);
}

Removing the MOZ_ASSERT.

That'd be enough for the first patch, then the last argument to AttributeWillChange can go away, since it will always be null, but that's another matter.

Comment 15

[Garvit Khatri [:garvitdelhi]]

Hi Emilio, I have made the changes on the my local system. Can you tell me which test should i run to make sure nothing is breaking?

Also, this looks more like an aesthetics change than a bug fix, can you help me understand the reason behind the change(I am contributing to Firefox for the first time, pardon my lack of knowledge).

Comment 16

[Emilio Cobos Álvarez [:emilio]]

(In reply to Garvit Khatri from comment #15)

Hi Emilio, I have made the changes on the my local system. Can you tell me which test should i run to make sure nothing is breaking?

Just starting up the browser should work. If something broke all the UI would look messed up. I'll push your patch to automation to ensure it passes all the automated tests.

Also, this looks more like an aesthetics change than a bug fix, can you help me understand the reason behind the change(I am contributing to Firefox for the first time, pardon my lack of knowledge).

Well, yes, it's not a correctness fix, it's a refactoring to remove useless code and make code more consistent. So the change itself should not change behavior, but it should make the code nicer :).

Comment 17

[Garvit Khatri [:garvitdelhi]]

Attachment: Bug 1519185: Remove the class attribute preparsing code.

Refactoring code base

Comment 18

[Garvit Khatri [:garvitdelhi]]

Attachment: Bug 1519185: Remove AttributeWillChange aNewValue parameter

Comment 19

[Garvit Khatri [:garvitdelhi]]

@emilio can we move this for merging? @bzbarsky has approved both the patches.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Note that the approval on https://phabricator.services.mozilla.com//D17067 was conditional on the comment fix I asked for there.

Comment 21

[Garvit Khatri [:garvitdelhi]]

@boris I missed that, I will remove that and update the diff.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00d8afb01890
Remove the class attribute preparsing code. r=emilio,bzbarsky
https://hg.mozilla.org/integration/autoland/rev/e1de5282e21a
Remove AttributeWillChange aNewValue parameter r=emilio,bzbarsky

Comment 23

[Boris Zbarsky [:bzbarsky]]

Garvit, thank you for the fixes!

Comment 24

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/074adc46f394
Backed out 2 changesets build bustages on nsMenuGroupOwnerX.mm. CLOSED TREE

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b986e5c1f8c8
Remove the class attribute preparsing code. r=emilio,bzbarsky
https://hg.mozilla.org/integration/autoland/rev/c5974fba1238
Remove AttributeWillChange aNewValue parameter r=emilio,bzbarsky

Comment 26

[Boris Zbarsky [:bzbarsky]]

Garvit, it looks like the second changeset missed an AttributeWillChange implementation. Do you mind updating it to fix that?

You may need to update both Phabricator revisions to get it to behave sanely.... See the last FAQ item on https://wiki.mozilla.org/Phabricator/FAQ#Lando_says_.22This_diff_does_not_have_the_proper_author_information_uploaded_to_Phabricator.22.2C_but_I_see_an_author_on_Phabricator._What.27s_wrong.3F

Comment 27

[Garvit Khatri [:garvitdelhi]]

@Boris Sure I will look into it, test it locally once again and fix author info too by tomorrow.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Also, do you have try access? I expect the reason your local tests missed this file is that it's only compiled on MacOS...

Comment 29

[Boris Zbarsky [:bzbarsky]]

Oh, nevermind. Looks like Emilio relanded things with the fixes they needed...

Comment 30

[Garvit Khatri [:garvitdelhi]]

Yes, I compiled it only on MacOs. I don't think I have "try access" unless it is something which is available to all new contributors.

Comment 31

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b986e5c1f8c8
https://hg.mozilla.org/mozilla-central/rev/c5974fba1238