Closed Bug 356023 Opened 18 years ago Closed 18 years ago

JS helpers for Composer

Categories

(Composer Graveyard :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glazou, Assigned: glazou)

Details

Attachments

(1 file)

just a few JS helpers for Composer, better organized than the old
and messy editorUtilities.js
Attached patch fix #1Splinter Review
This patch is very likely to break Composer and I perfectly know about it.
I just want to check the helpers first, and update composer.js and friend
afterwards, from a clean basis.
Attachment #241704 - Flags: review?(neil)
I think the +  isDocumentEditable: function () has been duplicated in your patch if I see this correctly...
(In reply to comment #2)
> I think the +  isDocumentEditable: function () has been duplicated in your
> patch if I see this correctly...

d'oh, copy/paste error. Consider it fixed, thanks for spotting this out.
Comment on attachment 241704 [details] [diff] [review]
fix #1

>Index: content/composer/editorUtils.js
>+  getCurrentCommandManager: function ()

Please name your functions for Venkman goodness:

getCurrentCommandManager: function getCurrentCommandManager()

>+  newCommandParams: function ()
>+  {
>+    try {
>+      return Components.classes["@mozilla.org/embedcomp/command-params;1"].createInstance(Components.interfaces.nsICommandParams);

Long line.  Suggest:

const contractId = "@mozilla.org/embedcomp/command-params;1";
const nsICommandParams = Components.interfaces.nsICommandParams;

return Components.classes[contractId].createInstance(nsICommandParams);

Or, similar to what you do below:

      return Components.classes["@mozilla.org/embedcomp/command-params;1"]
                       .createInstance(Components.interfaces.nsICommandParams);

>+  isDocumentEditable: function ()
>+  {
>+    try {
>+      return this.getCurrentEditor().isDocumentEditable;
>+    } catch (e) {}
>+    return false;
>+  },

This (empty catch blocks) is one of my pet peeves.  At least put a comment in there explaining why you leave it empty, or better yet, dump the error object to the console as you have done in other cases.

>+  setDocumentTitle: function (title)
>+  {
>+    try {
>+      this.getCurrentEditor().setDocumentTitle(title);
>+
>+      // Update window title (doesn't work if called from a dialog)
>+      if ("UpdateWindowTitle" in window)
>+        window.UpdateWindowTitle();

if (typeof window.UpdateWindowTitle == "function")

>Index: content/composer/scripts.inc
>+  <script type="application/x-javascript" src="chrome://composer/content/utils/notifiers.js"/>
>+  <script type="application/x-javascript" src="chrome://composer/content/utils/l10n.js"/>
>+  <script type="application/x-javascript" src="chrome://composer/content/utils/prompter.js"/>

Nit:  application/javascript is now a registered content-type, so we can use that if you want.

>Index: content/composer/utils/l10n.js
>+  _getStringBundleService: function()
>+  {
>+    if (!this.mStringBundleService)
>+    {
>+      try {
>+        this.mStringBundleService =
>+            Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService); 
>+      } catch (ex) {}
>+    }
>+    return this.mStringBundleService;
>+  },

Again, long lines, empty catch blocks and anonymous functions.  Not nice; I won't repeat these for other files in this patch.

>Index: content/composer/utils/prompter.js
>+  alertWithTitle: function (aTitle, aMsg, aParentWindow)
>+  {
>+    if (!aParentWindow)
>+      aParentWindow = window;

Strict warning for re-defining an argument in the function.  Please don't do that.  Instead:

var parentWindow = aParentWindow ? aParentWindow : window;

>+  confirmWithTitle: function (aTitle, aMsg, aOkBtnText, aCancelBtnText)
>+  {
>+    var promptService;
>+    if ((promptService = this._getPromptService()))

This is hiding a strict warning that you don't need to hide.

var promptService = this._getPromptService();
if (promptService) {
  // ...
}

>+    {
>+      var okFlag = aOkBtnText ? promptService.BUTTON_TITLE_IS_STRING
>+                               : promptService.BUTTON_TITLE_OK;
>+      var cancelFlag = aCancelBtnText ? promptService.BUTTON_TITLE_IS_STRING
>+                                       : promptService.BUTTON_TITLE_CANCEL;

Maybe line up the colon with the question mark?  Also, I'd prefer you actually referred to the constants on Components.interfaces.nsIPromptService directly instead of the variable promptService.

>+      return this.mPromptService.confirmEx(window, aTitle, aMsg,

Well, if you've already got promptService, why refer to this.mPromptService?  Save on line length again.

>Index: content/composer/utils/urls.js
>+      var c0 = noBackSlashUrl[0];
>+      var c1 = noBackSlashUrl[1]

Missing trailing semicolon.

>+      if (c0 == '/' ||
>+          ( ((c0 >= 'a' && c0 <= 'z') || (c0 >= 'A' && c0 <= 'Z')) && c1 == ":"))

if ((c0 == '/') || ((/^[a-zA-z]/.test(c0)) && (c1 == ":") ))
Regular expressions can be your friend.

>+        var p = new Object();

var p = {};

>+  getIOService: function ()
>+  {
>+    if (this.mIOService)
>+      return this.mIOService;
>+
>+    this.mIOService = Components.classes["@mozilla.org/network/io-service;1"]
>+                      .getService(Components.interfaces.nsIIOService);

Suggest lining up the periods.  This is one case where extra spacing is actually appropriate.

>+  isTextIsURI: function (aText)

The function name is confusing.

>+  makeRelativeUrl: function (aURLString)
>+    // XXX Should we use GetCurrentEditor().documentCharacterSet for 2nd param ?
>+    var docPath = IOService.newURI(docUrl,   EditorUtils.getCurrentEditor().documentCharacterSet, null).path;
>+    var urlPath = IOService.newURI(inputUrl, EditorUtils.getCurrentEditor().documentCharacterSet, null).path;

Comment's a little confusing; it asks if we should use what the argument is.  It might be better to suggest in the comment alternatives.

Also, you're calling on getCurrentEditor twice, with the same argument afterwards.  Just grab the documentCharacterSet once beforehand, and then pass that in:

// XXX Should we use GetCurrentEditor().documentCharacterSet for 2nd param ?
var docCharSet = EditorUtils.getCurrentEditor().documentCharacterSet;
var docPath = IOService.newURI(docUrl,   docCharSet, null).path;
var urlPath = IOService.newURI(inputUrl, docCharSet, null).path;

>+    var nextDocSlash = 0;
>+    var done = false;
>+
>+    // Remove all matching subdirs common to both doc and input urls
>+    do {
>+      nextDocSlash = docPath.indexOf("\/");
>+      var nextUrlSlash = urlPath.indexOf("\/");

Suggest declaring the nextUrlSlash variable outside the look, or the nextDocSlash variable inside, for consistency.

>+
>+      if (nextUrlSlash == -1)
>+      {
>+        // We're done matching and all dirs in url
>+        // what's left is the filename
>+        done = true;
>+
>+        // Remove filename for named anchors in the same file
>+        if (nextDocSlash == -1 && docFilename)
>+        { 
>+          var anchorIndex = urlPath.indexOf("#");
>+          if (anchorIndex > 0)
>+          {
>+            var urlFilename = doCaseInsensitive ? urlPath.toLowerCase() : urlPath;
>+          
>+            if (urlFilename.indexOf(docFilename) == 0)
>+              urlPath = urlPath.slice(anchorIndex);
>+          }
>+        }
>+      }

Perhaps a break statement here?  (Although you set firstDirTest to false later on, it doesn't seem to be needed.)

>+        if (urlDir == docDir)
>+        {
>+
>+          // Remove matching dir+"/" from each path
>+          //  and continue to next dir
>+          docPath = docPath.slice(nextDocSlash+1);
>+          urlPath = urlPath.slice(nextUrlSlash+1);

Perhaps a continue statement here, with firstDirTest being set to true?

>+    // Add "../" for each dir left in docPath
>+    while (nextDocSlash > 0)
>+    {
>+      urlPath = "../" + urlPath;
>+      nextDocSlash = docPath.indexOf("\/", nextDocSlash+1);

Suggest spaces before and after the + sign here.

>+  getDocumentUrl: function ()
>+  {
>+    try {
>+      var aDOMHTMLDoc = EditorUtils.getCurrentEditor().document.QueryInterface(Components.interfaces.nsIDOMHTMLDocument);
>+      return aDOMHTMLDoc.URL;
>+    }
>+    catch (e) {}
>     return "";
>+  },

Good news:  QI is no longer explicitly needed, so you don't need a try-catch block for that.  (You might still need one for getCurrentEditor() calls; I don't know.)

var doc = EditorUtils.getCurrentEditor().document;
if (doc && doc instanceof Components.interfaces.nsIDOMHTMLDocument) {
  return doc.URL;
}

>+  getHost: function (aURLSpec)
>+      host = IOService.newURI(aURLSpec, null, null).host;

>+  getUsername: function (aURLSpec)
>+      username = IOService.newURI(aURLSpec, null, null).username;

>+  getFilename: function (aURLSpec)
>+      var uri = IOService.newURI(aURLSpec, null, null);
>+  },

Are all three of these called by code that's close to one another?  If so, it might be worth consolidating them into a single function, which returns a single JS object with all these values as properties.

>+
>+  stripUsernamePassword: function (aURLSpec, usernameObj, passwordObj)
>+  {
>+          if (usernameStart != -1)
>+            return urlspec.slice(0, usernameStart) + urlspec.slice(atIndex+1);

You can probably use the splice method as one call to remove the section you don't want, and then it falls into "return urlspec" below naturally.

>+  getFileProtocolHandler: function ()
>+  {
>+    var ios = this.getIOService();
>+    var handler = ios.getProtocolHandler("file");
>+    return handler.QueryInterface(Components.interfaces.nsIFileProtocolHandler);
>+  }

If QI throws an exception here, you're going to have a bad day...
(In reply to comment #4)

> >+    var nextDocSlash = 0;
> >+    var done = false;
> >+
> >+    // Remove all matching subdirs common to both doc and input urls
> >+    do {
> >+      nextDocSlash = docPath.indexOf("\/");
> >+      var nextUrlSlash = urlPath.indexOf("\/");
> 
> Suggest declaring the nextUrlSlash variable outside the look, or the
> nextDocSlash variable inside, for consistency.

Thanks for all comments, but I am not going to fix that one since
nextDocSlash is used _outside_ of the loop afterwards ; and that's why it's
declared before.
Comment on attachment 241704 [details] [diff] [review]
fix #1

r=ajvincent with nits addressed (glazou's approved me as a peer for chrome reviews)
Attachment #241704 - Flags: review?(neil) → review+
fixed and checked in (trunk)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: