Implement HTMLMarqueeElement's animation in private scripts

Implement HTMLMarqueeElement's animation in private scripts

This CL completely migrates HTMLMarqueeElement's implementation to Blink-in-JS. This enables us to remove RenderMarquee.cpp and most of HTMLMarqueeElement.cpp.

The only regression this migration may cause is a case where HTMLMarqueeElement's attributes are updated during an animation. If you update 'behavior', 'direction', 'loop', 'scrollAmount', 'scrollDelay' or 'trueSpeed' during the animation, the animation is expected to be continued with the new attribute setting. However, in this CL, the animation is reset with the new attribute setting. I'm not sure if this regression causes any real issue (I hope not). I think we can land this and see how it goes.

This CL slightly changes the rendering of the HTMLMarqueeElement. I updated the following layout tests accordingly.

[fast/block/float/marquee-shrink-to-avoid-floats.html]
The old rendering is wrong, and the new rendering is correct. The old rendering matches WebKit. The new rendering matches the spec, IE and Firefox.

[fast/css/MarqueeLayoutTest.html]
The old rendering is wrong, and the new rendering is correct. The old rendering matches WebKit and IE. The new rendering matches the spec and Firefox.

[html/marquee-scroll.html]
Blink-in-JS uses Web animation APIs and it renders an animation more smoothly than the previous C++ implementation. As a result, the animation moves more quickly than before and thus makes the image result undeterministic. Thus I changed the color of characters displayed in the marquee element equal to the background color of the marquee element, just to make the image result timing-independent.

[fast/inline-block/003.html]
Ditto.

[http/tests/activedomobject/marquee.html]
Now that the HTMLMarqueeElement is not an ActiveDOMObject. Thus I changed the number of ActiveDOMObjects in the test.

BUG=341031
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183245

[haraken, 2014-07-15 12:57:07]

The CL is not yet for review.

https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
File Source/core/html/HTMLMarqueeElement.js (left):

https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
Source/core/html/HTMLMarqueeElement.js:123:
defineInlineEventHandler(HTMLMarqueeElementPrototype, 'bounce');

I removed these event handlers, since we haven't yet supported these event
handlers in Blink.

https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
Source/core/html/HTMLMarqueeElement.js:95:
HTMLMarqueeElementPrototype.createdCallback = function() {

abarth@: When and how are you assuming that these callbacks are called back? In
my understanding:

- createCallback should be called from C++ when the marquee element is created.

- detachedCallback should be called from C++ when ActiveDOMObject is suspended.

- attachedCallback should be called from C++ when ActiveDOMObject is resumed.

- attributeChangedCallback should be called from C++ when
RenderMarquee::styleDidChange is called.

- We'll be able to get rid of almost all implementations from
HTMLMarqueeElement.cpp and RenderMarquee.cpp, but a couple of code will stay
there to invoke those callbacks in private scripts.

Am I understanding correctly?

[arv (Not doing code reviews), 2014-07-15 15:01:10]

On 2014/07/15 at 12:57:07, haraken wrote:
> The CL is not yet for review.
> 
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> File Source/core/html/HTMLMarqueeElement.js (left):
> 
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> Source/core/html/HTMLMarqueeElement.js:123:
defineInlineEventHandler(HTMLMarqueeElementPrototype, 'bounce');
> 
> I removed these event handlers, since we haven't yet supported these event
handlers in Blink.
> 
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> File Source/core/html/HTMLMarqueeElement.js (right):
> 
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> Source/core/html/HTMLMarqueeElement.js:95:
HTMLMarqueeElementPrototype.createdCallback = function() {
> 
> abarth@: When and how are you assuming that these callbacks are called back?
In my understanding:
> 
> - createCallback should be called from C++ when the marquee element is
created.

Asyncish - This is queued using an "element queue" -
http://w3c.github.io/webcomponents/spec/custom/#custom-element-lifecycle

> 
> - detachedCallback should be called from C++ when ActiveDOMObject is
suspended.

I don't see how this is related to ActiveDOMObjects? This is called when the
node is removed from document.

> 
> - attachedCallback should be called from C++ when ActiveDOMObject is resumed.

And this when added to a document. In both cases the document has to have a
view.

http://w3c.github.io/webcomponents/spec/custom/#types-of-callbacks
> 
> - attributeChangedCallback should be called from C++ when
RenderMarquee::styleDidChange is called.

Why styleDdiChange? This is usually called when any attribute is changed.

> - We'll be able to get rid of almost all implementations from
HTMLMarqueeElement.cpp and RenderMarquee.cpp, but a couple of code will stay
there to invoke those callbacks in private scripts.
> 
> Am I understanding correctly?

The spec for custom elements outlines the timing and order of these and it is
designed to solve a lot of use cases. Hopefully we can reuse the CE
implementation?

[haraken, 2014-07-15 15:24:11]

On 2014/07/15 15:01:10, arv wrote:
> On 2014/07/15 at 12:57:07, haraken wrote:
> > The CL is not yet for review.
> > 
> >
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> > File Source/core/html/HTMLMarqueeElement.js (left):
> > 
> >
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> > Source/core/html/HTMLMarqueeElement.js:123:
> defineInlineEventHandler(HTMLMarqueeElementPrototype, 'bounce');
> > 
> > I removed these event handlers, since we haven't yet supported these event
> handlers in Blink.
> > 
> >
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> > File Source/core/html/HTMLMarqueeElement.js (right):
> > 
> >
>
https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
> > Source/core/html/HTMLMarqueeElement.js:95:
> HTMLMarqueeElementPrototype.createdCallback = function() {
> > 
> > abarth@: When and how are you assuming that these callbacks are called back?
> In my understanding:
> > 
> > - createCallback should be called from C++ when the marquee element is
> created.
> 
> Asyncish - This is queued using an "element queue" -
> http://w3c.github.io/webcomponents/spec/custom/#custom-element-lifecycle
> 
> > 
> > - detachedCallback should be called from C++ when ActiveDOMObject is
> suspended.
> 
> I don't see how this is related to ActiveDOMObjects? This is called when the
> node is removed from document.
> 
> > 
> > - attachedCallback should be called from C++ when ActiveDOMObject is
resumed.
> 
> And this when added to a document. In both cases the document has to have a
> view.
> 
> http://w3c.github.io/webcomponents/spec/custom/#types-of-callbacks
> > 
> > - attributeChangedCallback should be called from C++ when
> RenderMarquee::styleDidChange is called.
> 
> Why styleDdiChange? This is usually called when any attribute is changed.
> 
> > - We'll be able to get rid of almost all implementations from
> HTMLMarqueeElement.cpp and RenderMarquee.cpp, but a couple of code will stay
> there to invoke those callbacks in private scripts.
> > 
> > Am I understanding correctly?
> 
> The spec for custom elements outlines the timing and order of these and it is
> designed to solve a lot of use cases. Hopefully we can reuse the CE
> implementation?

Oh, it looks like I didn't understand the lifecycle of the custom elements :) I
agree that we can just ride on the custom element.

One thing I don't yet understand is how to handle
ActiveDOMObject::suspend/resume. We need to stop the marqee animation when the
ExecutionContext is suspended and restart the animation when the
ExecutionContext is resumed. Is there any easy way to call
this.start()/this.stop() when the ExecutionContext is suspended/resumed?

[abarth-chromium, 2014-07-15 17:10:07]

https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/1/Source/core/html/HTMLMarquee...
Source/core/html/HTMLMarqueeElement.js:95:
HTMLMarqueeElementPrototype.createdCallback = function() {
> createCallback / detachedCallback / attachedCallback /
attributeChangedCallback

These callbacks are all defined by
http://w3c.github.io/webcomponents/spec/custom/.  You shouldn't have to write
any code to trigger these callbacks.  All the machinery should already exist. 
Take a look at V8CustomElementLifecycleCallbacks, which knows how to call the
callbacks.  If you run the <i-marquee> script in a normal page, you can set some
breakpoints in V8CustomElementLifecycleCallbacks to see how the callbacks get
called.

The machinery for calling these callbacks seems to already understand isolated
worlds.  We just need to create them in the proper v8::Context, presumably
around the time the HTMLMarqueeElement is created by the HTMLElementFactory.

> We'll be able to get rid of almost all implementations from
HTMLMarqueeElement.cpp and RenderMarquee.cpp

We'll be able to delete RenderMarquee entirely.  I think we can delete almost
all of HTMLMarqueeElement.  We'll need to keep just enough to register the
custom script version as a custom element.

[abarth-chromium, 2014-07-15 17:11:07]

On 2014/07/15 at 15:24:11, haraken wrote:
> One thing I don't yet understand is how to handle
ActiveDOMObject::suspend/resume. We need to stop the marqee animation when the
ExecutionContext is suspended and restart the animation when the
ExecutionContext is resumed. Is there any easy way to call
this.start()/this.stop() when the ExecutionContext is suspended/resumed?

I don't think we should bother integrating with ActiveDOMObject.

[haraken, 2014-07-16 05:49:12]

I noticed that custom elements are not supported in isolated worlds.
CustomElementConstructorBuilder::isFeatureAllowed is saying that custom elements
are supported only in the main world.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

dominicc, dimitri: What's the reason we currently limit the custom element usage
to the main world? Do we have a plan to support isolated worlds as well?

[abarth-chromium, 2014-07-16 06:37:15]

On 2014/07/16 at 05:49:12, haraken wrote:
> I noticed that custom elements are not supported in isolated worlds.
CustomElementConstructorBuilder::isFeatureAllowed is saying that custom elements
are supported only in the main world.

We should try it for private scripts.

> dominicc, dimitri: What's the reason we currently limit the custom element
usage to the main world? Do we have a plan to support isolated worlds as well?

I don't think it can work for normal content scripts because the prototype chain
would cross over from the main world to the content script world.  That isn't a
problem for private scripts because we already know how to maintain two separate
prototype chains.  :)

[abarth-chromium, 2014-07-16 06:44:52]

Here's another way to think about it.  Custom elements do two things:

1) They rewire prototype chains of elements to pass through objects defined by
the author.
2) They generate callbacks at various points in the elements lifecycle.

Thing (1) is broken for isolated worlds because we don't want to wire up an
object in one world to a prototype in another.  Thankfully, we don't need to use
thing (1) here because ImplementedInPrivateScript solves the same problem in a
safe way.  (Specifically, foo.start() ends up calling the start function we
defined.)

From inspecting the code, it looks like thing (2) should work correctly for
isolated worlds, which is good because thing (2) is what we're looking for.  :)

[haraken, 2014-07-16 09:44:35]

On 2014/07/16 06:44:52, abarth wrote:
> Here's another way to think about it.  Custom elements do two things:
> 
> 1) They rewire prototype chains of elements to pass through objects defined by
> the author.
> 2) They generate callbacks at various points in the elements lifecycle.
> 
> Thing (1) is broken for isolated worlds because we don't want to wire up an
> object in one world to a prototype in another.  Thankfully, we don't need to
use
> thing (1) here because ImplementedInPrivateScript solves the same problem in a
> safe way.  (Specifically, foo.start() ends up calling the start function we
> defined.)
> 
> From inspecting the code, it looks like thing (2) should work correctly for
> isolated worlds, which is good because thing (2) is what we're looking for. 
:)

Thanks Adam! I agree that what we need is just lifecycle callbacks.

I chatted with dominicc@ offline and understand how things are organized in
custom elements. I'll try to make it workable.

[dominicc (has gone to gerrit), 2014-07-16 11:02:33]

Sorry for the slow response.

As haraken said, we chatted offline and we think this looks achievable:

- Early returns which bail out from isolated worlds will be removed. From
memory there's only a couple of these--one around registerElement (in
CustomElementConstructorBuilder?), one when allocating wrappers.
- CustomElementWrapper::wrap needs a tweak to handle when the per-context
data doesn't have a binding, which it handles just like the old early
return.
- The private script world will pass NameSet::EmbedderNames to
registerElement, which will let it register the name "marquee".
- The V8PerContextBinding - V8CustomElementCallbacks stuff looks amenable
to multiple worlds already; the callbacks already hold a ScriptState to
thunk back to the world that registered the element.
- There's an assumption that one kind of Custom Element has one set of
callbacks. You could imagine a design that permitted multiple simultaneous
registrations for the same tag name in multiple worlds. For now, we will
just go for the modest generalization that one world (possibly non-main
world) can register a Custom Element for a given name. (Scripts can't
register "marquee" anyway because of restrictions imposed by the spec.)
- The existing C++-wired up prototypes and constructor for
HTMLMarqueeElement will continue to be used for the main world.

One thing that might need some attention is the machinery will see
"marquee" and expect it to be a "marquee" element when wrapped, etc. but
the Custom Elements machinery will see something with a tag name and no
is="" and expect it to be HTMLElement when wrapped. It might be necessary
to stare at that code to make it go.

The above in total will have the happy side-effect of enabling Custom
Elements from extensions, in a limited way. There's a
Cr-Blink-WebComponents bug for that; someone reported it last week or so.

Dominic

On Wed, Jul 16, 2014 at 2:44 AM, <haraken@chromium.org> wrote:

> On 2014/07/16 06:44:52, abarth wrote:
>
>> Here's another way to think about it.  Custom elements do two things:
>>
>
>  1) They rewire prototype chains of elements to pass through objects
>> defined by
>> the author.
>> 2) They generate callbacks at various points in the elements lifecycle.
>>
>
>  Thing (1) is broken for isolated worlds because we don't want to wire up
>> an
>> object in one world to a prototype in another.  Thankfully, we don't need
>> to
>>
> use
>
>> thing (1) here because ImplementedInPrivateScript solves the same problem
>> in a
>> safe way.  (Specifically, foo.start() ends up calling the start function
>> we
>> defined.)
>>
>
>   From inspecting the code, it looks like thing (2) should work correctly
>> for
>> isolated worlds, which is good because thing (2) is what we're looking
>> for.
>>
> :)
>
> Thanks Adam! I agree that what we need is just lifecycle callbacks.
>
> I chatted with dominicc@ offline and understand how things are organized
> in
> custom elements. I'll try to make it workable.
>
>
> https://codereview.chromium.org/394773003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2014-07-16 16:28:31]

I looked at the implementation of custom elements and begin to think that we
might not want to use custom elements, for the following reasons.

- Even if we call document.registerElement("i-marquee", {prototype:
HTMLMarqueePrototype}), the lifecycle callbacks on the HTMLMarqueePrototype are
not invoked. This is because the <marquee> element we're interested in is
different from the i-marquee element we're registering. The <marquee> element is
already created as a C++ object (this is a normal HTMLMarqueeElement, not a
custom element) when we call document.registerElement. Thus, even if the
<marquee> element is attached/detached, the lifecycle callbacks on the i-marquee
element shouldn't be invoked. As a result, there is no chance where the
i-marquee element we registered comes into a play.

- One thing we could do is to forcibly mark the <marquee> element as a custom
element when created. However, this would be a bit too hacky.

- In any event, I think that what we want to do in Blink-in-JS is relatively
different from what registerElement does. As Adam mentioned above, we're not
interested in injecting a prototype chain to the element, because Blink-in-JS
cannot anyway share the prototype chain between the Blink-in-JS world and the
main world. What we want in Blink-in-JS is just to get lifecycle callbacks. In
addition, I'm not sure if created/attached/detached/attributeChanged callbacks
are enough for Blink-in-JS. We might need more lifecycle callbacks to implement
more DOM features in Blink-in-JS in the future (e.g.,
ActiveDOMObject::suspend/resume).

- Given the above, I think that it would be more straightforward to just
explicitly call the lifecycle callbacks from C++, instead of relying on custom
elements. Specifically we can write HTMLMarqueeElement as follows:

// HTMLMarqueeElement.cpp
HTMLMarqueeElement::HTMLMarqueeElement() {
  PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");
  V8HTMLMarqueeElement::createdCallback(...);  // Invoke
HTMLMarqueeElementPrototype.createdCallback
}
HTMLMarqueeElement::insertedInto() {
  V8HTMLMarqueeElement::attachedCallback(...);  // Invoke
HTMLMarqueeElementPrototype.attachedCallback
}
HTMLMarqueeElement::removedFrom() {
  V8HTMLMarqueeElement::detachedCallback(...);  // Invoke
HTMLMarqueeElementPrototype.detachedCallback
}
HTMLMarqueeElement::willModifyAttribute() {
  V8HTMLMarqueeElement::attributeChangeCallback(...);  // Invoke
HTMLMarqueeElementPrototype.attributeChangeCallback
}

What do you think?

[abarth-chromium, 2014-07-16 16:57:08]

On 2014/07/16 at 16:28:31, haraken wrote:
> I looked at the implementation of custom elements and begin to think that we
might not want to use custom elements, for the following reasons.
> 
> - Even if we call document.registerElement("i-marquee", {prototype:
HTMLMarqueePrototype}), the lifecycle callbacks on the HTMLMarqueePrototype are
not invoked.

I don't think we want to call document.registerElement.  Instead, we want to use
ImplementedInPrivateScript to forward the calls to start() from the normal JS
wrapper to the private script.  We probably need to hack into the
HTMLElementFactory or into the HTMLMarqueeElement constructor to register for
the callbacks.

> This is because the <marquee> element we're interested in is different from
the i-marquee element we're registering. The <marquee> element is already
created as a C++ object (this is a normal HTMLMarqueeElement, not a custom
element) when we call document.registerElement. Thus, even if the <marquee>
element is attached/detached, the lifecycle callbacks on the i-marquee element
shouldn't be invoked. As a result, there is no chance where the i-marquee
element we registered comes into a play.

Yes, we want to keep the C++ version of HTMLMarqueeElement.  We just need to
teach it to kick the custom element machinery at the right time during
construction to capture the callbacks from the private script and to trigger the
custom element machinery to call the callbacks at the appropriate time.

> - One thing we could do is to forcibly mark the <marquee> element as a custom
element when created. However, this would be a bit too hacky.

Hum...  That was what I had in mind...  Maybe I should try it and see how hacky
it is.

> - In any event, I think that what we want to do in Blink-in-JS is relatively
different from what registerElement does. As Adam mentioned above, we're not
interested in injecting a prototype chain to the element, because Blink-in-JS
cannot anyway share the prototype chain between the Blink-in-JS world and the
main world. What we want in Blink-in-JS is just to get lifecycle callbacks. In
addition, I'm not sure if created/attached/detached/attributeChanged callbacks
are enough for Blink-in-JS. We might need more lifecycle callbacks to implement
more DOM features in Blink-in-JS in the future (e.g.,
ActiveDOMObject::suspend/resume).

One of the goals of this work is to identify what's missing from web components
that developers might need to build custom elements that match the built-in
elements.  If we need suspend/resume for a built-in element, then authors are
going to need it too for their custom elements.  By using the same machinery, we
force ourselves to be honest and expose enough hooks to developers that they can
build the same sorts of things that we can build.

> - Given the above, I think that it would be more straightforward to just
explicitly call the lifecycle callbacks from C++, instead of relying on custom
elements. Specifically we can write HTMLMarqueeElement as follows:
> 
> // HTMLMarqueeElement.cpp
> HTMLMarqueeElement::HTMLMarqueeElement() {
>   PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");

This one I think we need.

>   V8HTMLMarqueeElement::createdCallback(...);  // Invoke
HTMLMarqueeElementPrototype.createdCallback
> }
> HTMLMarqueeElement::insertedInto() {
>   V8HTMLMarqueeElement::attachedCallback(...);  // Invoke
HTMLMarqueeElementPrototype.attachedCallback
> }
> HTMLMarqueeElement::removedFrom() {
>   V8HTMLMarqueeElement::detachedCallback(...);  // Invoke
HTMLMarqueeElementPrototype.detachedCallback
> }
> HTMLMarqueeElement::willModifyAttribute() {
>   V8HTMLMarqueeElement::attributeChangeCallback(...);  // Invoke
HTMLMarqueeElementPrototype.attributeChangeCallback
> }
> 
> What do you think?

I would like to avoid calling these manually.  We want to use the same hooks
that we make available to authors in order to demonstrate that those hooks are
sufficiently powerful that they can be used to create the built-in elements.

If you like, I can try to move this ball forward for a while.  Maybe I'll
discover that these problems are difficult to overcome and we can return to the
approach of using ad-hoc hooks.

[haraken, 2014-07-17 01:08:45]

On 2014/07/16 16:57:08, abarth wrote:
> On 2014/07/16 at 16:28:31, haraken wrote:
> > I looked at the implementation of custom elements and begin to think that we
> might not want to use custom elements, for the following reasons.
> > 
> > - Even if we call document.registerElement("i-marquee", {prototype:
> HTMLMarqueePrototype}), the lifecycle callbacks on the HTMLMarqueePrototype
are
> not invoked.
> 
> I don't think we want to call document.registerElement.  Instead, we want to
use
> ImplementedInPrivateScript to forward the calls to start() from the normal JS
> wrapper to the private script.  We probably need to hack into the
> HTMLElementFactory or into the HTMLMarqueeElement constructor to register for
> the callbacks.
> 
> > This is because the <marquee> element we're interested in is different from
> the i-marquee element we're registering. The <marquee> element is already
> created as a C++ object (this is a normal HTMLMarqueeElement, not a custom
> element) when we call document.registerElement. Thus, even if the <marquee>
> element is attached/detached, the lifecycle callbacks on the i-marquee element
> shouldn't be invoked. As a result, there is no chance where the i-marquee
> element we registered comes into a play.
> 
> Yes, we want to keep the C++ version of HTMLMarqueeElement.  We just need to
> teach it to kick the custom element machinery at the right time during
> construction to capture the callbacks from the private script and to trigger
the
> custom element machinery to call the callbacks at the appropriate time.
> 
> > - One thing we could do is to forcibly mark the <marquee> element as a
custom
> element when created. However, this would be a bit too hacky.
> 
> Hum...  That was what I had in mind...  Maybe I should try it and see how
hacky
> it is.
> 
> > - In any event, I think that what we want to do in Blink-in-JS is relatively
> different from what registerElement does. As Adam mentioned above, we're not
> interested in injecting a prototype chain to the element, because Blink-in-JS
> cannot anyway share the prototype chain between the Blink-in-JS world and the
> main world. What we want in Blink-in-JS is just to get lifecycle callbacks. In
> addition, I'm not sure if created/attached/detached/attributeChanged callbacks
> are enough for Blink-in-JS. We might need more lifecycle callbacks to
implement
> more DOM features in Blink-in-JS in the future (e.g.,
> ActiveDOMObject::suspend/resume).
> 
> One of the goals of this work is to identify what's missing from web
components
> that developers might need to build custom elements that match the built-in
> elements.  If we need suspend/resume for a built-in element, then authors are
> going to need it too for their custom elements.  By using the same machinery,
we
> force ourselves to be honest and expose enough hooks to developers that they
can
> build the same sorts of things that we can build.
> 
> > - Given the above, I think that it would be more straightforward to just
> explicitly call the lifecycle callbacks from C++, instead of relying on custom
> elements. Specifically we can write HTMLMarqueeElement as follows:
> > 
> > // HTMLMarqueeElement.cpp
> > HTMLMarqueeElement::HTMLMarqueeElement() {
> >   PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");
> 
> This one I think we need.
> 
> >   V8HTMLMarqueeElement::createdCallback(...);  // Invoke
> HTMLMarqueeElementPrototype.createdCallback
> > }
> > HTMLMarqueeElement::insertedInto() {
> >   V8HTMLMarqueeElement::attachedCallback(...);  // Invoke
> HTMLMarqueeElementPrototype.attachedCallback
> > }
> > HTMLMarqueeElement::removedFrom() {
> >   V8HTMLMarqueeElement::detachedCallback(...);  // Invoke
> HTMLMarqueeElementPrototype.detachedCallback
> > }
> > HTMLMarqueeElement::willModifyAttribute() {
> >   V8HTMLMarqueeElement::attributeChangeCallback(...);  // Invoke
> HTMLMarqueeElementPrototype.attributeChangeCallback
> > }
> > 
> > What do you think?
> 
> I would like to avoid calling these manually.  We want to use the same hooks
> that we make available to authors in order to demonstrate that those hooks are
> sufficiently powerful that they can be used to create the built-in elements.

Sounds reasonable.

Then the updated approach would be:

// HTMLMarqueeElement.js
installClass("HTMLMarqueeElement", function(global) {
  var HTMLMarqueeElementPrototype = {};
  HTMLMarqueeElementPrototype.createdCallback = function () { ... };
  HTMLMarqueeElementPrototype.attachedCallback = function () { ... };
  HTMLMarqueeElementPrototype.detachedCallback = function () { ... };
  HTMLMarqueeElementPrototype.attributeChangedCallback = function () { ... };

  ...;  // We don't call registerElement.

  return HTMLMarqueeElementPrototype;
});

// HTMLMarqueeElement.cpp
HTMLMarqueeElement::HTMLMarqueeElement() {
  v8::Handle<v8::Value> classObject =
PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");  // classObject
=== HTMLMarqueeElementPrototype
  PrivateScriptRunner::registerCustomElementLifecycleCallbacks(this,
classObject);  // Probably it's too late to register createdCallback. I'll see
how to work around it.
}

Does this look fine?

[dominicc (has gone to gerrit), 2014-07-17 01:59:35]

Just chatted to haraken, and this morning I separately synced up briefly
with Adam and Dimitri.

Here's what I think we should do; comments welcome.

haraken pursues the private script stuff by overriding HTMLMarqueeElement
virtuals and not integrating with Custom Elements. Thunking to private
scripts is useful for other things like XSLT, and ultimately getting rid of
HTMLMarqueeElement.cpp, so that is progress.

In parallel, I'll work with whomever (Kochi-san?) to generalize Custom
Elements machinery to support this case.

We will know we have achieved "the author could implement marquee in
JavaScript" when the overrides in HTMLMarqueeElement are gone.
(HTMLMarqueeElement.cpp may remain because it provides a convenient
initialization hook.)

There should be concurrent activity in making Shadow DOM also work for this
use case. (Perhaps it already works perfectly?)

Details:

Here's how I would frame this problem from the Custom Elements point of
view.

The "hard part" is not cross-isolate stuff, I think the current
implementation generalizes pretty readily to that.

The "hard part" is making <marquee is="x-foo"> continue to behave the way
it does today. The reason it is complicated is:

- Custom Elements has no notion of extending in this way. If you write
<x-foo is="x-bar"> for example, we ignore the x-bar. But if you squint at
marquee as a Custom Element, then that has to work.

- It is preferable to provide the marquee definition late. So when the
author registers x-bar extends: marquee, Custom Elements may not have seen
the definition of marquee.

I think making this work will lead us inevitably down the following path,
it is just a question of where we want to hop on the path and how far we go:

0. Don't integrate with Custom Elements, just get the private script stuff
up; invoke it by overriding the C++ HTMLElement, etc. virtuals.

1. Crawl: Use Custom Element callbacks but nothing else.

2. Walk: Make Custom Element definitions work in a tiered way. There's a
set of "built in" ones.

3. Run: "Custom Elements V2" generalizes Custom Elements and enables
extending one Custom Element definition with another. (If you write <x-foo
is="x-bar"> today, we ignore the x-bar.)

I think there are roughly two approaches to #1 and #2:

A. Make having a second "built in" definition for an element a first-class
concept. This is probably very expedient from an implementation point of
view.

B. Make having "extended" definitions a first-class concept. I think this
is what we have to arrive at for #3.

On Thu, Jul 17, 2014 at 10:08 AM, <haraken@chromium.org> wrote:

> On 2014/07/16 16:57:08, abarth wrote:
>
>> On 2014/07/16 at 16:28:31, haraken wrote:
>> > I looked at the implementation of custom elements and begin to think
>> that we
>> might not want to use custom elements, for the following reasons.
>> >
>> > - Even if we call document.registerElement("i-marquee", {prototype:
>> HTMLMarqueePrototype}), the lifecycle callbacks on the
>> HTMLMarqueePrototype
>>
> are
>
>> not invoked.
>>
>
>  I don't think we want to call document.registerElement.  Instead, we want
>> to
>>
> use
>
>> ImplementedInPrivateScript to forward the calls to start() from the
>> normal JS
>> wrapper to the private script.  We probably need to hack into the
>> HTMLElementFactory or into the HTMLMarqueeElement constructor to register
>> for
>> the callbacks.
>>
>
>  > This is because the <marquee> element we're interested in is different
>> from
>> the i-marquee element we're registering. The <marquee> element is already
>> created as a C++ object (this is a normal HTMLMarqueeElement, not a custom
>> element) when we call document.registerElement. Thus, even if the
>> <marquee>
>> element is attached/detached, the lifecycle callbacks on the i-marquee
>> element
>> shouldn't be invoked. As a result, there is no chance where the i-marquee
>> element we registered comes into a play.
>>
>
>  Yes, we want to keep the C++ version of HTMLMarqueeElement.  We just need
>> to
>> teach it to kick the custom element machinery at the right time during
>> construction to capture the callbacks from the private script and to
>> trigger
>>
> the
>
>> custom element machinery to call the callbacks at the appropriate time.
>>
>
>  > - One thing we could do is to forcibly mark the <marquee> element as a
>>
> custom
>
>> element when created. However, this would be a bit too hacky.
>>
>
>  Hum...  That was what I had in mind...  Maybe I should try it and see how
>>
> hacky
>
>> it is.
>>
>
>  > - In any event, I think that what we want to do in Blink-in-JS is
>> relatively
>> different from what registerElement does. As Adam mentioned above, we're
>> not
>> interested in injecting a prototype chain to the element, because
>> Blink-in-JS
>> cannot anyway share the prototype chain between the Blink-in-JS world and
>> the
>> main world. What we want in Blink-in-JS is just to get lifecycle
>> callbacks. In
>> addition, I'm not sure if created/attached/detached/attributeChanged
>> callbacks
>> are enough for Blink-in-JS. We might need more lifecycle callbacks to
>>
> implement
>
>> more DOM features in Blink-in-JS in the future (e.g.,
>> ActiveDOMObject::suspend/resume).
>>
>
>  One of the goals of this work is to identify what's missing from web
>>
> components
>
>> that developers might need to build custom elements that match the
>> built-in
>> elements.  If we need suspend/resume for a built-in element, then authors
>> are
>> going to need it too for their custom elements.  By using the same
>> machinery,
>>
> we
>
>> force ourselves to be honest and expose enough hooks to developers that
>> they
>>
> can
>
>> build the same sorts of things that we can build.
>>
>
>  > - Given the above, I think that it would be more straightforward to just
>> explicitly call the lifecycle callbacks from C++, instead of relying on
>> custom
>> elements. Specifically we can write HTMLMarqueeElement as follows:
>> >
>> > // HTMLMarqueeElement.cpp
>> > HTMLMarqueeElement::HTMLMarqueeElement() {
>> >   PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");
>>
>
>  This one I think we need.
>>
>
>  >   V8HTMLMarqueeElement::createdCallback(...);  // Invoke
>> HTMLMarqueeElementPrototype.createdCallback
>> > }
>> > HTMLMarqueeElement::insertedInto() {
>> >   V8HTMLMarqueeElement::attachedCallback(...);  // Invoke
>> HTMLMarqueeElementPrototype.attachedCallback
>> > }
>> > HTMLMarqueeElement::removedFrom() {
>> >   V8HTMLMarqueeElement::detachedCallback(...);  // Invoke
>> HTMLMarqueeElementPrototype.detachedCallback
>> > }
>> > HTMLMarqueeElement::willModifyAttribute() {
>> >   V8HTMLMarqueeElement::attributeChangeCallback(...);  // Invoke
>> HTMLMarqueeElementPrototype.attributeChangeCallback
>> > }
>> >
>> > What do you think?
>>
>
>  I would like to avoid calling these manually.  We want to use the same
>> hooks
>> that we make available to authors in order to demonstrate that those
>> hooks are
>> sufficiently powerful that they can be used to create the built-in
>> elements.
>>
>
> Sounds reasonable.
>
> Then the updated approach would be:
>
> // HTMLMarqueeElement.js
> installClass("HTMLMarqueeElement", function(global) {
>   var HTMLMarqueeElementPrototype = {};
>   HTMLMarqueeElementPrototype.createdCallback = function () { ... };
>   HTMLMarqueeElementPrototype.attachedCallback = function () { ... };
>   HTMLMarqueeElementPrototype.detachedCallback = function () { ... };
>   HTMLMarqueeElementPrototype.attributeChangedCallback = function () {
> ... };
>
>   ...;  // We don't call registerElement.
>
>   return HTMLMarqueeElementPrototype;
> });
>
> // HTMLMarqueeElement.cpp
> HTMLMarqueeElement::HTMLMarqueeElement() {
>   v8::Handle<v8::Value> classObject =
> PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");  //
> classObject
> === HTMLMarqueeElementPrototype
>   PrivateScriptRunner::registerCustomElementLifecycleCallbacks(this,
> classObject);  // Probably it's too late to register createdCallback. I'll
> see
> how to work around it.
> }
>
> Does this look fine?
>
>
> https://codereview.chromium.org/394773003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dominicc (has gone to gerrit), 2014-07-17 06:57:06]

Update: I talked to Kochi and he huddled with Takuya and Hayato and they
have decided not to work on this.

It would be nice to use this opportunity to ramp up a second person on
Custom Elements; if someone has a candidate let me know. Otherwise I think
some subset of {haraken, adam, me} can do it.

Dominic

On Thu, Jul 17, 2014 at 10:59 AM, Dominic Cooney <dominicc@chromium.org>
wrote:

> Just chatted to haraken, and this morning I separately synced up briefly
> with Adam and Dimitri.
>
> Here's what I think we should do; comments welcome.
>
> haraken pursues the private script stuff by overriding HTMLMarqueeElement
> virtuals and not integrating with Custom Elements. Thunking to private
> scripts is useful for other things like XSLT, and ultimately getting rid of
> HTMLMarqueeElement.cpp, so that is progress.
>
> In parallel, I'll work with whomever (Kochi-san?) to generalize Custom
> Elements machinery to support this case.
>
> We will know we have achieved "the author could implement marquee in
> JavaScript" when the overrides in HTMLMarqueeElement are gone.
> (HTMLMarqueeElement.cpp may remain because it provides a convenient
> initialization hook.)
>
> There should be concurrent activity in making Shadow DOM also work for
> this use case. (Perhaps it already works perfectly?)
>
> Details:
>
> Here's how I would frame this problem from the Custom Elements point of
> view.
>
> The "hard part" is not cross-isolate stuff, I think the current
> implementation generalizes pretty readily to that.
>
> The "hard part" is making <marquee is="x-foo"> continue to behave the way
> it does today. The reason it is complicated is:
>
> - Custom Elements has no notion of extending in this way. If you write
> <x-foo is="x-bar"> for example, we ignore the x-bar. But if you squint at
> marquee as a Custom Element, then that has to work.
>
> - It is preferable to provide the marquee definition late. So when the
> author registers x-bar extends: marquee, Custom Elements may not have seen
> the definition of marquee.
>
> I think making this work will lead us inevitably down the following path,
> it is just a question of where we want to hop on the path and how far we go:
>
> 0. Don't integrate with Custom Elements, just get the private script stuff
> up; invoke it by overriding the C++ HTMLElement, etc. virtuals.
>
> 1. Crawl: Use Custom Element callbacks but nothing else.
>
> 2. Walk: Make Custom Element definitions work in a tiered way. There's a
> set of "built in" ones.
>
> 3. Run: "Custom Elements V2" generalizes Custom Elements and enables
> extending one Custom Element definition with another. (If you write <x-foo
> is="x-bar"> today, we ignore the x-bar.)
>
> I think there are roughly two approaches to #1 and #2:
>
> A. Make having a second "built in" definition for an element a first-class
> concept. This is probably very expedient from an implementation point of
> view.
>
> B. Make having "extended" definitions a first-class concept. I think this
> is what we have to arrive at for #3.
>
> On Thu, Jul 17, 2014 at 10:08 AM, <haraken@chromium.org> wrote:
>
>> On 2014/07/16 16:57:08, abarth wrote:
>>
>>> On 2014/07/16 at 16:28:31, haraken wrote:
>>> > I looked at the implementation of custom elements and begin to think
>>> that we
>>> might not want to use custom elements, for the following reasons.
>>> >
>>> > - Even if we call document.registerElement("i-marquee", {prototype:
>>> HTMLMarqueePrototype}), the lifecycle callbacks on the
>>> HTMLMarqueePrototype
>>>
>> are
>>
>>> not invoked.
>>>
>>
>>  I don't think we want to call document.registerElement.  Instead, we
>>> want to
>>>
>> use
>>
>>> ImplementedInPrivateScript to forward the calls to start() from the
>>> normal JS
>>> wrapper to the private script.  We probably need to hack into the
>>> HTMLElementFactory or into the HTMLMarqueeElement constructor to
>>> register for
>>> the callbacks.
>>>
>>
>>  > This is because the <marquee> element we're interested in is different
>>> from
>>> the i-marquee element we're registering. The <marquee> element is already
>>> created as a C++ object (this is a normal HTMLMarqueeElement, not a
>>> custom
>>> element) when we call document.registerElement. Thus, even if the
>>> <marquee>
>>> element is attached/detached, the lifecycle callbacks on the i-marquee
>>> element
>>> shouldn't be invoked. As a result, there is no chance where the i-marquee
>>> element we registered comes into a play.
>>>
>>
>>  Yes, we want to keep the C++ version of HTMLMarqueeElement.  We just
>>> need to
>>> teach it to kick the custom element machinery at the right time during
>>> construction to capture the callbacks from the private script and to
>>> trigger
>>>
>> the
>>
>>> custom element machinery to call the callbacks at the appropriate time.
>>>
>>
>>  > - One thing we could do is to forcibly mark the <marquee> element as a
>>>
>> custom
>>
>>> element when created. However, this would be a bit too hacky.
>>>
>>
>>  Hum...  That was what I had in mind...  Maybe I should try it and see how
>>>
>> hacky
>>
>>> it is.
>>>
>>
>>  > - In any event, I think that what we want to do in Blink-in-JS is
>>> relatively
>>> different from what registerElement does. As Adam mentioned above, we're
>>> not
>>> interested in injecting a prototype chain to the element, because
>>> Blink-in-JS
>>> cannot anyway share the prototype chain between the Blink-in-JS world
>>> and the
>>> main world. What we want in Blink-in-JS is just to get lifecycle
>>> callbacks. In
>>> addition, I'm not sure if created/attached/detached/attributeChanged
>>> callbacks
>>> are enough for Blink-in-JS. We might need more lifecycle callbacks to
>>>
>> implement
>>
>>> more DOM features in Blink-in-JS in the future (e.g.,
>>> ActiveDOMObject::suspend/resume).
>>>
>>
>>  One of the goals of this work is to identify what's missing from web
>>>
>> components
>>
>>> that developers might need to build custom elements that match the
>>> built-in
>>> elements.  If we need suspend/resume for a built-in element, then
>>> authors are
>>> going to need it too for their custom elements.  By using the same
>>> machinery,
>>>
>> we
>>
>>> force ourselves to be honest and expose enough hooks to developers that
>>> they
>>>
>> can
>>
>>> build the same sorts of things that we can build.
>>>
>>
>>  > - Given the above, I think that it would be more straightforward to
>>> just
>>> explicitly call the lifecycle callbacks from C++, instead of relying on
>>> custom
>>> elements. Specifically we can write HTMLMarqueeElement as follows:
>>> >
>>> > // HTMLMarqueeElement.cpp
>>> > HTMLMarqueeElement::HTMLMarqueeElement() {
>>> >   PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");
>>>
>>
>>  This one I think we need.
>>>
>>
>>  >   V8HTMLMarqueeElement::createdCallback(...);  // Invoke
>>> HTMLMarqueeElementPrototype.createdCallback
>>> > }
>>> > HTMLMarqueeElement::insertedInto() {
>>> >   V8HTMLMarqueeElement::attachedCallback(...);  // Invoke
>>> HTMLMarqueeElementPrototype.attachedCallback
>>> > }
>>> > HTMLMarqueeElement::removedFrom() {
>>> >   V8HTMLMarqueeElement::detachedCallback(...);  // Invoke
>>> HTMLMarqueeElementPrototype.detachedCallback
>>> > }
>>> > HTMLMarqueeElement::willModifyAttribute() {
>>> >   V8HTMLMarqueeElement::attributeChangeCallback(...);  // Invoke
>>> HTMLMarqueeElementPrototype.attributeChangeCallback
>>> > }
>>> >
>>> > What do you think?
>>>
>>
>>  I would like to avoid calling these manually.  We want to use the same
>>> hooks
>>> that we make available to authors in order to demonstrate that those
>>> hooks are
>>> sufficiently powerful that they can be used to create the built-in
>>> elements.
>>>
>>
>> Sounds reasonable.
>>
>> Then the updated approach would be:
>>
>> // HTMLMarqueeElement.js
>> installClass("HTMLMarqueeElement", function(global) {
>>   var HTMLMarqueeElementPrototype = {};
>>   HTMLMarqueeElementPrototype.createdCallback = function () { ... };
>>   HTMLMarqueeElementPrototype.attachedCallback = function () { ... };
>>   HTMLMarqueeElementPrototype.detachedCallback = function () { ... };
>>   HTMLMarqueeElementPrototype.attributeChangedCallback = function () {
>> ... };
>>
>>   ...;  // We don't call registerElement.
>>
>>   return HTMLMarqueeElementPrototype;
>> });
>>
>> // HTMLMarqueeElement.cpp
>> HTMLMarqueeElement::HTMLMarqueeElement() {
>>   v8::Handle<v8::Value> classObject =
>> PrivateScriptRunner::installClassIfNeeded("HTMLMarqueeElement");  //
>> classObject
>> === HTMLMarqueeElementPrototype
>>   PrivateScriptRunner::registerCustomElementLifecycleCallbacks(this,
>> classObject);  // Probably it's too late to register createdCallback.
>> I'll see
>> how to work around it.
>> }
>>
>> Does this look fine?
>>
>>
>> https://codereview.chromium.org/394773003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2014-07-17 16:02:50]

Thanks dominicc for the details. I'm on the same page with you.

> 0. Don't integrate with Custom Elements, just get the private script stuff
> up; invoke it by overriding the C++ HTMLElement, etc. virtuals.
>
> 1. Crawl: Use Custom Element callbacks but nothing else.
>
> 2. Walk: Make Custom Element definitions work in a tiered way. There's a
> set of "built in" ones.
>
> 3. Run: "Custom Elements V2" generalizes Custom Elements and enables
> extending one Custom Element definition with another. (If you write <x-foo
> is="x-bar"> today, we ignore the x-bar.)

As a starting point, I'll start with 0. Specifically, I'll implement a mechanism
to invoke Blink-in-JS from C++ (this mechanism is needed anyway for XSLT,
editing, etc) and then explicitly dispatch
created/attached/detached/attributeChangedCallback from C++.

[haraken, 2014-07-23 13:46:40]

OK. I made the HTMLMarqueeElement workable. RenderMarquee was completely gone!

I verified that http://www.fillster.com/htmlcodes/marquees.html works correctly
with this CL.

That being said, there are a couple of FIXMEs that blocks us from landing this
CL. Any advice is appreciated :)

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:115: initializeAttribute_.call(this,
kPresentationalAttributes[i]);

FIXME1: We cannot write this code as:

  this.initializeAttribute_(kPresentationalAttributes[i]);

because |this| object is not an instance of HTMLMarqueeElementPrototype.

This is very confusing. Do you have any idea to make it better?

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:151: // FIXME: Not implemented.

FIXME2: Here we need to pause the animation, reconfigure the animation
attributes, and then restart the animation. I'm afraid there is no easy way to
do that. For example, when scrollAmount is changed, we need to update
AnimationNode.duration of the ongoing animation. However, AnimationNode.duration
is a readonly attribute.

One solution would be to pause the animation, get the current position of the
animation (is it possible?), and start a new animation with the new animation
configuration. However, it seems complicated to implement.

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:350: this.player_.play();

FIXME3: .play() is behind the WebAnimationAPI flag. It's not yet available by
default.

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:389: this.player_.pause();

FIXME4: .pause() is behind the WebAnimationAPI flag. It's not yet available by
default.

[arv (Not doing code reviews), 2014-07-23 16:51:15]

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:115: initializeAttribute_.call(this,
kPresentationalAttributes[i]);
On 2014/07/23 13:46:40, haraken wrote:
> 
> FIXME1: We cannot write this code as:
> 
>   this.initializeAttribute_(kPresentationalAttributes[i]);
> 
> because |this| object is not an instance of HTMLMarqueeElementPrototype.
> 
> This is very confusing. Do you have any idea to make it better?

`this` should be the element instance which should have
HTMLMarqueeElementPrototype on its prototype chain... however, I guess this is
due to how the private script is not really using custom elements?

For clarity, I would just change initializeAttribute_ to take the instance as a
parameter, then you can get rid of the .call

initializeAttribute(this, kPresentationalAttributes[i]);

...

function initializeAttribute_(element, name) {
  var value = element.getAttribute(name);
  ...
}

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:208: function getGetMetrics_() {
no trailing underscore for local binding names. Only use trailing _ for property
names that are considered private.

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:225: var metrics =
getGetMetrics_.call(this);
Same, I would change these away from using `this`.

[haraken, 2014-07-24 01:56:44]

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:115: initializeAttribute_.call(this,
kPresentationalAttributes[i]);
On 2014/07/23 16:51:15, arv wrote:
> On 2014/07/23 13:46:40, haraken wrote:
> > 
> > FIXME1: We cannot write this code as:
> > 
> >   this.initializeAttribute_(kPresentationalAttributes[i]);
> > 
> > because |this| object is not an instance of HTMLMarqueeElementPrototype.
> > 
> > This is very confusing. Do you have any idea to make it better?
> 
> `this` should be the element instance which should have
> HTMLMarqueeElementPrototype on its prototype chain... however, I guess this is
> due to how the private script is not really using custom elements?

It's not related to custom elements. It's related to how private scripts work
with proper world isolation.

Consider the following IDL file:

interface X : Y {
  void foo();
  [ImplementedInPrivateScript] void bar();
};

Then |this| object has the following prototype chain:

  |this| --> X's prototype --> Y's prototype --> ...

|foo| and |bar| are defined on X's prototype.

Given that |bar| has [ImplementedInPrivateScript], we want to implement |bar| in
a private script. For that goal, the private script defines XPrototype that
implements |bar| and registers the XPrototype into X. (This is what
installClass() is doing.)

When this.bar() is called in user's JS, the binding layer switches the world and
call XPrototype.bar() with setting a receiver as |this|. That's why |this| is
not an instance of XPrototype while XPrototype.bar() is running.

In summary:

- X's prototype defines |foo| and |bar|.
- X's prototype redirects |foo| to C++ implementation. |foo| is defined in
X.cpp.
- X's prototype redirects |bar| to private script implementation. |foo| is
defined in XPrototype.

In this way, XPrototype is just a prototype object that provides implementation
of DOM attributes/methods, not a prototype object from which |this| object is
instantiated.

One thing we could do is to set up the prototype chain as follows:

  // In user's JS
  |this| --> X's prototype --> Y's prototype --> ...

  // In private script
  |this| --> XPrototype --> X's prototype --> Y's prototype --> ...

Then we can use this.initializeAttribute_() in a private script. However, 'this
instanceof XPrototype' will return false and it might cause other confusing
issues I'm not yet aware of.

Any idea?

[haraken, 2014-07-24 08:58:36]

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/80001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.js:115: initializeAttribute_.call(this,
kPresentationalAttributes[i]);
On 2014/07/24 01:56:44, haraken wrote:
> On 2014/07/23 16:51:15, arv wrote:
> > On 2014/07/23 13:46:40, haraken wrote:
> > > 
> > > FIXME1: We cannot write this code as:
> > > 
> > >   this.initializeAttribute_(kPresentationalAttributes[i]);
> > > 
> > > because |this| object is not an instance of HTMLMarqueeElementPrototype.
> > > 
> > > This is very confusing. Do you have any idea to make it better?
> > 
> > `this` should be the element instance which should have
> > HTMLMarqueeElementPrototype on its prototype chain... however, I guess this
is
> > due to how the private script is not really using custom elements?
> 
> It's not related to custom elements. It's related to how private scripts work
> with proper world isolation.
> 
> Consider the following IDL file:
> 
> interface X : Y {
>   void foo();
>   [ImplementedInPrivateScript] void bar();
> };
> 
> Then |this| object has the following prototype chain:
> 
>   |this| --> X's prototype --> Y's prototype --> ...
> 
> |foo| and |bar| are defined on X's prototype.
> 
> Given that |bar| has [ImplementedInPrivateScript], we want to implement |bar|
in
> a private script. For that goal, the private script defines XPrototype that
> implements |bar| and registers the XPrototype into X. (This is what
> installClass() is doing.)
> 
> When this.bar() is called in user's JS, the binding layer switches the world
and
> call XPrototype.bar() with setting a receiver as |this|. That's why |this| is
> not an instance of XPrototype while XPrototype.bar() is running.
> 
> In summary:
> 
> - X's prototype defines |foo| and |bar|.
> - X's prototype redirects |foo| to C++ implementation. |foo| is defined in
> X.cpp.
> - X's prototype redirects |bar| to private script implementation. |foo| is
> defined in XPrototype.
> 
> In this way, XPrototype is just a prototype object that provides
implementation
> of DOM attributes/methods, not a prototype object from which |this| object is
> instantiated.
> 
> One thing we could do is to set up the prototype chain as follows:
> 
>   // In user's JS
>   |this| --> X's prototype --> Y's prototype --> ...
> 
>   // In private script
>   |this| --> XPrototype --> X's prototype --> Y's prototype --> ...
> 
> Then we can use this.initializeAttribute_() in a private script. However,
'this
> instanceof XPrototype' will return false and it might cause other confusing
> issues I'm not yet aware of.
> 
> Any idea?

I tried this in the patch set 6. Now |this| object has the following prototype
chain:

  // In user's JS
  |this| --> X's prototype --> Y's prototype --> ...

  // In private script
  |this| --> XPrototype --> X's prototype --> Y's prototype --> ...

Thus we can use this.initializeAttributes_().

[arv (Not doing code reviews), 2014-07-24 16:59:37]

LGTM

https://codereview.chromium.org/394773003/diff/90001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.cpp (right):

https://codereview.chromium.org/394773003/diff/90001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.cpp:79:
ScriptForbiddenScope::AllowUserAgentScript script;
Could/should this be baked into the generated code for
V8HTMLMarqueeElement::detachedCallbackMethodImplementedInPrivateScript?

[haraken, 2014-07-25 02:30:45]

Thanks. I won't land this until the remaining FIXMEs are resolved. The remaining
ones are:

- The HTMLMarqueeElement.js needs Web Animation APIs behind a runtime flag
(pause(), play(), ispaused).

- The HTMLMarqueeElement.js does not implement behavior when
behavior/direction/loop/scrollAmount/scrollDelay/trueSpeed attributes are
changed during an animation. I'm not sure if we can implement it using Web
Animation APIs. Also I'm not sure if we really needs to support the behavior.

I'll wait for a comment from Adam at the moment.


>
https://codereview.chromium.org/394773003/diff/90001/Source/core/html/HTMLMar...
> Source/core/html/HTMLMarqueeElement.cpp:79:
> ScriptForbiddenScope::AllowUserAgentScript script;
> Could/should this be baked into the generated code for
> V8HTMLMarqueeElement::detachedCallbackMethodImplementedInPrivateScript?

Addressed this in https://codereview.chromium.org/419163004.

[hajimehoshi, 2014-08-25 04:32:59]

https://codereview.chromium.org/394773003/diff/110001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.js (right):

https://codereview.chromium.org/394773003/diff/110001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.js:85: }
Why are these duplicated definition needed?

[haraken, 2014-08-25 04:36:43]

https://codereview.chromium.org/394773003/diff/110001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.js (right):

https://codereview.chromium.org/394773003/diff/110001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.js:85: }
On 2014/08/25 04:32:59, hajimehoshi wrote:
> Why are these duplicated definition needed?

No needed... I'll remove it shortly.

[haraken, 2014-08-25 04:38:05]

https://codereview.chromium.org/394773003/diff/110001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.js (right):

https://codereview.chromium.org/394773003/diff/110001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.js:85: }
On 2014/08/25 04:36:43, haraken wrote:
> On 2014/08/25 04:32:59, hajimehoshi wrote:
> > Why are these duplicated definition needed?
> 
> No needed... I'll remove it shortly.

I noticed that the duplication is already removed in the trunk.

[haraken, 2014-09-26 07:53:06]

abarth@: Would you take a look at this?

Now that the Web animation APIs needed by marquee-in-JS are landed, we're ready
to ship marquee-in-JS.

https://codereview.chromium.org/394773003/diff/130001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/130001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.js:149: // FIXME: Not implemented.

This is the only regression this CL may cause. This CL doesn't support a case
where the animation attributes are changed dynamically.

What we need to do here is to pause the animation, reconfigure the animation
attributes, and then restart the animation. However, there is no easy way to do
that. For example, when scrollAmount is changed, we need to update
AnimationNode.duration of the ongoing animation. However, AnimationNode.duration
is a readonly attribute.

I'm not sure if not supporting the attribute update causes any real issue (I
hope not). I think we can land this and see how it goes.

[haraken, 2014-09-27 01:29:37]

abarth@: Ping on this?

I verified that the marquee-in-JS passes
http://www.fillster.com/htmlcodes/marquees.html and
http://www.quackit.com/html/codes/stop_marquee.cfm for example.

[abarth-chromium, 2014-09-28 00:52:45]

LGTM

We might want to wait on landing this until after the branch (which I think is
soon or maybe just happened).

[haraken, 2014-09-28 01:08:18]

> We might want to wait on landing this until after the branch (which I think is
> soon or maybe just happened).

Sure.

[haraken, 2014-09-29 07:24:47]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-29 07:26:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/394773003/130001

[commit-bot: I haz the power, 2014-09-29 07:41:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-29 07:41:15]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/25070)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/29212)

[arv (Not doing code reviews), 2014-09-29 16:16:39]

LGTM

Nice to get rid of this render object.

https://codereview.chromium.org/394773003/diff/150001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.js (right):

https://codereview.chromium.org/394773003/diff/150001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.js:112: for (var i = 0; i <
kPresentationalAttributes.length; ++i)
Missing {}

https://codereview.chromium.org/394773003/diff/150001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.js:346: if (this.player_ &&
this.player_.playState === "paused") {
don't mix and match quotes. Stick to '.

[haraken, 2014-09-29 16:34:52]

Thanks for review. I need to fix a couple of failing tests about marquee.

[haraken, 2014-10-02 14:30:35]

abarth@: Please take a final look.

Now all tests pass. Since this CL changes the rendreing of the marquee element,
I had to update the results of some layout tests. I described the changes in the
CL description. Overall, the new rendering is more correct than the old
rendering.

[haraken, 2014-10-02 14:32:12]

On 2014/10/02 14:30:35, haraken wrote:
> abarth@: Please take a final look.
> 
> Now all tests pass. Since this CL changes the rendreing of the marquee
element,
> I had to update the results of some layout tests. I described the changes in
the
> CL description. Overall, the new rendering is more correct than the old
> rendering.

Note: Since your last review, I've just changed a couple of lines in
HTMLMarqueeElement.js (these are minor changes) and updated test results.

[haraken, 2014-10-04 00:59:05]

arv@: abarth@ looks busy. Would you mind just taking a look at changes of layout
test results? The rationale for the changes are written in the CL description.

[Mike West, 2014-10-05 19:30:02]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-05 19:30:03]

The changes since arv@ and abarth@ last commented LGTM. Thanks for the quite
detailed CL description; it made the patch quite a bit simpler to review.

Note: I didn't look at the .png's, but the changes to the layout tests (given
the rationale in the description) look totally reasonable.

[Mike West, 2014-10-05 19:33:12]

% two C++11 nits :)

https://codereview.chromium.org/394773003/diff/310001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.h (right):

https://codereview.chromium.org/394773003/diff/310001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.h:30: class HTMLMarqueeElement FINAL :
public HTMLElement {
Nit: s/FINAL/final/

https://codereview.chromium.org/394773003/diff/310001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.h:35: virtual void attributeWillChange(const
QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue)
OVERRIDE;
Nit: s/OVERRIDE/override/ for all three methods here.

[haraken, 2014-10-06 00:08:47]

Thanks for review!

https://codereview.chromium.org/394773003/diff/310001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.h (right):

https://codereview.chromium.org/394773003/diff/310001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.h:30: class HTMLMarqueeElement FINAL :
public HTMLElement {
On 2014/10/05 19:33:12, Mike West (OOO until 6th) wrote:
> Nit: s/FINAL/final/

Done.

https://codereview.chromium.org/394773003/diff/310001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.h:35: virtual void attributeWillChange(const
QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue)
OVERRIDE;
On 2014/10/05 19:33:12, Mike West (OOO until 6th) wrote:
> Nit: s/OVERRIDE/override/ for all three methods here.

Done.

[haraken, 2014-10-06 00:08:54]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-10-06 00:10:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/394773003/330001

[commit-bot: I haz the power, 2014-10-06 05:12:37]

Committed patchset #18 (id:330001) as 183245