Closed Bug 1244328 Opened 4 years ago Closed 4 years ago

Merge DOMTokenList and DOMSettableTokenList

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: zcorpan, Assigned: venkid, Mentored)

Details

(Keywords: dev-doc-needed, Whiteboard: [good first bug])

Attachments

(1 file, 3 obsolete files)

Happy to mentor someone who wants to do this.
Mentor: bzbarsky
Whiteboard: [good first bug]
But if this is somewhat urgent to do, please let me know!
Hello,

I am interested in taking this up.Should I clone whatwg/dom or whatwg.html repository?

Please tell me name of the file I should be working on.
You'd need to clone the Firefox repository.  That is, you would be writing C++ code, not specification prose.

If that's what you were expecting, please let me know and I'll point you in the right direction.
I have cloned and built mozilla- central repository. Please tell me the exact file I should be working on
You'll need to move the one thing in the DOMSettableTokenList interface from dom/webidl/DOMSettableTokenList.webidl to dom/webidl/DOMTokenList.webidl.

You'll want to move the corresponding implementation from dom/base/DOMSettableTokenList.h and dom/base/DOMSettableTokenList.cpp to dom/base/DOMTokenList.h and dom/base/DOMTokenList.cpp.

You'll need to change anything that currently creates DOMSettableTokenList or inherits from it to do that with DOMTokenList instead.  https://dxr.mozilla.org/mozilla-central/search?q=DOMSettableTokenList&redirect=true&case=false should give you a list of the relevant files.  You should ignore the ones under testing/web-platform, presumably, though also presumably the test metadata will need to be updated at some point here.
Should also add [PutForwards=value] in .webidl files anywhere it was using DOMTokenList, to match the spec (Element#classList, HTMLLinkElement#relList, etc.)

FYI, the PR to web-platform-tests has been merged now.
(In reply to Boris Zbarsky [:bz] from comment #6)

 
> You'll want to move the corresponding implementation from
> dom/base/DOMSettableTokenList.h and dom/base/DOMSettableTokenList.cpp to
> dom/base/DOMTokenList.h and dom/base/DOMTokenList.cpp.


Do you mean DOMTokenListBinding.cpp and DOMTokenListBinding.h or nsDOMTokenList.h and nsDOMTokenList.cpp?
I can find only these two set of files in dom/base.Have attached screenshots of the same.
Attached image Dom/Base (obsolete) —
> Do you mean DOMTokenListBinding.cpp and DOMTokenListBinding.h or nsDOMTokenList.h and nsDOMTokenList.cpp?

I meant the files in dom/base.  Looks like nsDOMTokenList.h/cpp, indeed; I thought we had dropped the "ns" prefix from them....
Attached patch MergeDOMTokenList.patch (obsolete) — Splinter Review
Made all the changes mentioned. Please review the patch and tell me the further steps.
Attachment #8715263 - Attachment is obsolete: true
Comment on attachment 8716318 [details] [diff] [review]
MergeDOMTokenList.patch

For future reference, you probably want to request review via the "review" flag on the patch.  You should be able to see it by clicking the "Details" link next to the patch in the bug.  Set the dropdown to "?" and the textfield to the address of the person you want review from.

>Bug 1244328 - Moving the implementation in >DOMSettableTokenList.webidl,DOMSettableTokenList.h,DOMSettableTokenList.cpp to >DOMTokenList.webidl,DOMTokenList.h,DOMTokenList.cpp. Also edited all the files that refer to >DOMSettableTokenList.

I think a reasonable commit message for this bug is:

  Bug 1244328 - Merge the functionality of DOMSettableTokenList into DOMTokenList and make everything that used to refer to DOMSettableTokenList refer to DOMTokenList instead.

>+++ b/dom/base/Element.cpp
>+nsDOMTokenListPropertyDestructor(void *aObject, nsIAtom *aProperty,
>                                          void *aPropertyValue, void *aData)

Please fix the indentation of the second line here so it remains lined up with the previous line.

>+++ b/dom/base/nsDOMTokenList.cpp
>+#include "mozilla/dom/DOMSettableTokenListBinding.h"

There should be no such file after this patch.  So no need to include it.

>+nsDOMTokenList::WrapObject(JSContext *cx, JS::Handle<JSObject*> aGivenProto)

There's an existing nsDOMTokenList::WrapObject, so there is no need to add a second one.  Please take this part out.

Note that this also causes this patch to not compile...

>+++ b/dom/base/nsDOMTokenList.h
>+    // WebIDL
>+  void GetValue(nsAString& aResult) { Stringify(aResult); }
>+  void SetValue(const nsAString& aValue, mozilla::ErrorResult& rv);

This should probably go next to the existing WebIDL methods in this header.  Probably right before the Stringify declaration, since that's where you added it to the IDL.

>+++ b/dom/base/test/test_classList.html
>   ok(DOMTokenList.prototype.hasOwnProperty("toString"),
>      "Should have own toString on DOMTokenList")
>+  ok(!DOMTokenList.prototype.hasOwnProperty("toString"),
>+     "Should not have own toString on DOMTokenList")

This test can't pass, since it's asserting mutually contradictory things, right?

Just take out the second assertion.  The first one (the one that was already there) is correct.

>+++ b/dom/webidl/Element.webidl
>+  [Constant][PutForwards=value]

This isn't valid IDL syntax.  Again, actually doing a compile with the patch would have caught it.

What you want is:

  [Constant, PutForwards=value]

This patch doesn't seem to remove DOMSettableTokenList.webidl (e.g. using "hg rm").  It should do that.  Same for nsDOMSettableTokenList.h and nsDOMSettableTokenList.cpp.

>+++ b/testing/web-platform/tests/dom/interfaces.html
>-  [SameObject] readonly attribute DOMTokenList classList;
>+  [SameObject] [PutForwards=value] readonly attribute DOMTokenList classList;

Again, that's not valid IDL syntax.  You want:

  [SameObject, PutForwards=value] readonly attribute DOMTokenList classList;

>+interface DOMTokenList : DOMTokenList {

This doesn't make sense, right?  Please just remove this interface block altogether.

>+++ b/testing/web-platform/tests/html/dom/interfaces.html
>+  [SameObject][PutForwards=value] readonly attribute DOMTokenList classList;

Again,

  [SameObject, PutForwards=value] readonly attribute DOMTokenList classList;

> interface DOMTokenList {
>   readonly attribute unsigned long length;
>+  attribute DOMString value;

Please put that after toggle(), right before "stringifier".

r=me with the above issues addressed.

As far as next steps, you update the patch, ideally make sure it builds locally, and upload the updated patch to the bug.  Then I push it to the try server to make sure tests pass with the patch.  If they do, I check it in for you.  If they don't, then the patch needs updating as needed.  Make sense?
Attachment #8716318 - Flags: review+
Oh, and just to be clear: this is awesome work; the above issues are just some minor nits.  Thank you for doing this!
Thank you so much for guiding me through this.I will submit the updated patch here tomorrow.
Attached patch MergeDOMTokenListV2.patch (obsolete) — Splinter Review
Attachment #8716879 - Flags: review?(bzbarsky)
I have made an attachment MergeDOMTokenListV2.patch.
I have a problem here:
I generated MergeDOMTokenList.patch(the previous attachment).
hg diff / hg status did not return anything after that.
Now I made all the changes you suggested in MergeDOMTokenListV2.patch.
After few futile attempts to club the changes (hg qpush MergeDOMTokenList.patch and hg qfold MergeDOMTokenListV2.patch), I have just attached the second patch here.

If you tell me there is no other option to combine these changes, I will undo all my changes, redo it and submit a single patch.thank you for the patience
I have also compiled after the second patch.There are no errors.
> If you tell me there is no other option to combine these changes

It really depends on how you generated the patches.  The simplest thing would be for you to connect via IRC to irc.mozilla.org, the #developers channel, and ask for help, because what needs to be done depends on how you actually set things up in your tree.

That said, if that's not an option...  Are you actually using mq?  If so, what does "hg qseries" show?
Comment on attachment 8716879 [details] [diff] [review]
MergeDOMTokenListV2.patch

>+++ b/dom/base/Element.cpp

> static void
>+    nsDOMTokenListPropertyDestructor(void *aObject, nsIAtom *aProperty,
>                                          void *aPropertyValue, void *aData)

No, this is still not indented correctly.  It should look like this:

static void
nsDOMTokenListPropertyDestructor(void *aObject, nsIAtom *aProperty,
                                 void *aPropertyValue, void *aData)

>+++ b/dom/base/nsDOMTokenList.h
>+    // WebIDL

Just take this comment out, please.  Having it somewhere in the middle of the webidl implementation for this class is weird.

r=me with those two nits addressed.
Attachment #8716879 - Flags: review?(bzbarsky) → review+
Since both those changes should not affect compilation or tests passing, pushed the patches so far in this bug to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b27c13aac3a8
OK, try run shows failures in the dom/base/test/test_classList.html test: this test has a bit like this:

97 function assignToClassListStrict(e) {
98   "use strict";
99   try {
100     e.classList = "foo";
101     ok(false, "assigning to classList didn't throw");
102   } catch (e) { }

but of course in the new world this will in fact not throw anymore.  

there's also an assignToClassList test which does the same in a non-strict-mode function.  These two tests should be updated to assert not throwing, and at the end of each function we should probably set the "class" attribute to "" to preserve the old behavior.

Note that this patch already changed this test; it would be good to just run changed tests locally to ensure they actually pass, as a general rule.

I think that's the only real failure in this run, though results aren't all in yet.
dom/imptests/html/dom/test_interfaces.html is also failing.
Just wanted to make sure you're not blocked on me.  Let me know if you are?
Flags: needinfo?(deepthivenkitaramanan)
I can submit the 2 latest changes requested in the third patch right away.
I did not understand what I should do about the tests that are failing. Should I remove them?
I waited hoping that I would find answers elsewhere. I have submitted 2 patches for a 'good first bug' which is not a good practice.
Flags: needinfo?(deepthivenkitaramanan)
> I did not understand what I should do about the tests that are failing. Should I remove them?

No, you need to fix them (or fix your changes to them, if those are the problem).

For test_classList.html I said in comment 21 how to fix it; let me know if that doesn't make sense.  For test_interfaces.html, you should run it locally (using "mach mochitest dom/imptests/html/dom/test_interfaces.html") and see if you can figure out what the problem is.  If that's not working out, let me know and I'll look.

Have you had any luck folding your patches together?  Again, if not, please let me know.  If it comes to it, I can just fold them locally and post the folded patch in the bug...
I am making all the changes.Please give me some time.Will submit all the folded patch soon.
Just to confirm that I am making the right changes for the test mentioned in comment 21 : 
I will now make the test fail inside the catch block.Am I right?
> Please give me some time.

Sure thing.  I have no problem with waiting for you; I just don't want you to be blocked waiting on _me_.  ;)

> I will now make the test fail inside the catch block.Am I right?

Yes.  Now the catch block needs to ok(false), the try block needs to ok(true).  And as I said at the end of the function we should probably e.removeAttribute("class").
I now have the folded patch. When I ran mach mochitest dom/imptests/html/dom/test_interfaces.html I am getting the following : 

No definition for packages found for
(The error message does not seem to be complete)

I could find only one possible issue : 
There were two interface implemented here like this : 
interface DOMTokenList {
  readonly attribute unsigned long length;
  getter DOMString? item(unsigned long index);
  boolean contains(DOMString token);
  void add(DOMString... tokens);
  void remove(DOMString... tokens);
  boolean toggle(DOMString token, optional boolean force);
  stringifier;
};

interface DOMTokenList : DOMTokenList {
            attribute DOMString value;
};

So I removed the second implementation and added the line:

attribute DOMString value; to the first implementation after stringifier.

I now ran mach mochitest again on this file.But I am getting the same error.

All other changes are done and a single patch is ready.Please tell me what to do in dom/imptests/html/dom/test_interfaces.html.
> When I ran mach mochitest dom/imptests/html/dom/test_interfaces.html I am getting the following

Did you compile the whole tree first?
I built the project again using ./mach build. The build was successful with thousands of warning messages
Anyway, if I apply the first two patches in this bug and run "mach mochitest dom/imptests/html/dom/test_interfaces.html" locally, the failure I get is:

> Harness status: Error
> Duplicate identifier DOMTokenList

If I then make the changes from comment 28 (remove the bogus interface declaration, add the "attribute DOMString value;") then the test passes for me.  So please make those changes and attach the patch.
This is the final folded patch containing all the above changes.

Please check and tell me if its ok.
Attachment #8716318 - Attachment is obsolete: true
Attachment #8716879 - Attachment is obsolete: true
Attachment #8718360 - Flags: review?(bzbarsky)
That still has the nsDOMTokenListPropertyDestructor bits mis-indented...

The rest looks great.  I pushed it to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05347f4e90db
Comment on attachment 8718360 [details] [diff] [review]
Bug1244328 - MergeDOMTokenList

r=me with the indent fixed
Attachment #8718360 - Flags: review?(bzbarsky) → review+
Try is green, tree is open, so I just fixed the indent locally.

Thank you for fixing this!
Thanks for guiding me! Can you please assign the bug to me and change the status to resolved?
(In reply to Deepthi Venkitaramanan[:venkid] from comment #37)
> Thanks for guiding me! Can you please assign the bug to me and change the
> status to resolved?

That will all be done automatically once the patch is merged over from mozilla-inbound to mozilla-central. It should happen within the next day.
Er, I should have assigned this earlier; I thought I had!

The resolution, as Andrew said, will happen once inbound merges to mozilla-central.
Assignee: nobody → deepthivenkitaramanan
Thanks Boris! This is my first bug.Learnt quite a few things :)
https://hg.mozilla.org/mozilla-central/rev/aac070f65cc7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Keywords: dev-doc-needed
OS: Unspecified → All
Hardware: Unspecified → All
You need to log in before you can comment on or make changes to this bug.