Closed Bug 843019 Opened 7 years ago Closed 6 years ago

Variables view input filter doesn't work until you press Escape

Categories

(DevTools :: Object Inspector, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: msucan, Assigned: bbenvie)

References

Details

Attachments

(1 file, 14 obsolete files)

In bug 808370 vview was added to the web console. It seems that the filter properties input doesn't work the first time you try to use it - you need to press Escape once. This needs further investigation.
Benvie, you've been doing a lot of good stuff to the VariablesView lately. Could you also maybe take a look at this if you have the time?
Flags: needinfo?(bbenvie)
Sure!
Flags: needinfo?(bbenvie)
QA Contact: bbenvie
Assignee: nobody → bbenvie
QA Contact: bbenvie
Status: NEW → ASSIGNED
Attached patch WIP1 (obsolete) — Splinter Review
The problem is that Scope#_performSearch can't recurse to children that have "_wasToggled" set to false (the default). It seems the debugger always ensures this is the case (probably because it actually uses Scope toggling) but the webconsole does not. The reason it works after pressing escape is because _performSearch forces "_wasToggled" to be true if the search is empty (AKA pressing escape, or entering a character and backspacing it).

This patch is the most direct and crude way to force the correct state for the webconsole's usage of search. It breaks encapsulation by directly setting a private property ("_wasToggled" to true). I experimented with a number of other ways of making it so _performSearch would recurse as desired (such as by checking !(this instanceof Variable) in the condition) but all of those attempts caused a few tests to break (specifically one of the sets in browser_dbg_propertyview-filter-08.js).
Attached patch WIP1 (obsolete) — Splinter Review
Attached the wrong patch.
Attachment #776556 - Attachment is obsolete: true
Attached patch WIP2 (obsolete) — Splinter Review
This patch has what I think is a good solution. I've added a method `VariablesView#setSingleVariable` (bikeshedding of the name welcome) that handles creating a single Scope, adding a single Variable to it, and then optionally populating it with a given actor or raw object. In patch 825039 I duplicate this code exactly in the Scratchpad to remote the VariablesView, so I think it's a good candidate for moving to the VariablesView itself, and it conveniently solves the problem with encapsulation of "_wasToggled".
Attachment #776559 - Attachment is obsolete: true
Arg nevermind, I just realized handling of actor stuff shouldn't happen in VariablesView. If anything, this would have to move to VariablesViewActor, but then it'd be breaking encapsulation by setting "_wasToggled".
The VariablesViewController accessing a private property/method of VariablesView would not be too bad. Victor, what do you think?
Attached patch WIP3 (obsolete) — Splinter Review
This version moves setSingleVariable to VariablesViewController. This seems like it is a good solution.
Attachment #776597 - Attachment is obsolete: true
Attached patch WIP4 (obsolete) — Splinter Review
Remove redundant line.
Attachment #776632 - Attachment is obsolete: true
Attached patch WIP5 (obsolete) — Splinter Review
Set the simpleEvaluationMacro on the container.
Attachment #776635 - Attachment is obsolete: true
Attachment #777170 - Attachment is patch: true
Attachment #777170 - Attachment mime type: text/x-patch → text/plain
Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.
Component: Developer Tools: Debugger → Developer Tools: Object Inspector
Attached patch WIP6 (obsolete) — Splinter Review
Adds a new test that exercises the search behavior to confirm it works.
Attachment #777170 - Attachment is obsolete: true
Attachment #780566 - Flags: review?(mihai.sucan)
Don't forget that this also affects the network monitor, since it uses the exact same mechanism for displaying jsons.

1. Open netmonitor
2. Go to https://en.wikipedia.org/wiki/Internet_media_type
3. Select https://en.wikipedia.org/w/api.php?action=ulslocalization&language=en
4. Inspect the Response tab
5. Try to filter.
Attachment #780566 - Flags: review?(mihai.sucan)
Attached patch WIP7 (obsolete) — Splinter Review
Fix filtering in netmonitor JSON view.

https://tbpl.mozilla.org/?tree=Try&rev=68a29f258ead
Attachment #780566 - Attachment is obsolete: true
Attachment #780630 - Flags: review?(vporof)
Attachment #780630 - Flags: review?(mihai.sucan)
Comment on attachment 780630 [details] [diff] [review]
WIP7

Review of attachment 780630 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks. I reviewed the webconsole-related changes. Victor should check the rest. Thanks for the fix!

::: browser/devtools/webconsole/test/browser_webconsole_bug_843019_variables_view_filter.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Check that variables view works as expected in the web console.

Please provide a description that fits the bug, and please mention the bug number.
Attachment #780630 - Flags: review?(mihai.sucan) → review+
Comment on attachment 780630 [details] [diff] [review]
WIP7

Review of attachment 780630 [details] [diff] [review]:
-----------------------------------------------------------------

GG, r+ with the changes below if you agree with them.

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +349,5 @@
> +    scope.expanded = true;
> +    scope.locked = true;
> +
> +    let container = scope.addItem();
> +    container._wasToggled = true;

I think it'd better to move this inside addItem(). If there's no name provided, assume _wasToggled = true, something like this._wasToggled = !!aName, before setting child.header in a similar fashion.

This way, setSingleVariable would be just sugar on top of working methods, not a workaround.

@@ +355,5 @@
> +    if (aOptions.objectActor) {
> +      this.expand(container, aOptions.objectActor);
> +    } else if (aOptions.rawObject) {
> +      container.populate(aOptions.rawObject, { expanded: true });
> +      this.view.commitHierarchy();

I don't think you should do this by default, remove the commitHierarchy call.

Hierarchies are only used if you want to briefly flash changed variables and properties when populating the variables view. This code assume a createHierarchy call was previously made, which may not always be the case (some consumers may not require this functionality).
Attachment #780630 - Flags: review?(vporof) → review+
Comment on attachment 780630 [details] [diff] [review]
WIP7

Review of attachment 780630 [details] [diff] [review]:
-----------------------------------------------------------------

GG, r+ with the changes below if you agree with them.

::: browser/devtools/shared/widgets/VariablesViewController.jsm
@@ +349,5 @@
> +    scope.expanded = true;
> +    scope.locked = true;
> +
> +    let container = scope.addItem();
> +    container._wasToggled = true;

I think it'd better to move this inside addItem(). If there's no name provided, assume _wasToggled = true, something like this._wasToggled = !!aName, before setting child.header in a similar fashion.

This way, setSingleVariable would be just sugar on top of working methods, not a workaround.

@@ +355,5 @@
> +    if (aOptions.objectActor) {
> +      this.expand(container, aOptions.objectActor);
> +    } else if (aOptions.rawObject) {
> +      container.populate(aOptions.rawObject, { expanded: true });
> +      this.view.commitHierarchy();

I don't think you should do this by default, remove the commitHierarchy call.

Hierarchies are only used if you want to briefly flash changed variables and properties when populating the variables view. This code assume a createHierarchy call was previously made, which may not always be the case (some consumers may not require this functionality).
Attached patch WIP8 (obsolete) — Splinter Review
Addresses Mihai's and Victor's comments.
Attachment #780630 - Attachment is obsolete: true
Tests passing locally, let's see if this does it...

https://tbpl.mozilla.org/?tree=Try&rev=cf88a40fdb5f
Attached patch WIP9 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=8dc75c01011c
Attachment #783417 - Attachment is obsolete: true
Attached patch WIP10 (obsolete) — Splinter Review
Ok I think this fixes the intermittent.

https://tbpl.mozilla.org/?tree=Try&rev=17278a3f19cc
Attachment #785074 - Attachment is obsolete: true
Attached patch WIP11 (obsolete) — Splinter Review
Had to make a small rebase. Also I'm doing another try run just to double confirm that the intermittents are fixed.

https://tbpl.mozilla.org/?tree=Try&rev=5ac161522080
Attachment #785245 - Attachment is obsolete: true
Attachment #786991 - Flags: review+
Intermittent on two of them. Odd thing is how both times it is preceded by a seemingly completely unrelated:

> TypeError: profile is undefined: Sidebar.prototype<.getItemByProfile@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/profiler/sidebar.js:91
I'm going to try and completely remove the need for _wasToggled, so let's see if that makes any difference. You can safely shelve this patch for a short while.
Ah good. This intermittent is annoying because it's just a totally test related one probably, related to timing if I had to guess.
I still like the setSingleVariable helper you added, so don't throw away this patch just yet. I'll want it to land after I'm done with things.
The 886848 + 876277 combo seems to fix this bug. Removing _wasToggled did the trick. I'm marking this patch as depending on those, so we can land the lovely setSingleVariable afterwards.
Depends on: 886848, 876277
Excellent!
The depending bugs are in. Let's land this bad boy. Benvie?
Flags: needinfo?(bbenvie)
Flags: needinfo?(bbenvie)
Excellent! I'll update the patch to remove the _wasToggled stuff and get this landed.
Attached patch WIP12 (obsolete) — Splinter Review
Removes the variable-view-filter test and reference to _wasToggled. I also updated setSingleVariable a bit to return both the created Variable as well as the Promise that completes when the Variable finishes expanding. This allows Scratchpad to make use of it, and also makes the webconsole emit "variablesview-updated" at a more accurate time (once the Variable has been expanded).
Attachment #786991 - Attachment is obsolete: true
I think you should still keep the test, to make sure things won't break in the future, but it's up to you!
(In reply to Victor Porof [:vp] from comment #34)
> I think you should still keep the test, to make sure things won't break in
> the future, but it's up to you!

I have no problem with that. But in that case, do you have any idea why that test would cause very common but intermittent failures? EventUtils.{sendChar|sendKey} seem to be really finicky.
Attached patch WIP13 (obsolete) — Splinter Review
Nevermind, I figured it out. This version adds the test back in.

https://tbpl.mozilla.org/?tree=Try&rev=5c2b440bc555
Attachment #809404 - Attachment is obsolete: true
Attachment #809468 - Flags: review+
Backed out for intermittent mochitest-bc failures in the new test.
https://hg.mozilla.org/integration/fx-team/rev/77380272cd4f
Attached patch WIP14Splinter Review
Ok I give up on this test. I'll spin it off into a new bug since it's no longer related to the main content of the patch.
Attachment #809468 - Attachment is obsolete: true
Relanded without the troublesome test.

https://hg.mozilla.org/integration/fx-team/rev/3732f69b6d81
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cf82863b6987
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Duplicate of this bug: 934010
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.