Change [ConstructorCallWith={Document => ExecutionContext}]

Change [ConstructorCallWith={Document => ExecutionContext}]

Drop support for [ConstructorCallWith=Document] from the IDL code
generator.

[ConstructorCallWith=ExecutionContext] provides equivalent functionality;
the implementation can simply extract the document from the execution
context using

  toDocument(executionContext)

instead if it needs to.

BUG=

[Jens Widell, 2014-06-18 10:39:08]

Dropping [ConstructorCallWith=Document], as discussed in
https://codereview.chromium.org/340443004/.

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:547: toV8(documentPtr, info.Holder(),
isolate);
I don't really understand what this code is about, so I left it intact.

[haraken, 2014-06-18 10:49:21]

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:547: toV8(documentPtr, info.Holder(),
isolate);
On 2014/06/18 10:39:08, Jens Lindström wrote:
> I don't really understand what this code is about, so I left it intact.

This is complicated to explain, but basically this code makes sure that a
document wrapper exists. This is needed for making the object grouping algorithm
workable (See V8GCController.cpp).

https://codereview.chromium.org/340763003/diff/1/Source/core/dom/Comment.h
File Source/core/dom/Comment.h (right):

https://codereview.chromium.org/340763003/diff/1/Source/core/dom/Comment.h#ne...
Source/core/dom/Comment.h:34: static PassRefPtrWillBeRawPtr<Comment>
create(Document&, const String&);

Would it be possible to drop this constructor? It doesn't make sense to have
both Document version and ExecutionContext version.

The same comment for other parts in this CL.

[Jens Widell, 2014-06-18 11:00:03]

https://codereview.chromium.org/340763003/diff/1/Source/core/dom/Comment.h
File Source/core/dom/Comment.h (right):

https://codereview.chromium.org/340763003/diff/1/Source/core/dom/Comment.h#ne...
Source/core/dom/Comment.h:34: static PassRefPtrWillBeRawPtr<Comment>
create(Document&, const String&);
On 2014/06/18 10:49:21, haraken wrote:
> 
> Would it be possible to drop this constructor? It doesn't make sense to have
> both Document version and ExecutionContext version.
> 
> The same comment for other parts in this CL.

All cases where I overloaded instead of just changing the argument type of an
existing function were cases where the existing function is used from
(non-generated) C++ code.

Naturally, all these create() functions that take a Document argument, create
objects that are connected to a document. When a constructor is called from a
script, a (single / the primary) document can be implied from the scope, but in
other situations, that might not be the right document to use.

For instance, Document::createComment() passes in *this, which might not be what
toDocument() returns for any execution context.

So I'd say that Comment, Text, DocumentFragment and Range at least need their
::create(Document&, ...) overloads for correctness.  That's not saying there
aren't calls to those create()s that couldn't be changed to use the
::create(ExecutionContext*, ...) variant instead, but it doesn't seem
worth-while to me to analyze them all and change them.

WDYT?

[Jens Widell, 2014-06-18 11:14:39]

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:547: toV8(documentPtr, info.Holder(),
isolate);
On 2014/06/18 10:49:21, haraken wrote:
> On 2014/06/18 10:39:08, Jens Lindström wrote:
> > I don't really understand what this code is about, so I left it intact.
> 
> This is complicated to explain, but basically this code makes sure that a
> document wrapper exists. This is needed for making the object grouping
algorithm
> workable (See V8GCController.cpp).

Why would this only be needed for "named" constructors, and not for the pretty
similar regular constructors that used to have ConstructorCallWith=Document, and
now have ConstructorCallWith=ExecutionContext but still use the document? That
is, how is "new Comment(...)" different from "new Image(...)"?

[haraken, 2014-06-18 11:16:35]

https://codereview.chromium.org/340763003/diff/1/Source/core/dom/Comment.h
File Source/core/dom/Comment.h (right):

https://codereview.chromium.org/340763003/diff/1/Source/core/dom/Comment.h#ne...
Source/core/dom/Comment.h:34: static PassRefPtrWillBeRawPtr<Comment>
create(Document&, const String&);
On 2014/06/18 11:00:02, Jens Lindström wrote:
> On 2014/06/18 10:49:21, haraken wrote:
> > 
> > Would it be possible to drop this constructor? It doesn't make sense to have
> > both Document version and ExecutionContext version.
> > 
> > The same comment for other parts in this CL.
> 
> All cases where I overloaded instead of just changing the argument type of an
> existing function were cases where the existing function is used from
> (non-generated) C++ code.
> 
> Naturally, all these create() functions that take a Document argument, create
> objects that are connected to a document. When a constructor is called from a
> script, a (single / the primary) document can be implied from the scope, but
in
> other situations, that might not be the right document to use.
> 
> For instance, Document::createComment() passes in *this, which might not be
what
> toDocument() returns for any execution context.
> 
> So I'd say that Comment, Text, DocumentFragment and Range at least need their
> ::create(Document&, ...) overloads for correctness.  That's not saying there
> aren't calls to those create()s that couldn't be changed to use the
> ::create(ExecutionContext*, ...) variant instead, but it doesn't seem
> worth-while to me to analyze them all and change them.
> 
> WDYT?

hmm, I cannot come up with any case where we cannot replace Document& with
ExecutionContext*.

> For instance, Document::createComment() passes in *this, which might not be
what
> toDocument() returns for any execution context.

I'm sure it's guaranteed that toDocument(executionContext) returns the same
Document as |this|. In this case, Document::createComment() can pass in |this|,
and catch it with Comment::create(ExecutionContext*).

My point is that it wouldn't make sense to have both Document version and
ExecutionContext version, because these two are always the same thing. (So I
don't think you need to investigate the correctness one by one.)

[haraken, 2014-06-18 11:24:52]

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:547: toV8(documentPtr, info.Holder(),
isolate);
On 2014/06/18 11:14:38, Jens Lindström wrote:
> On 2014/06/18 10:49:21, haraken wrote:
> > On 2014/06/18 10:39:08, Jens Lindström wrote:
> > > I don't really understand what this code is about, so I left it intact.
> > 
> > This is complicated to explain, but basically this code makes sure that a
> > document wrapper exists. This is needed for making the object grouping
> algorithm
> > workable (See V8GCController.cpp).
> 
> Why would this only be needed for "named" constructors, and not for the pretty
> similar regular constructors that used to have ConstructorCallWith=Document,
and
> now have ConstructorCallWith=ExecutionContext but still use the document? That
> is, how is "new Comment(...)" different from "new Image(...)"?

That is because Nodes can have named constructors but cannot ahve (non-named)
constructors (e.g., Comment is not a Node but Image is a Node). Once we
introduce a (non-named) constructor for Nodes, I think we need to add the
toV8(document) into the (non-named) constructor.

[Jens Widell, 2014-06-18 11:29:03]

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:547: toV8(documentPtr, info.Holder(),
isolate);
On 2014/06/18 11:24:52, haraken wrote:
> On 2014/06/18 11:14:38, Jens Lindström wrote:
> > On 2014/06/18 10:49:21, haraken wrote:
> > > On 2014/06/18 10:39:08, Jens Lindström wrote:
> > > > I don't really understand what this code is about, so I left it intact.
> > > 
> > > This is complicated to explain, but basically this code makes sure that a
> > > document wrapper exists. This is needed for making the object grouping
> > algorithm
> > > workable (See V8GCController.cpp).
> > 
> > Why would this only be needed for "named" constructors, and not for the
pretty
> > similar regular constructors that used to have ConstructorCallWith=Document,
> and
> > now have ConstructorCallWith=ExecutionContext but still use the document?
That
> > is, how is "new Comment(...)" different from "new Image(...)"?
> 
> That is because Nodes can have named constructors but cannot ahve (non-named)
> constructors (e.g., Comment is not a Node but Image is a Node). Once we
> introduce a (non-named) constructor for Nodes, I think we need to add the
> toV8(document) into the (non-named) constructor.

I don't understand. In what sense is Comment (and Text or DocumentFragment) not
a Node?

[haraken, 2014-06-18 11:34:14]

On 2014/06/18 11:29:03, Jens Lindström wrote:
>
https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/340763003/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:547: toV8(documentPtr, info.Holder(),
> isolate);
> On 2014/06/18 11:24:52, haraken wrote:
> > On 2014/06/18 11:14:38, Jens Lindström wrote:
> > > On 2014/06/18 10:49:21, haraken wrote:
> > > > On 2014/06/18 10:39:08, Jens Lindström wrote:
> > > > > I don't really understand what this code is about, so I left it
intact.
> > > > 
> > > > This is complicated to explain, but basically this code makes sure that
a
> > > > document wrapper exists. This is needed for making the object grouping
> > > algorithm
> > > > workable (See V8GCController.cpp).
> > > 
> > > Why would this only be needed for "named" constructors, and not for the
> pretty
> > > similar regular constructors that used to have
ConstructorCallWith=Document,
> > and
> > > now have ConstructorCallWith=ExecutionContext but still use the document?
> That
> > > is, how is "new Comment(...)" different from "new Image(...)"?
> > 
> > That is because Nodes can have named constructors but cannot ahve
(non-named)
> > constructors (e.g., Comment is not a Node but Image is a Node). Once we
> > introduce a (non-named) constructor for Nodes, I think we need to add the
> > toV8(document) into the (non-named) constructor.
> 
> I don't understand. In what sense is Comment (and Text or DocumentFragment)
not
> a Node?

I understand what you mean... you're right. I'm confused :) Looking.

[haraken, 2014-06-18 11:52:33]

hmm, it seems you're right.

- I don't see why we need the toV8() from the perspective of how V8GCController
groups objects. However, I'm not 100% confident.
- I don't see why we need the toV8() in named constructors but don't need in
(non-named) constructors. This sounds very strange, as you pointed out.

Would it be possible to blame the git history and see why the code is added?

[Jens Widell, 2014-06-18 12:11:51]

On 2014/06/18 11:52:33, haraken wrote:
> hmm, it seems you're right.
> 
> - I don't see why we need the toV8() from the perspective of how
V8GCController
> groups objects. However, I'm not 100% confident.
> - I don't see why we need the toV8() in named constructors but don't need in
> (non-named) constructors. This sounds very strange, as you pointed out.
> 
> Would it be possible to blame the git history and see why the code is added?

AFAICT, you added the code in r99641 (da1cf844aeb5f5280b7c0bdc5b5e8e4d3e55173b)
when initially adding support for named constructors in CodeGeneratorV8.pm.

[Jens Widell, 2014-06-18 12:13:21]

On 2014/06/18 11:16:35, haraken wrote:
> hmm, I cannot come up with any case where we cannot replace Document& with
> ExecutionContext*.

I'm sorry, I had assumed a relationship between Document and ExecutionContext
that was completely disconnected from reality. You're right, of course. I'll
update the patch.

[haraken, 2014-06-18 12:17:15]

On 2014/06/18 12:11:51, Jens Lindström wrote:
> On 2014/06/18 11:52:33, haraken wrote:
> > hmm, it seems you're right.
> > 
> > - I don't see why we need the toV8() from the perspective of how
> V8GCController
> > groups objects. However, I'm not 100% confident.
> > - I don't see why we need the toV8() in named constructors but don't need in
> > (non-named) constructors. This sounds very strange, as you pointed out.
> > 
> > Would it be possible to blame the git history and see why the code is added?
> 
> AFAICT, you added the code in r99641
(da1cf844aeb5f5280b7c0bdc5b5e8e4d3e55173b)
> when initially adding support for named constructors in CodeGeneratorV8.pm.

I'm sorry... I'm doing very poor communication here.

As far as I remember, I added the named constructors to CodeGeneratorV8.pm so
that the generated code is exactly the same as existing custom bindings (which I
removed after supporting the named constructors in CodeGeneratorV8.pm). Let me
blame the git history of the custom bindings I removed.

[haraken, 2014-06-18 12:37:34]

On 2014/06/18 12:17:15, haraken wrote:
> On 2014/06/18 12:11:51, Jens Lindström wrote:
> > On 2014/06/18 11:52:33, haraken wrote:
> > > hmm, it seems you're right.
> > > 
> > > - I don't see why we need the toV8() from the perspective of how
> > V8GCController
> > > groups objects. However, I'm not 100% confident.
> > > - I don't see why we need the toV8() in named constructors but don't need
in
> > > (non-named) constructors. This sounds very strange, as you pointed out.
> > > 
> > > Would it be possible to blame the git history and see why the code is
added?
> > 
> > AFAICT, you added the code in r99641
> (da1cf844aeb5f5280b7c0bdc5b5e8e4d3e55173b)
> > when initially adding support for named constructors in CodeGeneratorV8.pm.
> 
> I'm sorry... I'm doing very poor communication here.
> 
> As far as I remember, I added the named constructors to CodeGeneratorV8.pm so
> that the generated code is exactly the same as existing custom bindings (which
I
> removed after supporting the named constructors in CodeGeneratorV8.pm). Let me
> blame the git history of the custom bindings I removed.

OK, now I figured it out. In conclusion, I think we can remove the
toV8(document).

- Only HTMLAudioElement, HTMLImageElement and HTMLOptionsElement have named
constructors.

- Three years ago the named constructors were written in custom bindings. The
custom bindings for HTMLAudioElement and HTMLImageElement had toV8(document),
but HTMLOptionsElement didn't have toV8(document). The reason why we had
toV8(document) on HTMLAudioElement and HTMLImageElement was that these elements
had some asynchronous lifetime. We were ensuring the asynchronous lifetime by
creating the document wrapper and put the document wrapper and the element's
wrapper in the object group.

- Now we don't need to do that, because the lifetime of HTMLAudioElement is
managed by making the HTMLAudioElement an ActiveDOMObject. Also the lifetime of
HTMLImageElement is managed by hand-written hacks in V8GCController.cpp (In
terms of semantics, we should make HTMLImageElement an ActiveDOMObject, but we
didn't do that since we found that making HTMLImageElement an ActiveDOMObject
increases # of ActiveDOMObjects and it affects performance). Either way, the
lifetime is already managed by a separate mechanism, and thus we no longer need
rely on creating a document wrapper in the same tree.

- Therefore, I think we can now remove the toV8(document).

[Jens Widell, 2014-06-18 12:41:39]

On 2014/06/18 12:37:34, haraken wrote:

<lots of interesting information>

> - Therefore, I think we can now remove the toV8(document).

Great. Separate CL, in case removing it turns out to have unexpected
side-effects?

[haraken, 2014-06-18 12:45:51]

On 2014/06/18 12:41:39, Jens Lindström wrote:
> On 2014/06/18 12:37:34, haraken wrote:
> 
> <lots of interesting information>
> 
> > - Therefore, I think we can now remove the toV8(document).
> 
> Great. Separate CL, in case removing it turns out to have unexpected
> side-effects?

Yes!

[Jens Widell, 2014-06-18 12:53:29]

Patch updated to replace create(Document&, ...) with create(ExecutionContext*,
...) in all cases, instead of overloading.

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:248: operator ExecutionContext*() { return this; }
Operator added primarily to make authoring this patch quicker.

If we don't like it, I'll go through all the calls that now depend on it make
the simple changes necessary.

Didn't want to do all that first, in case the response had been "add a
conversion operator to Document" instead. :-)

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Range.h#...
Source/core/dom/Range.h:53: static PassRefPtrWillBeRawPtr<Range>
create(ExecutionContext*, Node* startContainer, int startOffset, Node*
endContainer, int endOffset);
This overload and the next are not used from generated code. Changed them too
only for consistency.

[haraken, 2014-06-18 13:15:01]

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:248: operator ExecutionContext*() { return this; }
On 2014/06/18 12:53:29, Jens Lindström wrote:
> Operator added primarily to make authoring this patch quicker.
> 
> If we don't like it, I'll go through all the calls that now depend on it make
> the simple changes necessary.
> 
> Didn't want to do all that first, in case the response had been "add a
> conversion operator to Document" instead. :-)

I think we want to avoid implicit conversions as much as possible, since
implicit conversions can easily lead to unexpected conversions.

> If we don't like it, I'll go through all the calls that now depend on it make
> the simple changes necessary.

How hard would it be to replace all the calls? If it's hard, I'm OK with adding
a FIXME here and remove the implicit conversion in follow-up CLs.

[Jens Widell, 2014-06-18 14:19:43]

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:248: operator ExecutionContext*() { return this; }
On 2014/06/18 13:15:01, haraken wrote:
> I think we want to avoid implicit conversions as much as possible, since
> implicit conversions can easily lead to unexpected conversions.

I don't disagree. OTOH, there's a public inheritance relationship in play here.
Implicit conversion is quite expected then, and we'd have had it already had it
not been for the fact that we tend to use Document& and ExecutionContext*.

> How hard would it be to replace all the calls?

Technically trivial.

It's mostly a matter of changing "create(document(), ...)" into
"create(&document(), ...)". But I think that's a somewhat sad thing to do,
adding "&" doesn't really make the code better in any way, we still have an
implicit conversion, and taking the address of a function return value is sort
of... iffy.

We could of course also make that "toExecutionContext(document())" or
"document().asExecutionContext()" or something, with added utility functions (or
maybe there's something like that already?)

[haraken, 2014-06-18 15:41:42]

On 2014/06/18 14:19:43, Jens Lindström wrote:
>
https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document.h
> File Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document...
> Source/core/dom/Document.h:248: operator ExecutionContext*() { return this; }
> On 2014/06/18 13:15:01, haraken wrote:
> > I think we want to avoid implicit conversions as much as possible, since
> > implicit conversions can easily lead to unexpected conversions.
> 
> I don't disagree. OTOH, there's a public inheritance relationship in play
here.
> Implicit conversion is quite expected then, and we'd have had it already had
it
> not been for the fact that we tend to use Document& and ExecutionContext*.
> 
> > How hard would it be to replace all the calls?
> 
> Technically trivial.
> 
> It's mostly a matter of changing "create(document(), ...)" into
> "create(&document(), ...)". But I think that's a somewhat sad thing to do,
> adding "&" doesn't really make the code better in any way, we still have an
> implicit conversion, and taking the address of a function return value is sort
> of... iffy.
> 
> We could of course also make that "toExecutionContext(document())" or
> "document().asExecutionContext()" or something, with added utility functions
(or
> maybe there's something like that already?)

tkent-san: Do you have any preference on this?

[tkent, 2014-06-18 23:44:24]

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.h:248: operator ExecutionContext*() { return this; }
On 2014/06/18 14:19:43, Jens Lindström wrote:
> On 2014/06/18 13:15:01, haraken wrote:
> > I think we want to avoid implicit conversions as much as possible, since
> > implicit conversions can easily lead to unexpected conversions.
> 
> I don't disagree. OTOH, there's a public inheritance relationship in play
here.
> Implicit conversion is quite expected then, and we'd have had it already had
it
> not been for the fact that we tend to use Document& and ExecutionContext*.
> 
> > How hard would it be to replace all the calls?
> 
> Technically trivial.
> 
> It's mostly a matter of changing "create(document(), ...)" into
> "create(&document(), ...)". But I think that's a somewhat sad thing to do,
> adding "&" doesn't really make the code better in any way, we still have an
> implicit conversion, and taking the address of a function return value is sort
> of... iffy.
> 
> We could of course also make that "toExecutionContext(document())" or
> "document().asExecutionContext()" or something, with added utility functions
(or
> maybe there's something like that already?)

Changing Document arguments of factory functions to ExecutionContext looks a
regression to me.  DOM Node factories really need Documents, not
ExecutionContexts.  So the arguments should be Documents.

This CL is harmful for non-binding C++ code.

[haraken, 2014-06-19 05:53:44]

On 2014/06/18 23:44:24, tkent wrote:
>
https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document.h
> File Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/340763003/diff/20001/Source/core/dom/Document...
> Source/core/dom/Document.h:248: operator ExecutionContext*() { return this; }
> On 2014/06/18 14:19:43, Jens Lindström wrote:
> > On 2014/06/18 13:15:01, haraken wrote:
> > > I think we want to avoid implicit conversions as much as possible, since
> > > implicit conversions can easily lead to unexpected conversions.
> > 
> > I don't disagree. OTOH, there's a public inheritance relationship in play
> here.
> > Implicit conversion is quite expected then, and we'd have had it already had
> it
> > not been for the fact that we tend to use Document& and ExecutionContext*.
> > 
> > > How hard would it be to replace all the calls?
> > 
> > Technically trivial.
> > 
> > It's mostly a matter of changing "create(document(), ...)" into
> > "create(&document(), ...)". But I think that's a somewhat sad thing to do,
> > adding "&" doesn't really make the code better in any way, we still have an
> > implicit conversion, and taking the address of a function return value is
sort
> > of... iffy.
> > 
> > We could of course also make that "toExecutionContext(document())" or
> > "document().asExecutionContext()" or something, with added utility functions
> (or
> > maybe there's something like that already?)
> 
> Changing Document arguments of factory functions to ExecutionContext looks a
> regression to me.  DOM Node factories really need Documents, not
> ExecutionContexts.  So the arguments should be Documents.
> 
> This CL is harmful for non-binding C++ code.

Thanks tkent-san, then the patch set 1 would make more sense.

- It won't affect non-binding code.
- It improves binding code in a sense that we can remove
[ConstructorCallWith=Document].

Although the patch set 1 adds a couple of methods to the C++ side, it's common
that we simplify binding code by adding some code into the C++ side. What do you
think?

[tkent, 2014-06-19 06:25:09]

On 2014/06/19 05:53:44, haraken wrote:
> Thanks tkent-san, then the patch set 1 would make more sense.
> 
> - It won't affect non-binding code.
> - It improves binding code in a sense that we can remove
> [ConstructorCallWith=Document].
> 
> Although the patch set 1 adds a couple of methods to the C++ side, it's common
> that we simplify binding code by adding some code into the C++ side. What do
you
> think?

Patch Set 1 looks also bad.  I don't want to have new factory functions though
auto-generated code for ConstructorCallWith=Document works well now.
I don't think dropping ConstructorCallWith=Document has benefit.

[Jens Widell, 2014-06-20 07:55:58]

On 2014/06/19 06:25:09, tkent wrote:
> On 2014/06/19 05:53:44, haraken wrote:
> > Thanks tkent-san, then the patch set 1 would make more sense.
> > 
> > - It won't affect non-binding code.
> > - It improves binding code in a sense that we can remove
> > [ConstructorCallWith=Document].
> > 
> > Although the patch set 1 adds a couple of methods to the C++ side, it's
common
> > that we simplify binding code by adding some code into the C++ side. What do
> you
> > think?
> 
> Patch Set 1 looks also bad.  I don't want to have new factory functions though
> auto-generated code for ConstructorCallWith=Document works well now.
> I don't think dropping ConstructorCallWith=Document has benefit.

So, shall I drop this CL then? haraken?

Personally, I'm quite neutral about this. We're not really avoiding much
complexity on the script side, since the [CallWith] mechanism remains much the
same, only with option one fewer. IOW, I see no significant problem keeping the
"call with Document" option alongside "call with ExecutionContext".

[haraken, 2014-06-20 13:13:06]

On 2014/06/20 07:55:58, Jens Lindström wrote:
> On 2014/06/19 06:25:09, tkent wrote:
> > On 2014/06/19 05:53:44, haraken wrote:
> > > Thanks tkent-san, then the patch set 1 would make more sense.
> > > 
> > > - It won't affect non-binding code.
> > > - It improves binding code in a sense that we can remove
> > > [ConstructorCallWith=Document].
> > > 
> > > Although the patch set 1 adds a couple of methods to the C++ side, it's
> common
> > > that we simplify binding code by adding some code into the C++ side. What
do
> > you
> > > think?
> > 
> > Patch Set 1 looks also bad.  I don't want to have new factory functions
though
> > auto-generated code for ConstructorCallWith=Document works well now.
> > I don't think dropping ConstructorCallWith=Document has benefit.
> 
> So, shall I drop this CL then? haraken?
> 
> Personally, I'm quite neutral about this. We're not really avoiding much
> complexity on the script side, since the [CallWith] mechanism remains much the
> same, only with option one fewer. IOW, I see no significant problem keeping
the
> "call with Document" option alongside "call with ExecutionContext".

Yeah, there is no strong reason we have to drop [ConstructorCallWith=Document],
so it looks better to just keep it. I'm sorry about wasting your time. The
redundant toV8() will be worth dropping though.

[Jens Widell, 2014-06-20 13:15:50]

On 2014/06/20 13:13:06, haraken wrote:
> The redundant toV8() will be worth dropping though.

Yes, I will upload a CL doing that.