1237385 - deduplicate populateProtoSpecificBox

Status: RESOLVED FIXED

(Instantbird Graveyard :: Account manager, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

deduplicate populateProtoSpecificBox[1] through moving the shared/duplicate code into helper functions in a separate .jsm

[1] https://dxr.mozilla.org/comm-central/search?q=populateprotospecificbox&redirect=true

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #0)
> deduplicate populateProtoSpecificBox[1] through moving the shared/duplicate
> code into helper functions in a separate .jsm

Since it's not good to use .jsm as file stays in memory until the end of the process after loading it, we will use .js file and it will be included with a <script> tag in the xul files.

Comment 3

[aleth [:aleth]]

Comment on attachment 8705645 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

::: chat/content/imAccountHelper.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: blank line after the header

Let's call this file accountOptionsHelper.js or imAccountOptionsHelper.js.

@@ +6,5 @@
> +function populateProtoSpecificBox(aDocument) {
> +  this.document = aDocument;
> +}
> +
> +populateProtoSpecificBox.prototype = {

better to do
var accountOptionsHelper = { ...

@@ +63,5 @@
> +    let visible = false;
> +    for (let opt in aOptions) {
> +      let text = opt.label;
> +      let name = aId + "-" + opt.name;
> +      switch (opt.type) {

nit: no space after switch

@@ +70,5 @@
> +        chk.setAttribute("label", text);
> +        chk.setAttribute("id", name);
> +        if (opt.getBool())
> +          chk.setAttribute("checked", "true");
> +        if(aAttributes && aAttributes[opt.typeBool])

see below

@@ +77,5 @@
> +        rows.appendChild(chk);
> +        break;
> +      case opt.typeInt:
> +        let txtNum = this.createTextbox("number", opt.getInt(), text, name);
> +        if(aAttributes && aAttributes[opt.typeInt])

nit: space after if, braces around for ... (as that block is two lines long)

@@ +84,5 @@
> +        rows.appendChild(txtNum);
> +        break;
> +      case opt.typeString:
> +        let txtStr = this.createTextbox(null, opt.getString(), text, name);
> +        if(aAttributes && aAttributes[opt.typeString])

same here

@@ +91,5 @@
> +        rows.appendChild(txtStr);
> +        break;
> +      case opt.typeList:
> +        let menuList = this.createMenulist(opt.getList(), text, name);
> +        if(aAttributes && aAttributes[opt.typeList])

and here

@@ +93,5 @@
> +      case opt.typeList:
> +        let menuList = this.createMenulist(opt.getList(), text, name);
> +        if(aAttributes && aAttributes[opt.typeList])
> +          for (let attr of aAttributes[opt.typeList])
> +            menuList.setAttribute(attr.name, attr.value);

Could you deduplicate these aAttribute loops by moving them below the switch statement?

::: im/content/account.js
@@ +143,5 @@
>      return aOpt.getListDefault();
>    },
>  
>    populateProtoSpecificBox: function account_populate() {
> +    let populateBox = new populateProtoSpecificBox(document);

You don't need to create a new object since a copy of the script is loaded into the local scope. You also don't need to pass document, you can just use it.

@@ +144,5 @@
>    },
>  
>    populateProtoSpecificBox: function account_populate() {
> +    let populateBox = new populateProtoSpecificBox(document);
> +    let visible = populateBox.addOptions(this.proto.id, this.getProtoOptions());

let haveOptions = ...

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

(In reply to aleth [:aleth] from comment #3)
> nit: no space after switch
According to coding style
> Control keywords if, for, while, and switch are always followed by
> a space to distinguish them from function calls which have no trailing space.

> Could you deduplicate these aAttribute loops by moving them below the switch
> statement?
Yes, it's done in this patch.

Comment 5

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #4)
> > nit: no space after switch
> According to coding style
> > Control keywords if, for, while, and switch are always followed by
> > a space to distinguish them from function calls which have no trailing space.

Ah OK, thanks!

Comment 6

[aleth [:aleth]]

Comment on attachment 8707987 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

Looks good! Only two small issues.

::: chat/content/imAccountOptionsHelper.js
@@ +50,5 @@
> +  },
> +
> +  // Adds options to UI according to their types with optional attributes for
> +  // each type and returns true if at least one option has been added to UI,
> +  // otherwise returns false.

Also mention what aId is for.

@@ +59,5 @@
> +      rows.lastChild.remove();
> +    let haveOptions = false;
> +    for (let opt in aOptions) {
> +      let text = opt.label;
> +      let name = aId + "-" + opt.name;

This will produce server.-name for TB am-im.js, which is wrong. Have you discovered why the name is different in am-im.js? What's the id attribute on the elements used for? Would be good to add a comment for the future. (Sorry I don't know this code very well either, flo-retina is probably the best person for questions about it if it's not clear from the code.)

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

(In reply to aleth [:aleth] from comment #6)
> This will produce server.-name for TB am-im.js, which is wrong. Have you
> discovered why the name is different in am-im.js? What's the id attribute on
> the elements used for? Would be good to add a comment for the future. (Sorry
> I don't know this code very well either, flo-retina is probably the best
> person for questions about it if it's not clear from the code.)

I asked florian about that, but he didn't remember that.

I checked some of the code and I noticed that this may be a convention used for naming elements which are related to server stuff.
(e.g. check [1] which it's usually used by variable named |server|)

[1] https://dxr.mozilla.org/comm-central/source/mail/components/im/imIncomingServer.js#68

Comment 8

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #7)
> I checked some of the code and I noticed that this may be a convention used
> for naming elements which are related to server stuff.
> (e.g. check [1] which it's usually used by variable named |server|)
> 
> [1]
> https://dxr.mozilla.org/comm-central/source/mail/components/im/
> imIncomingServer.js#68

Here's how you would investigate this:

The code you are deduplicating only sets up the UI. You have to ask yourself how the values the user then enters get stored in the account preferences.

That's what the attributes on the elements are for. The id is probably used to determine the pref name. If you look at the other account pages in the accounts dialog for TB (e.g. for news feeds or email accounts) with DOM inspector, you will see the server.name convention is used everywhere.

imIncomingServer (which then contains am-im) implements nsIMsgIncomingServer, which is where the "server" name comes from. So if you need to understand how this all works in the future, to find the code that stores the data, maybe the nsIMsgIncomingServer idl has some useful info? ;)

Comment 9

[aleth [:aleth]]

Comment on attachment 8711084 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

Looks like a nice simplification.

Comment 10

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/200f7919a357

Awesome clean-up! Thanks.

Comment 11

[aleth [:aleth]]

Backed out: https://hg.mozilla.org/comm-central/rev/23e6e5079983

Opening the account options in Thunderbird fails with 
JavaScript error: chrome://messenger/content/am-im.js, line 79: TypeError: this.getProtoOptions is not a function

Please test this a bit more thoroughly.

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

(In reply to aleth [:aleth] from comment #8)
> Here's how you would investigate this:
> 
> The code you are deduplicating only sets up the UI. You have to ask yourself
> how the values the user then enters get stored in the account preferences.
> 
> That's what the attributes on the elements are for. The id is probably used
> to determine the pref name. If you look at the other account pages in the
> accounts dialog for TB (e.g. for news feeds or email accounts) with DOM
> inspector, you will see the server.name convention is used everywhere.
> 
> imIncomingServer (which then contains am-im) implements
> nsIMsgIncomingServer, which is where the "server" name comes from. So if you
> need to understand how this all works in the future, to find the code that
> stores the data, maybe the nsIMsgIncomingServer idl has some useful info? ;)

Ah, thanks aleth :-)

(In reply to aleth [:aleth] from comment #11)
> Backed out: https://hg.mozilla.org/comm-central/rev/23e6e5079983
> 
> Opening the account options in Thunderbird fails with 
> JavaScript error: chrome://messenger/content/am-im.js, line 79: TypeError:
> this.getProtoOptions is not a function
> 
> Please test this a bit more thoroughly.

Fixed in this patch, let me test this patch with Thunderbird build (as this would take some time for me) and I'll confirm here if it's OK.

Comment 13

[aleth [:aleth]]

Comment on attachment 8711355 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

::: mail/components/im/content/am-im.js
@@ +75,5 @@
> +      {name: "preftype", value: "wstring"},
> +      {name: "genericattr", value: "true"}
> +    ];
> +    let haveOptions =
> +      accountOptionsHelper.addOptions("server.", this.proto.getOptions(), attributes);

This can't work. getOptions returns an enumerator, which addOptions doesn't expect as an argument.

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

Comment 15

[aleth [:aleth]]

Comment on attachment 8711366 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

If the user changes an advanced option in TB, the changed value doesn't get saved. Didn't you test this?

Comment 16

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

(In reply to aleth [:aleth] from comment #15)
> If the user changes an advanced option in TB, the changed value doesn't get
> saved. Didn't you test this?

I took some time to build TB. BTW, this patch solves this issue and tested.

Comment 17

[aleth [:aleth]]

Comment on attachment 8711375 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

I think by now you understand why forked code is evil ;)

::: chat/content/imAccountOptionsHelper.js
@@ +5,5 @@
> +var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +var accountOptionsHelper = {
> +  createTextbox: function(aType, aValue, aLabel, aName) {
> +    let row = document.createElement("row");

Creates a row element, but in am-im this is a vbox, as the parent element this will get added to is not a rows element in a grid.

Is it clear what happens when you add a row element as a child to a vbox?

@@ +72,5 @@
> +          rows.appendChild(element);
> +          break;
> +        case opt.typeInt:
> +          rows.appendChild(this.createTextbox("number", opt.getInt(), text, name));
> +          element = document.getElementById(name);

Let's move the element = document.getElementById(name); further down, above the element.setAttribute call, to avoid duplication and possible erros.

@@ +81,5 @@
> +          break;
> +        case opt.typeList:
> +          rows.appendChild(this.createMenulist(opt.getList(), text, name));
> +          element = document.getElementById(name);
> +          element.value = opt.getListDefault();

Should this be getListValue()? What's the difference? (The forked files seem to use one or the other, can you figure out why?)

Comment 18

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

(In reply to aleth [:aleth] from comment #17)
> Should this be getListValue()? What's the difference? (The forked files seem
> to use one or the other, can you figure out why?)
Yes, it checks if there is a user preference for that to use it, instead of default value.

Comment 19

[aleth [:aleth]]

Comment on attachment 8712462 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

::: chat/content/imAccountOptionsHelper.js
@@ +4,5 @@
> +
> +var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +var accountOptionsHelper = {
> +  createTextbox: function(aType, aValue, aLabel, aName, aLayout) {

aLayout -> aContainerType

@@ +5,5 @@
> +var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +var accountOptionsHelper = {
> +  createTextbox: function(aType, aValue, aLabel, aName, aLayout) {
> +    let parent = document.createElement(aLayout);

parent -> container

@@ +54,5 @@
> +  // with optional attributes for each type, preferences of user to set
> +  // default values, and layout of textboxes which are added and returns true
> +  // if at least one option has been added to UI, otherwise returns false.
> +  addOptions: function(aIdPrefix, aOptions, aAttributes, aPrefs,
> +                       aTextLayout = "row") {

I think you can do without the aTextLayout parameter, something like 
let containerType = "row";
let rows = document.getElementById("protoSpecific");
if (rows.localName != "rows")  // TB's account options dialog doesn't use a grid element
  containerType = "vbox";

(note the comment for future reference)

@@ +84,5 @@
> +        case opt.typeList:
> +          rows.appendChild(this.createMenulist(opt.getList(), text, name));
> +          document.getElementById(name).value =
> +            aPrefs && aPrefs.prefHasUserValue(aOpt.name) ?
> +              aPrefs.getCharPref(aOpt.name) : opt.getListDefault();

You've done this only for typeList, but not for the other types.

Since only account.js needs this pref-based initialization, it might be cleaner to not pass aPrefs and add lots of if clauses here, but instead from account.js to pass an aOptions iterator which overrides opt.getString etc with this.getString etc. in the opt's it returns. Then opt.getInt, opt.getListDefault here in this file will do the right thing.

Comment 20

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: deduplicate populateProtoSpecificBox

Comment 21

[aleth [:aleth]]

Comment on attachment 8713294 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

::: chat/content/imAccountOptionsHelper.js
@@ +68,5 @@
> +    for (let opt in aOptions) {
> +      let text = opt.label;
> +      let name = aIdPrefix + opt.name;
> +      switch (opt.type) {
> +        case Ci.prplIPref.typeBool:

you don't need these changes...

::: im/content/account.js
@@ +190,5 @@
>      for (let opt in this.getProtoOptions()) {
>        var name = this.proto.id + "-" + opt.name;
>        var val = this.getValue(name);
>        switch (opt.type) {
> +      case Ci.prplIPref.typeBool:

or these...

@@ +216,5 @@
>    getProtoOptions: function account_getProtoOptions() {
>      let options = this.proto.getOptions();
> +    while (options.hasMoreElements()) {
> +      let opt = options.getNext();
> +      let retOpt = {

...if you can do

retOpt.prototype = {
  __proto__: ClassInfo("prplIPref", "generic account option preference")
}

or even 

retOpt.prototype = opt;

Comment 22

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #21)
> ...if you can do
> 
> retOpt.prototype = {
>   __proto__: ClassInfo("prplIPref", "generic account option preference")
> }
> 
> or even 
> 
> retOpt.prototype = opt;

No, I can't as this object in JS is not wrapped, so there is no mapping to |prplIPref| interface for this object.

Comment 23

[aleth [:aleth]]

Comment on attachment 8713294 [details] [diff] [review]
deduplicate populateProtoSpecificBox

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

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #22)

> > retOpt.prototype = opt;
> 
> No, I can't as this object in JS is not wrapped, so there is no mapping to
> |prplIPref| interface for this object.

Yes, my suggestions don't seem to work. Since you've made the changes already, let's not worry about how to give retOpt the correct interface.

Thanks for deduplicating this code! Should make future improvements much safer and easier.

Comment 24

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/2f6efb061544e45caef0efd0d7d5730484e3fa80
Bug 1237385 - Deduplicate populateProtoSpecificBox. r=aleth a=aleth CLOSED TREE