Originally posted by:linclark see https://github.com/devtools-html/gecko-dev/issues/125 Depends on #101 IIRC, in the current console, we search against the rendered string. However, that won't be easy anymore since the messages are stored as objects. The string that the message contains isn't rendered until it reaches the Rep component. We may need to do a search of: - messageText - items in the message.parameters array, which can be primitive value grips, grips with just a type, or grips that are objects with properties. [See grip docs](https://wiki.mozilla.org/Remote_Debugging_Protocol#Grips). We could also JSON.stringify the grip and then do text matching on it like JSON Viewer does, but I'm not sure of the performance implications of that, and I believe it would also give us false positives (because of property names).

The attached patch requires this PR for Reps module: https://github.com/devtools-html/reps/pull/136 Honza

Comment on attachment 8860969 [details] Bug 1307879 - Search within grip preview items; https://reviewboard.mozilla.org/r/132984/#review135762 Thanks Honza r- because of the `getFlattenedGrips` call which would throw an exception since it is undefined in the file. It would be also nice to have some tests to make sure we're filtering grips the way we want and prevent regressions. Also, we should block this bug against the release of a v0.7.0 Reps bundle which would contain the change in `getGripPreviewItems` ::: devtools/client/webconsole/new-console-output/selectors/messages.js:171 (Diff revision 1) > + if (!parameters) { > + return false; > + } > + > + return getAllProps(parameters) > + .join(":") Couldn't this return false positive ? I think we're joining with `:` in `isTextInFrame` because of the way we render frames, i.e. script:line:column . ":" is not directly in the frame object so we have to add them to our check. In this case, I see this can be useful for keys of object (`foo:`), but for some grips, it could be wrong (searching `bar:` shouldn't return true for `{foo: bar}`). What do you think ? Maybe I am missing something here ::: devtools/client/webconsole/new-console-output/selectors/messages.js:186 (Diff revision 1) > + */ > +function getAllProps(grips) { > + let result = grips.reduce((res, grip) => { > + let previewItems = getGripPreviewItems(grip); > + let flatPreviewItems = previewItems.length > 0 > + ? getFlattenedGrips(previewItems) I think we don't have a reference to `getFlattenedGrips` here ::: devtools/client/webconsole/new-console-output/selectors/messages.js:192 (Diff revision 1) > + result = result.filter(grip => > + typeof grip != "object" && nit: we could have a comment here explaining why we filter out object

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3) > It would be also nice to have some tests to make sure we're filtering grips > the way we want and prevent regressions. > > Also, we should block this bug against the release of a v0.7.0 Reps bundle > which would contain the change in `getGripPreviewItems` Yes, I'll create separate patch for it. > ::: devtools/client/webconsole/new-console-output/selectors/messages.js:171 > (Diff revision 1) > > + if (!parameters) { > > + return false; > > + } > > + > > + return getAllProps(parameters) > > + .join(":") > > Couldn't this return false positive ? Good point, I changed to logic. Now join() isn't used at all. > I think we don't have a reference to `getFlattenedGrips` here Fixed > nit: we could have a comment here explaining why we filter out object Done Thanks for the review! Honza

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3) > Also, we should block this bug against the release of a v0.7.0 Reps bundle > which would contain the change in `getGripPreviewItems` Is there a bug report for landing Reps bundle 0.7.0? Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #6) > (In reply to Nicolas Chevobbe [:nchevobbe] from comment #3) > > Also, we should block this bug against the release of a v0.7.0 Reps bundle > > which would contain the change in `getGripPreviewItems` > Is there a bug report for landing Reps bundle 0.7.0? > > Honza Not yet, I'll file it tomorrow and do the bundling on Friday

Filed Bug 1359338 for the 0.7.0 reps bundle and blocked added it as a blocker of this one

Comment on attachment 8860969 [details] Bug 1307879 - Search within grip preview items; https://reviewboard.mozilla.org/r/132984/#review136132 Looks good, will r+ when we have test for this (and when the 0.7.0 bundle lands in m-c)

Comment on attachment 8861403 [details] Bug 1307879 - Implement test for searching in grips; https://reviewboard.mozilla.org/r/133396/#review136240 Looks good to me except an unused variable in the test. ::: devtools/client/webconsole/new-console-output/test/store/search.test.js:19 (Diff revision 1) > + let numMessages; > + > + beforeEach(() => { > + store = prepareBaseStore(); > + store.dispatch(actions.filtersClear()); > + numMessages = getAllMessages(store.getState()).size; I don't see it used anywhere, maybe we can remove it ?

Comment on attachment 8860969 [details] Bug 1307879 - Search within grip preview items; https://reviewboard.mozilla.org/r/132984/#review136244

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12) > I don't see it used anywhere, maybe we can remove it ? Yes, done. Thanks! Honza

Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9d12306a1afb Search within grip preview items; r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/95bc050cc465 Implement test for searching in grips; r=nchevobbe

https://hg.mozilla.org/mozilla-central/rev/9d12306a1afb https://hg.mozilla.org/mozilla-central/rev/95bc050cc465

The issue is verified fixed on 55.0a1 (2017-05-02) using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.