Remove the class attribute preparsing code.

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: emilio, Assigned: garvitdelhi, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments)

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

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(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.

Flags: needinfo?(emilio)

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

Flags: needinfo?(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 :)

Flags: needinfo?(emilio)

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

Flags: needinfo?(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!

Flags: needinfo?(emilio)

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

Flags: needinfo?(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!

Flags: needinfo?(emilio)

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

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.

Priority: -- → P2

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.

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

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

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

Attachment #9037809 - Attachment description: Bug 1519185: Deprecate AttributeWillChange aNewValue parameter → Bug 1519185: Remove AttributeWillChange aNewValue parameter

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

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

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

Pushed by bzbarsky@mozilla.com:
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

Garvit, thank you for the fixes!

Assignee: nobody → garvitdelhi
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/074adc46f394
Backed out 2 changesets build bustages on nsMenuGroupOwnerX.mm. CLOSED TREE
Pushed by emilio@crisal.io:
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

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

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

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

Flags: needinfo?(garvitdelhi)

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

Flags: needinfo?(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.

Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.