Closed Bug 583359 Opened 12 years ago Closed 12 years ago

Filter options on the Web Console should filter all messages

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Firefox 4.0b5
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: pcwalton, Assigned: pcwalton)

References

Details

(Whiteboard: [kd4b5])

Attachments

(8 files, 25 obsolete files)

6.81 KB, patch
dietrich
: review+
ddahl
: feedback+
Details | Diff | Splinter Review
10.21 KB, patch
dietrich
: review+
ddahl
: feedback+
Details | Diff | Splinter Review
7.30 KB, patch
dietrich
: review+
ddahl
: feedback+
Details | Diff | Splinter Review
10.23 KB, patch
ddahl
: feedback+
Details | Diff | Splinter Review
7.47 KB, patch
Details | Diff | Splinter Review
12.55 KB, patch
Details | Diff | Splinter Review
11.64 KB, patch
Details | Diff | Splinter Review
8.04 KB, patch
Details | Diff | Splinter Review
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.
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.
blocking2.0: --- → ?
Whiteboard: [kd4b4]
Also: it seems to me that this bug is blocked by the bug that changes how the log data is stored.
(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.
blocking2.0: ? → betaN+
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"?
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.
(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.
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" <jonas@sicking.cc>
To: "David Dahl" <ddahl@mozilla.com>
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);
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.
Attached patch Proposed patch, part 1. (obsolete) — Splinter Review
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.
Attachment #462615 - Flags: feedback?(ddahl)
Assignee: nobody → pwalton
New version fixes the inline documentation and chooses slightly saner function names.
Attachment #462615 - Attachment is obsolete: true
Attachment #462871 - Flags: feedback?(ddahl)
Attachment #462615 - Flags: feedback?(ddahl)
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).
Attachment #462871 - Attachment is obsolete: true
Attachment #462885 - Flags: feedback?(ddahl)
Attachment #462871 - Flags: feedback?(ddahl)
Fourth version of the patch corrects the omission of a display name for the new function added to the HUD service.
Attachment #462885 - Attachment is obsolete: true
Attachment #462902 - Flags: feedback?(ddahl)
Attachment #462885 - Flags: feedback?(ddahl)
Fifth version of the patch refactors the test case so that the second part can use the same infrastructure.
Attachment #462902 - Attachment is obsolete: true
Attachment #463321 - Flags: feedback?(ddahl)
Attachment #462902 - Flags: feedback?(ddahl)
Attached patch Proposed patch, part 2. (obsolete) — Splinter Review
Patch part 2 adds live filtering for the search box.
Attachment #463357 - Flags: feedback?(ddahl)
New version of the patch fixes a typo.
Attachment #463357 - Attachment is obsolete: true
Attachment #463362 - Flags: feedback?(ddahl)
Attachment #463357 - Flags: feedback?(ddahl)
Third version of the patch factors out the XPath construction into a separate function and adds the ability to search on multiple keywords.
Attachment #463362 - Attachment is obsolete: true
Attachment #463380 - Flags: feedback?(ddahl)
Attachment #463362 - Flags: feedback?(ddahl)
Attached patch Proposed patch, part 3. (obsolete) — Splinter Review
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.
Attachment #463383 - Flags: feedback?(ddahl)
Part 2 version 4 makes search queries that consist entirely of whitespace do the right thing (i.e. nothing).
Attachment #463380 - Attachment is obsolete: true
Attachment #463389 - Flags: feedback?(ddahl)
Attachment #463380 - Flags: feedback?(ddahl)
Part 3, version 2 retains the getFilterStringByHUDId() method, which turns out to be useful after all.
Attachment #463383 - Attachment is obsolete: true
Attachment #463392 - Flags: feedback?(ddahl)
Attachment #463383 - Flags: feedback?(ddahl)
Attached patch Proposed patch, part 4. (obsolete) — Splinter Review
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.
Attachment #463398 - Flags: feedback?(ddahl)
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();
(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!
(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.
Jonas did say that xpath is on main thread only:(
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.
(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?
>+  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.)
Patch part 1, version 6 removes the output node from the DOM while modifying it to prevent unnecessary reflows.
Attachment #463321 - Attachment is obsolete: true
Attachment #463728 - Flags: feedback?(ddahl)
Attachment #463321 - Flags: feedback?(ddahl)
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.
Attachment #463389 - Attachment is obsolete: true
Attachment #463729 - Flags: feedback?(ddahl)
Attachment #463389 - Flags: feedback?(ddahl)
Patch part 3, version 3 simply rebases this part of the patch.
Attachment #463392 - Attachment is obsolete: true
Attachment #463730 - Flags: feedback?(ddahl)
Attachment #463392 - Flags: feedback?(ddahl)
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.
Attachment #463398 - Attachment is obsolete: true
Attachment #463733 - Flags: feedback?(ddahl)
Attachment #463398 - Flags: feedback?(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
Attachment #463728 - Flags: feedback?(ddahl) → feedback+
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.
Attachment #463729 - Flags: feedback?(ddahl) → feedback+
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:)
Attachment #463730 - Flags: feedback?(ddahl) → feedback+
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?
Attachment #463733 - Flags: feedback?(ddahl) → feedback+
Version: unspecified → Trunk
Duplicate of this bug: 585985
Attachment #463728 - Flags: review?(dietrich)
Attachment #463729 - Flags: review?(dietrich)
Attachment #463730 - Flags: review?(dietrich)
Attachment #463733 - Flags: review?(dietrich)
Attachment #463728 - Flags: review?(dietrich) → review+
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.
Attachment #463729 - Flags: review?(dietrich) → review+
Attachment #463730 - Flags: review?(dietrich) → review+
Attachment #463733 - Flags: review?(dietrich) → review+
Attachment #463728 - Flags: approval2.0?
Attachment #463728 - Flags: approval2.0?
Patch doesn't apply cleanly.
Patch part 1 rebased to trunk.
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.
Attachment #463729 - Attachment is obsolete: true
Attachment #464925 - Flags: feedback?(ddahl)
Rebased part 3 to trunk.
Rebased part 4 to trunk.
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!
Attachment #464925 - Flags: feedback?(ddahl) → feedback+
Rebased part 1 to trunk.
Attachment #464913 - Attachment is obsolete: true
Made feedback-requested changes to the patch part 2. Rebased to trunk.
Attachment #465063 - Flags: review?(dietrich)
Part 3 still applies. Part 4 needed rebasing to trunk.
Attachment #464935 - Attachment is obsolete: true
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.
Attachment #465028 - Attachment is obsolete: true
Ditto for part 2. I had also neglected to add Windows CSS on this one as well...
Attachment #465063 - Attachment is obsolete: true
Attachment #465071 - Flags: review?(dietrich)
Attachment #465063 - Flags: review?(dietrich)
Likewise for part 3.
Attachment #464934 - Attachment is obsolete: true
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.
Attachment #465071 - Flags: review?(dietrich) → review+
Patch part 2 updated to pull out the constant delay per review.
Attachment #465071 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [kd4b4] → [kd4b5]
Patrick: just to MAKE SURE what patches to check in please enumerate them here like:

1. Part 1 V. 6 (trunk rebase)

etc...
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+ :)
Patch rebased to trunk.
Attachment #465070 - Attachment is obsolete: true
Rebased part 2 to trunk.
Attachment #465743 - Attachment is obsolete: true
Attachment #467075 - Attachment is obsolete: true
Rebased part 3 to trunk.
Attachment #465073 - Attachment is obsolete: true
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
Attachment #465065 - Attachment is obsolete: true
Just noticed a regression: the input line is no longer echoed to the user. Tracking down now.
Keywords: checkin-needed
Patch part 4 version 2.2 fixes a last-minute regression caused by the styling of the echoed JavaScript input.
Attachment #467116 - Attachment is obsolete: true
Whiteboard: [kd4b5] → [kd4b5] [patchbitrot]
Whiteboard: [kd4b5] [patchbitrot]
Whiteboard: [kd4b5]
Target Milestone: --- → Firefox 4.0b5
Blocks: 744732
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.