588418 - Add ability to open Data Manager to a specific domain (and panel)

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Robert Kaiser]

For better integration with other UI, we need ways to open Data Manager to a specific domain and possibly panel.

Comment 1

[Robert Kaiser]

Attachment: v1: implement a callback-based view loader

This patch implements a toDataManager() function that uses a callback-based solution to load views, similar to what Add-ons Manager is doing. Due to how Data Manager is built, we need to defer the actual logic to the data loader function though.

Currently, this is not used in SeaMonkey yet, but the test uses it and verifies that it works as required. Calls to this will be added into SeaMonkey code in other bugs.

Comment 2

[Robert Kaiser]

Hmm, I realized I still need to make a test and some code for opening a specific view when Data Manager is already open. This will probably just extend the code that is up for review here, though.

Comment 3

[Robert Kaiser]

Attachment: v1.1: callback-based view loader supporting subsequent loads

Here's a patch that supports subsequent loads as well, not just initial loads.

Comment 4

[Robert Kaiser]

Attachment: v1.2: allow full hosts in the calling string, include test

I forgot the test in the last attachment, and I realized we should allow full host names and not just pure domains for the calling string.

Comment 5

[Robert Kaiser]

Attachment: v1.3: also make sure that the domain is scrolled into view

Sorry to come with yet another patch, but in working with this patch enabled, and the page info patch in bug 588419, I realized we also should ensure that the domain is scrolled into view when it's selected and has been out of view before, so this patch also adds a command to ensure it's scrolled into view.

Comment 6

[Robert Kaiser]

Attachment: v1.4: use | instead of : as separator

When working on the final hooks in bug 588419, I discovered that we may happen to forward a host:port combination for adding a popup, even if permission manager ignores the port in the end. I realized that we may in the future want to to have the capability of using something more complex like that in the syntax here, though, and so I switched the separator from : to | to allow those. To make this fully work we need to ensure a host:port pair is not mistaken as scheme:host though, and we work around that in the view loading code.

Comment 7

[Ian Neal]

Comment on attachment 493542 [details] [diff] [review]
v1.4: use | instead of : as separator

>+++ b/suite/common/dataman/dataman.js

>+  loadView: function dataman_loadView(aView) {
>+    this.viewToLoad = aView.split('|');
>+    if (gDomains.listLoadCompleted)
>+      gDomains.loadView();
What happens if this is called before the loading is complete, do we really want to silently fail?


>+    if (!gDataman.loadViewOnInit)
As you mentioned on IRC when we discussed this, needs changing to !gDataman.viewToLoad.length
>+      this.tree.view.selection.select(0);
> 

>+  loadView: function domain_loadView() {
>+    // load the view set in the dataman object.
>+    gDataman.debugMsg("Load View: " + gDataman.viewToLoad.join(", "));
>+    let loaderInstance;
>+    function nextStep() {
>+      loaderInstance.next();
>+    }
>+    function loader() {
I can understand why we need this sort of loader for when populating data manager, but do we really need it for this?

>+        if (gDataman.viewToLoad.length > 1) {
>+          gDataman.debugMsg("Pane for view found");
>+          let loadTabID = gDataman.viewToLoad[1] + "Tab";
>+          if (gTabs[loadTabID] && !gTabs[loadTabID].disabled)
Should we check loadTabID is non-null here too?

>+      // Send a notification that we finished.
Nit: we have finished or we are finished

>+++ b/suite/common/dataman/tests/browser_dataman_callviews.js
>@@ -0,0 +1,69 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test loading views in data manager
Nit: Start with a cap end with a full stop.
>+
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+
>+// happen to match what's used in Data Manager itself
Nit: happens perhaps?

>+function test() {
>+  // Add cookie
Nit: Start with a cap end with a full stop.
>+  gLocSvc.cookie.add("getpersonas.com", "", "name0", "value0",
>+                     false, false, true, parseInt(Date.now() / 1000) + 600);
>+
>+  //Services.prefs.setBoolPref("data_manager.debug", true);
Remove?
>+
>+  var win;
>+  var testIndex = 0;
>+
>+  gBrowser.addTab();
>+  toDataManager("example.org");
>+
>+  let testObs = {
>+    observe: function(aSubject, aTopic, aData) {
>+      if (aTopic == DATAMAN_LOADED) {
>+        ok(true, "Step " + (testIndex + 1) + ": Data Manager is loaded");
>+
>+        win = content.wrappedJSObject;
>+        is(win.gDomains.tree.view.selection.count, 1,
>+          "Step " + (testIndex + 1) + ": One domain is selected");
>+        if (testIndex == 0) {
>+          is(win.gDomains.selectedDomain.title, "example.org",
>+            "Step " + (testIndex + 1) + ": The correct domain is selected");
>+          testIndex++;
>+          toDataManager("getpersonas.com|cookies");
Should we also be testing what happens if we try to go to a tab that is disabled, a tab that doesn't exist and to a domain that doesn't exist?

r=me with those addressed/fixed unless you feel you need another review.

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> What happens if this is called before the loading is complete, do we really
> want to silently fail?

Yes, as loading the list will call this anyhow.

> I can understand why we need this sort of loader for when populating data
> manager, but do we really need it for this?

Yes, as we need to wait for actions (like activating tabs, etc.) to happen between the steps.

> >+        if (gDataman.viewToLoad.length > 1) {
> >+          gDataman.debugMsg("Pane for view found");
> >+          let loadTabID = gDataman.viewToLoad[1] + "Tab";
> >+          if (gTabs[loadTabID] && !gTabs[loadTabID].disabled)
> Should we check loadTabID is non-null here too?

How can it ever be null when viewToLoad[1] exists (the first if checks for that) and we string-add "Tab" to it? What can happen is that its value doesn't appear in gTabs, but that's why we check for gTabs[loadTabID] there.

> >+// Test loading views in data manager
> Nit: Start with a cap end with a full stop.
> >+
> >+Components.utils.import("resource://gre/modules/Services.jsm");
> >+
> >+// happen to match what's used in Data Manager itself
> Nit: happens perhaps?

Fixed the same nits in the other test at the same time, thanks.

> >+  //Services.prefs.setBoolPref("data_manager.debug", true);
> Remove?

I left that in the other test as well, as it's a good reminder of how one can get more output when debugging the test.

> Should we also be testing what happens if we try to go to a tab that is
> disabled, a tab that doesn't exist and to a domain that doesn't exist?

Thanks, good ideas, adding on checkin (unless I notice them misbehaving).

Comment 9

[Robert Kaiser]

(In reply to comment #8)
> (In reply to comment #7)
> > Should we also be testing what happens if we try to go to a tab that is
> > disabled, a tab that doesn't exist and to a domain that doesn't exist?
> 
> Thanks, good ideas, adding on checkin (unless I notice them misbehaving).

I ended up converting the test to the exact same testing infrastructure we're also using on the other test, so that it's easier to extend it in followup bugs. I hope that's OK under the hood of this review (as the how-its-done has review on the other test already), I'll need to update the patches for the followup bugs though for this change.

Comment 10

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/d3fa7383d7eb with the requested/said changes, I hope I'm not overstepping review here. ;-)