#446 ✓ staged
db48x

Allow one plugin to inherit from another

Reported by db48x | March 23rd, 2011 @ 03:06 PM | in 0.5

I thought that there was already a bug open on this, but couldn't find it. Feel free to dup.

Anyway, I went ahead and implemented this (https://github.com/db48x/popcorn-js/tree/plugin-inheritance). Any plugin can be defined as inheriting from any number of previously-defined plugins (no forward references allowed). Obviously a plugin can inherit from a plugin that itself inherits from something (it would be pretty silly not to allow that). I haven't added testcases to the unit tests yet, but here's some sample code that I've tested it with:

Popcorn.plugin("a", function(options) {
  return {
    start: function() { print("a started"); },
    end: function() { print("a ended"); }
  }
});
Popcorn.plugin("b", function(options) {
  return {
    start: function() { print("b started"); },
    end: function() { print("b ended"); }
  }
});
Popcorn.pluginInherit("c", "b", function(options) {
  return {
    start: function() { print("c started"); },
    end: function() { print("c ended"); }
  }
});
Popcorn.pluginInherit("d", ["a", "c"], function(options) {
  return {
    start: function() { print("d started"); },
    end: function() { print("d ended"); }
  }
});

var p = Popcorn('#video');
p.code({ start: .5, end: 1, onStart: function() { print("== testing a =="); }})
 .a({ start: 1, end: 2})
 .code({ start: 2.5, end: 3, onStart: function() { print("== testing b =="); }})
 .b({ start: 3, end: 4})
 .code({ start: 4.5, end: 5, onStart: function() { print("== testing c =="); }})
 .c({ start: 5, end: 6})
 .code({ start: 6.5, end: 7, onStart: function() { print("== testing d =="); }})
 .d({ start: 7, end: 8});

Comments and changes to this ticket

  • Rick

    Rick March 23rd, 2011 @ 03:24 PM

    The parens around your link are being parsed as part of the link itself and 404'ing ...

    https://github.com/db48x/popcorn-js/tree/plugin-inheritance

  • Rick

    Rick March 23rd, 2011 @ 04:01 PM

    This is an interesting idea. I'm wondering if necessarily belongs in core or if it can be offered as an extension? Do you have any use case examples we can see in action?

    As for the code itself, please update following the style guide: https://webmademovies.lighthouseapp.com/projects/63272/styleguide

    This line is not appropriate:

    
    throw "you idiot";
    

    Please remove. ( https://github.com/db48x/popcorn-js/commit/4cde7093d7729220895e9c95... )

    ...

    With regard to approach... since every browser that Popcorn.js is expected to support has implemented Array.isArray, you can remove the fallback.

  • db48x

    db48x March 23rd, 2011 @ 04:44 PM

    Heh, forgot about the throw message. Also, I'm glad to hear that every browser supports isArray. Wish more things happened that way.

    I really think that this should go in the core. The primary use case that I can think of is to support visual effects, such as the ones we saw recently where they had to modify the plugins in order to add the animations that they wanted. A much cleaner way to do that is to put the animations into their own plugin, then use them as mixins to create a third plugin that shows the footnotes or whatever it was and also animates them. The third plugin is the one you create events for. Something like this:

    Popcorn.plugin("sparkle", function(options) {
      return {
        start: function() {
          // do an animated color transition which lasts a few hundred milliseconds
        }
      }
    });
    
    Popcorn.pluginInherit("sparkly-footnote", ["footnote", "sparkle"], {});
    

    Then, you use it in your xml file:

    <sparkly-footnote start="1" end="10" text="a footnote">
    <sparkly-footnote start="10" end="25" text="another footnote">
    

    Note that right now defining two plugins using the same name doesn't really work correctly, at least when it comes time to inherit from them, so in this example the inheriting plugin has to use a new name. That should be fixed in another bug.

  • Rick

    Rick March 23rd, 2011 @ 05:45 PM

    I get it now - Can you post a demo of this in action... using a few existing plugins composed with an effects plugin? That will give us something to start testing against

  • Rick

    Rick March 23rd, 2011 @ 06:29 PM

    To clarify - not ALL browsers support Array.isArray(), but thankfully the ones we support all do.

  • Scott Downe

    Scott Downe March 24th, 2011 @ 02:06 PM

    I am liking this idea as a way of solving #275

  • Brett Gaylor

    Brett Gaylor March 24th, 2011 @ 02:18 PM

    Would it help to avoid confusion to call this group of problems "UI cues" rather than effects? I only say this because I imagine soon we will actually be tackling real effects, ie visual effects.

  • db48x

    db48x March 24th, 2011 @ 05:35 PM

    • State changed from “assigned” to “peer-review-requested”
  • Rick

    Rick March 24th, 2011 @ 05:52 PM

    • State changed from “peer-review-requested” to “review-needs-work”

    Is there a different branch? I pulled your "plugin-inheritance" branch and there were no changes to review

  • db48x

    db48x March 24th, 2011 @ 06:18 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    124c1a80a2c90adc9f28a0d6ae7224840c403f0a is the change to get. try pulling again, I guess?

  • Rick

    Rick March 24th, 2011 @ 06:21 PM

    • State changed from “peer-review-requested” to “under-review”
  • Rick

    Rick March 24th, 2011 @ 06:45 PM

    • State changed from “under-review” to “review-needs-work”

    Please understand that this review is meant to be helpful and positive for the project and is not in anyway meant to be harsh. That being said, here goes...

    
    Popcorn.pluginInherit = function( name, parent, definition, manifest )
    {
    
    // Should be...
    
    Popcorn.pluginInherit = function( name, parent, definition, manifest ) {
    
    
    function getDefinition( p )
    {
    
    // Should be...
    
    function getDefinition( p ) {
    

    This syntax is not supported in ECMA (only in Firefox , seen in chrome: http://gyazo.com/9c770210132e09f2c0c8f6dd1b95ee61.png JSLint will not accept it either: http://gyazo.com/c2c50082257a920cf1c79d8028ae8d7a.png ). Also, r is a leaked global

    
    for each ( r in Popcorn.registry ) {
    

    There is no variable called "name" ( ... "Plugin "+ name +" can't inher ... )

    function getDefinition( p )
    {
      for each ( r in Popcorn.registry ) {
        if ( r.type === p ) {
          return r.definition;            
        }
      }
      throw new Error("Plugin "+ name +" can't inherit from "+ p +", which doesn't exist");
    }
    

    I don't understand the approach of wrapping fn.apply() in a function called apply?

    function apply( fn, args ) { return fn && fn.apply( self, args ); }
    
    // cant this just be...
    fn && fn.apply( self, args ); 
    
    // ... when you call it later in the delegate function?
    

    Overall, comments explain the "why" as well as single carriage returns between each statement line would improve overall readability

  • db48x

    db48x March 24th, 2011 @ 07:13 PM

    • State changed from “review-needs-work” to “under-review”

    There is no variable called "name" ( ... "Plugin "+ name +" can't inher ... )

    it's a few lines up

    I don't understand the approach of wrapping fn.apply() in a function called apply?

    Yea, could go either way there myself. It's actually a remnant from when I had more than one callsite... I'll go ahead and remove the function.

    Overall, comments explain the "why" as well as single carriage returns between each statement line would improve overall readability

    Ok, I've added some good comments, but I'm not sure what you mean by single carriage returns that improve readability. An empty line after every statement seems a bit much, to be honest.

  • Rick

    Rick March 24th, 2011 @ 07:26 PM

    • State changed from “under-review” to “review-needs-work”

    The comments look great, the style changes look great. But as discussed in the IRC, lets hold until all the ducks are in a row with regard to the inheritance pattern issue you explained to me.

  • db48x

    db48x March 24th, 2011 @ 08:15 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Ok, I got the necessary tests written as we discussed.

  • David Humphrey

    David Humphrey March 27th, 2011 @ 10:12 PM

    • State changed from “peer-review-requested” to “review-needs-work”

    r-, mostly due to style nits:

    • Popcorn.registry_hash = {}; --> registryHash ?

    • Enclose with { ... }

    +      if (p in Popcorn.registry_hash)
    +        return Popcorn.registry_hash[p];
    
    • Capital G in Get (not your error, but since you're touching this line):
    // get the names of all of the ancestor classes, in the order that
    
    • Opening { on same line:
    +      for (var i in parents)
    +      {
    

    Also, that loop should filter out Object properties via hasOwnProperty

    • Use === and enclose in { ... }
    +        if (ancestorNames.indexOf(p) == -1)
    +          ancestorNames.push(p);
    
    • Watch omissions of space around args to params in function calls (sometimes you do, sometimes you don't). Too many to list.

    Code looks good otherwise, thanks for this.

  • db48x

    db48x March 27th, 2011 @ 10:43 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Fixed the style problems, comments, etc and pushed.

  • David Humphrey

    David Humphrey March 27th, 2011 @ 10:54 PM

    • State changed from “peer-review-requested” to “super-review-requested”

    2 minor nits, and this is ready for SR:

    • { on same line as if
    +      if ( p in Popcorn.registryHash && Popcorn.registryHash.hasOwnProperty( p ) )
    +      {
    
    • Spaces around function args:
    
    +          if (ancestorNames.indexOf( p ) === -1) {
                   ^                                ^
    ...
    +      var plugins = ancestorNames.map(function( name ) {
                                           ^
    

    Ran tests, looks good otherwise. PR+ with those fixes.

  • Rick

    Rick March 28th, 2011 @ 07:26 PM

    • State changed from “super-review-requested” to “review-needs-work”

    When I try to merge this into the current 0.5 branch, there are 3 conflicts in popcorn.unit.js and 1 conflict in popcorn.js that need to be resolved.

  • db48x

    db48x March 29th, 2011 @ 08:30 AM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “db48x” to “Rick”

    I merged it

  • Rick

    Rick March 29th, 2011 @ 06:37 PM

    • State changed from “super-review-requested” to “review-needs-work”

    I'm not sure why, but it seems everytime I look at updates for this branch, the review parts are fixed but new issues are introduced.

    There are two occurences of Object.hasOwnProperty(foo) - Popcorn's closure has provided a cached reference to Object.hasOwnProperty() for faster calls and better minification (minifiers/compressors will see that this cached ref always points back to the same referenced function).

    So please change...

    
    Popcorn.registryHash.hasOwnProperty( p )
    

    to...

    
    hasOwn.call( Popcorn.registryHash, p )
    

    and

    
    parents.hasOwnProperty( i )
    

    to:

    
    hasOwn.call( parents, i )
    

    Additionally, the call to Popcorn.registryHash.hasOwnProperty( p ) below is unnecessary

    
    if ( p in Popcorn.registryHash && Popcorn.registryHash.hasOwnProperty( p ) ) {
    

    Furthermore, the entire function could do well to have the number of property lookups reduced:

    
    function getDefinition( plugin ) {
      var regHash = Popcorn.registryHash;
    
      if ( regHash[ plugin ] ) {
        return regHash[ plugin ];
      }
    
      Popcorn.error( "Plugin "+ name +" can't inherit from "+ plugin +", which doesn't exist" );
    }
    

    ... Which also improves code readability.

  • db48x

    db48x March 29th, 2011 @ 11:43 PM

    • State changed from “review-needs-work” to “super-review-requested”

    Ah, cool. I hadn't seen the cached hasOwn.

  • Rick

    Rick March 30th, 2011 @ 09:43 AM

    • State changed from “super-review-requested” to “review-needs-work”

    I just reviewed the new set of changes.

    With regard to checking for property existence vs. a hasown or hasownproperty call, please take a look at these findings: http://jsperf.com/existence-vs-hasown

    As I mentioned in my last review, I have to recommend this...

    
    function getDefinition( p ) {
      var hash = Popcorn.registryHash;
      if ( hash[ p  ] ) {
        return hash[ p ];
      }
      Popcorn.error( "Plugin "+ name +" can't inherit from "+ p +", which doesn't exist" );
    }
    

    ... Because it's faster and a hasown call is unnecessary

  • db48x

    db48x March 30th, 2011 @ 11:10 AM

    • State changed from “review-needs-work” to “super-review-requested”

    No, using hasOwnProperty is the right thing to do, as otherwise Popcorn.pluginInherit("foo", "toSource", {}) would do the wrong thing entirely. I don't care that the other way is faster, since this way is correct. Also, this code is not in any way performance critical; it's only run a few times for each plugin that inherits from another.

  • Rick

    Rick March 30th, 2011 @ 05:12 PM

    • State changed from “super-review-requested” to “review-looks-good”

    From my perspective, I guess I just assume if people are trying to name their composite plugins things like "constructor", "toString", "valueOf", etc... that we can simply point them to docs and in return we get to take advantage of a better performing approach.

    SR+

  • Rick

    Rick March 31st, 2011 @ 07:24 PM

    • Assigned user changed from “Rick” to “annasob”

    Tested with Test Suite Chrome 10, 11, 12; Firefox 4.0, 4.2a1pre

  • annasob

    annasob March 31st, 2011 @ 11:01 PM

    • State changed from “review-looks-good” to “review-needs-work”
    • Milestone order changed from “2” to “0”

    So apparently Array.isArray is not supported in FF3.6 A lot of unit test are failing in this browser. db48x can you look into this please.

  • Rick

    Rick March 31st, 2011 @ 11:22 PM

    Anna, firefox 3.6 also doesn't support the currentTime behaviour that we filed tickets for...

    I'll take the heat for that suggestion. If we're going to implement a polyfill for isArray, we should add it as a static method: Popcorn.isArray() instead of inside of Popcorn.pluginInherit (as it previously was)

    The more I look at this, the more concerned I am about burying "utility" function definitions inside of Popcorn.pluginInherit().

    Additionally, I think in following with the naming convention of the xhr static functions, this really should be relocated as a static method attached to Popcorn.plugin(), like this:

    Popcorn.plugin.inherit()

    with a remapping...

    Popcorn.inherit = Popcorn.plugin.inherit;

    Which will match the naming of Popcorn.xhr() and friends

  • Rick

    Rick April 1st, 2011 @ 04:47 PM

    • State changed from “review-needs-work” to “blocked”

    Blocked by #463

    When 463 lands, there will be a Popcorn.isArray() implementation that can be used

  • annasob

    annasob April 4th, 2011 @ 11:01 PM

    • State changed from “blocked” to “review-needs-work”
    • Assigned user changed from “annasob” to “db48x”

    Ok db48x #463 is staged, can you re-factor your patch to use it.

  • Rick
  • Rick

    Rick April 5th, 2011 @ 02:32 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “db48x” to “Scott Downe”
  • Rick
  • Rick

    Rick April 5th, 2011 @ 03:13 PM

    • Assigned user changed from “Scott Downe” to “annasob”
  • annasob

    annasob April 5th, 2011 @ 04:19 PM

    • State changed from “peer-review-requested” to “super-review-requested”

    This looks good to me. Nice unit tests
    Pass in Chrome, FF 3.6 & 4 - Vista

    PR+
    One nit pick: Problem at line 869 character 4: Missing semicolon.
    Ill get rick to fix that.

  • annasob

    annasob April 5th, 2011 @ 04:20 PM

    • Assigned user changed from “annasob” to “Scott Downe”

    Assigning to Scott db48x if you have time to take a look that would be great!

  • Rick
  • db48x

    db48x April 8th, 2011 @ 03:44 PM

    • Assigned user changed from “Scott Downe” to “Rick”

    I'd prefer it if you hadn't changed the variable names. Genus is not the plural of gene, and introducing a false analogy with biology doesn't make the code any more readable. Please change them back.

  • Rick
  • db48x

    db48x April 8th, 2011 @ 06:38 PM

    • State changed from “super-review-requested” to “review-needs-work”
  • db48x

    db48x April 8th, 2011 @ 06:39 PM

    • State changed from “review-needs-work” to “review-looks-good”
  • Rick

    Rick April 8th, 2011 @ 06:53 PM

    • Assigned user changed from “Rick” to “annasob”
  • annasob
  • annasob

    annasob April 11th, 2011 @ 09:29 PM

    • State changed from “review-looks-good” to “staged”

    Staged in annasob/popcorn-js commit

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

People watching this ticket

Referenced by

Pages