367537 - enhancements to httpd.js

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Robert Sayre]

I need a default directory handler, and I want a response.write("string") method.

Comment 1

[Robert Sayre]

Attachment: patch w/ unit tests

Comment 2

[Robert Sayre]

Attachment: fix nits

Comment 3

[Robert Sayre]

Attachment: Make nsIHttpRequestMetadata extend nsIPropertyBag

after discussion with jeff in irc, this seems like the way to go. nsIPropertyBag2 seems not needed so far, since lack of a directory property is pretty catastrophic for a directory handler, so it's not necessary for the consumer to wrap getProperty in a try/catch block.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 252495 [details] [diff] [review]
Make nsIHttpRequestMetadata extend nsIPropertyBag

>Index: httpd.js

>+  //
>+  // see nsIHttpServer.registerErrorHandler
>+  //
>+  registerDefaultDirHandler: function(code, handler)
>+  {
>+    this._handler.registerDefaultDirHandler(code, handler);
>+  },

This name doesn't feel right -- how about setIndexHandler?  Also, change the function name in the comment, and fix the function arguments to just be handler (both in the signature and in the function code).  You're getting somewhat lucky that this works, the way it's currently written.


>+  //
>+  // see nsIHttpServer.registerDefaultDirHandler
>+  //
>+  registerDefaultDirHandler: function(handler)
>+  {
>+    this._handlerToField(handler, this, "_defaultDirHandler");
>+  },

Reusing the make-it-a-function code is nice, but for this function I think we want something different than storing it this way.  Basically:

  function setIndexHandler(handler)
  {
    if (!handler)
      handler = defaultIndexHandler;
    else if (typeof(handler) != "function")
      handler = createHandlerFunc(handler);

    this._indexHandler = handler;
  }

Essentially, it's the standard accept-functions-or-objects plus logic to reset the handler to the default if handler == null.


>+  /*
>+   * set or unset a handler/field combination
>+   */

Expand on this, please, with descriptions of the parameters?  Also make unset be "remove" in the final description.


>       // the "file" might be a directory -- deal
>+      if (file.exists() && file.isDirectory() && this._defaultDirHandler) {
>+        try {
>+          metadata.bag.setPropertyAsInterface("directory", file);
>+          this._defaultDirHandler(metadata, response);
>+        } catch (ex) {
>+          throw HTTP_501;
>+        }
>+        return;
>+      }
>+
>       if (file.exists() && file.isDirectory())
>       {
>         file.append("index.html");  // make configurable?
>-        if (!file.exists() ||
>-            file.isDirectory())
>+        if (!file.exists() || file.isDirectory())
>           throw HTTP_501;  // need directory listings ftw!
>       }

If we make it setIndexHandler and guarantee a default index handler setting, we can shorten this a bit:

      // the "file" might be a directory, in which case we either serve the
      // contained index.html or make the index handler write the response
      if (file.exists() && file.isDirectory())
      {
        file.append("index.html"); // make configurable?
        if (!file.exists() || file.isDirectory())
        {
          metadata._bag.setPropertyAsInterface("directory", file.parent);
          this._indexHandler(metadata, response);
          return;
        }
      }

...and with the following default index handler, set in the ServerHandler constructor:

  function defaultIndexHandler(metadata, response)
  {
    var file = metadata.getProperty("directory");
    NS_ASSERT(file);

    throw HTTP_501;  // need directory listings ftw!
  }


>   //
>+  // see nsIHttpResponse.write
>+  //
>+  write: function(data) {
>+    if (typeof data == 'string') {
>+      this.bodyOutputStream.write(data, data.length);
>+      return;
>+    }
>+
>+    var dataAsString = data + "";
>+    this.bodyOutputStream.write(dataAsString, dataAsString.length);
>+  },

This can be condensed into:

  function write(data)
  {
    data = String(data);
    this.bodyOutputStream.write(data, data.length);
  }

(I doubt it's worth type-checking data before Stringing, since String probably takes about as long as type-checking would anyway if typeof data == "string", given string immutability.)


>   /**
>+   * For the addition of ad-hoc properties.
>+   */
>+  this._bag = Cc["@mozilla.org/hash-property-bag;1"]
>+                .createInstance(Ci.nsIWritablePropertyBag2);

Add "and new functionality without having to tweak nsIHttpRequestMetadata every time".


>+  // nsIHttpRequestMetadata extends nsIPropertyBag
>+  get bag()
>+  {
>+    return this._bag;
>+  },

Don't bother with the bag getter -- just use _bag the one place where the getter's ever used.


>+  get enumerator()
>+  getProperty: function(name) 

Add "see" comments in the style of the others used for functions implementing XPCOM methods, please.


>Index: nsIHttpServer.idl

>+
>+  /*

Need a second "*" here.

>+   * Registers a handler to display directories if no other handler matches.

"Sets the handler used to display the contents of a directory if the directory contains no index page."?


>+   * @param handler
>+   *   an object which will handle any requests for a directory
>+   *   listing, or null to remove any existing handler; if while the
>+   *   server is running the handler throws an exception while
>+   *   responding to a request, an HTTP 500 response will be returned.
>+   *   An nsIFile corresponding to the directory is available from the
>+   *   metadata object passed to the handler, under the key
>+   *   "directory".

"an object which will handle any requests for directories which do not contain index pages, or null to reset to the default index handler"?  The rest of it looks fine.


>+  /**
>+   * Write a string to the response's output stream.
>+   */
>+  void write(in string data);

Please add a @note saying that this method is only guaranteed to work with ASCII data, to be absolutely clear.


>Index: test/test_defaultdirhandler.js

Name probably should be test_setindexhandler.js with the method name change.


>+// make sure response.write works for strings, and coerces other args to strings

Fix the description here to be accurate.


>+        case 0:
>+          checkStatusLine(ch, 1, 1, 200, "OK");

Remove checkStatusLine (the function and uses, please) since you're not explicitly setting the status line in the handlers (and also in the other test, too).


>+          do_check_eq(ch.getResponseHeader("content-length"), "10");
>+          srv.registerDefaultDirHandler(null);
>+          break;

You could compare stream contents here if you wanted, but I haven't gotten the right utility functions in head_utils.js to make it easy yet.  I need to fix that soon...

Also, a nit: I prefer RFC-style header caps for headers ("Content-Length"), here and in the other test.


I like this, but we just need to do a little more neatening before it's good enough.  :-)

Comment 5

[Robert Sayre]

Attachment: address comments

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 252521 [details] [diff] [review]
address comments

>Index: httpd.js

>+  /**
>+   * Init our default handler for directories.
>+   */
>+  this.setIndexHandler(defaultIndexHandler);

Style here is to describe non-public fields when they're initialized, in the ctor as much as possible, so this should just be:

  this._indexHandler = defaultIndexHandler;

...with a comment like, "The handler used to serve directories when no index file is present."  (This also avoids the overhead of calling setIndexHandler even when we know that the passed-in handler is a function.)


>+  /**
>+   * Set or remove a handler in an ojbect with a key.
>+   * If handler is null, the key will be deleted.
>+   *
>+   * @param handler
>+   *   A function or an nsIHttpRequestHandler object.
>+   * 
>+   * @param dict
>+   *   The object to attach the handler to.
>+   *
>+   * @param key
>+   *   The field name of the handler.
>+   */

Remove the blank lines between param descriptions, please.


>+  _handlerToField: function(handler, dict, key) {

>+  write: function(data) {

Move the braces to the next lines, please.


>+    var dataAsString = String(data);
>+    this.bodyOutputStream.write(dataAsString, dataAsString.length);

I'd prefer not using a new local variable here, but meh, personal preference...


>   /**
>+   * For the addition of ad-hoc properties, and new functionality
>+   * without having to tweak nsIHttpRequestMetadata every time.
>+   */

Remove the unnecessary comma, please.


>Index: nsIHttpServer.idl

>+  /**
>+   * Write a string to the response's output stream.
>+   *
>+   * @note This method is only guaranteed to work with ASCII data.

Ultra-nit: move "This [...] data." down a line and indent two spaces, as with all params and notes in this file.


>Index: test/test_response_write.js

>+        case 0:
>+          do_check_eq(ch.getResponseHeader("content-length"), "4");
>+          break;
>+
>+        case 1:
>+          do_check_eq(ch.getResponseHeader("content-length"), "4");
>+          break;

RFC-style caps here, please.


>Index: test/test_setindexhandler.js

>+function performNextTest()
>+{
>+  do_test_pending();
>+
>+  var ch = makeChannel(paths[currPathIndex]);
>+  ch.asyncOpen(listener, null);
>+}

I didn't notice the first time through, but since you're loading the same URL twice here, you could hit the cache on the second load.  Add "ch.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE; // important!" just before calling asyncOpen to explicitly remove the cache from the picture.  (See, for example, netwerk/test/httpserver/test/test_registerdirectory.js line 289.)


>+  dirServ = Cc["@mozilla.org/file/directory_service;1"]
>+    .getService(Ci.nsIProperties);

Align the dot with the "[", please.


r=me with these last remaining nits fixed, and thanks for putting up with my finickiness!  :-)

Comment 7

[Robert Sayre]

/cvsroot/mozilla/netwerk/test/httpserver/httpd.js,v  <--  httpd.js
new revision: 1.2; previous revision: 1.1
done
Checking in nsIHttpServer.idl;
/cvsroot/mozilla/netwerk/test/httpserver/nsIHttpServer.idl,v  <--  nsIHttpServer.idl
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/netwerk/test/httpserver/test/test_response_write.js,v
done
Checking in test/test_response_write.js;
/cvsroot/mozilla/netwerk/test/httpserver/test/test_response_write.js,v  <--  test_response_write.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/netwerk/test/httpserver/test/test_setindexhandler.js,v
done
Checking in test/test_setindexhandler.js;
/cvsroot/mozilla/netwerk/test/httpserver/test/test_setindexhandler.js,v  <--  test_setindexhandler.js
initial revision: 1.1
done