755885 - try to merge am-offline.js::onCheckItem() with am-prefs.js::onCheckItem() and am-offline.js::disableIfLocked() with smtpEditOverlay.js::disableIfLocked() function in account manager

Status: ASSIGNED

(MailNews Core :: Account Manager, enhancement)

Description

[:aceman]

Bug 755454 should produce a common onCheckItem() in am-prefs.js.

The am-offline.js::onCheckItem() additionally uses gLockedPref array (as a cache of locked prefs) produced in am-offline.js::disableIfLocked() .

So if am-offline.js::disableIfLocked() is moved to am-prefs.js 
and merged with smtpEditOverlay.js::disableIfLocked() then also both onCheckItem()s could be merged.

The question is if the cache array functionality should be preserved (and then reused in the other panes). Is Services.prefs.prefIsLocked() slow?

Comment 1

[:aceman]

I think I will move the gLockedPref cache array to am-prefs.js so that the implementation can be shared. On the other hand, some panes do not use this cache, so will need to call getAccountValueIsLocked. I see what merging/reworking can be done with that (like add new elements to the cache as they are used).

Comment 2

[:aceman]

Me: there is also disableIfLocked in am-mdn.js ...

Comment 3

[:aceman]

Design intention:
in dialog onLoad() call disableIfLocked() with all elements that have a pref attribute. It should get locked state for all the prefs and disable the elements as necessary. Cache the locked state in a global array.
Then, on a check event, onCheckItem() should call disableIfLocked() on the element that is being checked. disableIfLocked() uses the cache to quickly determine the locked state. If the pref value is not yet in the cache (I am not sure if that is possible), gets its locked state and adds it to the cache.
Use getAccountValueIsLocked() somewhere in between.

Comment 4

[:aceman]

It seems am-offline.xul does not contain prefstring attributes on the elements. All the prefnames are constructed in the JS code. This may need more changes than expected :)

Comment 5

[:aceman]

There may be changes in functionality:
E.g. smtpEditOverlay.xul contains 6 elements with prefstring attribute set. smtpEditOverlay.js only called disableIfLocked() manually on 5 of them. The new version is intended to be more automatic so it should catch all 6.

The caching does not seem much effective. The resolving of the prefstring (the %token%) may be the most expensive part and that must be done before the cache is consulted (it contains resolvedPrefString=value pairs). If it is not in cache, Services.prefs.prefIsLocked() is called. Maybe maintaining the cache is more expensive than just calling services. I'll do some benchmarks.

Comment 6

[:aceman]

Attachment: WIP patch

This is going to be a monster patch :)
Attaching a WIP version so that anybody can comment if this is going in a good direction. There is still some debugging, watch the Error console. And commented out code that will go away at the end.

Open problems:
- The server.selectImapFoldersButton button is not properly checking the pref, I need to find out a way to pass "account" into getAccountValueIsLocked().
- For some reason a field that is locked gets it's value empty. I hope it is a sideeffect of locking the prefs (as debug) inside AM. I hope properly locking prefs will work fine.
- I had to massively rename IDs in am-offline.* to have proper prefixes so that prefstring attrs work. I hope it is OK. (I didn't change pref names.)
- Unfortunately there is code shared with AM in retention.js and folderProps.js for the offline and retention stuff so I had to synchronize IDs there too.

Comment 7

[:aceman]

I think I should (and could too) make a test for this.

Comment 8

[:aceman]

Attachment: WIP patch v2

So I fixed the first problem (buttons on "Disk space" pane not being locked).
However passing account got very ugly. I wonder why parent.getCurrentAccount() doesn't work straight from substPrefTokens(). It would become so much easier...

Comment 9

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

Comment on attachment 629967 [details] [diff] [review]
WIP patch v2

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

So, changing the IDs is not ideal, but specializing your function to prefix the IDs with "server." is even worse. So I'm inclined to be OK with those ID changes.

Comment 10

[:aceman]

I'll add a test for this in the test file created in bug 525024.

Comment 11

[:aceman]

The patch is probably badly bitrotted. But I need confirmation from Ian that the intention is good and I should continue with this.

Comment 12

[:aceman]

Now would be a good time in the TB release period to make these breaking changes. However I'd still like to hear from Seamonkey side. IanN, Neil?

Comment 13

[[e-mail]]

(In reply to from comment #0)
> The question is if the cache array functionality should be preserved (and
> then reused in the other panes). Is Services.prefs.prefIsLocked() slow?
Maybe I misread the code but as far as I can tell it only exists to conveniently relate the element and the preference. That being said I wonder what it's doing that it needs this code when the other panels don't. Maybe if you were to set the appropriate prefstring attributes as per comment #4 then you could remove some of that code.

Comment 14

[:aceman]

The cache seems to boil down to this:
+    if (prefstr in top.gLockedPrefs)
+      return top.gLockedPrefs[prefstr];
+    // See if the pref <prefstring> is locked and cache the value,
+    // it should not change until the program terminates.
+    top.gLockedPrefs[prefstr] = Services.prefs.prefIsLocked(prefstr);
+    return top.gLockedPrefs[prefstr];

So if the preference string is in the cache it saves access to Services.prefs.prefIsLocked(). That is why I ask if that is worth it. However the cache seems to persist until the whole Account manager is closed. So if the user repeatedly toggles the same panel, the cached values are used. So maybe I should benchmark how long Services.prefs.prefIsLocked() takes.

Comment 15

[Wayne Mery (:wsmwk)]

aceman, do you still want iann's feedback before handling comment 14?

Comment 16

[:aceman]

Probably not so urgently. I just need to get to refreshing the patch. I worry it is bitrotted a lot.