Last Comment Bug 588418 - Add ability to open Data Manager to a specific domain (and panel)
: Add ability to open Data Manager to a specific domain (and panel)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: DataManager
Blocks: 599097 666102 588417 588419 613795
  Show dependency treegraph
 
Reported: 2010-08-18 08:04 PDT by Robert Kaiser
Modified: 2011-06-21 19:57 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: implement a callback-based view loader (6.07 KB, patch)
2010-11-10 17:17 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.1: callback-based view loader supporting subsequent loads (7.61 KB, patch)
2010-11-16 13:24 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.2: allow full hosts in the calling string, include test (10.35 KB, patch)
2010-11-20 11:45 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.3: also make sure that the domain is scrolled into view (10.41 KB, patch)
2010-11-20 14:50 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.4: use | instead of : as separator (10.65 KB, patch)
2010-11-27 17:52 PST, Robert Kaiser
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-18 08:04:46 PDT
For better integration with other UI, we need ways to open Data Manager to a specific domain and possibly panel.
Comment 1 Robert Kaiser 2010-11-10 17:17:00 PST
Created attachment 489697 [details] [diff] [review]
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 2010-11-15 11:08:21 PST
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 2010-11-16 13:24:16 PST
Created attachment 490973 [details] [diff] [review]
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 2010-11-20 11:45:39 PST
Created attachment 492080 [details] [diff] [review]
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 2010-11-20 14:50:48 PST
Created attachment 492102 [details] [diff] [review]
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 2010-11-27 17:52:19 PST
Created attachment 493542 [details] [diff] [review]
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 2010-12-07 15:03:30 PST
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 2010-12-08 10:49:48 PST
(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 2010-12-08 11:14:51 PST
(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 2010-12-08 11:19:44 PST
Pushed as http://hg.mozilla.org/comm-central/rev/d3fa7383d7eb with the requested/said changes, I hope I'm not overstepping review here. ;-)

Note You need to log in before you can comment on or make changes to this bug.