Re: Implementation of XMLHttpRequest using gadgets.io.makeRequest (issue657043)

[jo...@gmail.com]

Hi Jacobo:

Sorry for the delay. This is awesome work. Mostly a few style/code
structure notes here.


http://codereview.appspot.com/657043/diff/1/11
File features/src/main/javascript/features/xhrwrapper/xhrwrapper.js
(right):

http://codereview.appspot.com/657043/diff/1/11#newcode46
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:46:
shindig.xhrwrapper.createXMLHttpRequestObject = function() {
for nominal byte size reduction, let's rename to createXHR

http://codereview.appspot.com/657043/diff/1/11#newcode48
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:48:
['MSXML2.XMLHTTP.6.0', 'MSXML2.XMLHTTP.3.0', 'MSXML2.XMLHTTP'];
This may not be a big deal, but this doesn't exactly emulate the old
behavior:

x = new ActiveXObject("Msxml2.XMLHTTP");
if (!x) {
   x = new ActiveXObject("Microsoft.XMLHTTP");
}
return x;

In particular, the previous logic suggests that an exception isn't
thrown -- just null/undefined is returned.  I haven't worked directly w/
this API (always using a wrapper) for a few years. Thoughts?

http://codereview.appspot.com/657043/diff/1/11#newcode84
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:84:
shindig.xhrwrapper.XhrWrapper.prototype.abort = function() {
so that this class can be easily and efficiently cajoled from the start,
let's write it in prototype-free form (using a closure), eg.

XhrWrapper = function() {
   var config_ = ...;
   var onreadystatechange = null;
   var aborted = false;

   function abort() {
     aborted = true;
   }

   function getAllResponseHeaders() {
     ...
   }

   return {
     abort: abort,
     getAllResponseHeaders: getAllResponseHeaders,
     ...
   }
};

The main restriction is that you not use "this", so all such references
turn into enclosed state. The XmlHttpResponse constructor can be faked
as you did for ActiveX.

http://codereview.appspot.com/657043/diff/1/11#newcode114
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:114: if
(!this.responseHeaders_) {
can avoid this check by initializing responseHeaders_ from the start

http://codereview.appspot.com/657043/diff/1/11#newcode173
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:173:
gadgets.io.makeRequest(this.url_.toString(), this.getCallback_(),
params);
may as well just inline the impl of getCallback_

http://codereview.appspot.com/657043/diff/1/11#newcode222
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:222: }
this block can be replaced with opensocial.xmlutil.parseXML, from
feature="xmlutil"

http://codereview.appspot.com/657043/diff/1/11#newcode245
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:245:
shindig.xhrwrapper.XhrWrapper.prototype.isSameOrigin_ = function() {
perhaps put this method on Url rather than XhrWrapper?

http://codereview.appspot.com/657043/diff/1/11#newcode273
features/src/main/javascript/features/xhrwrapper/xhrwrapper.js:273:
shindig.xhrwrapper.XhrWrapper.prototype.qualifyUrl_ = function() {
likewise, could be a method on Url (resolve(...))?

http://codereview.appspot.com/657043/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/657043/diff/1/3#newcode387
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java:387:
boolean isUsingXhrWrapper =
gadget.getSpec().getModulePrefs().getFeatures().containsKey("xhrwrapper");
nit: you can use gadget.getAllFeatures().contains(...) to take into
account xhrwrapper as a dependency rather than just as a direct
requirement.

http://codereview.appspot.com/657043/show