See bug 1007878 comment 16.

Hi,I would like to work on this. However since i am a newbie ,i am not understanding how to pull the patch into my repository.

Varun, pull which patch? If you mean the one from bug 1007878, you can do that now by just updating the tree, since it's been merged to mozilla-central now.

I would like to start working on this as my first bug.

Sounds good. Please feel free to mail me if you have any questions!

I'm the same person as jakehamo.gmail.com

That last patch was bad.

Comment on attachment 8624001 [details] [diff] [review] bug1015318.patch Sorry for the lag; I missed the bugmail and there was no review request. This patch seems to have unrelated changes to Link.cpp and whatnot, right?

I've been having problems with hg, sorry. This patch should be right.

Comment on attachment 8625108 [details] [diff] [review] bug1015318.patch No, you want to request review from someone, not set the review flag to '+' yourself. Will try to look at this tonight or tomorrow.

Comment on attachment 8625108 [details] [diff] [review] bug1015318.patch >+class IDLBaseType(IDLObject): Seems to me this should be called something more like IDLParametrizedType. And it should inherit from IDLType, and IDLNullableType and company should inherit just from it instead of the multiple inheritance setup here. >+ def __init__(self, location, parameterType): >+ self.name = name There is no "name" in scope here. Have you actually tested that this patch works? In any case, once you change the inheritance as I suggest above you won't need to assign self.name here at all; that will be handled by IDLType. Though maybe you want to hoist the bits that handle self.innner.isComplete() being true in __init__ up here. Not clear. That can be a followup, in any case. >+ self.builtin = False This, you do want here. But then it can be removed from the subclasses, right? >+ def __eq__(self, other): >+ return other and self.builtin == other.builtin and self.name == other.name So I'm not sure this makes sense, now that I look at it. The __eq__ impls for the things we care about check isinstance; I'm not sure we want an __eq__ at all on IDLParametrizedType. >+ def __ne__(self, other): Don't need this once we inherit from IDLType. >+ def tag(self): >+ return IDLType.Tags.sequence No, this is clearly wrong. The types involved all have different tags. >+ def complete(self, scope): >+ self.inner = self.inner.complete(scope) >+ self.name = self.inner.name + "Sequence" This produces the wrong self.name. It also loses the validity checks IDLNullableType.complete has. Maybe the right approach is to pass the "append this to the name" string ("OrNull", "Sequence", "MozMap") up to the superclass, use that here, and keep an override in IDLNullableType that calls up to IDLParametrizedType and then does its checks. >+ def isDistinguishableFrom(self, other): I thought we'd decided to not hoist this one. It's different across these different types... Please fix those issues and request review on the updated patch? Sorry this took so long; the mail about the attachment wasn't in the high priority "review request" mail folder. :(

OK I made fixes.

Comment on attachment 8626971 [details] [diff] [review] bug1015318.patch >+ def _getDependentObjects(self): >+ return self.inner._getDependentObjects() >+ Please remove the trailing whitespace on that last line and various other places. >@@ -1940,28 +1957,19 @@ class IDLNullableType(IDLType): >- def isSharedArrayBuffer(self): >- return self.inner.isSharedArrayBuffer() >- >- def isSharedArrayBufferView(self): >- return self.inner.isSharedArrayBufferView() ... >- def isSharedTypedArray(self): >- return self.inner.isSharedTypedArray() Why, exactly, are these removals OK? How did you test this patch, if at all? Please at least run "mach webidl-parser-test", or better yet compile the source tree. This patch fails both of those, starting with the fact that its name handling is broken. The isExposedInAllOf changes here look wrong to me too; that function should go on IDLParametrizedType. Its current lack on IDLSequenceType is a bug.

Jacob, are you still aiming to finish this patch up?

I'm looking to start this up again. Is it still available?

No one is working on this that I know of.

(In reply to Boris Zbarsky [:bz] from comment #14) > Created attachment 8696878 [details] [diff] [review] > Last patch merged to tip what exactly are you doing or do you mean by this?

I meant that attachment 8696878 [details] [diff] [review] is the same as attachment 8626971 [details] [diff] [review] but updated so it applies to the tree as of when I posted it. At that point attachment 8626971 [details] [diff] [review] no longer applied cleanly.

Review commit: https://reviewboard.mozilla.org/r/50995/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50995/

Review commit: https://reviewboard.mozilla.org/r/50997/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50997/

I forget to test it! But it should be okay, though. I hope I did this new mercurial workflow thing right.

You need to actually request review from someone... Note that if it's me I won't be able to get to it until Monday.

I tried to request a review from you but it said you weren't accepting requests right now. I can wait until Monday for you. If you want someone else to do it, I'm not sure who else I should ask?

I should be able to look at this tomorrow.

https://reviewboard.mozilla.org/r/50995/#review48497 ::: dom/bindings/parser/WebIDL.py:2057 (Diff revision 1) > "distinguishable from other things") > > > -class IDLNullableType(IDLType): > +class IDLParameterizedType(IDLType): > + def __init__(self, location, parameterType): > + IDLObject.__init__(self, location) You need to init IDLType here. Which, by the way, means you need to be passing `name` as the third argument to `__init__`, not `parameterType` (which is unused!). Certainly the subclasses pass a name as the third arg. ::: dom/bindings/parser/WebIDL.py:2204 (Diff revision 1) > > - def _getDependentObjects(self): > - return self.inner._getDependentObjects() > - > > class IDLSequenceType(IDLType): This needs to inherit from IDLParametrizedType, no? The fact that it does not, and hence `unroll()` would probably fail, makes me wonder whether the patch was tested... ::: dom/bindings/parser/WebIDL.py:2208 (Diff revision 1) > > class IDLSequenceType(IDLType): > def __init__(self, location, parameterType): > assert not parameterType.isVoid() > > IDLType.__init__(self, location, parameterType.name) This needs to become `IDLParametrizedType.__init__`. _Did_ you test this, and do we simply not have something that catches the lack of setting `self.builtin` here? ::: dom/bindings/parser/WebIDL.py (Diff revision 1) > def complete(self, scope): > self.inner = self.inner.complete(scope) > self.name = self.inner.name + "MozMap" > return self > > - def unroll(self): You're changing the behavior of IDLMozMapType.unroll, no? We even had an explicit comment explaining why its behavior is what it is. Please do not change that. Also, is there a reason `self.inner = parameterType` didn't get moved up to the shared constructor?

Two things to clarify: "This needs to become `IDLParametrizedType.__init__`. _Did_ you test this, and do we simply not have something that catches the lack of setting `self.builtin` here?" self.builtin = False has been moved to IDLParameterizeType, so I don't need it in IDLSequenceType right? "Also, is there a reason `self.inner = parameterType` didn't get moved up to the shared constructor?" Well the reason is that IDLNullableType has "self.inner = innerType" instead. Is this equivalent? And don't worry I will test this time.

> self.builtin = False has been moved to IDLParameterizeType, so I don't need it in IDLSequenceType right? Right, if IDLSequenceType calls IDLParametrizedType.__init__. > Well the reason is that IDLNullableType has "self.inner = innerType" instead. Is this equivalent? Yes. Just pass "the type that should be our inner" in.

I tried to fix what you said but mach webidl-parser-test is giving me errors. I tried to fix them but then I got even more errors. I'm not sure what I'm doing with that last argument, name/parameterType. Should I change them all to one or what? This is the error the test was giving me: http://sprunge.us/OcDG

Can you see my newest patch? Where this page and the review board intersect if at all is confusing me.

> This is the error the test was giving me: > http://sprunge.us/OcDG This is the assert in the IDLNullableType constructor failing for some reason, yes? What did you change in that constructor. > I'm not sure what I'm doing with that last argument, name/parameterType You're having two arguments: one for the name, and one for the parameterType. > Can you see my newest patch? Nope. > Where this page and the review board intersect if at all is confusing me. I have no idea where they do, sorry. I avoid review board if at all possible...

So I tried to fix it. It doesn't like the "name" argument. Here is the error when I run the test: http://sprunge.us/BRZg I tried to fix your reviews and committed that patch. Then I tried to fix it again and committed another patch. So there are two patches since you last saw it. Here's a link to the latest one on review board: https://reviewboard.mozilla.org/r/52153/diff/1#index_header I don't see where you can upload .patch files anymore, or else I would just do that.

OK here is the latest one.

https://reviewboard.mozilla.org/r/52153/diff/1/#index_header says: You don't have access to this review request. This review request is private. You must be a requested reviewer, either directly or on a requested group, and have permission to access the repository in order to view this review request. The "Patch the fix the name argument" looks like it's on top of some other patches, and I'm not sure which ones. But it's clearly wrong, since it's doing things like: >+ IDLParameterizedType.__init__(self, location, name) in IDLMozMapType.__init__, where there is no "name" thing at all...

What I suggest is that you either push the whole thing to reviewboard and request review from me there, or attach a diff that shows all your changes to this bug directly.

I can't request a review from you from the review board. It says you aren't accepting reviews. There are two patches on this page now. In the first one I tried to fix your issues. In the second I try to fix the errors I'm getting from testing. Let me try to fix this tonight, and I will make a diff that shows all the changes from the beginning of the bug. So in my init method for IDLNUllableType, I now need to pass 4 arguments: >class IDLNullableType(IDLParameterizedType): > def __init__(self, location, innerType, name): But I factored out "self.inner = innerType" into "self.inner = name". How can those two things be equivalent if they are referring to different arguments? Should I have a fourth argument in IDLParamterized type (self, location, name, parameterType) and factor out self.inner = parameterType? I'm not sure what's going on in the initialization of IDLSequenceType: >class IDLSequenceType(IDLParameterizedType): > def __init__(self, location, parameterType): > assert not parameterType.isVoid() > IDLParameterizedType.__init__(self, location, parameterType.name) > self.inner = parameterType Why parameterType.name rather than just parameterType or just name? I'm not sure what the "." is doing to the argument.

> I can't request a review from you from the review board. Oh, right. That's because I'm supposed to be focusing on something else entirely this week... Just attach the patches to the bug, please. > But I factored out "self.inner = innerType" into "self.inner = name". No, that's wrong. You want to do "self.inner = innerType" and you want to pass the name argument up to the IDLType constructor eventually. > Should I have a fourth argument in IDLParamterized type Yes! > I'm not sure what the "." is doing to the argument. It's getting the "name" property from the "parameterType" object.

So IDLParameterizedType is going to have four arguments, >class IDLParameterizedType(IDLType): > def __init__(self, location, name, innerType): Then every other class that inherits it also has to initialize those four arguments? Like.. >class IDLNullableType(IDLParameterizedType): > def __init__(self, location, name, innerType): What about IDLSequenceType and IDLMozmapType which are already passing parameterType? Would they have to pass self, location, name, innertype, AND parametertype, or should I combine innertype and parametertype into one argument? >class IDLSequenceType(IDLParameterizedType): > def __init__(self, location, name, parameterType, innerType): > assert not parameterType.isVoid() > > IDLParameterizedType.__init__(self, location, parameterType.name, innerType)

Nevermind I figured it out! Posting a patch shortly.

OK I figured out the argument issue. It now passes mach webidl-parser-test.

> Then every other class that inherits it also has to initialize those four arguments? Yes, exactly. > or should I combine innertype and parametertype into one argument? innerType and parameterType are the same thing. There should be four arguments: self, location, name, innerType/parameterType. Looking at the patch now.

Comment on attachment 8752006 [details] [diff] [review] rb50993.patch >+ IDLObject.__init__(self, location) No, you don't need that line. IDLType will handle calling that for you. >+ # lets us add headers for both ourselves and our inner as You added a trailing whitespace on that line. Please remove it. r=me with those two comments addressed. Please attach the updated patch? Thank you!

I still can't r you.

> I still can't r you. Yes, I know.

That last patch looks good, but is missing your name and the commit message and so forth... Could you add those, please? In the meantime I pushed this to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=416409fa0387 so we can set checkin-needed once we have an attachment with the right metadata.

Do I automatically add those comments in the patch header or do I have to add them manually?

They should be there in your local commit, right? Certainly https://reviewboard.mozilla.org/r/50995/ has your name associated with it (see the "Author" bit on the right), and a commit message. Though the commit message should probably be a little more specific about which common code is being factored out... If you have a bunch of commits locally and just used diff -r rev1 -r rev2 to generate the attachment, you probably want to squash the local commits into a single changeset.

Ok all that information at the top has been added.

You need the bug number in the comment message as well. Something like: Bug 1015318. Factored out the common code from IDLNullableType, IDLSequenceType, and IDLMozMapType into a new superclass, IDLParameterizedType. r=bzbarsky

Jacob, thank you for sticking with this!