Implement HTMLMarqueeElement's animation in private scripts

Source/core/html/HTMLMarqueeElement.js

Index: Source/core/html/HTMLMarqueeElement.js
diff --git a/Source/core/html/HTMLMarqueeElement.js b/Source/core/html/HTMLMarqueeElement.js
index b18756327e29177580caa63a03ee0f0a09b72ccc..867dfc2ca41da7ad53368ea1910ad9d8c8943113 100644
--- a/Source/core/html/HTMLMarqueeElement.js
+++ b/Source/core/html/HTMLMarqueeElement.js
@@ -81,33 +81,7 @@ installClass('HTMLMarqueeElement', function(global) {
         });
     }
 
-    function defineInlineEventHandler(prototype, eventName) {
-        var propertyName = 'on' + eventName;
-        // FIXME: We should use symbols here instead.
-        var functionPropertyName = propertyName + 'Function_';
-        var eventHandlerPropertyName = propertyName + 'EventHandler_';
-        Object.defineProperty(prototype, propertyName, {
-            get: function() {
-                var func = this[functionPropertyName];
-                return func || null;
-            },
-            set: function(value) {
-                var oldEventHandler = this[eventHandlerPropertyName];
-                if (oldEventHandler)
-                    this.removeEventListener(eventName, oldEventHandler);
-                // Notice that we wrap |value| in an anonymous function so that the
-                // author can't call removeEventListener themselves to unregister the
-                // inline event handler.
-                var newEventHandler = value ? function() { value.apply(this, arguments) } : null;
-                if (newEventHandler)
-                    this.addEventListener(eventName, newEventHandler);
-                this[functionPropertyName] = value;
-                this[eventHandlerPropertyName] = newEventHandler;
-            },
-        });
-    }
-
-    var HTMLMarqueeElementPrototype = Object.create(HTMLElement.prototype);
+    var HTMLMarqueeElementPrototype = {};
 
     reflectAttribute(HTMLMarqueeElementPrototype, 'behavior', 'behavior');
     reflectAttribute(HTMLMarqueeElementPrototype, 'bgcolor', 'bgColor');
@@ -118,14 +92,10 @@ installClass('HTMLMarqueeElement', function(global) {
     reflectAttribute(HTMLMarqueeElementPrototype, 'width', 'width');
     reflectBooleanAttribute(HTMLMarqueeElementPrototype, 'truespeed', 'trueSpeed');
 
-    defineInlineEventHandler(HTMLMarqueeElementPrototype, 'start');
-    defineInlineEventHandler(HTMLMarqueeElementPrototype, 'finish');
-    defineInlineEventHandler(HTMLMarqueeElementPrototype, 'bounce');
-
     HTMLMarqueeElementPrototype.createdCallback = function() {
         var shadow = this.createShadowRoot();
         var style = global.document.createElement('style');
-    style.textContent = ':host { display: inline-block; width: -webkit-fill-available; overflow: hidden; text-align: initial; }' +
+        style.textContent = ':host { display: inline-block; width: -webkit-fill-available; overflow: hidden; text-align: initial; }' +
             ':host([direction="up"]), :host([direction="down"]) { height: 200px; }';
         shadow.appendChild(style);
 
@@ -138,12 +108,12 @@ installClass('HTMLMarqueeElement', function(global) {
         this.mover_ = mover;
         this.player_ = null;
         this.continueCallback_ = null;
-
-        for (var i = 0; i < kPresentationalAttributes.length; ++i)
-            this.initializeAttribute_(kPresentationalAttributes[i]);
     };
 
     HTMLMarqueeElementPrototype.attachedCallback = function() {
+        for (var i = 0; i < kPresentationalAttributes.length; ++i)
+            initializeAttribute_.call(this, kPresentationalAttributes[i]);

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

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?

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

`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);
  ...
}

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

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]

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_().

+
         this.start();
     };
 
@@ -174,18 +144,20 @@ installClass('HTMLMarqueeElement', function(global) {
             break;
         case 'behavior':
         case 'direction':
-            this.stop();
-            this.loopCount_ = 0;
-            this.start();
+        case 'loop':
+        case 'scrollAmount':
+        case 'scrollDelay':
+        case 'trueSpeed':
+            // FIXME: Not implemented.

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

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.

             break;
         }
     };
 
-    HTMLMarqueeElementPrototype.initializeAttribute_ = function(name) {
+    function initializeAttribute_(name) {
         var value = this.getAttribute(name);
         if (value === null)
             return;
-        this.attributeChangedCallback(name, null, value);
+        HTMLMarqueeElementPrototype.attributeChangedCallback.call(this, name, null, value);
     };
 
     Object.defineProperty(HTMLMarqueeElementPrototype, 'scrollAmount', {
@@ -233,7 +205,7 @@ installClass('HTMLMarqueeElement', function(global) {
         },
     });
 
-    HTMLMarqueeElementPrototype.getGetMetrics_ = function() {
+    function getGetMetrics_() {

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

no trailing underscore for local binding names. Only use trailing _ for property
names that are considered private.

         this.mover_.style.width = '-webkit-max-content';
 
         var moverStyle = global.getComputedStyle(this.mover_);
@@ -249,8 +221,8 @@ installClass('HTMLMarqueeElement', function(global) {
         return metrics;
     };
 
-    HTMLMarqueeElementPrototype.getAnimationParameters_ = function() {
-        var metrics = this.getGetMetrics_();
+    function getAnimationParameters_() {
+        var metrics = getGetMetrics_.call(this);

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

Same, I would change these away from using `this`.

 
         var totalWidth = metrics.marqueeWidth + metrics.contentWidth;
         var totalHeight = metrics.marqueeHeight + metrics.contentHeight;
@@ -267,7 +239,7 @@ installClass('HTMLMarqueeElement', function(global) {
             case kDirectionLeft:
             default:
                 parameters.transformBegin = 'translateX(' + metrics.marqueeWidth + 'px)';
-                parameters.transformEnd = 'translateX(-100%)';
+                parameters.transformEnd = 'translateX(-' + metrics.contentWidth + 'px)';
                 parameters.distance = totalWidth;
                 break;
             case kDirectionRight:
@@ -349,7 +321,14 @@ installClass('HTMLMarqueeElement', function(global) {
         return parameters
     };
 
-    HTMLMarqueeElementPrototype.shouldContinue_ = function() {
+    function animationFinished_(event) {
+        var player = event.target;
+        var marquee = player.marquee_;
+        marquee.loopCount_++;
+        marquee.start();
+    };
+
+    function shouldContinue_() {
         var loop = this.loop;
 
         // By default, slide loops only once.
@@ -361,70 +340,55 @@ installClass('HTMLMarqueeElement', function(global) {
         return this.loopCount_ < loop;
     };
 
-    HTMLMarqueeElementPrototype.continue_ = function() {
-        if (!this.shouldContinue_()) {
-            this.player_ = null;
-            this.dispatchEvent(new Event('finish', false, true));
+    function continue_() {
+        if (!shouldContinue_.call(this)) {
             return;
         }
 
-        var parameters = this.getAnimationParameters_();
+        if (this.player_ && this.player_.paused) {
+            // FIXME: This needs the WebAnimationsAPI flag enabled.
+            this.player_.play();

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

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

+            return;
+        }
 
+        var parameters = getAnimationParameters_.call(this);
+        var scrollDelay = this.scrollDelay;
+        if (scrollDelay < kMinimumScrollDelayMS && !this.trueSpeed)
+            scrollDelay = kDefaultScrollDelayMS;
         var player = this.mover_.animate([
             { transform: parameters.transformBegin },
             { transform: parameters.transformEnd },
         ], {
-            duration: parameters.distance * this.scrollDelay / this.scrollAmount,
+            duration: parameters.distance * scrollDelay / this.scrollAmount,
             fill: 'forwards',
         });
+        player.marquee_ = this;
+        player.onfinish = animationFinished_;
 
         this.player_ = player;
-
-        player.addEventListener('finish', function() {
-            if (player != this.player_)
-                return;
-            ++this.loopCount_;
-            this.continue_();
-            if (this.player_ && this.behavior === kBehaviorAlternate)
-                this.dispatchEvent(new Event('bounce', false, true));
-        }.bind(this));
     };
 
     HTMLMarqueeElementPrototype.start = function() {
-        if (this.continueCallback_ || this.player_)
+        if (this.continueCallback_)
             return;
         this.continueCallback_ = global.requestAnimationFrame(function() {
             this.continueCallback_ = null;
-            this.continue_();
+            continue_.call(this);
         }.bind(this));
-        this.dispatchEvent(new Event('start', false, true));
     };
 
     HTMLMarqueeElementPrototype.stop = function() {
-        if (!this.continueCallback_ && !this.player_)
-            return;
-
         if (this.continueCallback_) {
             global.cancelAnimationFrame(this.continueCallback_);
             this.continueCallback_ = null;
             return;
         }
 
-        // FIXME: Rather than canceling the animation, we really should just
-        // pause the animation, but the pause function is still flagged as
-        // experimental.
         if (this.player_) {
-            var player = this.player_;
-            this.player_ = null;
-            player.cancel();
+            // FIXME: This needs the WebAnimationsAPI flag enabled.
+            this.player_.pause();

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

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

         }
     };
 
-    // FIXME: We have to inject this HTMLMarqueeElement as a custom element in order to make
-    // createdCallback, attachedCallback, detachedCallback and attributeChangedCallback workable.
-    // global.document.registerElement('i-marquee', {
-    //    prototype: HTMLMarqueeElementPrototype,
-    // });
-
     return HTMLMarqueeElementPrototype;
 });