Issue 29522650: Issue 5316 - Adds dynamic filter types to devtools panel (Closed)
|
Created:
Aug. 21, 2017, 3:24 p.m. by Jon Sonesen Modified:
Oct. 2, 2017, 6:48 p.m. Visibility:
Public. |
DescriptionIssue 5316 - Adds dynamic filter types to devtools panel
Patch Set 1 #
Total comments: 22
Patch Set 2 : add css, fix selector, change message name, fix redundant port #
Total comments: 6
Patch Set 3 : move OTHER filterType to end of array #
Total comments: 2
Patch Set 4 : #
MessagesTotal messages: 16
Here is my first patch set for the issue, I tested this on the current version of FirefoxNightly as well as Chrome. Basically, I just loaded the extension and opened the devtools panel checking that the expected filter types appeared, then looking for exceptions in the console. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js File devtools-panel.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:21: Should I comment here about the filter-type generation and the reasoning? https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:43: // Some resource types are not available on Chrome This comment probably needs to be improved
Mostly comments regarding best practices but please make sure that you're not breaking the request filtering feature. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js File devtools-panel.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:21: On 2017/08/21 15:31:07, Jon Sonesen wrote: > Should I comment here about the filter-type generation and the reasoning? Feel free to if you want. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:22: ext.backgroundPage.sendMessage({type: "request.getTypes", what: "filterTypes"}, Detail: You're not retrieving requests but filter types so for consistency with existing message types I'd suggest using "types.get" or "filterTypes.get" in which case you will also no longer need to specify the "what" option. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:25: for (let type in filterTypes) `filterTypes` is an array but for-in loops are iterating through object properties. That means that it'd loop through all enumerable properties which may or may not result in the expected behavior. What you'd want to do here is use a for-of loop. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:29: document.getElementById("filter-type").appendChild(optionNode); Suggestion: It's usually good practice to keep DOM operations to a minimum since those are quite expensive. One way to do that would be by moving `document.getElementById()` out of the loop and by using `optionNode.textContent`. https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:43: // Some resource types are not available on Chrome On 2017/08/21 15:31:07, Jon Sonesen wrote: > This comment probably needs to be improved Indeed… what are you trying to express here? https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:45: Array.from(require("requestBlocker").filterTypes)); Why did you decide to make `filterTypes` a `Set` if you're converting it into an array in the only place you're using it? https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:439: Array.from(require("requestBlocker").filterTypes)); This is exactly the same code as the one above so let's remove it. https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... File skin/devtools-panel.css (left): https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... skin/devtools-panel.css:146: #items[data-filter-type=OTHER] tr:not([data-type=OTHER]) Removing this code without an alternative solution will break the ability to filter requests by filter type. https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... File skin/devtools-panel.css (right): https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... skin/devtools-panel.css:133: #items[data-filter-state=whitelisted] tr:not([data-state=whitelisted]), This is an invalid selector due to the trailing comma.
https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:45: Array.from(require("requestBlocker").filterTypes)); On 2017/08/22 11:21:47, Thomas Greiner wrote: > Why did you decide to make `filterTypes` a `Set` if you're converting it into an > array in the only place you're using it? The way filter types are accumulated in adblockpluschrome there will naturally be duplicates. By using a Set these duplicates are automatically eliminated. Of course, we could do the conversion to an array already inside adblockpluschrome. But since allowing duplicates don't make any sense here anyway, a Set seems to be an appropriate data structure to expose.
Thanks alot for your comments, I will address them Cheers https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js File devtools-panel.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:22: ext.backgroundPage.sendMessage({type: "request.getTypes", what: "filterTypes"}, On 2017/08/22 11:21:46, Thomas Greiner wrote: > Detail: You're not retrieving requests but filter types so for consistency with > existing message types I'd suggest using "types.get" or "filterTypes.get" in > which case you will also no longer need to specify the "what" option. Acknowledged. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:25: for (let type in filterTypes) On 2017/08/22 11:21:46, Thomas Greiner wrote: > `filterTypes` is an array but for-in loops are iterating through object > properties. That means that it'd loop through all enumerable properties which > may or may not result in the expected behavior. What you'd want to do here is > use a for-of loop. Acknowledged. https://codereview.adblockplus.org/29522650/diff/29522651/devtools-panel.js#n... devtools-panel.js:29: document.getElementById("filter-type").appendChild(optionNode); On 2017/08/22 11:21:46, Thomas Greiner wrote: > Suggestion: It's usually good practice to keep DOM operations to a minimum since > those are quite expensive. One way to do that would be by moving > `document.getElementById()` out of the loop and by using > `optionNode.textContent`. Acknowledged. https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:43: // Some resource types are not available on Chrome On 2017/08/22 11:21:47, Thomas Greiner wrote: > On 2017/08/21 15:31:07, Jon Sonesen wrote: > > This comment probably needs to be improved > > Indeed… what are you trying to express here? Yeah I dunno, I thought it would make sense to explain the logic for exposing the requestBlocker.filterTypes to the devtools panel...but maybe it is not needed? https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:439: Array.from(require("requestBlocker").filterTypes)); On 2017/08/22 11:21:47, Thomas Greiner wrote: > This is exactly the same code as the one above so let's remove it. Acknowledged. https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... File skin/devtools-panel.css (left): https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... skin/devtools-panel.css:146: #items[data-filter-type=OTHER] tr:not([data-type=OTHER]) On 2017/08/22 11:21:47, Thomas Greiner wrote: > Removing this code without an alternative solution will break the ability to > filter requests by filter type. Is it better to just leave it then and add all potential types? https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... File skin/devtools-panel.css (right): https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... skin/devtools-panel.css:133: #items[data-filter-state=whitelisted] tr:not([data-state=whitelisted]), On 2017/08/22 11:21:47, Thomas Greiner wrote: > This is an invalid selector due to the trailing comma. Acknowledged.
https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:43: // Some resource types are not available on Chrome On 2017/08/24 11:29:06, Jon Sonesen wrote: > On 2017/08/22 11:21:47, Thomas Greiner wrote: > > On 2017/08/21 15:31:07, Jon Sonesen wrote: > > > This comment probably needs to be improved > > > > Indeed… what are you trying to express here? > > Yeah I dunno, I thought it would make sense to explain the logic for exposing > the requestBlocker.filterTypes to the devtools panel...but maybe it is not > needed? Agreed, I don't think it adds value to add a comment here. https://codereview.adblockplus.org/29522650/diff/29522651/messageResponder.js... messageResponder.js:45: Array.from(require("requestBlocker").filterTypes)); On 2017/08/22 11:38:28, Sebastian Noack wrote: > On 2017/08/22 11:21:47, Thomas Greiner wrote: > > Why did you decide to make `filterTypes` a `Set` if you're converting it into > an > > array in the only place you're using it? > > The way filter types are accumulated in adblockpluschrome there will naturally > be duplicates. By using a Set these duplicates are automatically eliminated. Of > course, we could do the conversion to an array already inside adblockpluschrome. > But since allowing duplicates don't make any sense here anyway, a Set seems to > be an appropriate data structure to expose. Ok, not great either way then, I guess. Thanks for explaining the reasoning. https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... File skin/devtools-panel.css (left): https://codereview.adblockplus.org/29522650/diff/29522651/skin/devtools-panel... skin/devtools-panel.css:146: #items[data-filter-type=OTHER] tr:not([data-type=OTHER]) On 2017/08/24 11:29:06, Jon Sonesen wrote: > On 2017/08/22 11:21:47, Thomas Greiner wrote: > > Removing this code without an alternative solution will break the ability to > > filter requests by filter type. > > Is it better to just leave it then and add all potential types? Since the ticket also refers to this CSS file I'd recommend that we should also remove the hardcoded filter types from here. One way to achieve this would be to add a `<style>` element that we populate with the according hiding rules at runtime. At some point in the future we should be able to simply use a reference combinator for that instead (see https://www.w3.org/TR/selectors4/#idref-combinators) but for now we have to go with a less elegant solution I'm afraid.
https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js... messageResponder.js:46: Array.from(require("requestBlocker").filterTypes)); I wonder whether it would be worth to make sure that "OTHER" is the last item in the list, and if so whether this should be done here or in adblockpluschrome. If we don't explicitly handle this, "OTHER" will appear somewhere in the middle of the list, which might appear weird/confusing in the user interface.
https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js... messageResponder.js:46: Array.from(require("requestBlocker").filterTypes)); On 2017/09/05 18:12:14, Sebastian Noack wrote: > I wonder whether it would be worth to make sure that "OTHER" is the last item in > the list, and if so whether this should be done here or in adblockpluschrome. If > we don't explicitly handle this, "OTHER" will appear somewhere in the middle of > the list, which might appear weird/confusing in the user interface. Good point. We're currently not imposing any order on the items so I think we can leave it the way it is though. If we want to start ordering items, I agree with the change you suggested. Such a change would be something I'd consider to be part of the UI code since it's only relevant for how information is laid out in the UI.
https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js... messageResponder.js:46: Array.from(require("requestBlocker").filterTypes)); On 2017/09/06 11:00:25, Thomas Greiner wrote: > On 2017/09/05 18:12:14, Sebastian Noack wrote: > > I wonder whether it would be worth to make sure that "OTHER" is the last item > in > > the list, and if so whether this should be done here or in adblockpluschrome. > If > > we don't explicitly handle this, "OTHER" will appear somewhere in the middle > of > > the list, which might appear weird/confusing in the user interface. > > Good point. We're currently not imposing any order on the items so I think we > can leave it the way it is though. > > If we want to start ordering items, I agree with the change you suggested. Such > a change would be something I'd consider to be part of the UI code since it's > only relevant for how information is laid out in the UI. What about removing the OTHER return from lib/requestBlocker and then hard coding it into devtool-panel.html right after any, so you have any and other appearing next to each other at the top of the list?
https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js... messageResponder.js:46: Array.from(require("requestBlocker").filterTypes)); On 2017/09/06 11:00:25, Thomas Greiner wrote: > Good point. We're currently not imposing any order on the items so I think we > can leave it the way it is though. I don't think the order matters much, except for OTHER, which is just weird to have in the middle of the list. So why not just move it to the end of the list? > If we want to start ordering items, I agree with the change you suggested. Such > a change would be something I'd consider to be part of the UI code since it's > only relevant for how information is laid out in the UI. Well, before this change, the items were in a hard-coded order, with OTHER being the last item. So if we agree that OTHER being last was not just coincidental, changing this would be a UX regression. On 2017/09/06 13:12:15, Jon Sonesen wrote: > What about removing the OTHER return from lib/requestBlocker and then hard > coding it into devtool-panel.html right after any, so you have any and other > appearing next to each other at the top of the list? Then requestBlocker.filterTypes would be incomplete/inconsistent which might be confusing and likely results in bugs if we should ever use it somewhere else. Also it doesn't seem any less code overall than just changing the order.
https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js... messageResponder.js:46: Array.from(require("requestBlocker").filterTypes)); On 2017/09/06 17:39:19, Sebastian Noack wrote: > On 2017/09/06 11:00:25, Thomas Greiner wrote: > > Good point. We're currently not imposing any order on the items so I think we > > can leave it the way it is though. > > I don't think the order matters much, except for OTHER, which is just weird to > have in the middle of the list. So why not just move it to the end of the list? > > > If we want to start ordering items, I agree with the change you suggested. > Such > > a change would be something I'd consider to be part of the UI code since it's > > only relevant for how information is laid out in the UI. > > Well, before this change, the items were in a hard-coded order, with OTHER being > the last item. So if we agree that OTHER being last was not just coincidental, > changing this would be a UX regression. > > On 2017/09/06 13:12:15, Jon Sonesen wrote: > > What about removing the OTHER return from lib/requestBlocker and then hard > > coding it into devtool-panel.html right after any, so you have any and other > > appearing next to each other at the top of the list? > > Then requestBlocker.filterTypes would be incomplete/inconsistent which might be > confusing and likely results in bugs if we should ever use it somewhere else. > Also it doesn't seem any less code overall than just changing the order. Yeah, I agree here. Will do.
https://codereview.adblockplus.org/29522650/diff/29541685/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29541685/messageResponder.js... messageResponder.js:48: filterTypes.push(filterTypes.splice(filterTypes.indexOf("OTHER"), 1)[0]); I would rather use the rest operator here. That way, in (the theoretical) case there is no OTHER in filterTypes, you wouldn't add undefined to filterTypes. filterTypes.push(...filterTypes.splice(filterTypes.indexOf("OTHER"), 1));
https://codereview.adblockplus.org/29522650/diff/29541685/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29541685/messageResponder.js... messageResponder.js:48: filterTypes.push(filterTypes.splice(filterTypes.indexOf("OTHER"), 1)[0]); On 2017/09/11 16:44:14, Sebastian Noack wrote: > I would rather use the rest operator here. That way, in (the theoretical) case > there is no OTHER in filterTypes, you wouldn't add undefined to filterTypes. > > filterTypes.push(...filterTypes.splice(filterTypes.indexOf("OTHER"), 1)); Oh, nifty. Thanks for pointing that out
https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29522650/diff/29533585/messageResponder.js... messageResponder.js:46: Array.from(require("requestBlocker").filterTypes)); On 2017/09/06 17:39:19, Sebastian Noack wrote: > Well, before this change, the items were in a hard-coded order, with OTHER being > the last item. So if we agree that OTHER being last was not just coincidental, > changing this would be a UX regression. Good point. In that case no objections from my end.
LGTM
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
