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

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect, P2)

Description

[Mihai Sucan [:msucan]]

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.

Comment 1

[Victor Porof [:vporof][:vp]]

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?

Comment 2

[Brandon Benvie [:benvie]]

Sure!

Comment 3

[Brandon Benvie [:benvie]]

Attachment: WIP1

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).

Comment 4

[Brandon Benvie [:benvie]]

Attachment: WIP1

Attached the wrong patch.

Comment 5

[Brandon Benvie [:benvie]]

Attachment: WIP2

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".

Comment 6

[Brandon Benvie [:benvie]]

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".

Comment 7

[Mihai Sucan [:msucan]]

The VariablesViewController accessing a private property/method of VariablesView would not be too bad. Victor, what do you think?

Comment 8

[Brandon Benvie [:benvie]]

Attachment: WIP3

This version moves setSingleVariable to VariablesViewController. This seems like it is a good solution.

Comment 9

[Brandon Benvie [:benvie]]

Attachment: WIP4

Remove redundant line.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: WIP5

Set the simpleEvaluationMacro on the container.

Comment 11

[Victor Porof [:vporof][:vp]]

Moving stuff to the Object Inspector component.
Filter on COSMICMIGRAINE.

Comment 12

[Brandon Benvie [:benvie]]

Attachment: WIP6

Adds a new test that exercises the search behavior to confirm it works.

Comment 13

[Victor Porof [:vporof][:vp]]

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.

Comment 14

[Brandon Benvie [:benvie]]

Attachment: WIP7

Fix filtering in netmonitor JSON view.

https://tbpl.mozilla.org/?tree=Try&rev=68a29f258ead

Comment 15

[Mihai Sucan [:msucan]]

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.

Comment 16

[Victor Porof [:vporof][:vp]]

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).

Comment 17

[Victor Porof [:vporof][:vp]]

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).

Comment 18

[Brandon Benvie [:benvie]]

Attachment: WIP8

Addresses Mihai's and Victor's comments.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Two failures on your push so far doesn't leave me feeling very warm and fuzzy. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/393828fbbfb1

https://tbpl.mozilla.org/php/getParsedLog.php?id=25940968&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=25941101&tree=Fx-Team

Comment 20

[Brandon Benvie [:benvie]]

Tests passing locally, let's see if this does it...

https://tbpl.mozilla.org/?tree=Try&rev=cf88a40fdb5f

Comment 21

[Brandon Benvie [:benvie]]

Attachment: WIP9

https://tbpl.mozilla.org/?tree=Try&rev=8dc75c01011c

Comment 22

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=67b8030cdbf0

Comment 23

[Brandon Benvie [:benvie]]

Attachment: WIP10

Ok I think this fixes the intermittent.

https://tbpl.mozilla.org/?tree=Try&rev=17278a3f19cc

Comment 24

[Brandon Benvie [:benvie]]

Attachment: WIP11

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

Comment 25

[Brandon Benvie [:benvie]]

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

Comment 26

[Victor Porof [:vporof][:vp]]

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.

Comment 27

[Brandon Benvie [:benvie]]

Ah good. This intermittent is annoying because it's just a totally test related one probably, related to timing if I had to guess.

Comment 28

[Victor Porof [:vporof][:vp]]

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.

Comment 29

[Victor Porof [:vporof][:vp]]

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.

Comment 30

[Brandon Benvie [:benvie]]

Excellent!

Comment 31

[Victor Porof [:vporof][:vp]]

The depending bugs are in. Let's land this bad boy. Benvie?

Comment 32

[Brandon Benvie [:benvie]]

Excellent! I'll update the patch to remove the _wasToggled stuff and get this landed.

Comment 33

[Brandon Benvie [:benvie]]

Attachment: WIP12

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).

Comment 34

[Victor Porof [:vporof][:vp]]

I think you should still keep the test, to make sure things won't break in the future, but it's up to you!

Comment 35

[Brandon Benvie [:benvie]]

(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.

Comment 36

[Brandon Benvie [:benvie]]

Attachment: WIP13

Nevermind, I figured it out. This version adds the test back in.

https://tbpl.mozilla.org/?tree=Try&rev=5c2b440bc555

Comment 37

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/272dabb40b3d

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for intermittent mochitest-bc failures in the new test.
https://hg.mozilla.org/integration/fx-team/rev/77380272cd4f

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28359803&tree=Fx-Team

Comment 40

[Brandon Benvie [:benvie]]

Attachment: WIP14

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.

Comment 41

[Brandon Benvie [:benvie]]

Relanded without the troublesome test.

https://hg.mozilla.org/integration/fx-team/rev/3732f69b6d81

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/d5b167f3d0d2

https://tbpl.mozilla.org/php/getParsedLog.php?id=28427243&tree=Fx-Team

Comment 43

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=39133b6b74a8

Comment 44

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/cf82863b6987

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf82863b6987