583359 - Filter options on the Web Console should filter all messages

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

Currently, the filter options on the Web Console (including the search box) filter only incoming messages. They should also filter messages that have already been logged.

Comment 1

[Kevin Dangoor]

I believe this should be a blocker because it violates both the principle of least surprise and it also makes the original idea of having lots of logged data that you search through to locate what you're looking for impossible.

Comment 2

[Kevin Dangoor]

Also: it seems to me that this bug is blocked by the bug that changes how the log data is stored.

Comment 3

[Julian Viereck]

(In reply to comment #2)
> Also: it seems to me that this bug is blocked by the bug that changes how the
> log data is stored.

Which bug do you mean exactly.

Comment 4

[David Dahl :ddahl]

I think we should try following the technique in the existing error console where a search is done on the dom nodes in the output and hidden or shown via css. 

Now that I write this, I wonder how performant this will be. the existing error console is only masking things based on css classes. You cannot actually "search", you are filtering via type: warn, info, error, log.

I wonder if we can create a new attribute like logMessageContents="Error: foo bar baz" on each output node, would this allow us to search the output easily?

Or would we still need to construct an array of the textContent of each node, do an indexOf() search and then apply display:none to each node not "found"?

Comment 5

[David Dahl :ddahl]

typeaheadfind is also an option:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml

I imagine we would need a document to throw at the API, since we only have a bunch of nodes right now.

Comment 6

[David Dahl :ddahl]

(In reply to comment #5)
> I imagine we would need a document to throw at the API, since we only have a
> bunch of nodes right now.

As usual, I am probably wrong about this. It seems you can do a search in a highlighted selection object, and inside of nodes like textarea.

Comment 7

[David Dahl :ddahl]

Jonas showed me this xpath snippet on how we could incrementally search nodes.  The only caveat is that if there are 50k nodes, it will have to search through all of them on each iteration to get the N segment we are looking for. 

He said it would make sense to write tests with large datasets to see what is faster. The other caveat is that this is all on main tread and runs "to completion".

----- Forwarded Message -----
From: "Jonas Sicking" <[e-mail]>
To: "David Dahl" <[e-mail]>
Sent: Monday, August 2, 2010 4:31:55 PM
Subject: xpath

div[position() > 99 and position() < 200][contains(., "hello world")]

function res(prefix) {
  if (prefix == "html") return "….";
  return null;
}
document.evaluate(expression, thevboxelement, res,
XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, null);

Comment 8

[David Dahl :ddahl]

Also, jonas said xpath is no doubt the most performant way to do this. There are also "nodeIterartors" we can play with, but this seems like a good way to go on round 1.

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1.

This patch adds support for live filtering with the checkboxes (i.e. Net/CSS/JS/etc., but not for the search box yet). It uses querySelectorAll() and adjusts the style manually for each element.

Comment 10

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2.

New version fixes the inline documentation and chooses slightly saner function names.

Comment 11

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 3.

Third version of the patch uses CSS classes to adjust visibility, which will simplify the interaction between filtering by type (with the checkboxes) and filtering by string (with the search box).

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 4.

Fourth version of the patch corrects the omission of a display name for the new function added to the HUD service.

Comment 13

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 5.

Fifth version of the patch refactors the test case so that the second part can use the same infrastructure.

Comment 14

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2.

Patch part 2 adds live filtering for the search box.

Comment 15

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 2.

New version of the patch fixes a typo.

Comment 16

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 3.

Third version of the patch factors out the XPath construction into a separate function and adds the ability to search on multiple keywords.

Comment 17

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3.

Part 3 of the patch removes the old filtering logic. As nodes are no longer being filtered as they are logged, this causes some tests to fail. This will be fixed in part 4 of the patch.

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 4.

Part 2 version 4 makes search queries that consist entirely of whitespace do the right thing (i.e. nothing).

Comment 19

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 2.

Part 3, version 2 retains the getFilterStringByHUDId() method, which turns out to be useful after all.

Comment 20

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4.

Part 4 applies the filters to new console nodes being created. Rather than handle all the different ways that nodes can be added, it listens for the DOMNodeInserted event.

This should be the final part necessary to complete this patch.

Comment 21

[Julian Viereck]

Gave these patches a try. Works nice, filtering by category and by string works. The only thing I'm concerned about is the speed if you have a lot of output. Filtering causes the UI to freeze.

Haven't done benchmarks on what is slow but I guess changing a lot of dom elements is what makes things slow. 

PS: The test function to generate "a lot" of nodes is:

var i = 0; 
function testData() 
{ 
  console.log("testLog"); 
  console.info("testInfo");  
  console.warn("testWarn"); 
  console.error("testError", i); 
  i++;
  if (i < 500) { 
    setTimeout(testData, 0);
  } else { 
    i=0 
  }
};
testData();

Comment 22

[Patrick Walton (:pcwalton)]

(In reply to comment #21)
> Gave these patches a try. Works nice, filtering by category and by string
> works. The only thing I'm concerned about is the speed if you have a lot of
> output. Filtering causes the UI to freeze.
> 
> Haven't done benchmarks on what is slow but I guess changing a lot of dom
> elements is what makes things slow. 

Yes, I've done those tests (I have a "spam-messages" patch that makes a "Spam Messages" button) :) In fact, the XPath is very fast; the problem is that iterating through the NodeList and changing the class of every element is slow.

If there's a way at the platform level to mass-change properties of a NodeList, I'm all ears!

Comment 23

[Johnathan Nightingale [:johnath]]

(In reply to comment #22)
> (In reply to comment #21)
> > Gave these patches a try. Works nice, filtering by category and by string
> > works. The only thing I'm concerned about is the speed if you have a lot of
> > output. Filtering causes the UI to freeze.
> > 
> > Haven't done benchmarks on what is slow but I guess changing a lot of dom
> > elements is what makes things slow. 
> 
> Yes, I've done those tests (I have a "spam-messages" patch that makes a "Spam
> Messages" button) :) In fact, the XPath is very fast; the problem is that
> iterating through the NodeList and changing the class of every element is slow.
> 
> If there's a way at the platform level to mass-change properties of a NodeList,
> I'm all ears!

How about filtering off-thread? You also might find joy in taking the parent of the log elements out of the DOM, doing the changes on the elements, and then adding them back in, so that each change doesn't trigger a reflow.

Comment 24

[David Dahl :ddahl]

Jonas did say that xpath is on main thread only:(

Comment 25

[Johnathan Nightingale [:johnath]]

The xpath's not the slow part, though, right?

xpath to get a nodelist, spin off somewhere else to do the filtering?

Ask in #fx-team, we've solved similar problems elsewhere without UI lagginess. At a high level, the constraints I'd put in here, frustrating though they might be, are:

1) filtration has to feel snappy or people won't use it
2) filtration should never freeze the UI
2b) no UI interaction should ever freeze the UI
3) We needneedneed filtration, so we can't just not do it.

Comment 26

[Patrick Walton (:pcwalton)]

(In reply to comment #25)
> The xpath's not the slow part, though, right?

Right.

> xpath to get a nodelist, spin off somewhere else to do the filtering?

If possible, although I thought DOM manipulation was main thread only?

Comment 27

[Julian Viereck]

>+  adjustVisibilityOnSearchStringChange:
>+  function HS_adjustVisibilityOnSearchStringChange(aHUDId, aSearchString) {
>+    let fn = this.buildXPathFunctionForString(aSearchString);
>+
>+    let displayNode = this.getOutputNodeById(aHUDId);
>+    let doc = displayNode.ownerDocument;
>+
>+    let xpath = './/*[contains(@class, "hud-msg-node") and not(' + fn + ')]';
>+    let result = doc.evaluate(xpath, displayNode, null,
>+      Ci.nsIDOMXPathResult.UNORDERED_NODE_SNAPSHOT_TYPE, null);
>+    for (let i = 0; i < result.snapshotLength; i++) {
>+      result.snapshotItem(i).classList.add("hud-filtered-by-string");
>+    }
>+
>+    xpath = './/*[contains(@class, "hud-msg-node") and ' + fn + ']';
>+    result = doc.evaluate(xpath, displayNode, null,
>+      Ci.nsIDOMXPathResult.UNORDERED_NODE_SNAPSHOT_TYPE, null);
>+    for (let i = 0; i < result.snapshotLength; i++) {
>+      result.snapshotItem(i).classList.remove("hud-filtered-by-string");
>+    }
>+  },

Doesn't solve the basic problem but might increase the speed:

o Query only for the nodes that don't have the "hud-filtered-by-string" class so that it has to be added:

    let xpath = './/*[contains(@class, "hud-msg-node") and not contains(@class, "hud-filtered-by-string") and not(' + fn + ')]';

o Query only for the nodes that have the "hud-filtered-by-string" class so that it has to be removed:

      xpath = './/*[contains(@class, "hud-msg-node") not contains(@class, "hud-filtered-by-string") and ' + fn + ']';

(Never done something with XPath till now - that could might not work.)

Comment 28

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 6.

Patch part 1, version 6 removes the output node from the DOM while modifying it to prevent unnecessary reflows.

Comment 29

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 5.

Patch part 2, version 5 does the same as above for string searches. It also optimizes the searches per Julian's suggestion, and it delays performing the search for a little while after the user types into the search box, to let the user finish typing. This helps perceived performance significantly when there are many nodes.

Comment 30

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3.

Patch part 3, version 3 simply rebases this part of the patch.

Comment 31

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2.

Patch part 4, version 2 fixes a bug in the DOMNodeInserted event listener that the reflow optimization exposed.

We will need to cap the number of lines that the console displays for now; see bug 585237.

Comment 32

[David Dahl :ddahl]

Comment on attachment 463728 [details] [diff] [review]
Proposed patch, part 1, version 6.

The only thing I question is the ".hud-filtered-by-type" class name. Of course, I have not read the other patches yet

Comment 33

[David Dahl :ddahl]

Comment on attachment 463729 [details] [diff] [review]
Proposed patch, part 2, version 5.

>+   * Returns the XPath contains() function necessary to match the given query
>+   * string.
>+   *
>+   * @param string The query string to convert.
@param string aStr
       comment on next line
>+   * @returns string
>+   */

>+  adjustVisibilityOnSearchStringChange:
>+  function HS_adjustVisibilityOnSearchStringChange(aHUDId, aSearchString) {
>+    let fn = this.buildXPathFunctionForString(aSearchString);
I know most reviewers prefer longer/ more verbose variable names, although fn is not that bad to me. maybe xpathFn?


>+    var filterBox = this.filterBox;
>+    function onChange() {
>+      // To improve responsiveness, we let the user finish typing before we
>+      // perform the search.
>+
>+      if (this.timer == null) {
>+        let timerClass = Cc["@mozilla.org/timer;1"];
>+        this.timer = timerClass.createInstance(Ci.nsITimer);
the reference to the timerClass I would think should be made in the constructor, maybe? I'm not sure if that matters. 

>+      } else {
>+        this.timer.cancel();
>+      }
>+
>+      let timerEvent = {
>+        notify: function() { HUDService.updateFilterText(filterBox); }
>+      };
>+
>+      this.timer.initWithCallback(timerEvent, 200, Ci.nsITimer.TYPE_ONE_SHOT);
This can be tricky setting the callback milliseconds, as you never know if your user has an OLD cpu. no worries.

>-.hud-filtered-by-type {
>+.hud-filtered-by-type, .hud-filtered-by-string {
>     display: none;
ok, cool, i see where you are going here. np. 

+ with minor changes. don't worry about  my musings, as the reviewer will know what should be done.

Comment 34

[David Dahl :ddahl]

Comment on attachment 463730 [details] [diff] [review]
Proposed patch, part 3, version 3.

awesome. the simplicity is nice. as this project moves forward it is getting easier to dissect:)

Comment 35

[David Dahl :ddahl]

Comment on attachment 463733 [details] [diff] [review]
Proposed patch, part 4, version 2.

>+    let xpath = ".[" + this.buildXPathFunctionForString(searchString) + "]";

The only thing I am thinking is do we need some more low-level unittests that more extensively test these functions that generate the xpath functions/queries? Maybe file a follow up bug for this?

Comment 37

[Dietrich Ayala (:dietrich)]

Comment on attachment 463729 [details] [diff] [review]
Proposed patch, part 2, version 5.

>+    var filterBox = this.filterBox;
>+    function onChange() {
>+      // To improve responsiveness, we let the user finish typing before we
>+      // perform the search.
>+
>+      if (this.timer == null) {
>+        let timerClass = Cc["@mozilla.org/timer;1"];
>+        this.timer = timerClass.createInstance(Ci.nsITimer);
>+      } else {
>+        this.timer.cancel();
>+      }
>+
>+      let timerEvent = {
>+        notify: function() { HUDService.updateFilterText(filterBox); }
>+      };
>+
>+      this.timer.initWithCallback(timerEvent, 200, Ci.nsITimer.TYPE_ONE_SHOT);

make this value a global const or a class property up top to improve findability in the event it needs tweaking.

>+// Tests the live filtering on search strings.
>+function testLiveFilteringForSearchStrings() {
>+  testLiveFiltering(function(countNetworkNodes) {
>+    setStringFilter("http");
>+    isnot(countNetworkNodes(), 0, "the network nodes are not hidden when " +
>+      "the search string is set to \"http\"");
>+
>+    setStringFilter("hxxp");
>+    is(countNetworkNodes(), 0, "the network nodes are hidden when the " +
>+      "search string is set to \"hxxp\"");
>+
>+    setStringFilter("ht tp");
>+    isnot(countNetworkNodes(), 0, "the network nodes are not hidden when " +
>+      "the search string is set to \"ht tp\"");
>+
>+    setStringFilter(" zzzz   zzzz ");
>+    is(countNetworkNodes(), 0, "the network nodes are hidden when the " +
>+      "search string is set to \" zzzz   zzzz \"");
>+
>+    setStringFilter("");
>+    isnot(countNetworkNodes(), 0, "the network nodes are not hidden when " +
>+      "the search string is removed");
>+  })

should add tests that search for unicode chars, non-printing chars, and strings that flex your ("|'|both) code.

r=me with these changes.

Comment 38

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

Patch doesn't apply cleanly.

Comment 39

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 6 (trunk rebase 2010-08-11).

Patch part 1 rebased to trunk.

Comment 40

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 6.

Patch part 2, version 6 rebases to trunk and makes the reviewer-suggested changes. One bug relating to the quote escaping was fixed (the search was not being done globally). For this reason I'm requesting re-review.

Comment 41

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3 (trunk rebase 2010-08-11).

Rebased part 3 to trunk.

Comment 42

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2 (trunk rebase 2010-08-11).

Rebased part 4 to trunk.

Comment 43

[David Dahl :ddahl]

Comment on attachment 464925 [details] [diff] [review]
Proposed patch, part 2, version 6.

>   /**
>+   * Returns the XPath contains() function necessary to match the given query
>+   * string.
mongo confused. does this return a string or  function?

>+   *
>+   * @param string The query string to convert.
>+   * @returns string
>+   */
>+  buildXPathFunctionForString: function(str) {
nit: brace on newline, please name the function

>+    let words = str.split(/\s+/), results = [];
>+    for (let i = 0; i < words.length; i++) {
>+      let word = words[i];
>+      if (word === "") {
>+        continue;
>+      }
>+
>+      let result;
>+      if (word.indexOf('"') === -1) {
>+        result = '"' + word + '"';
>+      } else if (word.indexOf("'") === -1) {
nit: else if on new line

>+        result = "'" + word + "'";
>+      } else {
ditto

>+    var filterBox = this.filterBox;
>+    function onChange() {
>+      // To improve responsiveness, we let the user finish typing before we
>+      // perform the search.
>+
>+      if (this.timer == null) {
>+        let timerClass = Cc["@mozilla.org/timer;1"];
>+        this.timer = timerClass.createInstance(Ci.nsITimer);
>+      } else {
nit: else on new line

>+        this.timer.cancel();
>+      }
>+
>+      let timerEvent = {
>+        notify: function() { HUDService.updateFilterText(filterBox); }
you should even name this function. it really helps in debugging :)

f+ with nits fixed. awesome!

Comment 44

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 6 (trunk rebase 2010-08-11 2).

Rebased part 1 to trunk.

Comment 45

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2 (revised per feedback, trunk rebase 2010-08-11).

Made feedback-requested changes to the patch part 2. Rebased to trunk.

Comment 46

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2 (trunk rebase 2010-08-11 2).

Part 3 still applies. Part 4 needed rebasing to trunk.

Comment 47

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 6 (trunk rebase 2010-08-11 3).

Now that the console is themed separately on Linux, we need to add all CSS changes to the gnomestripe/ directory as well. New part 1 addresses this.

Comment 48

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 6 (revised per feedback, trunk rebase 2010-08-11 2).

Ditto for part 2. I had also neglected to add Windows CSS on this one as well...

Comment 49

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3 (trunk rebase 2010-08-11 2).

Likewise for part 3.

Comment 50

[Dietrich Ayala (:dietrich)]

Comment on attachment 465071 [details] [diff] [review]
Proposed patch, part 2, version 6 (revised per feedback, trunk rebase 2010-08-11 2).

you didn't pull out the timer delay value per the last review. r=me with that fixed though.

Comment 51

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 6 (revised per review).

Patch part 2 updated to pull out the constant delay per review.

Comment 52

[David Dahl :ddahl]

Patrick: just to MAKE SURE what patches to check in please enumerate them here like:

1. Part 1 V. 6 (trunk rebase)

etc...

Comment 53

[David Dahl :ddahl]

Attachment: Proposed patch, part 2, version 6 (revised per review) [rebased 2010-08-16 ddahl]

Very simple rebase. Part 3 fails pretty badly on latest trunk. Can you rebase again and mark all patches obsolete that are obsolete. You can mark them r+ if they are r+ :)

Comment 54

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 6.1.

Patch rebased to trunk.

Comment 55

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 6.1.

Rebased part 2 to trunk.

Comment 56

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3.1.

Rebased part 3 to trunk.

Comment 57

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2.1.

Part 4 rebased to trunk.

These should be pushed:
* Part 1 version 6.1
* Part 2 version 6.1
* Part 3 version 3.1
* Part 4 version 2.1

Comment 58

[Patrick Walton (:pcwalton)]

Just noticed a regression: the input line is no longer echoed to the user. Tracking down now.

Comment 59

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2.2.

Patch part 4 version 2.2 fixes a last-minute regression caused by the styling of the echoed JavaScript input.

Comment 60

[Patrick Walton (:pcwalton)]

http://hg.mozilla.org/mozilla-central/rev/5d07b7870058
http://hg.mozilla.org/mozilla-central/rev/d58ccc4fe579
http://hg.mozilla.org/mozilla-central/rev/2c052379ae85
http://hg.mozilla.org/mozilla-central/rev/eb02ab4b914f