Issue 4644 - Filter requests in devtools panel by search string

Issue 4644 - Filter requests in devtools panel by search string

[kzar, 2016-11-14 19:55:11]

Patch Set 1

[kzar, 2016-11-14 20:04:14]

Patch Set 2 : Allow filtering by request type

[kzar, 2016-11-15 09:56:27]

Patch Set 3 : Fixed typo

[Thomas Greiner, 2016-11-29 12:17:07]

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:127: var filtered = true;
Detail: Unused variable.

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:135: for (var elements of elementsToSearch)
Detail: For consistency and to reduce the potential for accidents, please use
braces around blocks with multiple lines.

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:191: case "update-record":
Since you're adapting "add-record" to ensure that the filter also applies to
newly added records, why not do that for updated records?

Generally, it seems that the class should be added in `createRecord()` instead
of here.

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:211: window.addEventListener("message", function(event)
Detail: Separating those two listeners by a newline would be great to further
differentiate those.

[kzar, 2016-11-30 14:50:20]

Patch Set 4 : Addressed feedback

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:127: var filtered = true;
On 2016/11/29 12:17:06, Thomas Greiner wrote:
> Detail: Unused variable.

Whoops well spotted, done.

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:135: for (var elements of elementsToSearch)
On 2016/11/29 12:17:07, Thomas Greiner wrote:
> Detail: For consistency and to reduce the potential for accidents, please use
> braces around blocks with multiple lines.

Done.

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:191: case "update-record":
On 2016/11/29 12:17:07, Thomas Greiner wrote:
> Since you're adapting "add-record" to ensure that the filter also applies to
> newly added records, why not do that for updated records?
> 
> Generally, it seems that the class should be added in `createRecord()` instead
> of here.

Done.

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:211: window.addEventListener("message", function(event)
On 2016/11/29 12:17:07, Thomas Greiner wrote:
> Detail: Separating those two listeners by a newline would be great to further
> differentiate those.

Done.

[Thomas Greiner, 2016-11-30 15:40:16]

LGTM

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js
File devtools-panel.js (right):

https://codereview.adblockplus.org/29362522/diff/29362532/devtools-panel.js#n...
devtools-panel.js:135: for (var elements of elementsToSearch)
On 2016/11/30 14:50:20, kzar wrote:
> On 2016/11/29 12:17:07, Thomas Greiner wrote:
> > Detail: For consistency and to reduce the potential for accidents, please
use
> > braces around blocks with multiple lines.
> 
> Done.

Thanks!