Closed Bug 1364386 Opened 3 years ago Closed 3 years ago

Search in frame function shouldn't check for the host

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox55 verified)

VERIFIED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- verified

People

(Reporter: nchevobbe, Assigned: Honza)

Details

(Whiteboard: [console-html])

Attachments

(2 files)

STR: 
1. Go to https://bgrins.github.io/devtools-demos/console/stress.html
2. Click the stream button and again to stop the stream
3. Open the console
4. In the filter field, enter "htt"

Expected result:
All the messages should be hidden

Actual result:
The messages are still visible

---

This is because we check for a match in the frame of a message, which contains the whole URL https://bgrins.github.io/devtools-demos/console/stress.html.

But in the UI, the Frame component only show "stress.html".
So maybe we should run the getSourceNames function (http://searchfox.org/mozilla-central/source/devtools/client/shared/source-utils.js#112) and compare to the short name.

The old console was checking only what was shown on screen and we should do the same.
Priority: -- → P2
Whiteboard: [console-html]
Flags: qe-verify?
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Flags: qe-verify? → qe-verify+
QA Contact: iulia.cristescu
Iteration: 55.5 - May 15 → 55.6 - May 29
Comment on attachment 8868148 [details]
Bug 1364386 - Use only file name for search;

https://reviewboard.mozilla.org/r/139756/#review143122

Thanks Honza !
Could you add a mochitest to make sure we don't pick the whole file path in search anymore ?

::: devtools/client/webconsole/new-console-output/selectors/messages.js:137
(Diff revision 1)
> +  return [short, frame.line, frame.column]
>      .join(":")

nit: since we're explicitely picking the things that we need now, I wonder if it wouldn't be faster to directly use a string

```
return `${short}:{frame.line}:{frame.column}`
  .toLocaleLowerCase()
  .includes(text.toLocaleLowerCase());
```

so we don't have to create an array and join it.

Feel free to keep it like it is, I'm not feeling strong about this
Comment on attachment 8868148 [details]
Bug 1364386 - Use only file name for search;

https://reviewboard.mozilla.org/r/139756/#review143122

I meant a mocha test
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> Comment on attachment 8868148 [details]
> Bug 1364386 - Use only file name for search;
> 
> https://reviewboard.mozilla.org/r/139756/#review143122
> 
> Thanks Honza !
> Could you add a mochitest to make sure we don't pick the whole file path in
> search anymore ?
Done 

> ::: devtools/client/webconsole/new-console-output/selectors/messages.js:137
> (Diff revision 1)
> > +  return [short, frame.line, frame.column]
> >      .join(":")
> 
> nit: since we're explicitely picking the things that we need now, I wonder
> if it wouldn't be faster to directly use a string
Done

Honza
Comment on attachment 8868148 [details]
Bug 1364386 - Use only file name for search;

https://reviewboard.mozilla.org/r/139756/#review143778

Looks good !
Attachment #8868148 - Flags: review?(nchevobbe) → review+
Comment on attachment 8868539 [details]
Bug 1364386 - Update tests;

https://reviewboard.mozilla.org/r/140154/#review143780

Thanks Honza, looking all good
Attachment #8868539 - Flags: review?(nchevobbe) → review+
https://hg.mozilla.org/mozilla-central/rev/75bc9f142325
https://hg.mozilla.org/mozilla-central/rev/b02281ac790d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Managed to reproduce the initial issue on 55.0a1 (2017-04-23). I can confirm the bug is verified fixed on 55.0a1 (2017-05-21), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.