http://codereview.appspot.com/63209/diff/1/5 File features/src/main/javascript/features/rpc/dpm.transport.js (right):
http://codereview.appspot.com/63209/diff/1/5#newcode28 Line 28: */ I vote to delete this entirely. http://codereview.appspot.com/63209/diff/1/2 File features/src/main/javascript/features/rpc/rpc.js (right): http://codereview.appspot.com/63209/diff/1/2#newcode36 Line 36: * as such by core code. The last sentence doesn't make sense and is unnecessary. This is an interface (no ambiguity), and you don't need to explain how javascript works in the documentation. Just because it doesn't look like a Java-style interface doesn't mean that it isn't an interface. Duck typing is a well understood concept in the javascript community, and people who aren't familiar with javascript have no business trying to write a new transport. Also, referring to the various objects used to implement transports as 'singletons' is very odd. There is no prototype, so there isn't really the possibility of there being multiple copies anyway. They're just objects. http://codereview.appspot.com/63209/diff/1/2#newcode39 Line 39: * + getCode(): returns a string identifying the transport. For debugging. If I'm debugging, I can see the actual object. How does this aid debugging? http://codereview.appspot.com/63209/diff/1/2#newcode123 Line 123: return typeof window.postMessage === 'function' ? gadgets.rpctx.Wpm : Properties should only be capitalized if they are ctors. Aside from the consistency argument, lint tools actually complain about anything that is capitalized that isn't used as a ctor. http://codereview.appspot.com/63209/diff/1/2#newcode227 Line 227: * Helper method returning a canonicalized schema://host[:port] for scheme://host[:port]. "schema" is a nonsensical term in a URI. Alternatively, referencing the URI RFC would explain what an origin is. http://codereview.appspot.com/63209/diff/1/2#newcode235 Line 235: function getDomainRoot(url) { This is the origin. Why don't you call it 'getOrigin'? I have no idea what a 'domain root' is, but an origin is unambiguous in the browser world. http://codereview.appspot.com/63209/diff/1/2#newcode323 Line 323: transport = fallbackTransport; This is going to be buggy code. The 'fallback' transport is the one most likely to fail to initialize. Under which transport will retrying ever make a difference? It seems better to just fail and blow up here than to get into an even buggier, hard to debug state. http://codereview.appspot.com/63209/diff/1/2#newcode339 Line 339: function callSameDomain(target, rpc) { This really belongs in a file shared by 'slow' methods. window.postMessage is as fast as callSameDomain, so we can ignore this entirely when we are using a browser that doesn't suck. http://codereview.appspot.com/63209/diff/1/2#newcode360 Line 360: // Shouldn't happen due to domain root check. Caught just in case. This is wholly unnecessary. If the getDomainRoot check somehow passes and then the call fails, the browser itself is hosed. Why not have similar checks in all the other places where calls might fail if the browser is hosed? http://codereview.appspot.com/63209/diff/1/2#newcode447 Line 447: */ unregister isn't really used for anything. we may as well delete it. http://codereview.appspot.com/63209/diff/1/2#newcode477 Line 477: */ same here http://codereview.appspot.com/63209/diff/1/2#newcode597 Line 597: */ setAuthToken, getAuthToken, and receive are only necessary for slow channels as well. http://codereview.appspot.com/63209/diff/1/2#newcode623 Line 623: getRelayChannel: function() { Why do we need this? http://codereview.appspot.com/63209/diff/1/8 File features/src/main/javascript/features/rpc/wpm.transport.js (right): http://codereview.appspot.com/63209/diff/1/8#newcode39 Line 39: * - Safari (latest nightlies as of 26/6/2008) Safari 4+ http://codereview.appspot.com/63209/diff/1/8#newcode41 Line 41: * - Opera 9+ and Chrome 2.0 ("Chrome Beta") http://codereview.appspot.com/63209/diff/1/8#newcode49 Line 49: }, Why do we need this? http://codereview.appspot.com/63209/diff/1/8#newcode76 Line 76: var targetWin = targetId === '..' ? parent : frames[targetId]; Where is 'frames' defined? http://codereview.appspot.com/63209/diff/1/8#newcode80 Line 80: } This should throw or log or something when relay is null. The silent failure is a scary, hard to debug problem. Even simpler would be to blindly send the request regardless of whether relay was truthy or not. You'd get an exception either way. http://codereview.appspot.com/63209
