Start GenerateDispatch refactoring.

Start GenerateDispatch refactoring.

R=podivilov@chromium.org

[podivilov, 2012-05-05 16:44:11]

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py
File lib/dom/scripts/generator.py (right):

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py#n...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
I don't think it's a good idea to move this method here.

[sra1, 2012-05-05 18:02:06]

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
File lib/dom/scripts/generator.py (right):

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
On 2012/05/05 16:44:12, podivilov wrote:
> I don't think it's a good idea to move this method here.

Pavel, could you say why?

My concern is that native-specific tests are in this general code.
Perhaps we should have a DispatchGenerator class with overridable methods for
ShouldGenerateSingleOperation and for passing the current optional argument (to
fix the openCursor issue)

Anton: perhaps you could say what you plan to do next?

[podivilov, 2012-05-10 10:51:43]

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
File lib/dom/scripts/generator.py (right):

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
On 2012/05/05 18:02:06, sra1 wrote:
> On 2012/05/05 16:44:12, podivilov wrote:
> > I don't think it's a good idea to move this method here.
> 
> Pavel, could you say why?
> 
> My concern is that native-specific tests are in this general code.
> Perhaps we should have a DispatchGenerator class with overridable methods for
> ShouldGenerateSingleOperation and for passing the current optional argument
(to
> fix the openCursor issue)
> 
> Anton: perhaps you could say what you plan to do next?

We currently use this method in two systems - native and htmldartium. Since we
are deprecating dart:dom really soon, there will be just a single system that
uses it. I think moving this code in generator.py is wrong, because it will be
used by the native system only.

[Anton Muhin, 2012-05-10 11:30:13]

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
File lib/dom/scripts/generator.py (right):

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
I am not sure we should be that picky about how many systems use this
functionality or if it's only native/wrapping system.  After all this
functionality looks generic enough for any other system so ideally I'd like to
move it out of systems altogether, esp. given that it depends on a single
operation provided by a system: GenerateSingleOperation.

And we can always reconsider this solution later, so I'd ask not to block this
CL on this regard.

On 2012/05/10 10:51:43, podivilov wrote:
> On 2012/05/05 18:02:06, sra1 wrote:
> > On 2012/05/05 16:44:12, podivilov wrote:
> > > I don't think it's a good idea to move this method here.
> > 
> > Pavel, could you say why?
> > 
> > My concern is that native-specific tests are in this general code.
> > Perhaps we should have a DispatchGenerator class with overridable methods
for
> > ShouldGenerateSingleOperation and for passing the current optional argument
> (to
> > fix the openCursor issue)
> > 
> > Anton: perhaps you could say what you plan to do next?
> 
> We currently use this method in two systems - native and htmldartium. Since we
> are deprecating dart:dom really soon, there will be just a single system that
> uses it. I think moving this code in generator.py is wrong, because it will be
> used by the native system only.

https://chromiumcodereview.appspot.com/10375026/diff/1/lib/dom/scripts/genera...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
Stephen, this CL just gets rid of code duplication we have.

Next move would be to simplify interface to GenerateDispatch---we have this code
which generates overloads, position etc. all over the call sites.

Ultimate goal is to rewrite dispatcher to match one for v8 bindings at least for
native system.

The problem I am addessing is as openCursore which in v8 case translates to:

if ((arg0 === null || arg0 is IDBKeyRange) and (arg2 == null || arg2 is int)) {
  // Invoke IDBRange overload
} else (<fake check as IDBKey can be anything you wish>) {
  // Invoke IDBKet variant.
}
throw 'Dispatch failed.'
}  

On 2012/05/05 18:02:06, sra1 wrote:
> On 2012/05/05 16:44:12, podivilov wrote:
> > I don't think it's a good idea to move this method here.
> 
> Pavel, could you say why?
> 
> My concern is that native-specific tests are in this general code.
> Perhaps we should have a DispatchGenerator class with overridable methods for
> ShouldGenerateSingleOperation and for passing the current optional argument
(to
> fix the openCursor issue)
> 
> Anton: perhaps you could say what you plan to do next?

[podivilov, 2012-05-10 15:57:13]

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py
File lib/dom/scripts/generator.py (right):

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py#n...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
On 2012/05/10 11:30:13, antonmuhin wrote:
> Stephen, this CL just gets rid of code duplication we have.

Why is it needed? We are going to get rid of duplication anyway by merging the
systems together.

[Anton Muhin, 2012-05-10 16:11:55]

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py
File lib/dom/scripts/generator.py (right):

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py#n...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
What do you suggest?  Wait until we get rid of it?  Or change only wrapping
part?

On 2012/05/10 15:57:13, podivilov wrote:
> On 2012/05/10 11:30:13, antonmuhin wrote:
> > Stephen, this CL just gets rid of code duplication we have.
> 
> Why is it needed? We are going to get rid of duplication anyway by merging the
> systems together.

[podivilov, 2012-05-10 16:20:36]

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py
File lib/dom/scripts/generator.py (right):

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py#n...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
On 2012/05/10 16:11:55, antonmuhin wrote:
> What do you suggest?  Wait until we get rid of it?  Or change only wrapping
> part?

Personally, I prefer that you wait until migration and cleanup are over to avoid
conflicts. But I'm also ok with changing wrapping part now and merging later.

> 
> On 2012/05/10 15:57:13, podivilov wrote:
> > On 2012/05/10 11:30:13, antonmuhin wrote:
> > > Stephen, this CL just gets rid of code duplication we have.
> > 
> > Why is it needed? We are going to get rid of duplication anyway by merging
the
> > systems together.
>

[Anton Muhin, 2012-05-10 16:24:07]

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py
File lib/dom/scripts/generator.py (right):

http://codereview.chromium.org/10375026/diff/1/lib/dom/scripts/generator.py#n...
lib/dom/scripts/generator.py:725: def GenerateDispatch(generator, emitter, info,
indent, position, overloads):
What's ETA?  If it's a day or two, I'd better wait.  Otherwise I'd prefer to
proceed.

On 2012/05/10 16:20:36, podivilov wrote:
> On 2012/05/10 16:11:55, antonmuhin wrote:
> > What do you suggest?  Wait until we get rid of it?  Or change only wrapping
> > part?
> 
> Personally, I prefer that you wait until migration and cleanup are over to
avoid
> conflicts. But I'm also ok with changing wrapping part now and merging later.
> 
> > 
> > On 2012/05/10 15:57:13, podivilov wrote:
> > > On 2012/05/10 11:30:13, antonmuhin wrote:
> > > > Stephen, this CL just gets rid of code duplication we have.
> > > 
> > > Why is it needed? We are going to get rid of duplication anyway by merging
> the
> > > systems together.
> > 
>