Closed
Bug 570573
Opened 14 years ago
Closed 14 years ago
Clean up uses of switch (Svc.AppInfo.ID) for app-specific hacks
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
1.4
People
(Reporter: Mardak, Assigned: philikon)
References
Details
Attachments
(2 files, 6 obsolete files)
2.67 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
6.76 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
Assignee: nobody → philipp
Reporter | ||
Updated•14 years ago
|
Target Milestone: --- → 1.4
Assignee | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
* 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.
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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 7•14 years ago
|
||
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-
Assignee | ||
Comment 8•14 years ago
|
||
(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? :/
Assignee | ||
Comment 9•14 years ago
|
||
(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?!?
Assignee | ||
Comment 10•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #450438 -
Flags: review?(mconnor) → review+
Reporter | ||
Comment 11•14 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-
Assignee | ||
Comment 12•14 years ago
|
||
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)
Assignee | ||
Comment 13•14 years ago
|
||
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)
Assignee | ||
Comment 14•14 years ago
|
||
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)
Assignee | ||
Comment 15•14 years ago
|
||
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)
Assignee | ||
Comment 16•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #450517 -
Flags: review?(mconnor) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 17•14 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.
Updated•6 years ago
|
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.
Description
•