Any time something breaks, it's easy to look back and admonish oneself for what didn't work out. I know I have. My challenge is pragmatically answering for this lib, what constitutes proper peer review of this code? What's "large"? Many bugs came from small changes. The "largest" changes were RMR (peer reviewed, still had bugs found in testing) and a code move/refactor (announced on this list, tested extensively locally, still had issues in real-world deployments). To date, only Brian Eaton and Joey Schorr have shown much interest (reticence is understandable, if you ask me) to look at this code in depth. The fact is, it's a gnarly bunch of browser hacks deployed and used in all sorts of ways, often unanticipated but ways that end up needing to be supported. So testing has been just about the only way that bugs have been found in it. To that end, I've improved the test harness in Shindig, spent countless hours (sanity...dwindling...) using it, and as mentioned, we'll have automated per-browser testing within Google, which is an effective requirement since as many bugs have been found in "real" deployments as in a "standard" container/gadget integration.
I'm as big a fan of extensive peer review as anybody. If nothing else, it gives me some cover especially on this nasty lib :) I also know there are problems with the library as it stands, and want to find a proper balance in improving them. The good news is this patch is pretty well tested at this point. All constructive feedback welcome. --j On Tue, Jun 9, 2009 at 10:54 AM, Kevin Brown <[email protected]> wrote: > On Mon, Jun 8, 2009 at 7:20 PM, John Hjelmstad <[email protected]> wrote: > > > Hello all, > > Many of you have likely seen a series of rpc.js-related changes I've made > > in > > the past few weeks. These implemented several rpc.js improvements: > > > > 1. RMR, new transport for Safari < 4, Chrome < 2, and should work as a > > backup on several other browsers (the latter claim requires more > testing). > > - Paves the way to sunsetting IFPC, in turn sunsetting the need for > > containers to host "active" relay file rpc_relay.html and configure it > > properly. > > > > 2. Refactoring transport code into separate "classes". This cleans up the > > code a bit and makes it possible to optimize later by emitting only the > JS > > a > > given browser needs rather than JS for all techniques. > > > > 3. Early-message queueing. Makes it possible to call > gadgets.rpc.call(...) > > in a container to a gadget before the gadget is initialized (or even > > rendered), with the call still going through when the gadget is ready. It > > also ensures that G -> C calls made before G -> C initialization is > > complete > > also succeed (this is much less prevalent). > > > > 4. Various cleanups including tentative backward-setup support. As a > > convenience and to prevent user error, makes it possible to call > > gadgets.rpc.setAuthToken(gadget, token); before the gadget IFRAME is > > rendered. While not recommended in any case, this is intended to make the > > library easier to use. > > > > At Google we found several pain points and subtle bugs associated with > the > > combination of these features, so I've rolled back the code for the > moment > > to give others another look at it. The code is here: > > http://codereview.appspot.com/63209 > > > > > > > A few things learned during this process: > > > > * During integration testing of this library, it became exceedingly clear > > that having automated integration testing for as many configurations of > it > > as possible is a must. In particular, one should test each browser, > > container config, gadget server build, and container build (ideally the > > matrix of all). We have implemented this within Google and will be > > subjecting all rpc improvements to it. > > > > * Containers taking an rpc.js "snapshot" make improvements/upgrades > > exceedingly difficult to impossible. As has been noted in several places, > > the rpc.js version that the container loads should always be the exact > same > > as what the gadget loads. This is often achieved by the container > including > > <script src=" > > http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1[&debug=1][&v=< > http://gadgetserver.com/gadgets/js/rpc.js?c=1&container=1%5B&debug=1%5D%5B&v= > > > > <hash>]"></script>. > > > You forgot the most important lesson: Nobody is supposed to be committing > large patches without review. That is completely unacceptable and has > always > been. This code wasn't hosed because of the extent of the changes or the > nature of rpc.js itself, it was hosed because it was not properly peer > reviewed. Even the most alert and bright person will make mistakes in any > patch of this size, and that's why we review big patches before we commit > them. > > > > > > > > In addition, I've implemented the base support for per-browser rpc.js > > optimization on the server side, mentioned as the rationale for rpc.js > > refactoring (#2 above). This code is experimental for the moment, and any > > real-world deployment should be handled very carefully. Code reviewer > link: > > http://codereview.appspot.com/63210 > > > > Best, > > John > > >
