632817 - Cannot filter search for NET events in the Web console

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Build ID: 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre

STR: 

1. Open a blank tab 
2. Open console 
3. Open any page. e.g. www.aol.com 
4. Search for a NET event 

Your search will have no results when filtering for NET events.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

Request for blocking

Comment 2

[George Carstoiu]

Regression range:

Last good build:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
http://hg.mozilla.org/mozilla-central/rev/bb9089ae2322

First bad build:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre
http://hg.mozilla.org/mozilla-central/rev/b5314bc1a926

Push log:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb9089ae2322&tochange=b5314bc1a926

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Thanks for providing the regression window, George!

Comment 4

[David Dahl :ddahl]

wow, do we not have a test that handles this functionality?

Comment 5

[Kevin Dangoor]

looking through the pushlog, it seems plausible that the change from bug 628110 (commit f28ee131df7a) may have broken this.

I'm going to tentatively assign this to Patrick, because he is familiar both with the searching code and with the bug that may have broken this.

Comment 6

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

Kevin/Rob: input on blocking question?

Comment 7

[Patrick Walton (:pcwalton)]

Yeah, this is because we're using XUL labels instead of descriptions. There are a couple of potential workarounds:

(a) Rework the XPath. (At some point we should move off XPath and to IndexedDB or something... this code is gnarly.)
(b) Add an invisible description element for the XPath to match.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> Yeah, this is because we're using XUL labels instead of descriptions. There are
> a couple of potential workarounds:
> 
> (a) Rework the XPath. (At some point we should move off XPath and to IndexedDB
> or something... this code is gnarly.)

I am not sure if it would be nice of us to store all the messages in an IndexedDB, just for filtering purposes. It would be indeed an interesting endeavor if we want to do more fun stuff with the logged messages in the future - then an IndexedDB would prove very useful. We could have fancy features on top of that.

Ultimately, this should perhaps be handled by the ConsoleStorageService.


> (b) Add an invisible description element for the XPath to match.

This would be a very ugly hack. :)

or ..
(c) we could write a function that walks the output nodes and filters them.

Comment 9

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #6)
> Kevin/Rob: input on blocking question?

Regarding blocking release, I'm not sure this should hold us up. It is probably annoying though.

Re Comment #7, I was thinking about doing exactly what you suggested in comment B. I do think we need to get off the Xpath train though as it's ugly and incomprehensible.

I've already run into a case where it would be nice to have messages stored for fast lookup (bug 611795).

I've already had one patch rejected for walking nodes. Maybe it's more applicable in this case?

Comment 10

[Mihai Sucan [:msucan]]

Invisible description elements would be a very ugly hack. If we are to go this route, why not switch from labels to descriptions? Duplicating information feels like "well, doh, we couldn't come up with a better solution, so we did something ugly, but hey it works!"

If walking nodes is a problem, then certainly IndexedDB should be the choice. And yes, this would also help with bug 611795.

Comment 11

[Kevin Dangoor]

(In reply to comment #6)
> Kevin/Rob: input on blocking question?

I concur with Rob. This is annoying but not a release blocker.

Comment 12

[Patrick Walton (:pcwalton)]

(In reply to comment #10)
> Invisible description elements would be a very ugly hack. If we are to go this
> route, why not switch from labels to descriptions? Duplicating information
> feels like "well, doh, we couldn't come up with a better solution, so we did
> something ugly, but hey it works!"

Labels are necessary for the smart URL abbreviation.

Comment 13

[Patrick Walton (:pcwalton)]

Another possibility is to store the filter text in a specially named attribute, and to have the XPath match on the values of those attributes.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Attachment: filter strings

string based filter implementation

Comment 15

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #13)
> Another possibility is to store the filter text in a specially named attribute,
> and to have the XPath match on the values of those attributes.

I just thought of that while talking about this bug with ddahl in irc.

moving the clipboardText into an actual attribute in the message node should give us an xpath searchable attribute.

Comment 16

[Patrick Walton (:pcwalton)]

Comment on attachment 511393 [details] [diff] [review]
filter strings

Works for me, as long as the performance is reasonable.

Comment 17

[Rob Campbell [:rc] (:robcee)]

It seems to work pretty well, doesn't feel any slower than the current solution.

I think I'd be happy shipping this until we can get an indexeddb-based solution.

Comment 18

[Rob Campbell [:rc] (:robcee)]

Attachment: filter strings 2

updated version. included an extra comment.

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 511401 [details] [diff] [review]
filter strings 2

The patch is fine, but you have introduced a regression.

In filterMessageNode() you have:

+    searchStrings = HUDService.buildSearchStringsFromString(search);
+
+    // no searchStrings or no clipboardText, return
+    if (!searchStrings || !aNode.clipboardText)
+      return;

... which stops filtering the message node if there's no clipboardText or no searchString in the filter input field. This prevents filtering by type to occur.

Steps to reproduce:

1. Open Firefox and the Web Console.
2. Open a page which has console.log() calls.
3. Make sure you trigger those console.log() calls in the page (say have a button which does this).
4. Disable the console log messages in the Web Console. Notice they are hidden (as expected).
5. Now click that button again to trigger new console.log() calls. These messages should not show up, but they do.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Attachment: filter strings 3

updated. Reorganized method.

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 512654 [details] [diff] [review]
filter strings 3

The patch is fine now. The regression has been fixed.

Shouldn't we have a test for this? We had no tests failing without this fix. The tests suite should catch such problems.

Comment 22

[Rob Campbell [:rc] (:robcee)]

yeah, we should. I'll see if I can create one.

Comment 23

[Rob Campbell [:rc] (:robcee)]

and thanks for the feedback+! :)

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 512654 [details] [diff] [review]
filter strings 3

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  buildSearchStringsFromString:

>+    let searchStrings = aString.toLowerCase().split(" ");
>+
>+    // remove empty strings
>+    return searchStrings.filter(function(a) {
>+      return a.trim();

Why not just split(/\s+/) as before, to avoid the need to then filter? I think much of this patch could be simplified with a helper that looks like:

function stringMatchesFilter(str, filter) {
  if (!filter || !str)
    return true;

  let searchStr = str.toLowerCase();
  let filterStrings = filter.split(/\s+/);
  return !filterStrings.some(function (f) {
    return str.indexOf(f) == -1;
  });
}

Comment 25

[Rob Campbell [:rc] (:robcee)]

hm! I like it.

Comment 26

[Rob Campbell [:rc] (:robcee)]

Attachment: filter gavins

gavinified version of the filter patch

Comment 27

[Rob Campbell [:rc] (:robcee)]

Attachment: net filter test

test code for net filtering patch.

Comment 28

[Shawn Wilsher :sdwilsh]

Comment on attachment 512964 [details] [diff] [review]
filter gavins

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+   * Does the given string match the passed filter string?
nit: it feels weird for a function description to only ask the consumer a question :P

>+   * @param String aString to search for filter words in.
>+   * @param String aFilter is a string containing all of the words to filter on.
nit: I know the style isn't consistent in this file, but whenever anybody else touches the javadoc comments on a method, I've been having them follow this form:
https://hg.mozilla.org/mozilla-central/annotate/a63e2fb78899/toolkit/components/console/hudservice/HUDService.jsm#l234

>+  stringMatchesFilters:
>+  function stringMatchesFilters(aString, aFilter)
why the newline?

>+    if (!aFilter || !aString)
>+      return true;
global-nit (three instances): one line ifs should be braced

>+    let searchStr = aString.toLowerCase();
>+    let filterStrings = aFilter.toLowerCase().split(/\s+/);
In both of these, you actually want toLocalLowerCase for those non-english speakers out there :)

r=sdwilsh

Comment 29

[Shawn Wilsher :sdwilsh]

Comment on attachment 513523 [details] [diff] [review]
net filter test

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_632817.js
>@@ -0,0 +1,207 @@
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ *
>+ * Contributor(s):
>+ *  Julian Viereck <[e-mail]>
>+ *  Patrick Walton <[e-mail]>
>+ *  Mihai Èucan <[e-mail]>
>+ *  Rob Campbell <[e-mail]>
>+ *
>+ * ***** END LICENSE BLOCK ***** */
Note: this should just be https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

r=sdwilsh

Comment 30

[Rob Campbell [:rc] (:robcee)]

Attachment: [checked-in-with-fix] filter final, revised, folded tests

folded patch, revised based upon sdwilsh's review comments.

Comment 31

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

Comment on attachment 513545 [details] [diff] [review]
[checked-in-with-fix] filter final, revised, folded tests

a=beltzner

Comment 32

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 513545 [details] [diff] [review]
[checked-in-with-fix] filter final, revised, folded tests

http://hg.mozilla.org/mozilla-central/rev/e949d8f936f9

Comment 33

[Rob Campbell [:rc] (:robcee)]

test failures

Comment 34

[Rob Campbell [:rc] (:robcee)]

backed out in http://hg.mozilla.org/mozilla-central/rev/a3f202325d27

Comment 35

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 513545 [details] [diff] [review]
[checked-in-with-fix] filter final, revised, folded tests

http://hg.mozilla.org/mozilla-central/rev/8a4f5201dc6a

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

Verified