Clean up uses of switch (Svc.AppInfo.ID) for app-specific hacks

RESOLVED FIXED in 1.4

Status

()

defect
RESOLVED FIXED
9 years ago
7 months ago

People

(Reporter: Mardak, Assigned: philikon)

Tracking

unspecified
Points:
---
Bug Flags:
blocking-fx-sync1.4 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

9 years ago
We should make sure each use has a way for applications to change the behavior and provide sane defaults.
Flags: blocking-fx-sync1.4+
(Reporter)

Updated

9 years ago
Assignee: nobody → philipp
(Reporter)

Updated

9 years ago
Target Milestone: --- → 1.4
There are five occurrences of switch (Svc.AppInfo.ID) all of which have sane fallback behaviour. The only thing I would propose to do is decouple autoconnect from importing service.js as discussed in comment 3 ff. of bug 557589.
It should also be added that code reorg is likely going to rip options.xul apart, thus getting rid of that switch statement. Code reorg may also implicate different/additional default preferences per application, making the switch statement in engines/clients.js unnecessary.
* options.xul will be in addon-only space after reorg and is fine for now.

* engines/client.js already reads the client.type from a preference, but it needs the switch statement for now as a fallback for the addon case.

* service.js onStartup() should trigger autoconnect after a fixed delay specified in a preference. If that preference is absent, nothing happens and apps are responsible for triggering autoconnect in a weave:service:ready observer themselves.

* service.js _registerEngines() should read the list of engines from a preference so that apps can specify it this way. The switch logic needs to stay as fallback for the addon case, though.
Weave.Service._registerEngines now reads the list of engines from a preference so that apps can specify it this way. The switch logic stays as fallback for the addon case.
Attachment #450258 - Flags: review?(mconnor)
Weave.Service.onStartup now triggers autoconnect after a fixed delay specified in a preference. If that preference is absent, nothing happens and apps are responsible for triggering autoconnect in a weave:service:ready observer themselves. Provide such observers for Firefox and Fennec.
Attachment #450259 - Flags: review?(mconnor)
Comment on attachment 450258 [details] [diff] [review]
Part 1: Weave.Service._registerEngines()

the disconnect between engine.name and what we register feels like a ticking time bomb. (i.e. "Tab" vs. "tabs")  Maybe we should clean that up?
Attachment #450258 - Flags: review?(mconnor) → review+
Comment on attachment 450259 [details] [diff] [review]
Part 2: Weave.Service.onStartup()


>   init: function () {
>+    Components.utils.import("resource://weave/util.js");
>+    Components.utils.import("resource://weave/ext/Observers.js");

You shouldn't need to import these here

Observers == Weave.Svc.Obs in this scope

And Svc is on the Weave object as above, so you can do Weave.Svc.WinMediator

>+    Observers.add("weave:service:ready", function (subject, data) {
>+        let wait = 5;
>+        // Add one second delay for each busy tab in every window
>+        let enum = Svc.WinMediator.getEnumerator("navigator:browser");
>+        while (enum.hasMoreElements()) {
>+          Array.forEach(enum.getNext().gBrowser.mTabs, function(tab) {
>+              wait += tab.hasAttribute("busy");
>+          });
>+        }
>+        Weave.Service.delayedAutoConnect(wait);
>+    });
>+
Attachment #450259 - Flags: review?(mconnor) → review-
(In reply to comment #7)
> (From update of attachment 450259 [details] [diff] [review])
> 
> >   init: function () {
> >+    Components.utils.import("resource://weave/util.js");
> >+    Components.utils.import("resource://weave/ext/Observers.js");
> 
> You shouldn't need to import these here
> 
> Observers == Weave.Svc.Obs in this scope

I'm quite aware of that. But the 'Weave' object is provided by service.js and we must not import it before we've registered the observer. In fact, accessing the 'Weave' global will trigger the import of service.js in load-weave.js... Don't you love import side-effects? :/
(In reply to comment #6)
> (From update of attachment 450258 [details] [diff] [review])
> the disconnect between engine.name and what we register feels like a ticking
> time bomb. (i.e. "Tab" vs. "tabs")  Maybe we should clean that up?

The 'tabs' spelling (engine.name) is part of preference settings and other things, so that will be nearly impossible to change. The "Tab" spelling corresponds to the naming of the engine constructor that gets imported into the 'Weave' namespace, e.g. Weave.TabEngine. I guess that could be changed if we care enough?!?
Updated: In delayedAutoConnect() check whether we're already logged in. Document why we can't use the 'Weave' namespace when registering the autoconnect observer.
Attachment #450259 - Attachment is obsolete: true
Attachment #450438 - Flags: review?(mconnor)
Attachment #450438 - Flags: review?(mconnor) → review+
(Reporter)

Comment 11

9 years ago
Comment on attachment 450438 [details] [diff] [review]
Part 2 (updated): Weave.Service.onStartup()

Already gave feedback on irc, but summary:

Delay the weave:service:ready notification in a setTimeout() so scripts importing service.js can use Weave.* as well as observe the topic without needing to do other imports/setTimeout.
Attachment #450438 - Flags: review-
Do the setTimeout dance for the notify call so that observers don't have to.
Attachment #450438 - Attachment is obsolete: true
Attachment #450448 - Flags: review?(mconnor)
Since we now do the notification async, we don't have to be careful about when to register the observer anymore. Which mean we can use the Weave namespace to do it.

Also fixed a couple of styling nits pointed out by Mardak.
Attachment #450448 - Attachment is obsolete: true
Attachment #450467 - Flags: review?(mconnor)
Attachment #450448 - Flags: review?(mconnor)
Reupload patch. Previous version got concatenated to an existing file... Sorry about that.
Attachment #450467 - Attachment is obsolete: true
Attachment #450472 - Flags: review?(mconnor)
Attachment #450467 - Flags: review?(mconnor)
The latest revision of the patch (v4) broke some tests. Tests are now fixed and somewhat extended.
Attachment #450472 - Attachment is obsolete: true
Attachment #450500 - Flags: review?(mconnor)
Attachment #450472 - Flags: review?(mconnor)
Fixed a nit in the test pointed out by Mardak.
Attachment #450500 - Attachment is obsolete: true
Attachment #450517 - Flags: review?(mconnor)
Attachment #450500 - Flags: review?(mconnor)
Attachment #450517 - Flags: review?(mconnor) → review+
(Reporter)

Comment 17

9 years ago
http://hg.mozilla.org/services/fx-sync/rev/8ec846f4c820
Weave.Service._registerEngines now reads the list of engines from a preference so that apps can specify it this way. The switch logic stays as fallback for the addon case. 

http://hg.mozilla.org/services/fx-sync/rev/5935ac54e30e
Weave.Service.onStartup now triggers autoconnect after a fixed delay specified in a preference. If that preference is absent, nothing happens and apps are responsible for triggering autoconnect in a weave:service:ready observer themselves. Provide such observers for Firefox and Fennec.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: 572436
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.