Last Comment Bug 741125 - Fix up Paris Bindings WebIDL parser
: Fix up Paris Bindings WebIDL parser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
: 742165 (view as bug list)
Depends on:
Blocks: 740069
  Show dependency treegraph
 
Reported: 2012-03-31 14:54 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-04-12 21:14 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Moving ply/ply/*.py to ply/ (for sane importing). (475 bytes, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Return a list in finish(), rather than a set. (773 bytes, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Add .gitignore (for .pyc). You'll probably want to delete this before committing to hg. (290 bytes, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Allow runtests.py to be run from other directories. (1.07 KB, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Let runtests.py take --quiet and a list of tests to run. (3.30 KB, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Remove list(set()) in finish() -- it breaks test_attrs. We now need a failing test for the lack of de-duplication afforded by this checkin. (863 bytes, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Adding test_deduplicate, which ensures that duplicate forward-declarations of an interface show up only once. (819 bytes, patch)
2012-03-31 15:03 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
In runtests.py, properly dump the traceback when an exception occurs. (2.08 KB, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
De-duplicate results in finish(), making test_deduplicate pass. (1010 bytes, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Remove webidlyacc.py -- it's a generated file! (46.09 KB, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Define Location._line as an instance variable, not a class variable. (956 bytes, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Ignore parser.out. (238 bytes, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Don't cut off the end of the lines without terminating '\n's in error messages, rewrite error message '^' placement code, and add a test. (2.13 KB, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Remove unnecessary assertion that list(foo) is iterable. (897 bytes, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Add enum.length, and use it. (1.64 KB, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Explicitly set self._finished = false, rather than relying on hasattr. (1.55 KB, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Don't need to do foo=None before setting foo in both branches of a branch. (830 bytes, patch)
2012-03-31 15:04 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Some review comments. (3.33 KB, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Make special method checking shorter. (5.55 KB, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Use more pythonic check for any constants in self.members. (856 bytes, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Don't pass a billion False's to IDLMethod constructor -- add default params instead. (3.10 KB, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
More review comments. (1.96 KB, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag (7.13 KB, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Adding three new tests. They don't all pass. (4.84 KB, patch)
2012-03-31 15:05 PDT, Justin Lebar (not reading bugmail)
khuey: review+
khuey: checkin+
Details | Diff | Splinter Review
Rollup patch (418.71 KB, patch)
2012-03-31 15:11 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Rollup patch (33.21 KB, patch)
2012-03-31 15:14 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch for m-c (43.17 KB, patch)
2012-04-04 09:38 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
ted: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-03-31 14:54:37 PDT
I've been reviewing and editing the Paris Bindings WebIDL parser, and Kyle and I can use this bug to track at least one iteration of fixing it up.
Comment 1 Justin Lebar (not reading bugmail) 2012-03-31 15:03:00 PDT
Kyle, I'm not entirely sure how we want to do this.  I have a bunch of patches, which I'm going to attach here.  I'll also attach a roll-up patch with all the changes.

Some of these patches are fixes, which you'll need to review.  Some of them are questions of mine for you -- these are marked with "REVIEW".

I also added some tests which currently fail.  In particular, test_array_of_interface asserts, and test_nullable_equivalency fails for the same reason.  There's a separate failure in test_nullable_equivalency, which you'll see after we fix the assertion.

I'm not done with my review, but we may want to use more than one bug for the review.  It seems to me that the easiest way to handle this would be to push the code to github, hack on it there, and eventually push back to m-c, but I'm not sure you'd like that.  :)
Comment 2 Justin Lebar (not reading bugmail) 2012-03-31 15:03:21 PDT
Created attachment 611194 [details] [diff] [review]
Moving ply/ply/*.py to ply/ (for sane importing).
Comment 3 Justin Lebar (not reading bugmail) 2012-03-31 15:03:26 PDT
Created attachment 611195 [details] [diff] [review]
Return a list in finish(), rather than a set.
Comment 4 Justin Lebar (not reading bugmail) 2012-03-31 15:03:32 PDT
Created attachment 611196 [details] [diff] [review]
Add .gitignore (for .pyc).  You'll probably want to delete this before committing to hg.
Comment 5 Justin Lebar (not reading bugmail) 2012-03-31 15:03:38 PDT
Created attachment 611197 [details] [diff] [review]
Allow runtests.py to be run from other directories.
Comment 6 Justin Lebar (not reading bugmail) 2012-03-31 15:03:44 PDT
Created attachment 611198 [details] [diff] [review]
Let runtests.py take --quiet and a list of tests to run.
Comment 7 Justin Lebar (not reading bugmail) 2012-03-31 15:03:51 PDT
Created attachment 611199 [details] [diff] [review]
Remove list(set()) in finish() -- it breaks test_attrs.  We now need a failing test for the lack of de-duplication afforded by this checkin.
Comment 8 Justin Lebar (not reading bugmail) 2012-03-31 15:03:57 PDT
Created attachment 611200 [details] [diff] [review]
Adding test_deduplicate, which ensures that duplicate forward-declarations of an interface show up only once.
Comment 9 Justin Lebar (not reading bugmail) 2012-03-31 15:04:02 PDT
Created attachment 611201 [details] [diff] [review]
In runtests.py, properly dump the traceback when an exception occurs.
Comment 10 Justin Lebar (not reading bugmail) 2012-03-31 15:04:08 PDT
Created attachment 611202 [details] [diff] [review]
De-duplicate results in finish(), making test_deduplicate pass.
Comment 11 Justin Lebar (not reading bugmail) 2012-03-31 15:04:15 PDT
Created attachment 611203 [details] [diff] [review]
Remove webidlyacc.py -- it's a generated file!
Comment 12 Justin Lebar (not reading bugmail) 2012-03-31 15:04:21 PDT
Created attachment 611204 [details] [diff] [review]
Define Location._line as an instance variable, not a class variable.
Comment 13 Justin Lebar (not reading bugmail) 2012-03-31 15:04:27 PDT
Created attachment 611205 [details] [diff] [review]
Ignore parser.out.
Comment 14 Justin Lebar (not reading bugmail) 2012-03-31 15:04:33 PDT
Created attachment 611206 [details] [diff] [review]
Don't cut off the end of the lines without terminating '\n's in error messages, rewrite error message '^' placement code, and add a test.
Comment 15 Justin Lebar (not reading bugmail) 2012-03-31 15:04:39 PDT
Created attachment 611207 [details] [diff] [review]
Remove unnecessary assertion that list(foo) is iterable.
Comment 16 Justin Lebar (not reading bugmail) 2012-03-31 15:04:45 PDT
Created attachment 611208 [details] [diff] [review]
Add enum.length, and use it.
Comment 17 Justin Lebar (not reading bugmail) 2012-03-31 15:04:52 PDT
Created attachment 611209 [details] [diff] [review]
Explicitly set self._finished = false, rather than relying on hasattr.
Comment 18 Justin Lebar (not reading bugmail) 2012-03-31 15:04:58 PDT
Created attachment 611210 [details] [diff] [review]
Don't need to do foo=None before setting foo in both branches of a branch.
Comment 19 Justin Lebar (not reading bugmail) 2012-03-31 15:05:04 PDT
Created attachment 611211 [details] [diff] [review]
Some review comments.
Comment 20 Justin Lebar (not reading bugmail) 2012-03-31 15:05:10 PDT
Created attachment 611212 [details] [diff] [review]
Make special method checking shorter.
Comment 21 Justin Lebar (not reading bugmail) 2012-03-31 15:05:17 PDT
Created attachment 611213 [details] [diff] [review]
Use more pythonic check for any constants in self.members.
Comment 22 Justin Lebar (not reading bugmail) 2012-03-31 15:05:24 PDT
Created attachment 611214 [details] [diff] [review]
Don't pass a billion False's to IDLMethod constructor -- add default params instead.
Comment 23 Justin Lebar (not reading bugmail) 2012-03-31 15:05:31 PDT
Created attachment 611215 [details] [diff] [review]
More review comments.
Comment 24 Justin Lebar (not reading bugmail) 2012-03-31 15:05:38 PDT
Created attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag
Comment 25 Justin Lebar (not reading bugmail) 2012-03-31 15:05:44 PDT
Created attachment 611217 [details] [diff] [review]
Adding three new tests.  They don't all pass.
Comment 26 Justin Lebar (not reading bugmail) 2012-03-31 15:11:55 PDT
Created attachment 611219 [details] [diff] [review]
Rollup patch
Comment 27 Justin Lebar (not reading bugmail) 2012-03-31 15:14:44 PDT
Created attachment 611220 [details] [diff] [review]
Rollup patch
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-31 16:49:27 PDT
I knew my parser code was less than great, but wow.
Comment 29 Justin Lebar (not reading bugmail) 2012-03-31 22:14:37 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> I knew my parser code was less than great, but wow.

Well, it might have been smaller as a set of review comments, I'm not sure.  I was hoping a patch queue would be easier overall, but I might have been wrong.  :)

The big thing is the edge cases where the parser falls over or does the wrong thing.  We're not out of the woods in that respect.
Comment 30 Justin Lebar (not reading bugmail) 2012-03-31 22:18:42 PDT
Comment on attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

This one is necessary to pass some of the tests I added.  There are cases when we'd try to read _file on a BuiltinLocation, which didn't work.

Although looking at this, it has a bug which I'm surprised isn't caught by any tests:

+    def filename(self):
+        return self.filename
+

should be

        return self._file.
Comment 31 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 08:59:42 PDT
Comment on attachment 611196 [details] [diff] [review]
Add .gitignore (for .pyc).  You'll probably want to delete this before committing to hg.

What about the various other things in .hgignore?
Comment 32 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 09:03:43 PDT
Comment on attachment 611202 [details] [diff] [review]
De-duplicate results in finish(), making test_deduplicate pass.

Review of attachment 611202 [details] [diff] [review]:
-----------------------------------------------------------------

::: WebIDL.py
@@ +2865,5 @@
> +        for p in self._productions:
> +            if p not in seen:
> +                seen.add(p)
> +                result.append(p)
> +        return result

Why do we need the set and the array here?  Isn't if p not in result: enough?
Comment 33 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 09:04:37 PDT
Comment on attachment 611204 [details] [diff] [review]
Define Location._line as an instance variable, not a class variable.

Review of attachment 611204 [details] [diff] [review]:
-----------------------------------------------------------------

Is _line actually used?
Comment 34 Justin Lebar (not reading bugmail) 2012-04-01 09:29:47 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> Comment on attachment 611202 [details] [diff] [review]
> De-duplicate results in finish(), making test_deduplicate pass.
> 
> Review of attachment 611202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: WebIDL.py
> @@ +2865,5 @@
> > +        for p in self._productions:
> > +            if p not in seen:
> > +                seen.add(p)
> > +                result.append(p)
> > +        return result
> 
> Why do we need the set and the array here?  Isn't if p not in result: enough?

Sure, but that's O(n^2) in the number of interfaces.  Which may be fine -- I dunno.
Comment 35 Justin Lebar (not reading bugmail) 2012-04-01 09:30:54 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #33)
> Comment on attachment 611204 [details] [diff] [review]
> Define Location._line as an instance variable, not a class variable.
> 
> Review of attachment 611204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is _line actually used?

Yes, in lots of places (e.g. Location.__str__).
Comment 36 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 09:31:48 PDT
(In reply to Justin Lebar [:jlebar] from comment #34)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> > Comment on attachment 611202 [details] [diff] [review]
> > De-duplicate results in finish(), making test_deduplicate pass.
> > 
> > Review of attachment 611202 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: WebIDL.py
> > @@ +2865,5 @@
> > > +        for p in self._productions:
> > > +            if p not in seen:
> > > +                seen.add(p)
> > > +                result.append(p)
> > > +        return result
> > 
> > Why do we need the set and the array here?  Isn't if p not in result: enough?
> 
> Sure, but that's O(n^2) in the number of interfaces.  Which may be fine -- I
> dunno.

Aha.  For some reason I only think about perf when writing C++ :-/
Comment 37 Justin Lebar (not reading bugmail) 2012-04-01 09:33:29 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #31)
> Comment on attachment 611196 [details] [diff] [review]
> Add .gitignore (for .pyc).  You'll probably want to delete this before
> committing to hg.
> 
> What about the various other things in .hgignore?

.hgignore will take care of .pyc.  The gitignore also has webidlyacc.py and parser.out, but I thought you were planning to move these to the objdir?
Comment 38 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 14:14:45 PDT
Comment on attachment 611198 [details] [diff] [review]
Let runtests.py take --quiet and a list of tests to run.

Review of attachment 611198 [details] [diff] [review]:
-----------------------------------------------------------------

::: runtests.py
@@ +102,5 @@
>  
>  if __name__ == '__main__':
> +    usage = """%prog [OPTIONS] [TESTS]
> +               Where TESTS are relative to the tests directory."""
> +    parser = optparse.OptionParser(usage='%prog [OPTIONS] [TESTS]')

Why have a usage string and then not use it?
Comment 39 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 14:15:55 PDT
Comment on attachment 611211 [details] [diff] [review]
Some review comments.

Review of attachment 611211 [details] [diff] [review]:
-----------------------------------------------------------------

::: WebIDL.py
@@ +196,5 @@
>              return self._name.QName() + "::"
>          return "::"
>  
>      def ensureUnique(self, identifier, object):
> +        # REVIEW: This is tricky; please add a docstring.

What part exactly do you find tricky about this?

@@ +289,5 @@
>          return "<unresolved scope>::" + self.name
>  
>      def resolve(self, scope, object):
> +        # REVIEW: It would be good to explain the difference between resolve
> +        # and finish, in general.

Agreed.

@@ +378,5 @@
>      def __str__(self):
>          return "Interface '%s'" % self.identifier.name
>  
>      def ctor(self):
> +        # REVIEW: getCtor(), getConstructor()?

Yeah, probably.

@@ +899,5 @@
> +        # REVIEW: Is this, and the rest of the forwarding to inner below,
> +        # correct?  Like, why should the type "string[]" return true for
> +        # isString()?
> +        #
> +        # This appears in other types, too.

No, its totally bogus.
Comment 40 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 14:17:11 PDT
Comment on attachment 611215 [details] [diff] [review]
More review comments.

Review of attachment 611215 [details] [diff] [review]:
-----------------------------------------------------------------

::: WebIDL.py
@@ +606,5 @@
>      def isType(self):
>          return True
>  
>      def nullable(self):
> +        # REVIEW: isNullable?

Yes.

@@ +694,5 @@
>          assert obj
>          if obj.isType():
>              return obj
>  
> +        name = self.name.resolve(scope, None) # Review: why None, rather than obj, here?

Good question.  I'll have to try to page that back in.
Comment 41 Justin Lebar (not reading bugmail) 2012-04-01 16:47:39 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #40)
> Comment on attachment 611215 [details] [diff] [review]
>
> More review comments.
> @@ +694,5 @@
> >          assert obj
> >          if obj.isType():
> >              return obj
> >  
> > +        name = self.name.resolve(scope, None) # Review: why None, rather than obj, here?
> 
> Good question.  I'll have to try to page that back in.

Just add a comment once you figure it out, if you don't mind.  Seems important, whatever is going on here.  :)
Comment 42 Justin Lebar (not reading bugmail) 2012-04-01 16:50:02 PDT
> > +    parser = optparse.OptionParser(usage='%prog [OPTIONS] [TESTS]')
>
> Why have a usage string and then not use it?

The usage string is just the first line of '--help':

> Usage: runtests.py [OPTIONS] [TESTS]
>
> Options:
>   -h, --help   show this help message and exit
>   -q, --quiet  Don't print passing tests.

Does this look OK to you?
Comment 43 Justin Lebar (not reading bugmail) 2012-04-01 16:54:22 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #39)
> Comment on attachment 611211 [details] [diff] [review]
> Some review comments.
> 
> Review of attachment 611211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: WebIDL.py
> @@ +196,5 @@
> >              return self._name.QName() + "::"
> >          return "::"
> >  
> >      def ensureUnique(self, identifier, object):
> > +        # REVIEW: This is tricky; please add a docstring.
> 
> What part exactly do you find tricky about this?

Specifically the object argument.  It wasn't clear to me what object I should be passing in.  And there's a rule that you can't pass in the same object twice (and, iirc, the object may be null, so what does that mean?).
Comment 44 Justin Lebar (not reading bugmail) 2012-04-01 16:55:35 PDT
How do you want to handle landing these changes plus review comments?  Do you want to qimport these patches (and qfold some of them, or whatever) and make the changes we've agreed on here yourself?  Or do you want some help from me somehow?
Comment 45 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-01 21:09:17 PDT
(In reply to Justin Lebar [:jlebar] from comment #44)
> How do you want to handle landing these changes plus review comments?  Do
> you want to qimport these patches (and qfold some of them, or whatever) and
> make the changes we've agreed on here yourself?

Yeah, I think that's the way to go.
Comment 46 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-03 14:02:10 PDT
Comment on attachment 611198 [details] [diff] [review]
Let runtests.py take --quiet and a list of tests to run.

I changed this to actually use the usage variable.
Comment 47 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-03 19:43:25 PDT
Comment on attachment 611211 [details] [diff] [review]
Some review comments.

Made some, will file bugs on the others.
Comment 48 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-03 19:45:05 PDT
Comment on attachment 611214 [details] [diff] [review]
Don't pass a billion False's to IDLMethod constructor -- add default params instead.

Review of attachment 611214 [details] [diff] [review]:
-----------------------------------------------------------------

::: WebIDL.py
@@ +1464,5 @@
>      def __init__(self, location, identifier, returnType, arguments,
> +                 static=False, getter=False, setter=False, creator=False,
> +                 deleter=False, specialType=NamedOrIndexed.Neither,
> +                 legacycaller=False, stringifier=False):
> +        # REVIEW: specialType is NamedOrIndexed -- wow, this is messed up.

Will file a bug on this.
Comment 49 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-03 19:50:13 PDT
Comment on attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

Review of attachment 611216 [details] [diff] [review]:
-----------------------------------------------------------------

::: WebIDL.py
@@ +785,5 @@
>          self.name = self.inner.name
>          return self
>  
>      def unroll(self):
> +        # REVIEW: Should this be self.inner.unroll()?

Yes, and that's not the only place that has this problem.
Comment 50 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-03 21:02:59 PDT
Comment on attachment 611217 [details] [diff] [review]
Adding three new tests.  They don't all pass.

Review of attachment 611217 [details] [diff] [review]:
-----------------------------------------------------------------

And a commit to make these tests pass.
Comment 51 Boris Zbarsky [:bz] 2012-04-04 06:58:11 PDT
*** Bug 742165 has been marked as a duplicate of this bug. ***
Comment 52 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-04 09:38:06 PDT
Created attachment 612230 [details] [diff] [review]
Patch for m-c
Comment 53 Justin Lebar (not reading bugmail) 2012-04-04 09:47:38 PDT
Comment on attachment 612230 [details] [diff] [review]
Patch for m-c

Do you think Ted needs to review the changes we made to WebIDL.py?  They had two sets of eyes on them...
Comment 54 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-04 09:49:18 PDT
No I think Ted should review the changes I made to client.py.
Comment 55 Ted Mielczarek [:ted.mielczarek] 2012-04-04 10:01:44 PDT
Comment on attachment 612230 [details] [diff] [review]
Patch for m-c

Review of attachment 612230 [details] [diff] [review]:
-----------------------------------------------------------------

::: client.py
@@ +52,5 @@
> +    if os.path.exists(fulldir):
> +        shutil.rmtree(fulldir)
> +
> +    assert not os.path.exists(fulldir)
> +    check_call_noisy([hg, 'clone', '-u', tag, repository, fulldir])

Maybe you should just call do_hg_pull here, or merge this function with do_hg_pull? There's a bit of overlap.
Comment 56 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-04 13:50:48 PDT
I attempted to land this, but it broke the tree :-)
Comment 57 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-04 13:51:05 PDT
I meant :-( of course.
Comment 58 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-04 14:13:06 PDT
Comment on attachment 611216 [details] [diff] [review]
Make Location and BuiltinLocation have the same interface, and rename BuiltinType::type to BuiltinType::_typeTag

Review of attachment 611216 [details] [diff] [review]:
-----------------------------------------------------------------

::: WebIDL.py
@@ +106,5 @@
>          return self._lexpos == other._lexpos and \
>                 self._file == other._file
>  
> +    def filename(self):
> +        return self.filename

Should be self._file of course.
Comment 59 Justin Lebar (not reading bugmail) 2012-04-04 14:14:59 PDT
Like I said in comment 30, I'm pretty surprised that's not caught by a test.
Comment 60 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-12 08:57:19 PDT
https://hg.mozilla.org/mozilla-central/rev/adef80d2b5a0
Comment 61 Justin Lebar (not reading bugmail) 2012-04-12 09:08:03 PDT
FYI, I'm planning to do another pass over the parser during the trip to or from Australia.
Comment 62 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-12 09:08:53 PDT
And backed out again :-/

https://hg.mozilla.org/mozilla-central/rev/b7c1c0145079
Comment 63 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-12 21:14:50 PDT
I sprinkled some fairy dust on it and it stuck this time.

Note You need to log in before you can comment on or make changes to this bug.