795158 - Switch to Services.jsm: /editor/ui/

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/, v1

This bug is for switching /editor/ui/ to use Services.jsm, see https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm for more information.

I haven't tested the patch (Seamonkey try server seems to be burning today).

Comment 1

[Philip Chee]

I think you can test now.

Comment 2

[Ian Neal]

As this code is shared, moving to a shared product/component.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The patch passed tests on Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=8d5653062b63 (I only tested Windows because the code is platform independent.)

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 665684 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/, v1

Review of attachment 665684 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, i'm not an /editor reviewer. Try Ian maybe?

Comment 5

[Ian Neal]

Comment on attachment 665684 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/, v1

This won't be a full review yet, just where I spot Services.jsm imports are not needed and anything else that jumps out at me.
I would prefer this patch is split into smaller parts (to make it easier to review) - for example: do the Services.prefs in one patch, Services.prompt in another, Services.io in a 3rd and the rest of the Services in a final patch.

>+++ b/editor/ui/composer/content/ComposerCommands.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported elsewhere within the files that load this JS file.

>       // First check pref to always show publish dialog
>       try {
>+        showPublishDialog = Services.prefs.getBoolPref("editor.always_show_publish_dialog");
>       } catch(e) {}
As "editor.always_show_publish_dialog" has a default set in composer.js, it does not need a try/catch so please remove the try/catch whilst you are here.


>   {
>     try {
>       // Should we prettyprint? Check the pref
>+      if (Services.prefs.getBoolPref("editor.prettyprint"))
>         outputFlags |= webPersist.ENCODE_FLAGS_FORMATTED;
As "editor.prettyprint" has a default set in composer.js, the start of the try can be moved to just before the "editor.encode_entity" retrieval.
> 
>       // How much entity names should we output? Check the pref
>+      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");

>     // First check pref for saving associated files
>     var saveAssociatedFiles = false;
>     try {
>-      var prefs = GetPrefs();
>-      saveAssociatedFiles = prefs.getBoolPref("editor.save_associated_files");
>+      saveAssociatedFiles = Services.prefs.getBoolPref("editor.save_associated_files");
>     } catch (e) {}
> 
>     // Only change links or move files if pref is set 
>     //  and we are saving to a new location
>     if (saveAssociatedFiles && aSaveAs)
As "editor.save_associated_files" has a default set in composer.js, it does not need a try/catch so please remove the try/catch whilst you are here and simplify the code into the if condition.

>@@ -2697,36 +2640,35 @@ var nsHLineCommand =

>+        var width = Services.prefs.getIntPref("editor.hrule.width");
>+        var percent = Services.prefs.getBoolPref("editor.hrule.width_percent");
Nit: You could inline percent here.
>         if (percent)
>           width = width +"%";

>+        var shading = Services.prefs.getBoolPref("editor.hrule.shading");
Nit: You could inline shading here.
>         if (shading)
>           hLine.removeAttribute("noshade");
>         else
>           hLine.setAttribute("noshade", "noshade");

>+++ b/editor/ui/composer/content/editingOverlay.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported elsewhere within the files that load this JS file.

>+++ b/editor/ui/composer/content/editor.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported elsewhere within the files that load this JS file.

>     try { 
>+      var prettyPrint = Services.prefs.getBoolPref("editor.prettyprint");
>       if (prettyPrint)
>         flags |= kOutputFormatted;
> 
>     } catch (e) {}
As "editor.prettyprint" has a default set in composer.js, the try/catch can probably be removed and inline prettyPrint.

>   var historyCount = 10;
>   try {
>+    historyCount = Services.prefs.getIntPref("editor.history.url_maximum"); 
>   } catch(e) {}
As "history.url_maximum" has a default set in composer.js, the try/catch can probably be removed.

>   // Get editor color prefs
>   var use_custom_colors = false;
>   try {
>+    use_custom_colors = Services.prefs.getBoolPref("editor.use_custom_colors");
>   }
>   catch (ex) {}
As "editor.use_custom_colors" has a default set in all.js, the try/catch can probably be removed.

>       // try to get the default color values.  ignore them if we don't have them.
>       var text_color;
>       var link_color;
>       var active_link_color;
>       var followed_link_color;
>       var background_color;
> 
>+      try { text_color = Services.prefs.getCharPref("editor.text_color"); } catch (e) {}
>+      try { link_color = Services.prefs.getCharPref("editor.link_color"); } catch (e) {}
>+      try { active_link_color = Services.prefs.getCharPref("editor.active_link_color"); } catch (e) {}
>+      try { followed_link_color = Services.prefs.getCharPref("editor.followed_link_color"); } catch (e) {}
>+      try { background_color = Services.prefs.getCharPref("editor.background_color"); } catch(e) {}
As these all have a default set in composer.js, the try/catch can probably be removed from each of them and inline each of them too.

> function FindEditorWithInsertCharDialog()
> {
>   try {
>     // Find window with an InsertCharsWindow and switch association to this one
>+    var enumerator = Services.wm.getEnumerator( null );
Nit: remove spaces from around null.

> function SwitchInsertCharToAnotherEditorOrClose()
> {
>   if ("InsertCharWindow" in window && window.InsertCharWindow)
>   {
>     var enumerator;
>     try {
>+      enumerator = Services.wm.getEnumerator( null );
Nit: remove spaces from around null.

>+++ b/editor/ui/composer/content/editorApplicationOverlay.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported elsewhere within the files that load this JS file and use the editPage function.

>+++ b/editor/ui/composer/content/pref-editing.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported elsewhere within the files that load this JS file.

>+++ b/editor/ui/composer/content/publishprefs.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported elsewhere within the files that load this JS file.

>+++ b/editor/ui/dialogs/content/EdColorPicker.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdConvertToTable.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdHLineProps.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdImageOverlay.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdInsertTable.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdLinkChecker.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdPageProps.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EdTableProps.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EditorPublish.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EditorPublishProgress.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EditorPublishSettings.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

>+++ b/editor/ui/dialogs/content/EditorSaveAsCharset.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
Not needed as imported by the change you have made to editorUtilities.js

r- for the moment so I can review the smaller patches in more detail.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.prefs, v1

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.io, v1

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.prompt, v1

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.<other>, v1

Comments from last time have been fixed.

Try run with all four patches applied: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=92547f382b84

Comment 10

[Ian Neal]

Comment on attachment 690312 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.prefs, v1

>+++ b/editor/ui/composer/content/ComposerCommands.js
>+      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");
>       switch (encodeEntity) {
You could inline this pref.

>+++ b/editor/ui/composer/content/editor.js
>     try { 
>-      var encodeEntity = gPrefs.getCharPref("editor.encode_entity");
>+      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");
>       switch (encodeEntity) {
>         case "basic"  : flags = kOutputEncodeBasicEntities; break;
>         case "latin1" : flags = kOutputEncodeLatin1Entities; break;
>         case "html"   : flags = kOutputEncodeHTMLEntities; break;
>         case "none"   : flags = 0;     break;
>       }
>     } catch (e) { }
You removed the try/catch in ComposerCommands.js for this code.

>-      // try to get the default color values.  ignore them if we don't have them.
>-      var text_color;
>-      var link_color;
>-      var active_link_color;
>-      var followed_link_color;
>-      var background_color;
>-
>-      try { text_color = gPrefs.getCharPref("editor.text_color"); } catch (e) {}
>-      try { link_color = gPrefs.getCharPref("editor.link_color"); } catch (e) {}
>-      try { active_link_color = gPrefs.getCharPref("editor.active_link_color"); } catch (e) {}
>-      try { followed_link_color = gPrefs.getCharPref("editor.followed_link_color"); } catch (e) {}
>-      try { background_color = gPrefs.getCharPref("editor.background_color"); } catch(e) {}
Are you sure that these will not throw?

>+++ b/editor/ui/composer/content/editorUtilities.js

> function GetStringPref(name)
> {
>   try {
>+    return Services.prefs.getComplexValue(name, Components.interfaces.nsISupportsString).data;
>   } catch (e) {}
>   return "";
> }
How is this different to GetUnicharPref?

> 
>+function SetUnicharPref(aPrefName, aPrefValue)
> {
>   try {

>+    var str = Components.classes["@mozilla.org/supports-string;1"]
>+                        .createInstance(Components.interfaces.nsISupportsString);
>+    str.data = aPrefValue;
>+    Services.prefs.setComplexValue(aPrefName, Components.interfaces.nsISupportsString, str);
>   }
>+  catch(e) {}
> }
> 
> function GetUnicharPref(aPrefName, aDefVal)
> {
>+  try {
>+    return Services.prefs.getComplexValue(aPrefName, Components.interfaces.nsISupportsString).data;
>   }
>+  catch(e) {}
You've lost the return "";

> }
There is a place in editor.js that could use this function too.

Comment 11

[Ian Neal]

Comment on attachment 690316 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.prompt, v1

>+++ b/editor/ui/composer/content/ComposerCommands.js

>+    // Reload page if first button (Revert) was pressed
>+    if(result == 0)
Nit: Space between if and (

>+++ b/editor/ui/composer/content/editorUtilities.js
>+  if (!title)
>+    title = GetString("Alert");
> 
>+  // "window" is the calling dialog window
>+  Services.prompt.alert(parentWindow, title, message);
Could inline title so:
Services.prompt.alert(parentWindow, title || GetString("Alert"), message);

Comment 12

[Ian Neal]

Comment on attachment 690317 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.<other>, v1

>+++ b/editor/ui/composer/content/publishprefs.js

>+  // Remove existing entry by finding all logins that match
Nit: end comment with full stop.

>+  // If SavePassword is true, add new password
Nit: end comment with full stop.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Ian Neal from comment #10)
> Comment on attachment 690312 [details] [diff] [review]
> Switch to Services.jsm: /editor/ui/: Services.prefs, v1
> 
> >+++ b/editor/ui/composer/content/editor.js
> >     try { 
> >-      var encodeEntity = gPrefs.getCharPref("editor.encode_entity");
> >+      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");
> >       switch (encodeEntity) {
> >         case "basic"  : flags = kOutputEncodeBasicEntities; break;
> >         case "latin1" : flags = kOutputEncodeLatin1Entities; break;
> >         case "html"   : flags = kOutputEncodeHTMLEntities; break;
> >         case "none"   : flags = 0;     break;
> >       }
> >     } catch (e) { }
> You removed the try/catch in ComposerCommands.js for this code.
No, ComposerCommands.js also still calls editor.encode_entity in a try catch block because it doesn't ship by default.

> >-      // try to get the default color values.  ignore them if we don't have them.
> >-      var text_color;
> >-      var link_color;
> >-      var active_link_color;
> >-      var followed_link_color;
> >-      var background_color;
> >-
> >-      try { text_color = gPrefs.getCharPref("editor.text_color"); } catch (e) {}
> >-      try { link_color = gPrefs.getCharPref("editor.link_color"); } catch (e) {}
> >-      try { active_link_color = gPrefs.getCharPref("editor.active_link_color"); } catch (e) {}
> >-      try { followed_link_color = gPrefs.getCharPref("editor.followed_link_color"); } catch (e) {}
> >-      try { background_color = gPrefs.getCharPref("editor.background_color"); } catch(e) {}
> Are you sure that these will not throw?
They ship both by default for Thunderbird and SeaMonkey because both get them from http://mxr.mozilla.org/comm-central/source/editor/ui/composer.js . If you think that this isn't safe enough or the try catch block should remain for third parties using the code without having the preferences in the default file, please tell me.

> >+++ b/editor/ui/composer/content/editorUtilities.js
> 
> > function GetStringPref(name)
> > {
> >   try {
> >+    return Services.prefs.getComplexValue(name, Components.interfaces.nsISupportsString).data;
> >   } catch (e) {}
> >   return "";
> > }
> How is this different to GetUnicharPref?
Removed GetUnicharPref and replaced its calls with GetStringPref.
> 

Successful Thunderbird-Try run:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9e08404b9c53&noignore=1

Comment 14

[Ian Neal]

(In reply to Archaeopteryx [:aryx] from comment #13)
> (In reply to Ian Neal from comment #10)
> > Comment on attachment 690312 [details] [diff] [review]
> > Switch to Services.jsm: /editor/ui/: Services.prefs, v1
> > 
> > >+++ b/editor/ui/composer/content/editor.js
> > >     try { 
> > >-      var encodeEntity = gPrefs.getCharPref("editor.encode_entity");
> > >+      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");
> > >       switch (encodeEntity) {
> > >         case "basic"  : flags = kOutputEncodeBasicEntities; break;
> > >         case "latin1" : flags = kOutputEncodeLatin1Entities; break;
> > >         case "html"   : flags = kOutputEncodeHTMLEntities; break;
> > >         case "none"   : flags = 0;     break;
> > >       }
> > >     } catch (e) { }
> > You removed the try/catch in ComposerCommands.js for this code.
> No, ComposerCommands.js also still calls editor.encode_entity in a try catch
> block because it doesn't ship by default.
Sorry, misread that change somehow.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.prefs, v2, r=IanN

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.io, v2, r=IanN

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.prompt, v2, r=IanN

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Switch to Services.jsm: /editor/ui/: Services.<other>, v2, r=IanN

Comment 19

[Ian Neal]

Someone from the TB side will need to review before checkin.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698113 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.prefs, v2, r=IanN

Review of attachment 698113 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Sebastian,

This looks pretty good - I'm just a little concerned about some try-catch's you've removed. Also just some style nits.

Thanks,

-Mike

::: editor/ui/composer/content/ComposerCommands.js
@@ +649,3 @@
>  
>        // First check pref to always show publish dialog
> +      var showPublishDialog = Services.prefs.getBoolPref("editor.always_show_publish_dialog");

let, not var

@@ +959,5 @@
>      outputFlags |= webPersist.ENCODE_FLAGS_FORMATTED;
>    }
>    else
>    {
> +    // Should we prettyprint? Check the pref

Why was this moved outside of the try-catch? Is the try-catch there in order to catch the case where this pref does not exist?

@@ +968,2 @@
>        // How much entity names should we output? Check the pref
> +      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");

let, not var

@@ +1703,5 @@
>      }
>  
>      // this is the location where the related files will go
>      var relatedFilesDir = null;
>      

Please remove this trailing whitespace while you're here.

@@ +1708,1 @@
>      // Only change links or move files if pref is set 

Please fix the trailing whitespace on this line, and the indentation on line 1709 while you're here.

@@ +1708,3 @@
>      // Only change links or move files if pref is set 
>      //  and we are saving to a new location
> +    if (Services.prefs.getBoolPref("editor.save_associated_files") && aSaveAs)

It looks to me as if the try-catch block was important before...are we able to guarantee that editor.save_associated_files exists? If not, this will throw.

@@ +2700,5 @@
>        try {
>          hLine = editor.createElementWithDefaults(tagName);
>  
>          // We change the default attributes to those saved in the user prefs
> +        var align = Services.prefs.getIntPref("editor.hrule.align");

let, not var

@@ +2708,5 @@
>            editor.setAttributeOrEquivalent(hLine, "align", "right", true);
>  
>          //Note: Default is center (don't write attribute)
>    
> +        var width = Services.prefs.getIntPref("editor.hrule.width");

let, not var

@@ +2714,5 @@
>            width = width +"%";
>  
>          editor.setAttributeOrEquivalent(hLine, "width", width, true);
>  
> +        var height = Services.prefs.getIntPref("editor.hrule.height");

let, not var

::: editor/ui/composer/content/editor.js
@@ +83,5 @@
>  }
>  
>  function ShowHideToolbarButtons()
>  {
> +  var array = Services.prefs.getChildList(kEditorToolbarPrefs);

let, not var

@@ +134,5 @@
>      if (prefName == kUseCssPref)
>      {
>        var cmd = document.getElementById("cmd_highlight");
>        if (cmd) {
> +        var useCSS = Services.prefs.getBoolPref(prefName);

let, not var

@@ +1160,5 @@
>      gColorObj.SelectedType = gColorObj.Type;
>    }
>    else
>    {
> +    var IsCSSPrefChecked = Services.prefs.getBoolPref(kUseCssPref);

let, not var

@@ +1594,5 @@
>      var flags = (editor.documentCharacterSet == "ISO-8859-1")
>        ? kOutputEncodeLatin1Entities
>        : kOutputEncodeBasicEntities;
>      try { 
> +      var encodeEntity = Services.prefs.getCharPref("editor.encode_entity");

let, not var

@@ +1603,5 @@
>          case "none"   : flags = 0;     break;
>        }
>      } catch (e) { }
>  
> +    if (Services.prefs.getBoolPref("editor.prettyprint"))

We might still want a try-catch here in case editor.prettyprint is not defined.

@@ +1860,2 @@
>    var curUrl = StripPassword(GetDocumentUrl());
> +  var historyCount = Services.prefs.getIntPref("editor.history.url_maximum");

Same as above - if editor.history.url_maximum is not guaranteed to exist in the prefs database, we should wrap in a try-catch to handle the case where it's missing.

@@ +1872,5 @@
>    }
>  
>    for (var i = 0; i < historyCount && urlArray.length < historyCount; i++)
>    {
> +    var url = GetStringPref("editor.history_url_" + i);

let, not var.

@@ +1880,5 @@
>  
>      // Skip over current an "data" URLs
>      if (url && url != curUrl && GetScheme(url) != "data")
>      {
> +      var title = GetStringPref("editor.history_title_" + i);

let not var for these two

@@ +2253,5 @@
>    // find body node
>    var bodyelement = GetBodyElement();
>    if (bodyelement)
>    {
> +    if (Services.prefs.getBoolPref("editor.use_custom_colors"))

Same as above, re: try-catch block for missing pref value.

@@ +2258,2 @@
>      {
> +      var text_color = Services.prefs.getCharPref("editor.text_color");

let, not var for these two.

@@ +2261,4 @@
>  
>        // add the color attributes to the body tag.
>        // and use them for the default text and background colors if not empty
> +      editor.setAttributeOrEquivalent(bodyelement, "text", text_color, true);

As above, I don't think we can guarantee the presence of any of these values in the prefs DB. They'll throw, so I think we need the try-catch.

@@ +2271,4 @@
>      }
>      // Default image is independent of Custom colors???
>      try {
> +      var background_image = Services.prefs.getCharPref("editor.default_background_image");

let, not var.

::: editor/ui/composer/content/editorUtilities.js
@@ +478,3 @@
>  {
>    try {
> +    var str = Components.classes["@mozilla.org/supports-string;1"]

let not var

@@ +496,2 @@
>  
> +      var location = Services.prefs.getComplexValue("editor.lastFileLocation."+fileType,

let, not var. Spaces on either side of +.

@@ +512,5 @@
>        var fileDir;
>        if (filePicker.file.parent)
>          fileDir = filePicker.file.parent.QueryInterface(Components.interfaces.nsILocalFile);
>  
> +        Services.prefs.setComplexValue("editor.lastFileLocation."+fileType,

Spaces on either side of +

@@ +518,1 @@
>      

Mind removing this trailing whitespace while you're here?

::: editor/ui/dialogs/content/EdColorPicker.js
@@ +47,5 @@
>    {
>      ColorType = gColorObj.Type;
>      // Get string for dialog title from passed-in type 
>      //   (note constraint on editor.properties string name)
> +    var IsCSSPrefChecked = Services.prefs.getBoolPref("editor.use_css");

let, not var

::: editor/ui/dialogs/content/EdConvertToTable.js
@@ +295,3 @@
>      var firstRow;
>      try {
> +      if (Services.prefs.getBoolPref("editor.table.maintain_structure") )

We don't need the space before the last parenthesis.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

I've just read Ian's review, and now realize that I'm contradicting him. Sorry about that.

Yeah, you can leave the try-catch's out. :)

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698114 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.io, v2, r=IanN

Review of attachment 698114 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the var's switched to let's, and the whitespace removed. Thanks!

::: editor/ui/composer/content/ComposerCommands.js
@@ +747,5 @@
>    // check for existing file name we can use
>    if (aDocumentURLString && !IsUrlAboutBlank(aDocumentURLString))
>    {
>      try {
> +      var docURI = Services.io.newURI(aDocumentURLString,

let, not var

@@ +807,2 @@
>      var fileHandler = GetFileProtocolHandler();
>      

Please remove this trailing whitespace while you're here.

@@ +807,5 @@
>      var fileHandler = GetFileProtocolHandler();
>      
>      var isLocalFile = true;
>      try {
> +      var docURI = Services.io.newURI(aDocumentURLString, GetCurrentEditor().documentCharacterSet, null);

let, not var

::: editor/ui/composer/content/editorUtilities.js
@@ +450,3 @@
>  function GetFileProtocolHandler()
>  {
> +  var handler = Services.io.getProtocolHandler("file");

let, not var

@@ +582,5 @@
>  
>  
>    // Get just the file path part of the urls
>    // XXX Should we use GetCurrentEditor().documentCharacterSet for 2nd param ?
> +  var docPath = Services.io.newURI(docUrl, GetCurrentEditor().documentCharacterSet, null).path;

let, not var for these two

@@ +697,3 @@
>    // Make a URI object to use its "resolve" method
>    var absoluteUrl = resultUrl;
> +  var docUri = Services.io.newURI(docUrl, GetCurrentEditor().documentCharacterSet, null);

let, not var

@@ +793,4 @@
>    var filename;
>  
>    try {
> +    var uri = Services.io.newURI(urlspec, null, null);

let, not var

@@ +823,5 @@
>    var atIndex = urlspec.indexOf("@");
>    if (atIndex > 0)
>    {
>      try {
> +      var uri = Services.io.newURI(urlspec, null, null);

let, not var

@@ +856,5 @@
>    var atIndex = urlspec.indexOf("@");
>    if (atIndex > 0)
>    {
>      try {
> +      var password = Services.io.newURI(urlspec, null, null).password;

let, not var

@@ +900,5 @@
>    if (!urlspec || !username)
>      return urlspec;
>  
>    try {
> +    var URI = Services.io.newURI(urlspec, GetCurrentEditor().documentCharacterSet, null);

let, not var

::: editor/ui/composer/content/publishprefs.js
@@ +878,5 @@
>  {
>    if (!publishData || !publishData.publishUrl)
>      return false;
>  
> +  var url = Services.io.newURI(publishData.publishUrl, null, null);

let, not var

::: editor/ui/dialogs/content/EdImageOverlay.js
@@ +287,5 @@
>    try {
>      // Remove the image URL from image cache so it loads fresh
>      //  (if we don't do this, loads after the first will always use image cache
>      //   and we won't see image edit changes or be able to get actual width and height)
>      

Please remove this trailing whitespace.

@@ +296,2 @@
>      {
> +      var uri = Services.io.newURI(imageSrc, null, null);

let, not var

@@ +296,5 @@
>      {
> +      var uri = Services.io.newURI(imageSrc, null, null);
> +      if (uri)
> +      {
> +        var imgCacheService = Components.classes["@mozilla.org/image/cache;1"].getService();

let, not var for these two

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698113 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.prefs, v2, r=IanN

My last review was an r- because of the try-catch thing. But Ian was right when he said we don't need the try-catch's, so this is r=me with the style fixes I pointed out fixed.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698115 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.prompt, v2, r=IanN

Review of attachment 698115 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these nits fixed.

::: editor/ui/composer/content/ComposerCommands.js
@@ +863,4 @@
>    var result = {value:null};
>    var captionStr = GetString("DocumentTitle");
>    var msgStr = GetString("NeedDocTitle") + '\n' + GetString("DocTitleHelp");
> +  var confirmed = Services.prompt.prompt(window, captionStr, msgStr, result, null, {value:0});

let, not var

@@ +1264,5 @@
>        DumpDebugStatus(aStatus);
>  
>        if (gPersistObj)
>        {
> +        if (gPersistObj.currentState == gPersistObj.PERSIST_STATE_READY)

Thanks for cleaning up the style here. :)

@@ +1438,5 @@
>        if (!userObj.value)
>          userObj.value = gPublishData.username;
>      }
>  
> +    ret = Services.prompt.promptUsernameAndPassword(gProgressDialog ? gProgressDialog : window, 

Please fix the trailing whitespace here.

@@ +2009,5 @@
>    doCommand: function(aCommand)
>    {
>      // Confirm with the user to abandon current changes
> +    // Put the page title in the message string
> +    var title = GetDocumentTitle();

Please replace the var's with let's

@@ +3622,5 @@
>          // We need a cell and either > 1 selected cell or a cell to the right
>          //  (this cell may originate in a row spanned from above current row)
>          // Note that editor returns "td" for "th" also.
>          // (this is a pain! Editor and gecko use lowercase tagNames, JS uses uppercase!)
> +        if ( cell && (tagNameObj.value == "td"))

Nit - no space after the first (

::: editor/ui/composer/content/editorUtilities.js
@@ +38,5 @@
>  
>  function AlertWithTitle(title, message, parentWindow)
>  {
> +  // "window" is the calling dialog window
> +  Services.prompt.alert(parentWindow || window, title || GetString("Alert"), message);

Not even sure if AlertWithTitle really gains us anything except another level of indirection. Oh well.

@@ +44,5 @@
>  
>  // Optional: Caller may supply text to substitue for "Ok" and/or "Cancel"
>  function ConfirmWithTitle(title, message, okButtonText, cancelButtonText)
>  {
> +  var okFlag = okButtonText ? Services.prompt.BUTTON_TITLE_IS_STRING : Services.prompt.BUTTON_TITLE_OK;

let, not var for these two.

@@ +101,5 @@
>  }
>  
>  function TrimStringLeft(string)
>  {
> +  if (!string) return "";

Nit - please format like this:

if (!string)
  return "";

::: editor/ui/dialogs/content/EditorPublishProgress.js
@@ +298,5 @@
> +    const buttonFlags = (Services.prompt.BUTTON_TITLE_IS_STRING *
> +                         Services.prompt.BUTTON_POS_0) +
> +                        (Services.prompt.BUTTON_TITLE_CANCEL *
> +                         Services.prompt.BUTTON_POS_1);
> +    var button = Services.prompt.confirmEx(window,

let instead of var

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 698116 [details] [diff] [review]
Switch to Services.jsm: /editor/ui/: Services.<other>, v2, r=IanN

Review of attachment 698116 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few more nits, nothing major. Great work here!

::: editor/ui/composer/content/ComposerCommands.js
@@ +2130,5 @@
>      {
>        var browser;
>        try {
>          // Find a browser with this URL
> +        var enumerator = Services.wm.getEnumerator("navigator:browser");

let, not var

::: editor/ui/composer/content/editor.js
@@ +2806,5 @@
>  function FindEditorWithInsertCharDialog()
>  {
>    try {
>      // Find window with an InsertCharsWindow and switch association to this one
> +    var enumerator = Services.wm.getEnumerator(null);

let, not var

::: editor/ui/composer/content/editorApplicationOverlay.js
@@ +72,5 @@
>  
>    try {
>      var uri = createURI(url, null, null);
>  
> +    var enumerator = Services.wm.getEnumerator("composer:" + aFileType);

let, not var

::: editor/ui/composer/content/editorUtilities.js
@@ +60,5 @@
>  {
>    if (!gStringBundle)
>    {
>      try {
> +      gStringBundle = Services.strings.createBundle("chrome://editor/locale/editor.properties"); 

Please trim the trailing whitespace

@@ +77,5 @@
>  {
>    if (!gStringBundle)
>    {
>      try {
> +      var gStringBundle = Services.strings.createBundle("chrome://editor/locale/editor.properties"); 

See, this is the problem with var. gStringBundle is already declared at the top of the file. We don't need to re-var it.

Also, please trim the trailing whitespace.

::: editor/ui/composer/content/publishprefs.js
@@ +819,2 @@
>    var url = GetUrlForPasswordManager(publishData);
> +  var logins = Services.logins.findLogins({}, url, null, url);

let, not var

@@ +831,5 @@
>  {
>    if (!publishData || !publishData.publishUrl || !publishData.username)
>      return false;
>  
> +  var url = GetUrlForPasswordManager(publishData);

let, not var

@@ +836,3 @@
>  
> +  // Remove existing entry by finding all logins that match.
> +  var logins = Services.logins.findLogins({}, url, null, url);

let, not var

@@ +846,5 @@
>  
> +  // If SavePassword is true, add new password.
> +  if (publishData.savePassword)
> +  {
> +    var authInfo = Components.classes["@mozilla.org/login-manager/loginInfo;1"]

let, not var

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Services.prefs, v3, r=IanN,mconley

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Services.io, v3, r=IanN,mconley

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Services.prompt, v3, r=IanN,mconley

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Services.<other>, v3, r=IanN,mconley

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/65556fa263e6
https://hg.mozilla.org/comm-central/rev/8a508a9ee485
https://hg.mozilla.org/comm-central/rev/e7f920afc8bd
https://hg.mozilla.org/comm-central/rev/c8d667fee51c

Comment 31

[Ian Neal]

Missed two in editor.js:
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#113
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#121