Remove a hack that is currently necessary to trigger an "account loaded" notification

NEW
Assigned to

Status

5 years ago
2 years ago

People

(Reporter: tessarakt, Assigned: tessarakt, NeedInfo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

5 years ago
AccountWizard.js contains the following hack to cause an "account loaded" notification:

http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/AccountWizard.js#466

466     // hack to cause an account loaded notification now the server is valid
467     account.incomingServer = account.incomingServer;

The same hack is about to be introduced into mail/test/mozmill/shared-modules/test-nntp-helpers.js by bug 903804.

The goal of this bug is to make both hacks unnecessary by fixing the root cause of this problem.
(Assignee)

Updated

5 years ago
Assignee: nobody → blog
(Assignee)

Comment 1

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #0)
> The same hack is about to be introduced into
> mail/test/mozmill/shared-modules/test-nntp-helpers.js by bug 903804.

See here: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-nntp-helpers.js#140

138   // hack to cause an account loaded notification now the server is valid
139   // (see also Bug 903804)
140   account.incomingServer = account.incomingServer;

Next steps to investigate:
 - What kind of object is "account"?
 - Which code gets triggered by the assignment to the incomingServer property? Does the same happen for other properties?
 - Would it be feasible to trigger the same notification when setting valid to true?
(Assignee)

Comment 2

5 years ago
(In reply to Jens Müller (:tessarakt) from comment #1)
>  - What kind of object is "account"?

It is returned from here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#1697

Therefore it is an nsIMsgAccount. The returned object is created using kMsgAccountCID, which is apparently nsMsgAccount.
In addition, there is only one implementation of this interface, nsMsgAccount.

See here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccount.cpp

>  - Which code gets triggered by the assignment to the incomingServer property? Does the same happen for other properties?

See here: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccount.cpp#138

134   // only notify server loaded if server is valid so
135   // account manager only gets told about finished accounts.

Notifications only happen in createIncomingServer and setIncomingServer.

>  - Would it be feasible to trigger the same notification when setting valid to true?

It is not the account but the server that becomes valid ... This makes things a bit tricky.

Further investigations are needed:
 - Is the mapping server->account 1:1 or 1:n?
 - Is it feasible for the server to notify the account about becoming valid?
(Assignee)

Comment 3

5 years ago
Hi David,

as you wrote http://hg.mozilla.org/comm-central/annotate/6f71fa7c2970/mailnews/base/src/nsMsgAccount.cpp#l138 : do you have any opinion on this bug?

Best regards,

Jens
Flags: needinfo?(mozilla)
(Assignee)

Comment 4

5 years ago
Based on comment 2, my idea for a solution would be as follows:

- Add a boolean member variable (flag) to nsMsgAccount: mSetServerNotYetValid.
  - Initially, this flag is false.
- When setIncomingServer is called with a server that is not valid:
  - Set mSetServerNotYetValid to true.
  - Register an observer to be notified when the server becomes valid.
- When the observer is called:
  - Do all the stuff that would have already been done in setIncomingServer had the server been valid back then.
    (including sending out the notification)
  - Unregister the observer.
  - Set mSetServerNotYetValid to true.

[Actully, for this to work, the flag is not even necessary ...]

I will only investigate how to implement the observer mechanism in nsMsgIncomingServer when I have some feedback whether this is an acceptable solution ...
Irving might have some input on this.
Flags: needinfo?(irving)
I'd rather not add a layer of observer/callback complexity around this; a couple of other approaches to look at:

1) Can we re-arrange the surrounding code so that we only call setIncomingServer() with valid servers?

2) Can we split the logic into two separate methods, setIncomingServer() and enableIncomingServer() and call enableIncomingServer() directly in the cases where we start with an invalid server and then enable it later?
Flags: needinfo?(irving)

Comment 7

4 years ago
(In reply to Jens Müller (:tessarakt) from comment #3)
> Hi David,
> 
> as you wrote
> http://hg.mozilla.org/comm-central/annotate/6f71fa7c2970/mailnews/base/src/
> nsMsgAccount.cpp#l138 : do you have any opinion on this bug?
> 
> Best regards,
> 
> Jens

Jens, Bienvenu isn't active.  If Irving hasn't or can't answer your question then we'll need to asked someone else. (Unless bienvenu is the only person who can answer it, then we'll have to contact him outside of bugzilla.)
Flags: needinfo?(mozilla)

Comment 8

2 years ago
(In reply to :Irving Reid (No longer working on Firefox) from comment #6)
> I'd rather not add a layer of observer/callback complexity around this; a
> couple of other approaches to look at:
> 
> 1) Can we re-arrange the surrounding code so that we only call
> setIncomingServer() with valid servers?
> 
> 2) Can we split the logic into two separate methods, setIncomingServer() and
> enableIncomingServer() and call enableIncomingServer() directly in the cases
> where we start with an invalid server and then enable it later?

perhaps aceman
Flags: needinfo?(acelists)

Comment 9

2 years ago
(In reply to Jens Müller (:tessarakt) from comment #2)
> It is not the account but the server that becomes valid ... This makes
> things a bit tricky.
> 
> Further investigations are needed:
>  - Is the mapping server->account 1:1 or 1:n?

Yes, 1:1.

>  - Is it feasible for the server to notify the account about becoming valid?

The account only contains a reference to the server and some identities. The server is the main object containing all the important stuff. I'm not sure notifying the account is needed, it also doesn't seem to be done now.
All the notification code at https://hg.mozilla.org/comm-central/file/tip/mailnews/base/src/nsMsgAccount.cpp#l135 does not seem to reference the account as far as I can see.

> Notifications only happen in createIncomingServer and setIncomingServer.
> 
> >  - Would it be feasible to trigger the same notification when setting valid to true?

Would you try moving all the code in the block at
https://hg.mozilla.org/comm-central/file/tip/mailnews/base/src/nsMsgAccount.cpp#l135
to the SetValid method of nsIMsgIncomingServer? Run it when 'valid' is set to true.

Updated

2 years ago
Flags: needinfo?(acelists) → needinfo?(blog)
You need to log in before you can comment on or make changes to this bug.