Last Comment Bug 754579 - convert smtpEditOverlay.js and SmtpServerEdit.js to Services.jsm and MailServices.js
: convert smtpEditOverlay.js and SmtpServerEdit.js to Services.jsm and MailServ...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 15.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 720356 720358 755885
  Show dependency treegraph
 
Reported: 2012-05-12 07:41 PDT by :aceman
Modified: 2012-05-22 16:26 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (14.25 KB, patch)
2012-05-12 08:05 PDT, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v2 (26.60 KB, patch)
2012-05-12 10:17 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v3 (26.62 KB, patch)
2012-05-12 10:51 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v4 (26.72 KB, patch)
2012-05-16 13:37 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v5 (26.91 KB, patch)
2012-05-22 14:32 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-05-12 07:41:52 PDT
SmtpServerEdit.js:            Components.classes["@mozilla.org/messengercompose/smtp;1"].getService(Components.interfaces.nsISmtpService);
SmtpServerEdit.js:      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
smtpEditOverlay.js:var gPrefBranch = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
smtpEditOverlay.js:var gSmtpService = Components.classes["@mozilla.org/messengercompose/smtp;1"].getService(Components.interfaces.nsISmtpService);
smtpEditOverlay.js:    var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService);
Comment 1 :aceman 2012-05-12 08:05:03 PDT
Created attachment 623430 [details] [diff] [review]
patch
Comment 2 Ian Neal 2012-05-12 08:41:47 PDT
From what I can see the only consumer of smtpEditOverlay is SmtpServerEdit, so potentially worth merging the files (whether that is in the scope of this bug is another matter).
Comment 3 :aceman 2012-05-12 08:46:56 PDT
I would not be able to do that, so let's keep it outside the bug.
But it seems I forgot to throw am-smtp.js into the patch. That could be in scope.
Comment 4 Ian Neal 2012-05-12 09:19:04 PDT
Comment on attachment 623430 [details] [diff] [review]
patch

>+++ b/mailnews/base/prefs/content/SmtpServerEdit.js

> var smtpServer;
As this is a global it should probably be something like gSmtpServer
> 
> function onLoad(event)
> {
>+    let server = window.arguments[0].server;
>     initializeDialog(server);
As this is the only consumer, you might as well get rid of the initializeDialog function.
> }
> 
> function initializeDialog(server)
> {
>     smtpServer = server;
If you do:
gSmtpServer = window.arguments[0].server;
>     initSmtpSettings(server);
You can use gSmtpServer as the argument here.

>+++ b/mailnews/base/prefs/content/smtpEditOverlay.js

> // Disables xul elements that have associated preferences locked.
> function onLockPreference()
> {
>   try {
>+    let finalPrefString = "mail.smtpserver." +
>+      MailServices.smtp.defaultServer.key + ".";
> 
>+    let allPrefElements = {
>       hostname:     gSmtpHostname,
>       description:  gSmtpDescription,
>       port:         gSmtpPort,
>       authMethod:   gSmtpAuthMethod,
>       try_ssl:      gSmtpSocketType
>     };
> 
>+    gSmtpPrefBranch = Services.prefs.getBranch(finalPrefString);
>+    disableIfLocked(allPrefElements);
Rather than setting a global variable why not either pass it as an argument to the disableIfLocked function or just inline the disableIfLocked function.

> 
> // Does the work of disabling an element given the array which contains xul id/prefstring pairs.
> // Also saves the id/locked state in an array so that other areas of the code can avoid
> // stomping on the disabled state indiscriminately.
This comment is not correct.
>+function disableIfLocked(prefstrArray)
> {
>+  for (let prefstring in prefstrArray)
>     if (gSmtpPrefBranch.prefIsLocked(prefstring))
>       prefstrArray[prefstring].disabled = true;
> }

>+++ b/mailnews/base/prefs/content/smtpEditOverlay.xul
>@@ -42,54 +42,61 @@
> 
> <overlay xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
>   <script type="application/javascript"
>           src="chrome://messenger/content/smtpEditOverlay.js"/>
> 
>   <vbox id="smtpServerEditor">
> 
>+    <separator class="thin"/>
> 
>       <groupbox>
The indentation is incorrect from here down.
>         <caption label="&security.caption;"/>
> 
>         <grid flex="1">
>           <columns>
>             <column/>
>             <column flex="1"/>
>@@ -137,11 +144,11 @@
>                      accesskey="&userName.accesskey;"
>                      control="smtp.username"/>
>               <textbox id="smtp.username" flex="1"
>                        preftype="string"
>                        prefstring="mail.smtpserver.%serverkey%.username"/>
>             </row>
>           </rows>
>         </grid>
>-     </groupbox>
>+      </groupbox>
This is not correctly indented either.
>   </vbox>
> </overlay>
Comment 5 :aceman 2012-05-12 09:24:34 PDT
(In reply to Ian Neal from comment #4)
> >+    <separator class="thin"/>
> > 
> >       <groupbox>
> The indentation is incorrect from here down.
> >         <caption label="&security.caption;"/>
> > 
> >         <grid flex="1">
> >           <columns>
> >             <column/>
> >             <column flex="1"/>
> >@@ -137,11 +144,11 @@
> >                      accesskey="&userName.accesskey;"
> >                      control="smtp.username"/>
> >               <textbox id="smtp.username" flex="1"
> >                        preftype="string"
> >                        prefstring="mail.smtpserver.%serverkey%.username"/>
> >             </row>
> >           </rows>
> >         </grid>
> >-     </groupbox>
> >+      </groupbox>
> This is not correctly indented either.
> >   </vbox>
> > </overlay>

I didn't want to reindent the whole file, but I can do it :)
Comment 6 Ian Neal 2012-05-12 09:32:11 PDT
(In reply to :aceman from comment #5)
> (In reply to Ian Neal from comment #4)
> > >+    <separator class="thin"/>
> > > 
> > >       <groupbox>
> > The indentation is incorrect from here down.
> > >         <caption label="&security.caption;"/>
> > > 
> > >         <grid flex="1">
> > >           <columns>
> > >             <column/>
> > >             <column flex="1"/>
> > >@@ -137,11 +144,11 @@
> > >                      accesskey="&userName.accesskey;"
> > >                      control="smtp.username"/>
> > >               <textbox id="smtp.username" flex="1"
> > >                        preftype="string"
> > >                        prefstring="mail.smtpserver.%serverkey%.username"/>
> > >             </row>
> > >           </rows>
> > >         </grid>
> > >-     </groupbox>
> > >+      </groupbox>
> > This is not correctly indented either.
> > >   </vbox>
> > > </overlay>
> 
> I didn't want to reindent the whole file, but I can do it :)
Well, if not the whole file, the last change needs to be correctly indented.
Comment 7 :aceman 2012-05-12 09:45:54 PDT
(In reply to Ian Neal from comment #4)
> > // Does the work of disabling an element given the array which contains xul id/prefstring pairs.
> > // Also saves the id/locked state in an array so that other areas of the code can avoid
> > // stomping on the disabled state indiscriminately.
> This comment is not correct.
> >+function disableIfLocked(prefstrArray)
> > {
> >+  for (let prefstring in prefstrArray)
> >     if (gSmtpPrefBranch.prefIsLocked(prefstring))
> >       prefstrArray[prefstring].disabled = true;
> > }

There is the same comment in am-offline.js where the code does what it says.
I wonder if the code here shouldn't do the same. It actually never disables any elements, just sets the array values, that are never used.
Comment 8 :aceman 2012-05-12 10:03:00 PDT
As said, there are similar functions in am-offline.js. It seems these features should be consolidated in am-prefs.js (that is commented as being the place for that). But I am not up to that now.

So, do we want to find out what disableIfLocked in smtpEditrOverlay.js is supposed to do? Or we just ignore it even if it is broken as fixing it may not be in scope here?
Comment 9 :aceman 2012-05-12 10:17:09 PDT
Created attachment 623442 [details] [diff] [review]
patch v2

What about this?
Comment 10 Ian Neal 2012-05-12 10:36:04 PDT
(In reply to :aceman from comment #8)
> As said, there are similar functions in am-offline.js. It seems these
> features should be consolidated in am-prefs.js (that is commented as being
> the place for that). But I am not up to that now.
> 
> So, do we want to find out what disableIfLocked in smtpEditrOverlay.js is
> supposed to do? Or we just ignore it even if it is broken as fixing it may
> not be in scope here?
prefstrArray[prefstring] is the XUL element, so it is setting the XUL element to be disabled.
Comment 11 :aceman 2012-05-12 10:51:14 PDT
Created attachment 623443 [details] [diff] [review]
patch v3

Those JS arrays sometimes deceive me :)
Comment 12 Ian Neal 2012-05-15 15:02:30 PDT
Comment on attachment 623443 [details] [diff] [review]
patch v3

>+++ b/mailnews/base/prefs/content/am-smtp.js

>   refreshServerList: function(aServerKeyToSelect, aFocusList)
>   {
>     // remove all children
>     while (this.mServerList.hasChildNodes())
>       this.mServerList.removeChild(this.mServerList.lastChild);
> 
>+    var defaultServer = MailServices.smtp.defaultServer;
>+    this.fillSmtpServers(this.mServerList, MailServices.smtp.smtpServers, defaultServer);
To be consistent either get rid of the defaultServer variable and inline it into the call, or add one for smtpServers.

>+++ b/mailnews/base/prefs/content/smtpEditOverlay.js

> // Disables xul elements that have associated preferences locked.
> function onLockPreference()
> {
>   try {
>+    let allPrefElements = {
>       hostname:     gSmtpHostname,
>       description:  gSmtpDescription,
>       port:         gSmtpPort,
>       authMethod:   gSmtpAuthMethod,
>       try_ssl:      gSmtpSocketType
>     };
>+    disableIfLocked(allPrefElements);
>+  } catch (e) { // non-fatal
>+    Components.utils.reportError("Error while getting locked prefs: " + e);
>+  }
>+}
> 
>+/**
>+ * Does the work of disabling an element given the array which contains xul id/prefstring pairs.
>+ *
>+ * @param prefstrArray  array of XUL elements to check
>+ */
>+function disableIfLocked(prefstrArray)
>+{
>+  let finalPrefString = "mail.smtpserver." +
>+    MailServices.smtp.defaultServer.key + ".";
>+  let smtpPrefBranch = Services.prefs.getBranch(finalPrefString);
> 
>+  for (let prefstring in prefstrArray)
>+    if (smtpPrefBranch.prefIsLocked(prefstring))
>       prefstrArray[prefstring].disabled = true;
> }
I still don't see why you need a separate disableIfLocked function, unless you are planning for some future shared function.
If not, then just merge disableIfLocked into the onLockPreference function.

r=me with those issues addressed.
Comment 13 :aceman 2012-05-16 13:37:27 PDT
Created attachment 624511 [details] [diff] [review]
patch v4

Ian, yes, I'll try to merge disableIfLocked in bug 755885.
Comment 14 Mike Conley (:mconley) 2012-05-22 13:52:14 PDT
Comment on attachment 624511 [details] [diff] [review]
patch v4

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

I'm pretty happy with this, except for the nits I found.

With those fixed, r=me.

::: mailnews/base/prefs/content/SmtpServerEdit.js
@@ +41,2 @@
>  
> +var gSmtpServer;

We prefer let over var.

@@ +43,4 @@
>  
>  function onLoad(event)
>  {
> +    gSmtpServer = window.arguments[0].server;

While you're here, these lines should only be indented 2 spaces.

@@ +48,5 @@
>  }
>  
>  function onAccept()
>  {
>      if (hostnameIsIllegal(gSmtpHostname.value)) {

This function looks great, but each line needs to have two spaces dropped from its indentation.
Comment 15 :aceman 2012-05-22 14:32:03 PDT
Created attachment 626185 [details] [diff] [review]
patch v5
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-05-22 16:26:12 PDT
https://hg.mozilla.org/comm-central/rev/b7a65151271e

Note You need to log in before you can comment on or make changes to this bug.