Get rid of System class.

Get rid of System class.


R=antonm@google.com

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

[Anton Muhin, 2012-10-01 13:30:35]

Overall approach is LGTM, let's discuss some details.

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

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
could it go into outer closure?

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
shouldn't HtmlSystemShared be renamed into something new now once notion of
System went away?

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/dartdomgener...
lib/html/scripts/dartdomgenerator.py:89: interface_generator =
HtmlDartInterfaceGenerator(
this is longer term, but the question: should it be a class or a function?

Overall, when instance is created, a single method is invoked on it, it smells
pretty much like a function to me.

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

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.p...
lib/html/scripts/systemhtml.py:383: class HtmlSystemShared(object):
this class becomes pretty thin and many of methods look like perfect
freestanding functions (e.g. IsPrivate and many more).

Do we really need it as a class at all?

The only shared state seems to be _event_classes, but it's not trivial to
understand who operates on it.

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.p...
lib/html/scripts/systemhtml.py:398: if parent != interface and parent.id in
self._event_classes:
when parent can equal to some of interface in hiearchy?

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.p...
lib/html/scripts/systemhtml.py:1304: if not emitter:
emitter is None might be slightly more robust, or even good old path in
self._path_to_emitter

https://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.p...
lib/html/scripts/systemhtml.py:1306: if template:
ditto for is None check

[podivilov, 2012-10-01 14:49:18]

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

http://codereview.chromium.org/11018012/diff/1/lib/html/scripts/dartdomgenera...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
On 2012/10/01 13:30:35, Anton Muhin wrote:
> shouldn't HtmlSystemShared be renamed into something new now once notion of
> System went away?

It's all about events now, I'm going to rename it to HtmlEventGenerator in a
separate cl.

http://codereview.chromium.org/11018012/diff/1/lib/html/scripts/dartdomgenera...
lib/html/scripts/dartdomgenerator.py:89: interface_generator =
HtmlDartInterfaceGenerator(
On 2012/10/01 13:30:35, Anton Muhin wrote:
> this is longer term, but the question: should it be a class or a function?
> 
> Overall, when instance is created, a single method is invoked on it, it smells
> pretty much like a function to me.

The instance is needed to store current interface while traversing it's members.
I think we should stay with instance. Inline Generate() method here might be an
option though.

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

http://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.py...
lib/html/scripts/systemhtml.py:383: class HtmlSystemShared(object):
On 2012/10/01 13:30:35, Anton Muhin wrote:
> this class becomes pretty thin and many of methods look like perfect
> freestanding functions (e.g. IsPrivate and many more).
> 
> Do we really need it as a class at all?
> 
> The only shared state seems to be _event_classes, but it's not trivial to
> understand who operates on it.

I have another cl to fix this: http://codereview.chromium.org/11020008/.

http://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.py...
lib/html/scripts/systemhtml.py:398: if parent != interface and parent.id in
self._event_classes:
On 2012/10/01 13:30:35, Anton Muhin wrote:
> when parent can equal to some of interface in hiearchy?

database.Hierarchy() includes the interface itself (this is what most callers
actually need).

http://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.py...
lib/html/scripts/systemhtml.py:1304: if not emitter:
On 2012/10/01 13:30:35, Anton Muhin wrote:
> emitter is None might be slightly more robust, or even good old path in
> self._path_to_emitter

Done.

http://codereview.chromium.org/11018012/diff/1/lib/html/scripts/systemhtml.py...
lib/html/scripts/systemhtml.py:1306: if template:
On 2012/10/01 13:30:35, Anton Muhin wrote:
> ditto for is None check

Done.

[Anton Muhin, 2012-10-01 15:44:43]

lgtm

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
File lib/html/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
ping: should it belong here?

[podivilov, 2012-10-02 15:55:18]

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
File lib/html/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
On 2012/10/01 15:44:44, Anton Muhin wrote:
> ping: should it belong here?

kind of :) see http://codereview.chromium.org/11020008/

[Anton Muhin, 2012-10-02 15:59:22]

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
File lib/html/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
I meant it doesn't close on RunGenerator and can probably lifted into outer
scope.

On 2012/10/02 15:55:18, podivilov wrote:
> On 2012/10/01 15:44:44, Anton Muhin wrote:
> > ping: should it belong here?
> 
> kind of :) see http://codereview.chromium.org/11020008/

[podivilov, 2012-10-02 17:11:55]

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
File lib/html/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
On 2012/10/02 15:59:22, Anton Muhin wrote:
> I meant it doesn't close on RunGenerator and can probably lifted into outer
> scope.
> 
> On 2012/10/02 15:55:18, podivilov wrote:
> > On 2012/10/01 15:44:44, Anton Muhin wrote:
> > > ping: should it belong here?
> > 
> > kind of :) see http://codereview.chromium.org/11020008/
> 

HtmlEventGenerator depends on template_loader and can't be lifted.

[Anton Muhin, 2012-10-03 06:20:36]

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
File lib/html/scripts/dartdomgenerator.py (right):

http://codereview.chromium.org/11018012/diff/2002/lib/html/scripts/dartdomgen...
lib/html/scripts/dartdomgenerator.py:85: shared_system =
HtmlSystemShared(webkit_database)
Pavel, given that referenced CL is already submitted, may you rebase this one to
see how it looks now?

On 2012/10/02 17:11:55, podivilov wrote:
> On 2012/10/02 15:59:22, Anton Muhin wrote:
> > I meant it doesn't close on RunGenerator and can probably lifted into outer
> > scope.
> > 
> > On 2012/10/02 15:55:18, podivilov wrote:
> > > On 2012/10/01 15:44:44, Anton Muhin wrote:
> > > > ping: should it belong here?
> > > 
> > > kind of :) see http://codereview.chromium.org/11020008/
> > 
> 
> HtmlEventGenerator depends on template_loader and can't be lifted.