483608 - Disable Forget This Site in Private Browsing mode for Firefox 3.5

Status: VERIFIED FIXED

(Toolkit :: Data Sanitization, defect, P1)

Description

[(no longer active)]

The same issue with the CRH dialog in Private Browsing mode happens with the Forget This Site command.  We need to disable it for Firefox 3.5, if we don't want to make false promises to the user.

Comment 1

[Mike Connor [:mconnor]]

Oy, yeah.

Though it should be hidden, not disabled, since it's a context action.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

IMO this isn't blocking, it's wanted and should be fixed, but I'd ship without it.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Patch + unit test.

Requesting review from Marco on places parts, and from gavin on the test.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 369870 [details] [diff] [review]
Patch (v1)

>From: Ehsan Akhgari <[e-mail]>
>
>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -115,16 +115,24 @@ function PlacesController(aView) {
> }
> 
> PlacesController.prototype = {
>   /**
>    * The places view.
>    */
>   _view: null,
> 
>+  _privateBrowsing: null,
>+  get privateBrowsing PC_get_privateBrowsing() {
>+    if (!this._privateBrowsing)
>+      this._privateBrowsing = Cc["@mozilla.org/privatebrowsing;1"].
>+                              getService(Ci.nsIPrivateBrowsingService);
>+    return this._privateBrowsing;
>+  },
>+

i'd prefer this being in browser/components/places/utils.js (aka PlacesUIUtils) so it's globally available to Places frontend.

>@@ -405,16 +411,17 @@ PlacesController.prototype = {
>    *    "bookmark"          node is a bookamrk
>    *    "livemarkChild"     node is a child of a livemark
>    *    "tagChild"          node is a child of a tag
>    *    "folder"            node is a folder
>    *    "query"             node is a query
>    *    "dynamiccontainer"  node is a dynamic container
>    *    "separator"         node is a separator line
>    *    "host"              node is a host
>+   *    "privateBrowsing"   private browsing mode is currently active

this is not a property of a places node, is a global toggle that can be valid or invalid for all nodes, so i think you should instead threat this like the "hideifnoinsertionpoint" attribute (see buildContextMenu), and provide an "hideifprivatebrowsing" attribute.

also, in PlacesController::isCommandEnabled, disable the command if pb is active, better not relying only on hidden and is more consistent.

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_forgetthissite.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_forgetthissite.js

>+  history.addVisit(PlacesUtils._uri(TEST_URI), Date.now() * 1000,
>+                   null, PlacesUtils.history.TRANSITION_TYPED, false, 0);

get returned visitId and check it's > 0 (so for sanity ensure visit has been added)

>+  function testForgetThisSiteVisibility(expected, funcNext) {
>+    let observer = {
>+      observe: function(aSubject, aTopic, aData) {
>+        if (aTopic === "domwindowopened") {
>+          ww.unregisterNotification(this);
>+          let organizer = aSubject.QueryInterface(Ci.nsIDOMWindow);
>+          organizer.addEventListener("load", function onLoad(event) {
>+            organizer.removeEventListener("load", onLoad, false);
>+            executeSoon(function () {
>+              // Select History in the left pane.
>+              organizer.PlacesOrganizer.selectLeftPaneQuery('History');
>+              // Select the first history entry.
>+              let doc = organizer.document;
>+              let tree = doc.getElementById("placeContent");

PlacesOrganizer object contains handles to both trees, you can use PlacesOrganizer._places or PlacesOrganizer._content

>+              let selection = tree.view.selection;
>+              selection.clearSelection();
>+              selection.rangedSelect(0, 0, true);

check tree.selectedNode.uri and ensure you selected the expected node.

>+              // Open the context menu
>+              let contextmenu = doc.getElementById("placesContext");
>+              contextmenu.addEventListener("popupshown", function() {
>+                contextmenu.removeEventListener("popupshown", arguments.callee, false);
>+                let forgetThisSite = doc.getElementById("placesContext_deleteHost");
>+                is(forgetThisSite.hidden, !expected,
>+                  "The Forget this site menu item should " + (expected ? "not " : "") + "be hidden");
>+                // Close the context menu
>+                contextmenu.hidePopup();
>+                // Close Library window.
>+                organizer.close();
>+                // Proceed
>+                funcNext();
>+              }, false);
>+              let event = document.createEvent("MouseEvents");
>+              event.initMouseEvent("contextmenu", true, true, organizer, 0,
>+                                   0, 0, 0, 0, false, false, false, false,
>+                                   0, null);
>+              tree.dispatchEvent(event);

i suppose you could use EventUtils.synthesizeMouse(element, x, y, { type : "contextmenu", button: 2 });

>+        // Cleanup
>+        history.QueryInterface(Ci.nsIBrowserHistory)
>+               .removePage(PlacesUtils._uri(TEST_URI));

you can directly call .removeAllPages(); that will clear history, so you don't have to bother adding more visits and removing them one by one (in case in future test needs to be expanded)

Comment 5

[(no longer active)]

Attachment: Patch (v2)

Addressed review comments.  Please note that synthesizeMouse cannot be used inside the test, as it only works in the current JS |window|.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 370828 [details] [diff] [review]
Patch (v2)

>         // We allow pasting into tag containers, so special case that.
>         item.hidden = (item.getAttribute("hideifnoinsertionpoint") == "true" &&
>                        noIp && !(ip && ip.isTag && item.id == "placesContext_paste")) ||
>+                      (item.getAttribute("hideifprivatebrowsing") == "true" &&
>+                       PlacesUIUtils.privateBrowsing.privateBrowsingEnabled) ||
>                       !this._shouldShowMenuItem(item, metadata);


this is becoming unreadable, please split the conditions in temp vars, one for hideifnoip, one for hideifpb, so the things sounds like item.hidden = hideifnoip || hideifpb || !this._shouldShowMenuItem(item, metadata);, that should be far more readable.

Looks good for me, thanks.

Comment 7

[(no longer active)]

Attachment: Patch (v2.1)

(In reply to comment #6)
> this is becoming unreadable, please split the conditions in temp vars, one for
> hideifnoip, one for hideifpb, so the things sounds like item.hidden =
> hideifnoip || hideifpb || !this._shouldShowMenuItem(item, metadata);, that
> should be far more readable.

Done.

> Looks good for me, thanks.

Handing it off to Gavin for his stamp on the privatebrowsing part (the test).

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/76d7ad0f5c63

Comment 9

[Marcia Knous [:marcia]]

Verified fixed on the trunk using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090410 Minefield/3.6a1pre.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 370829 [details] [diff] [review]
Patch (v2.1)

a191=beltzner

Comment 11

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4d140dbe88dd

Comment 12

[(no longer active)]

As mak pointed out on IRC, on branch the misspelled hideifnoinsetionpoint should be used; pushed that as <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/16ca05d5b6ff>

Comment 13

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre as well as the equivalent Win Vista nightly. Adding keyword.