1244328 - Merge DOMTokenList and DOMSettableTokenList

Status: RESOLVED FIXED

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

Description

[Simon Pieters [:zcorpan]]

Implement this spec change:

https://github.com/whatwg/dom/pull/120
https://github.com/whatwg/html/issues/361

Tests:

https://github.com/w3c/web-platform-tests/pull/2522

Comment 1

[Boris Zbarsky [:bzbarsky]]

Happy to mentor someone who wants to do this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

But if this is somewhat urgent to do, please let me know!

Comment 3

[Deepthi Venkitaramanan[:venkid]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Deepthi Venkitaramanan[:venkid]]

I have cloned and built mozilla- central repository. Please tell me the exact file I should be working on

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Simon Pieters [:zcorpan]]

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.

Comment 8

[Deepthi Venkitaramanan[:venkid]]

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

Comment 9

[Deepthi Venkitaramanan[:venkid]]

Attachment: Dom/Base

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Deepthi Venkitaramanan[:venkid]]

Attachment: MergeDOMTokenList.patch

Made all the changes mentioned. Please review the patch and tell me the further steps.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and just to be clear: this is awesome work; the above issues are just some minor nits.  Thank you for doing this!

Comment 14

[Deepthi Venkitaramanan[:venkid]]

Thank you so much for guiding me through this.I will submit the updated patch here tomorrow.

Comment 15

[Deepthi Venkitaramanan[:venkid]]

Attachment: MergeDOMTokenListV2.patch

Comment 16

[Deepthi Venkitaramanan[:venkid]]

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

Comment 17

[Deepthi Venkitaramanan[:venkid]]

I have also compiled after the second patch.There are no errors.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> 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 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

dom/imptests/html/dom/test_interfaces.html is also failing.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Just wanted to make sure you're not blocked on me.  Let me know if you are?

Comment 24

[Deepthi Venkitaramanan[:venkid]]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Deepthi Venkitaramanan[:venkid]]

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?

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Deepthi Venkitaramanan[:venkid]]

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> When I ran mach mochitest dom/imptests/html/dom/test_interfaces.html I am getting the following

Did you compile the whole tree first?

Comment 30

[Deepthi Venkitaramanan[:venkid]]

I built the project again using ./mach build. The build was successful with thousands of warning messages

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Deepthi Venkitaramanan[:venkid]]

Attachment: Bug1244328 - MergeDOMTokenList

This is the final folded patch containing all the above changes.

Please check and tell me if its ok.

Comment 33

[Boris Zbarsky [: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 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718360 [details] [diff] [review]
Bug1244328 - MergeDOMTokenList

r=me with the indent fixed

Comment 35

[Boris Zbarsky [:bzbarsky]]

Try is green, tree is open, so I just fixed the indent locally.

Thank you for fixing this!

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aac070f65cc7

Comment 37

[Deepthi Venkitaramanan[:venkid]]

Thanks for guiding me! Can you please assign the bug to me and change the status to resolved?

Comment 38

[Andrew McCreight [:mccr8]]

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

Er, I should have assigned this earlier; I thought I had!

The resolution, as Andrew said, will happen once inbound merges to mozilla-central.

Comment 40

[Deepthi Venkitaramanan[:venkid]]

Thanks Boris! This is my first bug.Learnt quite a few things :)

Comment 41

[Carsten Book [:Tomcat]]

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