Open Bug 662096 Opened 13 years ago Updated 8 years ago

Remove usage of switchToTabHavingURI callback from Data Manager

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: kairo, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

To be able to fix bug 633715 correctly in SeaMonkey, we need to get Data Manager off of using the callback on switchToTabHavingURI - and I have a variant working on my add-on version now that does that, using observer notifications.
Here's a patch that does it, using observer notifications to communicate between the browser and the Data Manager.
This also contains a test fix, as once I had done the changes, I couldn't get the test to run cleanly - it ended up calling the next view via toDataManager() when the last one hadn't shut down yet. I fixed that with having another notification be sent when shutting down and catching that before trying to launch another one.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #537394 - Flags: review?(iann_bugzilla)
Comment on attachment 537394 [details] [diff] [review]
v1: use notifiers to communicate

I think Neil has a better understanding on how observers work and whether there is a simpler way of doing this, so passing review request across.
Attachment #537394 - Flags: review?(iann_bugzilla) → review?(neil)
Comment on attachment 537394 [details] [diff] [review]
v1: use notifiers to communicate

>+    Services.obs.notifyObservers(window, "dataman-exists", "");
>+    Services.obs.addObserver(this, "dataman-exist-request", false);
Correct is to only notify dataman-exists when requested.

>+  Services.obs.addObserver(function loadview(aSubject, aTopic, aData) {
>+    Services.obs.notifyObservers(null, "dataman-loadview", aView);
>+    Services.obs.removeObserver(loadview, "dataman-exists");
>+  }, "dataman-exists", false);
>+  Services.obs.notifyObservers(null, "dataman-exist-request", "");
The point of the ping-pong protocol used by the Add-on Manager is to find the "nearest" tab that already has about:addons loaded (although it occurs to me that it doesn't seem to favour the current tab which perhaps it should). If you're not going to do that then there's no point implementing the ping-pong. In fact this code is worse because if the data manager is open in multiple tabs it will cause each of them to switch view once per open instance.

>+  switchToTabHavingURI("about:data", true);
No view gets selected in this case...
Attachment #537394 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk from comment #3)
> >+    Services.obs.notifyObservers(window, "dataman-exists", "");
> >+    Services.obs.addObserver(this, "dataman-exist-request", false);
> Correct is to only notify dataman-exists when requested.

But then we would never notify if a new Data Manager tab is opened - see below.

> If you're not going to do that then there's no point implementing
> the ping-pong.

Then what should I do instead?

> In fact this code is worse because if the data manager is
> open in multiple tabs it will cause each of them to switch view once per
> open instance.

And why is that a problem? That only happens if a user forces it in some way by manually entering about:data in the url bar - we never open multiple dataman instances ourselves.

> >+  switchToTabHavingURI("about:data", true);
> No view gets selected in this case...

But it does. At least in the automated and manual tests.
Unassigning myself, I'm not working on Data Manager any more.
Assignee: kairo → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: