64060 - bulk delete history by hostname, by domain

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, defect, P2)

Description

[Alec Flett]

Since deleting individual items from history was such a well-recieved feature, I
decided to also implement the ability to delete history entries by hostname, and
by domain.. so that porn surfers could quickly and easily hide the traces of
their elicit wanderings from their loved ones.

patch forthcoming, looking for reviewers.

Comment 1

[Alec Flett]

Attachment: proposed fix

Comment 2

[Alec Flett]

other random junk in the patch:
- cleaning up case sensitivity stuff
- fixing JS warnings

the basic parts of this are:
- C++ implementation of a matching function with matches on the hostname, or the
domain (can really be used to match any postfix that's passed in)
- JS controller to update everything.. I implemented the full controller because
I think we'll be adding more to history in the future
- JS glue to update the controller, change selection, and so forth.
- new strings to show stuff in the menu

I am open to suggestions on the strings - I just put some dummy ones in there.

Comment 3

[Blake Ross]

this is very cool.  one step closer to becoming the browser of choice for porn 
surfers everywhere...

Comment 4

[Brendan Eich [:brendan]]

+    dump("Selection changed, have .." + selection.length + " items\n");
+    if (selection && selection.length > 0) {

That dump will throw an exception if selection is null -- split the if in two
and have the outer one guard the dump as well as the inner?

+        var match = gLastHostname.match(/([^\.]+\.[^\.]+$)/);

No need for \ before . in [] bracketed character classes.

+        switch(command)
+        {
+            case "cmd_deleteByHostname":
+            case "cmd_deleteByDomain":
+                return true;

Non-conforming brace-style alert!

Also, here and moreso in subsequent switches, burning an entire four-space
indentation level for the case labels tends to overindent code and spread it out
horizontally too much.  Why not use half-stop indentation (two spaces)?  This is
a total style nit, I know, and it doesn't come up with two-space indentation
stops, but it bugged me so I thought I'd mention it.

+            case "cmd_deleteByDomain":
+            dump("Removing all domain from " + gLastDomain + "\n");

Indentation glitch.

I didn't finish reviewing, but I wanted to update now (I feel a crash coming
on ;-), and hope you can use these comments.

/be

Comment 5

[Alec Flett]

oops, that dump wasn't supposed to be in there at all...

I'll fix the other stuff too, like the regexp thing.. as for the brace issues, I
just do what emacs indents for me in Java mode (oh, to have a JS mode for
emacs...*sigh*) but I'll fix it up.

I'll fix those up, and attach a new patch..

Comment 6

[Alec Flett]

oops, still haven't attached the new patch, but putting fix in hand, so I can
keep my buglist straight

Comment 7

[Alec Flett]

Attachment: updated patch

Comment 8

[Alec Flett]

I now have a patch, looking for reviewer/super reviewer.

Comment 9

[timeless]

what's the story on the BookmarksTree name?

odd indentation: [tab?]
+        gHistoryTree.selectItem(children.firstChild);
+    gHistoryTree.focus();

Curiousity, why is this signed:
PRInt64* expirationDate

your function definition newline style is inconsistent:
+nsGlobalHistory::MatchHost(nsIMdbRow *aRow,
+                           matchHost_t *hostInfo)

your code looks good, but i'm sure you don't want an r=timeless.

Comment 10

[Alec Flett]

- still trying to exorcise the bookmarkstree name
- Indentation fixed in my tree, whoops
- signed because a NSPR uses PRInt64 - using unsigned would requiring annoying
and unnecessary casting.
- newline indentation looks fine to me. emacs agrees with me.

Comment 11

[Brendan Eich [:brendan]]

Time numbers must be negative for dates before 1970, and to hold time number
differences.  Plus, casting between unsigned and signed is rarely useful (so
much so that Java dispensed with unsigned types, but that hurds division and
modulus counterexamples, and valid unsigned range tests).

/be

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

hooray for porn browsing!

a=[e-mail]

Comment 13

[Alec Flett]

fix is in

Comment 14

[Jesse Ruderman]

Posted bug 67387 for adding mnemonics for the new commands.

Comment 15

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]