Closed Bug 632817 Opened 9 years ago Closed 9 years ago

Cannot filter search for NET events in the Web console

Categories

(DevTools :: General, defect, major)

x86
Windows 7
defect
Not set
major

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: vladmaniac, Assigned: rcampbell)

Details

(Whiteboard: [softblocker])

Attachments

(2 files, 4 obsolete files)

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.
Request for blocking
blocking2.0: --- → ?
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
Thanks for providing the regression window, George!
wow, do we not have a test that handles this functionality?
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.
Assignee: nobody → pwalton
Kevin/Rob: input on blocking question?
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.
(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.
(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?
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.
(In reply to comment #6)
> Kevin/Rob: input on blocking question?

I concur with Rob. This is annoying but not a release blocker.
Assignee: pwalton → rcampbell
(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.
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.
Attached patch filter strings (obsolete) — Splinter Review
string based filter implementation
Attachment #511393 - Flags: feedback?(pwalton)
Attachment #511393 - Flags: feedback?(mihai.sucan)
Attachment #511393 - Flags: feedback?(ddahl)
(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 on attachment 511393 [details] [diff] [review]
filter strings

Works for me, as long as the performance is reasonable.
Attachment #511393 - Flags: feedback?(pwalton) → feedback+
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.
Attached patch filter strings 2 (obsolete) — Splinter Review
updated version. included an extra comment.
Attachment #511393 - Attachment is obsolete: true
Attachment #511401 - Flags: review?(gavin.sharp)
Attachment #511393 - Flags: feedback?(mihai.sucan)
Attachment #511393 - Flags: feedback?(ddahl)
blocking2.0: ? → final+
Whiteboard: [softblocker]
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.
Attachment #511401 - Flags: feedback-
Attached patch filter strings 3 (obsolete) — Splinter Review
updated. Reorganized method.
Attachment #511401 - Attachment is obsolete: true
Attachment #512654 - Flags: feedback?(mihai.sucan)
Attachment #511401 - Flags: review?(gavin.sharp)
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.
Attachment #512654 - Flags: feedback?(mihai.sucan) → feedback+
yeah, we should. I'll see if I can create one.
and thanks for the feedback+! :)
Attachment #512654 - Flags: review?(gavin.sharp)
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;
  });
}
hm! I like it.
Status: NEW → ASSIGNED
Attached patch filter gavinsSplinter Review
gavinified version of the filter patch
Attachment #512654 - Attachment is obsolete: true
Attachment #512964 - Flags: review?(gavin.sharp)
Attachment #512654 - Flags: review?(gavin.sharp)
Attached patch net filter test (obsolete) — Splinter Review
test code for net filtering patch.
Attachment #513523 - Flags: review?(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
Attachment #512964 - Flags: review?(gavin.sharp) → review+
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 <jviereck@mozilla.com>
>+ *  Patrick Walton <pcwalton@mozilla.com>
>+ *  Mihai Șucan <mihai.sucan@gmail.com>
>+ *  Rob Campbell <rcampbell@mozilla.com>
>+ *
>+ * ***** END LICENSE BLOCK ***** */
Note: this should just be https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

r=sdwilsh
Attachment #513523 - Flags: review?(sdwilsh) → review+
folded patch, revised based upon sdwilsh's review comments.
Attachment #513523 - Attachment is obsolete: true
Attachment #513545 - Flags: approval2.0?
Comment on attachment 513545 [details] [diff] [review]
[checked-in-with-fix] filter final, revised, folded tests

a=beltzner
Attachment #513545 - Flags: approval2.0? → approval2.0+
Comment on attachment 513545 [details] [diff] [review]
[checked-in-with-fix] filter final, revised, folded tests

http://hg.mozilla.org/mozilla-central/rev/e949d8f936f9
Attachment #513545 - Attachment description: filter final, revised, folded tests → [checked-in] filter final, revised, folded tests
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #513545 - Attachment description: [checked-in] filter final, revised, folded tests → [backed-out] filter final, revised, folded tests
Comment on attachment 513545 [details] [diff] [review]
[checked-in-with-fix] filter final, revised, folded tests

http://hg.mozilla.org/mozilla-central/rev/8a4f5201dc6a
Attachment #513545 - Attachment description: [backed-out] filter final, revised, folded tests → [checked-in-with-fix] filter final, revised, folded tests
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Verified
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.