Dartium changes for custom element lifecycle events.

Dartium changes for custom element lifecycle events.

These changes bring us to parity with the current behavior but using the proper custom element infrastructure.

Next steps are to hook up the rest of the lifecycle events and transition to the constructor for created.

BUG=
R=vsm@google.com

Committed: https://src.chromium.org/viewvc/multivm?view=rev&revision=1383

[vsm, 2013-09-19 23:12:26]

nice!  lgtm with comments

Note: asiva refactored the create code to split out lookup and changed the
previous custom element creation accordingly.

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementBinding.h (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementBinding.h:45: ~DartCustomElementBinding()
{ }
Don't think you need an explicit destructor here.

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementConstructorBuilder.cpp (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementConstructorBuilder.cpp:79: // if
(!Document::isValidName(localName)) {
Should this be commented out?  Looks like reasonable validation.

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementLifecycleCallbacks.cpp (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementLifecycleCallbacks.cpp:108: //   
call(m_enteredDocument, element);
Can you either delete or add FIXMEs to commented out code in this file?

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementLifecycleCallbacks.h (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementLifecycleCallbacks.h:45: class
CustomElementDefinition;
nit: alpha ordering

[blois, 2013-09-20 00:10:25]

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementBinding.h (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementBinding.h:45: ~DartCustomElementBinding()
{ }
On 2013/09/19 23:12:26, vsm wrote:
> Don't think you need an explicit destructor here.

Done.

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementConstructorBuilder.cpp (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementConstructorBuilder.cpp:79: // if
(!Document::isValidName(localName)) {
On 2013/09/19 23:12:26, vsm wrote:
> Should this be commented out?  Looks like reasonable validation.

Sorry, had added a comment which I forgot to upload.

This is for once we upgrade to the latest Blink code- it's not available in the
current version.

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementLifecycleCallbacks.cpp (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementLifecycleCallbacks.cpp:108: //   
call(m_enteredDocument, element);
On 2013/09/19 23:12:26, vsm wrote:
> Can you either delete or add FIXMEs to commented out code in this file?

Done.

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
File Source/bindings/dart/DartCustomElementLifecycleCallbacks.h (right):

https://chromiumcodereview.appspot.com/24294002/diff/1/Source/bindings/dart/D...
Source/bindings/dart/DartCustomElementLifecycleCallbacks.h:45: class
CustomElementDefinition;
On 2013/09/19 23:12:26, vsm wrote:
> nit: alpha ordering

Done.

[blois, 2013-09-20 18:30:24]

Committed patchset #4 manually as r1383.

[siva, 2013-09-20 20:57:36]

DBC

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartCustomElementBinding.h (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
Source/bindings/dart/DartCustomElementBinding.h:45: Dart_Handle customType() {
return Dart_HandleFromPersistent(m_customType); }
I would really like to avoid such implicit conversion of handles from Persistent
to regular handles. We should make explicit the places were
this conversion is necessary and do it there. For instance when
returning values back to dart it is not necessary to convert a PersistentHandle.

This method could be
Dart_PersistentHandle customType() { return m_customType; }

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartCustomElementConstructorBuilder.h (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
Source/bindings/dart/DartCustomElementConstructorBuilder.h:77: Dart_Handle
m_customType;
How do we ensure that this handle will not go out of scope before
DartCustomElementConstructorBuilder is destroyed?

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartUtilities.cpp (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
Source/bindings/dart/DartUtilities.cpp:386: ASSERT(!Dart_IsError(other));
Is this typeName one of the predefined 5000 types?

If it is then DartDOMWrapper::getClass(libraryName, typeName, cid)
is the more efficient version to use.

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/cust...
File Source/bindings/dart/custom/DartDocumentCustom.cpp (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/cust...
Source/bindings/dart/custom/DartDocumentCustom.cpp:91: Dart_SetReturnValue(args,
DartElement::toDart(result));
This is an inefficient way to return stuff, you should use
DartElement::returnToDart(args, result);

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/cust...
Source/bindings/dart/custom/DartDocumentCustom.cpp:135:
Dart_SetReturnValue(args, DartElement::toDart(result));
Ditto.

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/gyp/...
File Source/bindings/dart/gyp/scripts/massage_factories.py (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/gyp/...
Source/bindings/dart/gyp/scripts/massage_factories.py:98: r'return
DartHTMLUnknownElement::toDart(toHTMLUnknownElement(element));'),
if you are creating a new wrapper you should be using createWrapper not toDart.

[blois, 2013-09-20 22:18:03]

Addressed in https://codereview.chromium.org/23513085/

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartCustomElementBinding.h (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
Source/bindings/dart/DartCustomElementBinding.h:45: Dart_Handle customType() {
return Dart_HandleFromPersistent(m_customType); }
On 2013/09/20 20:57:37, siva wrote:
> I would really like to avoid such implicit conversion of handles from
Persistent
> to regular handles. We should make explicit the places were
> this conversion is necessary and do it there. For instance when
> returning values back to dart it is not necessary to convert a
PersistentHandle.
> 
> This method could be
> Dart_PersistentHandle customType() { return m_customType; }

Done.

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartCustomElementConstructorBuilder.h (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/Dart...
Source/bindings/dart/DartCustomElementConstructorBuilder.h:77: Dart_Handle
m_customType;
On 2013/09/20 20:57:37, siva wrote:
> How do we ensure that this handle will not go out of scope before
> DartCustomElementConstructorBuilder is destroyed?

This class is only used as a stack variable from DartNativeUtilities.cpp.

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/cust...
File Source/bindings/dart/custom/DartDocumentCustom.cpp (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/cust...
Source/bindings/dart/custom/DartDocumentCustom.cpp:91: Dart_SetReturnValue(args,
DartElement::toDart(result));
On 2013/09/20 20:57:37, siva wrote:
> This is an inefficient way to return stuff, you should use
> DartElement::returnToDart(args, result);

Done.

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/cust...
Source/bindings/dart/custom/DartDocumentCustom.cpp:135:
Dart_SetReturnValue(args, DartElement::toDart(result));
On 2013/09/20 20:57:37, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/gyp/...
File Source/bindings/dart/gyp/scripts/massage_factories.py (right):

https://codereview.chromium.org/24294002/diff/13001/Source/bindings/dart/gyp/...
Source/bindings/dart/gyp/scripts/massage_factories.py:98: r'return
DartHTMLUnknownElement::toDart(toHTMLUnknownElement(element));'),
On 2013/09/20 20:57:37, siva wrote:
> if you are creating a new wrapper you should be using createWrapper not
toDart.

Done.