Get rid of System classes for backend generators.

Get rid of System classes for backend generators.

They introduce unnecessary level of indirection with no added value.

R=antonm@google.com,vsm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=12946

[Anton Muhin, 2012-09-26 14:20:36]

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
File lib/html/scripts/dartdomgenerator.py (right):

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:72: def RunGenerator(dart_library_template,
dart_output_dir, dart_library_path,
overall logic becomes rather complicated, so, sorry, couple of comments in
attempt to make understanding the code below easier

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:73: create_generator_backend):
I'd rather name it generator_backend_factory.

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:73: create_generator_backend):
generator backend sounds somewhat too much, can it be just generator?  Overall
it looks weird to see class FooGeneratorBackend extending BaseGenerator,
FooBackendGenerator or even FooGenerator looks ways more readable.

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:74: template_loader = TemplateLoader(
cannot we lift template_loader creation out of this function?

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:76: options = GeneratorOptions(
cannot we lift options creation out of this function?

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:92: def
create_generator_backend(interface):
that might be a perfect case for lambda, esp. if you agree to change names

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systembase.py
File lib/html/scripts/systembase.py (right):

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systembase.p...
lib/html/scripts/systembase.py:56: def __init__(self, database, type_registry,
interface):
maybe you should pass options here as well to unify the approach

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemdart2j...
File lib/html/scripts/systemdart2js.py (left):

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemdart2j...
lib/html/scripts/systemdart2js.py:25: class Dart2JSSystem(System):
does anybody use it now?

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemhtml.py
File lib/html/scripts/systemhtml.py (right):

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemhtml.p...
lib/html/scripts/systemhtml.py:763: class
Dart2JSGeneratorBackend(Dart2JSInterfaceGenerator):
if you want to keep both generator and backend, I think it should go in oposite
order: Dart2JSBackendGenerator, although I indeed think Dart2JSGenerator is
enough or Dart2JSImplementationGenerator to keep Interface vs. Implementation
distinction

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative.py
File lib/html/scripts/systemnative.py (right):

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative...
lib/html/scripts/systemnative.py:15: class
DartiumGeneratorBackend(BaseGenerator):
NativeGeneratorBackend or even NativeGenerator?

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative...
lib/html/scripts/systemnative.py:842: def _GenerateCPPIncludes(self, includes):
why it is method now, not a free function?

https://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative...
lib/html/scripts/systemnative.py:845: def _ToWebKitName(self, name):
ditto

[podivilov, 2012-09-26 15:02:04]

PTAL.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgenera...
File lib/html/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgenera...
lib/html/scripts/dartdomgenerator.py:73: create_generator_backend):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> I'd rather name it generator_backend_factory.

Done.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgenera...
lib/html/scripts/dartdomgenerator.py:73: create_generator_backend):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> generator backend sounds somewhat too much, can it be just generator?  Overall
> it looks weird to see class FooGeneratorBackend extending BaseGenerator,
> FooBackendGenerator or even FooGenerator looks ways more readable.

Renamed to FooBackend. Backends should not extend BaseGenerator (subject to a
separate cl).

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgenera...
lib/html/scripts/dartdomgenerator.py:74: template_loader = TemplateLoader(
On 2012/09/26 14:20:36, Anton Muhin wrote:
> cannot we lift template_loader creation out of this function?

Done.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/dartdomgenera...
lib/html/scripts/dartdomgenerator.py:92: def
create_generator_backend(interface):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> that might be a perfect case for lambda, esp. if you agree to change names

Done.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systembase.py
File lib/html/scripts/systembase.py (right):

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systembase.py...
lib/html/scripts/systembase.py:56: def __init__(self, database, type_registry,
interface):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> maybe you should pass options here as well to unify the approach

This class will be eventually merged into HtmlDartInterfaceGenerator, so i'm
trying to minimize the diff here.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemdart2js.py
File lib/html/scripts/systemdart2js.py (left):

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemdart2js...
lib/html/scripts/systemdart2js.py:25: class Dart2JSSystem(System):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> does anybody use it now?

The whole file is a dead code.  I think we should merge
Dart2JSInterfaceGenerator into HtmlDartInterfaceGenerator and remove the file
(in a separate cl).

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemhtml.py
File lib/html/scripts/systemhtml.py (right):

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemhtml.py...
lib/html/scripts/systemhtml.py:763: class
Dart2JSGeneratorBackend(Dart2JSInterfaceGenerator):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> if you want to keep both generator and backend, I think it should go in
oposite
> order: Dart2JSBackendGenerator, although I indeed think Dart2JSGenerator is
> enough or Dart2JSImplementationGenerator to keep Interface vs. Implementation
> distinction

Renamed to Dart2JSBackend. Interface vs. Implementation distinction is
unfortunate, because it leads to significant code duplication.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative.py
File lib/html/scripts/systemnative.py (right):

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative....
lib/html/scripts/systemnative.py:15: class
DartiumGeneratorBackend(BaseGenerator):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> NativeGeneratorBackend or even NativeGenerator?

"native" is confusing. Renamed to DartiumBackend.

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative....
lib/html/scripts/systemnative.py:842: def _GenerateCPPIncludes(self, includes):
On 2012/09/26 14:20:36, Anton Muhin wrote:
> why it is method now, not a free function?

It's only used in DartiumBackend class.

[Anton Muhin, 2012-09-26 15:17:25]

lgtm

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative.py
File lib/html/scripts/systemnative.py (right):

http://codereview.chromium.org/10986048/diff/1/lib/html/scripts/systemnative....
lib/html/scripts/systemnative.py:842: def _GenerateCPPIncludes(self, includes):
Up to you.  I'd vote for keeping it free function or making static to make it
explicit it doesn't depend on state.

On 2012/09/26 15:02:04, podivilov wrote:
> On 2012/09/26 14:20:36, Anton Muhin wrote:
> > why it is method now, not a free function?
> 
> It's only used in DartiumBackend class.

[vsm, 2012-09-26 16:27:40]

lgtm.  Do you plan to get rid of them for interface generation as well?

[podivilov, 2012-09-26 16:52:34]

On 2012/09/26 16:27:40, vsm wrote:
> lgtm.  Do you plan to get rid of them for interface generation as well?

Yes, I do. May I also drop systemdart2js.py and systeminterface.py, or there is
a reason to keep them?

[vsm, 2012-09-26 22:18:34]

On 2012/09/26 16:52:34, podivilov wrote:
> On 2012/09/26 16:27:40, vsm wrote:
> > lgtm.  Do you plan to get rid of them for interface generation as well?
> 
> Yes, I do. May I also drop systemdart2js.py and systeminterface.py, or there
is
> a reason to keep them?

I don't think there is any reason to keep those.