Details

      Description

      [Original description below]

      Current shindig code uses www.gmodules.com to generate UserPrefs UI. Shindig should have code that implements this that does not depend on that host.


      It takes several seconds to open the gadgets dialog.

      In gadgets.js the following code should be replaced:
      http://gmodules.com/ig/gadgetsettings

      With a call not requiring a 301 redirect:
      http://www.gmodules.com/ig/gadgetsettings

      You can use a redirect checker to verify this change should be made:
      http://www.internetofficer.com/seo-tool/redirect-check/

      Thanks to Carla Geisser, Google SRE for suggesting this change.

      1. fix-1188-bug.patch
        17 kB
        Charl van Niekerk
      2. patch.zip
        3 kB
        Benjamin McCann

        Activity

        Hide
        Charl van Niekerk added a comment -

        Attached a possible servlet-based solution.

        Show
        Charl van Niekerk added a comment - Attached a possible servlet-based solution.
        Hide
        Robert Peterson added a comment -

        Just to confirm that this works, we applied this patch on the BETA6 snapshot (it doesn't patch automatically anymore, but the changes are intuitive).

        I definitely second the suggestion to rewrite it to create the table using plain Javascript (either with innerHTML or DOM) rather than google-closure. We are not using closure in our project, so we have added it solely for this purpose.

        Show
        Robert Peterson added a comment - Just to confirm that this works, we applied this patch on the BETA6 snapshot (it doesn't patch automatically anymore, but the changes are intuitive). I definitely second the suggestion to rewrite it to create the table using plain Javascript (either with innerHTML or DOM) rather than google-closure. We are not using closure in our project, so we have added it solely for this purpose.
        Hide
        Benjamin McCann added a comment -

        Patch attached with minor fix from previous version. This file does not include the Google Closure dependency, which will need to be downloaded from http://code.google.com/closure/ in order to run this.

        Show
        Benjamin McCann added a comment - Patch attached with minor fix from previous version. This file does not include the Google Closure dependency, which will need to be downloaded from http://code.google.com/closure/ in order to run this.
        Hide
        Benjamin McCann added a comment -

        Yeah, both library itself and the compiler are pretty cool. I ran a portion of the Shindig code through the compiler not too long. There were only a few errors when I tried and I submitted most of them back to Shindig, so it should be pretty easy to get it to compile.

        Show
        Benjamin McCann added a comment - Yeah, both library itself and the compiler are pretty cool. I ran a portion of the Shindig code through the compiler not too long. There were only a few errors when I tried and I submitted most of them back to Shindig, so it should be pretty easy to get it to compile.
        Hide
        Paul Lindner added a comment -

        yeah.. I'm looking at that now. Looks like this would reduce js code size significantly.

        Show
        Paul Lindner added a comment - yeah.. I'm looking at that now. Looks like this would reduce js code size significantly.
        Hide
        Benjamin McCann added a comment -

        The DOM library I used was just released as the Closure library (http://code.google.com/closure/library/)

        Show
        Benjamin McCann added a comment - The DOM library I used was just released as the Closure library ( http://code.google.com/closure/library/ )
        Hide
        Benjamin McCann added a comment -

        Right, it's just DOM manipulation and certainly could be done without the DOM helper. However, it would be extremely tedious and verbose code. I wrote it like this for another project I'm working on and thought I would contribute it back. You're welcome to change it to remove the dependency, but I unfortunately don't have time to myself. And like I said, the resulting code wouldn't be fun or pretty, so I'm not sure you'd wan to.

        Show
        Benjamin McCann added a comment - Right, it's just DOM manipulation and certainly could be done without the DOM helper. However, it would be extremely tedious and verbose code. I wrote it like this for another project I'm working on and thought I would contribute it back. You're welcome to change it to remove the dependency, but I unfortunately don't have time to myself. And like I said, the resulting code wouldn't be fun or pretty, so I'm not sure you'd wan to.
        Hide
        Paul Lindner added a comment -

        Do you think there's any way to not use the goog.* JS classes? It appears to be simple DOM manipulation...

        Show
        Paul Lindner added a comment - Do you think there's any way to not use the goog.* JS classes? It appears to be simple DOM manipulation...
        Hide
        Benjamin McCann added a comment -

        Provides a JS implementation of the gadgets setting menu. Removes dependency upon gmodules.com and prefs dialog now loads instantaneously instead of taking a second or so. Also provides a much better UI by replicating the DOM structure and CSS used by iGoogle.

        Show
        Benjamin McCann added a comment - Provides a JS implementation of the gadgets setting menu. Removes dependency upon gmodules.com and prefs dialog now loads instantaneously instead of taking a second or so. Also provides a much better UI by replicating the DOM structure and CSS used by iGoogle.
        Hide
        Benjamin McCann added a comment -

        I have a JS implementation that creates the preferences dialog using the functions in this library:
        http://code.google.com/p/doctype/source/browse/trunk/goog/dom/dom.js

        However, it does not support the user preference of type list. Instead of giving the add and x buttons that you get in iGoogle, you just get an input of type text. This basically means you can only input one item (unless you know to separate them using a pipe). It'd probably be pretty easy to extend this to accept commas and then replace them with pipes when submitting the form. Given that the current implementation seems to be worse (https://issues.apache.org/jira/browse/SHINDIG-1212), I'm not sure list support is that big of an issue. If including the library above and having somewhat bad list support is okay, then I can clean it up and submit a patch.

        Show
        Benjamin McCann added a comment - I have a JS implementation that creates the preferences dialog using the functions in this library: http://code.google.com/p/doctype/source/browse/trunk/goog/dom/dom.js However, it does not support the user preference of type list. Instead of giving the add and x buttons that you get in iGoogle, you just get an input of type text. This basically means you can only input one item (unless you know to separate them using a pipe). It'd probably be pretty easy to extend this to accept commas and then replace them with pipes when submitting the form. Given that the current implementation seems to be worse ( https://issues.apache.org/jira/browse/SHINDIG-1212 ), I'm not sure list support is that big of an issue. If including the library above and having somewhat bad list support is okay, then I can clean it up and submit a patch.
        Hide
        Benjamin McCann added a comment -

        Whoops, my bad. It looks like you did fix this. It's just not showing up yet in the web view for some reason (http://svn.apache.org/repos/asf/incubator/shindig/trunk/javascript/container/gadgets.js)
        Thanks! I'm fine if you close this issue.

        Show
        Benjamin McCann added a comment - Whoops, my bad. It looks like you did fix this. It's just not showing up yet in the web view for some reason ( http://svn.apache.org/repos/asf/incubator/shindig/trunk/javascript/container/gadgets.js ) Thanks! I'm fine if you close this issue.
        Hide
        Paul Lindner added a comment -

        okay, changing this.

        Show
        Paul Lindner added a comment - okay, changing this.
        Hide
        Benjamin McCann added a comment -

        I don't think we should close this issue. We either need to not point to gmodules at all as Kevin Brown suggested, which is a larger work item. Or for the meantime we should change the call to gmodules.com to point to www.gmodules.com, which would be a trivial change. This is unusably slow right now because we're pointing at the wrong domain and it's really simple to fix.

        Show
        Benjamin McCann added a comment - I don't think we should close this issue. We either need to not point to gmodules at all as Kevin Brown suggested, which is a larger work item. Or for the meantime we should change the call to gmodules.com to point to www.gmodules.com, which would be a trivial change. This is unusably slow right now because we're pointing at the wrong domain and it's really simple to fix.
        Hide
        Paul Lindner added a comment -

        lgtm.

        Show
        Paul Lindner added a comment - lgtm.
        Hide
        Kevin Brown added a comment -

        That code really shouldn't be touching igoogle at all, and it's nothing but a hack that nobody has fixed in the last 2 years.

        Show
        Kevin Brown added a comment - That code really shouldn't be touching igoogle at all, and it's nothing but a hack that nobody has fixed in the last 2 years.

          People

          • Assignee:
            Unassigned
            Reporter:
            Benjamin McCann
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development