#556 ✓ staged
Rick

GoogleFeed plugin needs a tune up

Reported by Rick | June 3rd, 2011 @ 04:37 PM | in 0.7

There are a few issues that could use some love, mainly revolving around the race conditions.

  • Re-engineer plugin logic in way the eliminates all race conditions (anytime a setTimeout is being used as a "waiting mechanism" - this should be altered to use async callback patterns). A single recursing setTimeout loop should be sufficient for handling async resource loading.

  • This ticking timebomb is loading resources with no regard for the host object:


Popcorn.getScript( "http://www.google.com/jsapi", callBack );

in the other google plugins, we check for the "google" var before attempting to load "http://www.google.com/jsapi", because if it already exists, there is no reason to re-request it

... This has to be changed to check for the existance of the google object

  • Generally style fixes, quotes consistency etc.

Comments and changes to this ticket

  • David Seifried

    David Seifried June 6th, 2011 @ 12:51 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “Rick”

    Hey Rick,

    So I went through the code and fixed everything according to what you mentioned above. I think I got all of the styling mistakes and hopefully what I've written is what you are looking for.

    https://github.com/dseif/popcorn-js/commits/t556

  • Rick

    Rick June 7th, 2011 @ 02:28 PM

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

    Two things:

    1) I'm stoked that you jumped right in and took this ticket

    2) Great work :)

    Tested: google test suite;

    Passing in:

    • FF 3.6, 4.x, 7.x

    • Chrome 11, 12, 13

    PR+

  • Scott Downe

    Scott Downe June 13th, 2011 @ 01:18 PM

    • State changed from “super-review-requested” to “under-review”
  • Scott Downe

    Scott Downe June 13th, 2011 @ 01:31 PM

    • State changed from “under-review” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “David Seifried”

    Ran tests. works in chrome and ff.

    Line 34 of popcorn.googlefeed.js need to lose two spaces worth of indent.

    Just quickly update that and this is fine.

  • David Seifried

    David Seifried June 16th, 2011 @ 05:03 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “David Seifried” to “Scott Downe”
  • cadecairos

    cadecairos June 21st, 2011 @ 02:23 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “David Seifried”
    • Milestone order changed from “38” to “0”

    alrighty I have a few requests on this one:

    Can you compress these var declarations?

    // PLUGIN: Google Feed
    (function (Popcorn) {
    
      var i = 1,
          scriptLoaded  = false;
        //scriptLoaded  = false,
    
      var dynamicFeedLoad = function() {
    //dynamicFeedLoad = function() {
        var dontLoad = false;
    

    for speed, make this:

      var dynamicFeedLoad = function() {
        var dontLoad = false;
    
        if ( typeof GFdynamicFeedControl === "undefined" ) {
    
          Popcorn.getScript( "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.js", function() {
            scriptLoaded = true;
          }); 
    
        } else {
          scriptLoaded = true;
        }
    
        //  Checking if the css file is already included
        for ( var k = 0; k < document.getElementsByTagName( "link" ).length; k++ ){
          if ( document.getElementsByTagName( "link" )[ k ].href == "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.css" ) {
            dontLoad = true;
          }
        }
    

    more like this:

      dynamicFeedLoad = function() {
        var dontLoad = false,
            k = 0,                                           //
            links = document.getElementsByTagName( "link" ), //changes here
            len = links.length;                              //
    
        if ( typeof GFdynamicFeedControl === "undefined" ) {
    
          Popcorn.getScript( "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.js", function() {
            scriptLoaded = true;
          }); 
    
        } else {
          scriptLoaded = true;
        }
    
        //  Checking if the css file is already included
        for ( ;  k < len; k++ ){  //changes here
          if ( links[ k ].href == "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.css" ) { //changes here
            dontLoad = true;
          }
        }
    

    compress these var declarations:

        if ( !dontLoad ) {
          var head = document.getElementsByTagName( "head" )[ 0 ];
          var css = document.createElement( "link" );
    

    move these two var declarations above so they are declared at the top of the function:

    var newdiv = document.createElement( "div" );
        newdiv.style.display = "none";
        newdiv.id = "_feed"+i;
        newdiv.style.width = "100%";
        newdiv.style.height = "100%";
        i++;
    
        document.getElementById( options.target ).appendChild( newdiv );
    
        var initialize = function() {
    

    use "===" here and get rid of extra whitespace:

    vertical:   options.orientation.toLowerCase() == "vertical" ? true : false,
    horizontal: options.orientation.toLowerCase() == "horizontal" ? true : false,
    title:      options.title = options.title || "Blog"
    

    lastly, add some whitespace here:

    about: {
          name:    "Popcorn Google Feed Plugin",
          version: "0.1",
          author:  "David Seifried",
          website: "dseifried.wordpress.com"
        },
        options: {
          start          : { elem:"input", type:"text", label:"In" },
          end            : { elem:"input", type:"text", label:"Out" },
          target         : "feed-container",
          url            : { elem:"input", type:"text", label:"url" },
          title          : { elem:"input", type:"text", label:"title" },
          orientation    : {elem:"select", options:["Vertical","Horizontal"], label:"orientation" }
        }
      });
    
  • David Seifried

    David Seifried June 21st, 2011 @ 03:10 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Nice, thanks for the input on the performance stuff, def makes sense. Here is everything updated like requested, should be good to go.
    https://github.com/dseif/popcorn-js/commits/t556

  • cadecairos

    cadecairos June 21st, 2011 @ 05:33 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    Three things before this can go forward:

    1: options.orientation is used in initialize(), but it isn't defaulted or validated until start(). Please move that check into initialize, and add a test for defaulting options.orientation.

    2: add whitespace between the keys and values here.

    options: {
          start: {
            elem:"input", 
            type:"text", 
            label:"In" 
          },
          end: { 
            elem:"input", 
            type:"text", 
            label:"Out" 
          },
          target: "feed-container",
          url: { 
            elem:"input", 
            type:"text", 
            label:"url" 
          },
          title: { 
            elem:"input", 
            type:"text", 
            label:"title" 
          },
          orientation: {
            elem:"select", 
            options:["Vertical","Horizontal"], 
            label:"orientation" 
          }
        }
    

    3: This if should be using "===" (missed it in past reviews

    if ( links[ k ].href == "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.css" )
    

    I Think thats everything... but we'll see :D

  • David Seifried

    David Seifried June 22nd, 2011 @ 10:26 AM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Alright man, everything should be good now.
    https://github.com/dseif/popcorn-js/commits/t556

  • cadecairos

    cadecairos June 22nd, 2011 @ 10:54 AM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    looks great!

    BUT... one problem (sorry!):

    Linting ./plugins/googlefeed/popcorn.googlefeed.js
      }
        Problem at line 38 character 4: Missing semicolon.
    1 Error(s) found.
    
  • David Seifried

    David Seifried June 22nd, 2011 @ 11:01 AM

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

    Rick June 22nd, 2011 @ 11:19 AM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    David, A couple things from me:

    Checking for global vars...

    
    if ( typeof GFdynamicFeedControl === "undefined" ) {
    
    // ...
    
    if ( typeof google === "undefined" ) {
    

    Is better written as...

    
    if ( !window.GFdynamicFeedControl ) {
    
    // ...
    
    if ( !window.google ) {
    

    This is because the vars you are evaluating are global and this approach will explicitly check within the right context (in the odd event that a "google" or "GFdynamicFeedControl" var are declared in your closure, those will be evaluated and not the correct global reference)

    Since string literals cannot be compressed or minified (but can be gzipped), we want to try to reduce the number of repeated occurrences -- so we can take this:

    
    dynamicFeedLoad = function() {
      var dontLoad = false,
          k = 0,
          links = document.getElementsByTagName( "link" ),
          len = links.length,
          head = document.getElementsByTagName( "head" )[ 0 ],
          css = document.createElement( "link" );
    
      if ( typeof GFdynamicFeedControl === "undefined" ) {
    
        Popcorn.getScript( "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.js", function() {
          scriptLoaded = true;
        }); 
    
      } else {
        scriptLoaded = true;
      }
    
      //  Checking if the css file is already included
      for ( ; k < len; k++ ){
        if ( links[ k ].href === "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.css" ) {
          dontLoad = true;
        }
      }
    
      if ( !dontLoad ) {
        css.type = "text/css";
        css.rel = "stylesheet";
        css.href =  "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.css";
        head.insertBefore( css, head.firstChild );
      }
    };
    

    And reduce it to:

    
    dynamicFeedLoad = function() {
      var dontLoad = false,
          k = 0,
          links = document.getElementsByTagName( "link" ),
          len = links.length,
          head = document.getElementsByTagName( "head" )[ 0 ],
          css = document.createElement( "link" ),
    
          // create a reference to the common, repeated string...
          resource = "http://www.google.com/uds/solutions/dynamicfeed/gfdynamicfeedcontrol.";
    
      if ( typeof GFdynamicFeedControl === "undefined" ) {
                           
                        // Replace the string literal 
                        // with a concat expression
      
        Popcorn.getScript( resource + "js", function() {
          scriptLoaded = true;
        }); 
    
      } else {
        scriptLoaded = true;
      }
    
      //  Checking if the css file is already included
      for ( ; k < len; k++ ){
    
                        // Replace the string literal 
                        // with a concat expression
    
        if ( links[ k ].href === resource + "css" ) {
          dontLoad = true;
        }
      }
    
      if ( !dontLoad ) {
        css.type = "text/css";
        css.rel = "stylesheet";
    
                  // Replace the string literal 
                  // with a concat expression
        css.href =  resource + "css";
    
        head.insertBefore( css, head.firstChild );
      }
    };
    
  • cadecairos

    cadecairos June 22nd, 2011 @ 11:21 AM

    • State changed from “review-needs-work” to “review-looks-good”
    • Assigned user changed from “David Seifried” to “Jon Buckley”

    awesome!

    Googlefeed unit tests all passing in:

    Firefox 4, Chrome 12

    Passes lint.

    SR+

  • David Seifried

    David Seifried June 22nd, 2011 @ 11:26 AM

    • State changed from “review-looks-good” to “review-needs-work”
    • Assigned user changed from “Jon Buckley” to “David Seifried”

    Jon hold off on this, I'm going to make the changes Rick mentioned above, I will assign to Rick after for review

  • David Seifried

    David Seifried June 22nd, 2011 @ 11:45 AM

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

    Alright made the requested changes, thanks for the suggestions everyone!
    https://github.com/dseif/popcorn-js/commits/t556

  • Rick

    Rick June 22nd, 2011 @ 12:40 PM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “Rick” to “Jon Buckley”

    SR+

    Tested: googlefeed test suite;

    Passing in:

    • FF 3.6 (stable), 5.x (stable), 6.x (Aurora), 7.x (Nightly)

    • Chrome 12 (stable), 13 (beta), 14 (Canary)

    Lint Passes

  • Jon Buckley
  • Rick
  • annasob
  • Rick

    Rick August 14th, 2011 @ 08:44 PM

    • State changed from “staged” to “under-review”

    Jon, Not sure my last changes ever made it in. Also, it seems anna has a patch to contribute

  • Jon Buckley

    Jon Buckley August 23rd, 2011 @ 02:25 PM

    • Assigned user changed from “Jon Buckley” to “Rick”

    What changes didn't make it in? I don't see a 556 branch on your repo, and I confirmed that that last commit on dseif's t556 branch makes what's in the repo.

    Anna's changes were her updating popcorn on her repo, it just showed dseif's commit.

  • Rick

    Rick August 23rd, 2011 @ 03:01 PM

    • State changed from “under-review” to “staged”
    • Assigned user cleared.

    I think I misunderstood what actually occurred.

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

Referenced by

Pages