Closed
Bug 1015318
Opened 11 years ago
Closed 9 years ago
Factor out the common code from IDLSequenceType and IDLMozMapType (and maybe IDLNullableType) into a superclass
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jakehm, Mentored)
Details
(Whiteboard: [lang=python])
Attachments
(1 file, 12 obsolete files)
8.86 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•11 years ago
|
||
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.
Updated•10 years ago
|
Mentor: bzbarsky
Whiteboard: [mentor=bzbarsky@mit.edu][lang=python] → [lang=python]
Reporter | ||
Comment 4•9 years ago
|
||
Sounds good. Please feel free to mail me if you have any questions!
Assignee | ||
Comment 5•9 years ago
|
||
I'm the same person as jakehamo.gmail.com
Assignee | ||
Comment 6•9 years ago
|
||
That last patch was bad.
Reporter | ||
Comment 7•9 years ago
|
||
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?
Flags: needinfo?(jacob.harrowmortelliti)
Assignee | ||
Comment 8•9 years ago
|
||
I've been having problems with hg, sorry. This patch should be right.
Attachment #8623987 -
Attachment is obsolete: true
Attachment #8624001 -
Attachment is obsolete: true
Flags: needinfo?(jacob.harrowmortelliti)
Attachment #8625108 -
Flags: review+
Reporter | ||
Comment 9•9 years ago
|
||
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.
Attachment #8625108 -
Flags: review+ → review?(bzbarsky)
Reporter | ||
Comment 10•9 years ago
|
||
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. :(
Attachment #8625108 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 11•9 years ago
|
||
OK I made fixes.
Attachment #8625108 -
Attachment is obsolete: true
Attachment #8626971 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 12•9 years ago
|
||
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.
Attachment #8626971 -
Flags: review?(bzbarsky) → review-
Reporter | ||
Comment 13•9 years ago
|
||
Jacob, are you still aiming to finish this patch up?
Flags: needinfo?(jacob.harrowmortelliti)
Reporter | ||
Comment 14•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8626971 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
I'm looking to start this up again. Is it still available?
Reporter | ||
Comment 16•9 years ago
|
||
No one is working on this that I know of.
Assignee | ||
Comment 17•9 years ago
|
||
(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?
Reporter | ||
Comment 18•9 years ago
|
||
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.
Assignee | ||
Comment 19•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50995/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50995/
Assignee | ||
Comment 20•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50997/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50997/
Assignee | ||
Comment 21•9 years ago
|
||
I forget to test it! But it should be okay, though.
I hope I did this new mercurial workflow thing right.
Reporter | ||
Comment 22•9 years ago
|
||
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.
Assignee | ||
Comment 23•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jacob.harrowmortelliti) → needinfo?(bzbarsky)
Reporter | ||
Comment 24•9 years ago
|
||
I should be able to look at this tomorrow.
Reporter | ||
Comment 25•9 years ago
|
||
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?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 26•9 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 27•9 years ago
|
||
> 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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 28•9 years ago
|
||
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
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 29•9 years ago
|
||
Can you see my newest patch? Where this page and the review board intersect if at all is confusing me.
Reporter | ||
Comment 30•9 years ago
|
||
> 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...
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 31•9 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8749494 -
Attachment is obsolete: true
Attachment #8749495 -
Attachment is obsolete: true
Assignee | ||
Comment 33•9 years ago
|
||
OK here is the latest one.
Reporter | ||
Comment 34•9 years ago
|
||
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...
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 35•9 years ago
|
||
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.
Assignee | ||
Comment 36•9 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 37•9 years ago
|
||
> 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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 38•9 years ago
|
||
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)
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 39•9 years ago
|
||
Nevermind I figured it out! Posting a patch shortly.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 40•9 years ago
|
||
OK I figured out the argument issue. It now passes mach webidl-parser-test.
Attachment #8751678 -
Attachment is obsolete: true
Attachment #8751679 -
Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 41•9 years ago
|
||
> 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.
Reporter | ||
Comment 42•9 years ago
|
||
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!
Flags: needinfo?(bzbarsky)
Attachment #8752006 -
Flags: review+
Assignee | ||
Comment 43•9 years ago
|
||
I still can't r you.
Attachment #8752006 -
Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 45•9 years ago
|
||
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.
Assignee | ||
Comment 46•9 years ago
|
||
Do I automatically add those comments in the patch header or do I have to add them manually?
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 47•9 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 48•9 years ago
|
||
Ok all that information at the top has been added.
Attachment #8752148 -
Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8752584 -
Flags: review-
Assignee | ||
Updated•9 years ago
|
Attachment #8752584 -
Flags: review-
Reporter | ||
Updated•9 years ago
|
Attachment #8696878 -
Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 49•9 years ago
|
||
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
Flags: needinfo?(jacob.harrowmortelliti)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → jacob.harrowmortelliti
Assignee | ||
Comment 50•9 years ago
|
||
Attachment #8752584 -
Attachment is obsolete: true
Flags: needinfo?(jacob.harrowmortelliti) → needinfo?(bzbarsky)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bzbarsky)
Keywords: checkin-needed
Comment 51•9 years ago
|
||
Keywords: checkin-needed
Comment 52•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter | ||
Comment 53•9 years ago
|
||
Jacob, thank you for sticking with this!
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•