Closed
Bug 1169096
Opened 9 years ago
Closed 9 years ago
Filter not working for object inspection with nested properties in variables view
Categories
(DevTools :: Object Inspector, defect)
Tracking
(firefox40 unaffected, firefox41+ fixed, firefox42 fixed)
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox40 | --- | unaffected |
firefox41 | + | fixed |
firefox42 | --- | fixed |
People
(Reporter: bgrins, Assigned: ochameau)
References
Details
(Keywords: regression)
Attachments
(4 files, 7 obsolete files)
84.56 KB,
image/png
|
Details | |
2.13 KB,
patch
|
past
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.95 KB,
patch
|
past
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
ochameau
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
I believe this is due to Bug 1023386 since it's working in nightly but not a recent build: STR: console.log({foo: {bar: {baz: 1}}}) Click on the Object link Expand the properties Search for 'baz' Expected: 'baz' is visible Actual Nothing is found. Also, oddly the text 'Object' is replaced with the string that was searched for (see screenshot)
Assignee | ||
Comment 1•9 years ago
|
||
Oh yes, that's because of bug 1023386. I didn't realized filtering was recursive :/ That makes it much more complex! Whenever you search for something, it recompute a new variable tree and reset what you already expanded. And, it only search for properties in the top level object... Also I just realized, the new filter only searches for property names, whereas the previous search was also looking at property preview string. Regarding the 'Object' text, that's much simplier story. It is defined here, the addScope argument is the title: https://hg.mozilla.org/integration/fx-team/file/07cc8095956b/browser/devtools/shared/widgets/VariablesView.jsm#l554
Assignee | ||
Comment 2•9 years ago
|
||
I can remove the remote filtering feature, that will make that work more like before. i.e. it will only be able to filter, but deeply, whatever has been expanded. Except that now, we do not show all the properties if the object is big... Implementing it with the exact same behaviour, with nice performances with remote target (e10s!), sounds like a quite significant work!
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #1) > Oh yes, that's because of bug 1023386. I didn't realized filtering was > recursive :/ > That makes it much more complex! > Whenever you search for something, it recompute a new variable tree and > reset what you already expanded. And, it only search for properties in the > top level object... I don't think searching for properties in the top level object only is desirable behavior, but apparently that is how it previously worked. Ideally filtering would always work as expected (i.e. include any nested properties / values regardless of expansion). I'm not sure if that makes this easier or harder..
Assignee | ||
Comment 4•9 years ago
|
||
Searching for properties names and values regarless of expansion is something we can't do. Imagine doing that on the window object... it is like searching in almost everything! We could do such deep search, but only partially, with a reasonable limit. Searching in nested that has been expanded is one way to do it, but it gives the feeling that we are searching on everything whereas we aren't.
Reporter | ||
Updated•9 years ago
|
Keywords: regression
Reporter | ||
Comment 5•9 years ago
|
||
We've discussed a possible hybrid approach where the server returns all top level results (even non loaded) and the client adds in results from visible expanded properties. Currently the first part of this is already working. The issue is right now is that the client is the only place that knows which tree objects have been expanded.
Assignee | ||
Comment 6•9 years ago
|
||
Wasn't that complex if we make the assumption that already existing attributes aren't duplicated... but I may have missed some corner cases! There is at least this one: let a = new Uint8Array(new ArrayBuffer(2000)); a.aaa=999; inspect(a) Try to search for 100 and then reset the filter, you will end up with a mixed view of regular splits, plus the search results. I'm not sure that's really bad, it just looks living!
Assignee | ||
Comment 7•9 years ago
|
||
And here is a patch to filter property values. (without that, we were just looking at property names)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 8•9 years ago
|
||
brian, what do you think of these two patches? Do you feel reviewing them or should I ask Panos to take a look?
Flags: needinfo?(bgrinstead)
Reporter | ||
Comment 9•9 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #6) > Created attachment 8614130 [details] [diff] [review] > patch v1 > > Wasn't that complex if we make the assumption that already existing > attributes > aren't duplicated... but I may have missed some corner cases! > > There is at least this one: > let a = new Uint8Array(new ArrayBuffer(2000)); a.aaa=999; inspect(a) > > Try to search for 100 and then reset the filter, > you will end up with a mixed view of regular splits, plus the search results. > I'm not sure that's really bad, it just looks living! Thanks for the patches. Two things I noticed: 1) When I reset the filter with the clear button it isn't clearing out the results (I have to press enter once it's already cleared out or ESC to get it to work. This appears to be a separate bug though - I've filed Bug 1171723 for this 2) Having the mixed in loaded results after clearing is a little confusing. Luckily the results still show up in the splits as I'd expect so it's not too bad. But how hard it would be to remove these from the results once the search has been cleared - so things go back to how they were before the search?
Flags: needinfo?(bgrinstead)
See Also: → 1171723
Reporter | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8614130 [details] [diff] [review] patch v1 Review of attachment 8614130 [details] [diff] [review]: ----------------------------------------------------------------- Looking through the patches, I think it'd be best to have Panos review this code since I'm not as familiar with the VariablesView. We will surely need tests for both of these changes, but flagging for initial feedback to start
Attachment #8614130 -
Flags: feedback?(past)
Comment 11•9 years ago
|
||
Comment on attachment 8614130 [details] [diff] [review] patch v1 Review of attachment 8614130 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +556,3 @@ > this.controller.performSearch(scope, aToken); > + // Filter already displayed attributes > + scope._performSearch(aToken.toLowerCase()); Don't we need to treat null, undefined and empty string specially here, too?
Attachment #8614130 -
Flags: feedback?(past) → feedback+
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8614130 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8614671 -
Attachment is obsolete: true
Assignee | ||
Comment 14•9 years ago
|
||
And a test that covers both patches.
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd510ccae06 (In reply to Brian Grinstead [:bgrins] from comment #9) > (In reply to Alexandre Poirot [:ochameau] from comment #6) > > Created attachment 8614130 [details] [diff] [review] > > patch v1 > > > > Wasn't that complex if we make the assumption that already existing > > attributes > > aren't duplicated... but I may have missed some corner cases! > > > > There is at least this one: > > let a = new Uint8Array(new ArrayBuffer(2000)); a.aaa=999; inspect(a) > > > > Try to search for 100 and then reset the filter, > > you will end up with a mixed view of regular splits, plus the search results. > > I'm not sure that's really bad, it just looks living! > > Thanks for the patches. Two things I noticed: > > 1) When I reset the filter with the clear button it isn't clearing out the > results (I have to press enter once it's already cleared out or ESC to get > it to work. This appears to be a separate bug though - I've filed Bug > 1171723 for this Submitted a patch there. > 2) Having the mixed in loaded results after clearing is a little confusing. > Luckily the results still show up in the splits as I'd expect so it's not > too bad. But how hard it would be to remove these from the results once the > search has been cleared - so things go back to how they were before the > search? I did that in new patch, the result is cleared when we clear the filter box completely. We still have mixed results if we just remove some characters. I also added a test in a 3rd patch.
Status: ASSIGNED → NEW
Assignee | ||
Comment 16•9 years ago
|
||
Fixed the failing test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7308c2d7dacb
Attachment #8617384 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•9 years ago
|
||
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6947b934775f
Attachment #8627665 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8628802 -
Flags: review?(past)
Assignee | ||
Updated•9 years ago
|
Attachment #8617388 -
Flags: review?(past)
Assignee | ||
Updated•9 years ago
|
Attachment #8617386 -
Flags: review?(past)
Updated•9 years ago
|
Attachment #8617386 -
Flags: review?(past) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8617388 [details] [diff] [review] Test attribute filtering - v1 Review of attachment 8617388 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/test/browser_console_variables_view_filter.js @@ +81,5 @@ > + view = yield fetched; > + > + assertAttrs(view, "foo", "If we reset again, we get back to original state again"); > + > + gWebConsole = gJSTerm = gVariablesView = null; There is also |hud|, but why do you need all these variables in the global scope if you are only accessing them in the test function?
Attachment #8617388 -
Flags: review?(past) → review+
Updated•9 years ago
|
Attachment #8628802 -
Flags: review?(past) → review+
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Panos Astithas [:past] from comment #18) > Comment on attachment 8617388 [details] [diff] [review] > Test attribute filtering - v1 > > Review of attachment 8617388 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/webconsole/test/browser_console_variables_view_filter.js > @@ +81,5 @@ > > + view = yield fetched; > > + > > + assertAttrs(view, "foo", "If we reset again, we get back to original state again"); > > + > > + gWebConsole = gJSTerm = gVariablesView = null; > > There is also |hud|, but why do you need all these variables in the global > scope if you are only accessing them in the test function? I just copy pasted some existing test... I refactored this to use local-scoped variables.
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8617388 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8631539 -
Flags: review+
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f66d1bf589
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2ac055164ed1 https://hg.mozilla.org/integration/fx-team/rev/97382f93c043 https://hg.mozilla.org/integration/fx-team/rev/497ca22d4399
Comment 23•9 years ago
|
||
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=682a89113d92 for timeouts https://treeherder.mozilla.org/logviewer.html#?job_id=3716320&repo=fx-team that started with this changes
Comment 24•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/01e7a095d179 https://hg.mozilla.org/integration/fx-team/rev/747d201febba https://hg.mozilla.org/integration/fx-team/rev/975013913aef
Assignee | ||
Comment 25•9 years ago
|
||
Same patch with macos 10.10 run, to see if that reproduces on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbf20620033
Assignee | ||
Comment 26•9 years ago
|
||
Another try with additional tweaks that may fix such failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b1f02df54c8
Assignee | ||
Comment 27•9 years ago
|
||
Ok, so this is an intermittent, that only reproduces during high load on slaves... The good news is that the additional tweaks seems to fix it. The bad news is that I see at least one another intermittent, reproduced only once for now: browser/devtools/webconsole/test/browser_console_variables_view_filter.js | If we don't manually expand nested attr, we don't see them in search - Got foo, expected I just launched some more runs to see if I can see it being reproduced more than just once!
Assignee | ||
Comment 28•9 years ago
|
||
Looks like a real intermittent, it has been reproduced twice. MacOS seems to be sensible on Command event being dispatched on xul:textbox... Let's try another approach to write this test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97436c93adb
Assignee | ||
Comment 29•9 years ago
|
||
Here is how to fix intermittent on Mac. i.e. do not rely on asynchronous code for firing command event from textbox.xml (XBL)... This is an interdiff on top of attachment 8631539 [details] [diff] [review]. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97436c93adb
Assignee | ||
Updated•9 years ago
|
Attachment #8636571 -
Flags: review?(bgrinstead)
Reporter | ||
Updated•9 years ago
|
Attachment #8636571 -
Flags: review?(bgrinstead) → review+
Comment 30•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/63afe6a4010a https://hg.mozilla.org/integration/fx-team/rev/71c1cea7ff97 https://hg.mozilla.org/integration/fx-team/rev/b8787cf5a4d3
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8631539 -
Attachment is obsolete: true
Attachment #8636571 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8636764 -
Flags: review+
Comment 32•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63afe6a4010a https://hg.mozilla.org/mozilla-central/rev/71c1cea7ff97 https://hg.mozilla.org/mozilla-central/rev/b8787cf5a4d3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
[Tracking Requested - why for this release]: Consider uplift to repair object filtering in DevTools
Tracked.
Flags: needinfo?(jryans)
Ryan, could you please request uplift to Aurora whenever the patch seems stable and ready?
I don't know if the landed patches are the same as the bug attachments, so I'll leave to Alex to request uplift.
Flags: needinfo?(jryans) → needinfo?(poirot.alex)
Assignee | ||
Comment 37•9 years ago
|
||
Try on aurora looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ecfd75f2c9 But the patches may not apply as-is on aurora.
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 38•9 years ago
|
||
Comment on attachment 8617386 [details] [diff] [review] Filter property values remotely - v2 Approval Request Comment [Feature/regressing bug #]: Fixing regression from bug 1023386 [User impact if declined]: Different variable inspector behavior between releases [Describe test coverage new/current, TreeHerder]: introduce new test covering this regressed feature [Risks and why]: [String/UUID change made/needed]:
Attachment #8617386 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8628802 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8636764 -
Flags: approval-mozilla-aurora?
Comment on attachment 8617386 [details] [diff] [review] Filter property values remotely - v2 Approved as this has been in m-c for a while and seems safe to uplift to Aurora.
Attachment #8617386 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8628802 [details] [diff] [review] Merge local and remote results - v4 Approved.
Attachment #8628802 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8636764 [details] [diff] [review] Test attribute filtering - v3 Approved.
Attachment #8636764 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 42•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b015222e675e https://hg.mozilla.org/releases/mozilla-aurora/rev/08f7921cb810 https://hg.mozilla.org/releases/mozilla-aurora/rev/70a61acf1e30
Flags: in-testsuite+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•