Issue 29321198: Issue 2376 - Implement custom filters in new options page

saroyanm

Thomas can you please have a look, when you have time.

June 29, 2015, 11:30 a.m. (2015-06-29 11:30:38 UTC) #1

saroyanm

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#newcode709 skin/options.css:709: #custom-filters-add-btn::after I do think we need to show icon ...

June 29, 2015, 11:34 a.m. (2015-06-29 11:34:18 UTC) #2

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29321199/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29321199/background.js#newcode263 background.js:263: global.WhitelistFilter = modules.filterClasses.WhitelistFilter; This does not need to be ...

June 30, 2015, 9:23 a.m. (2015-06-30 09:23:30 UTC) #3

saroyanm

Thomas can you please have a look, we are having an error msg in devenv. ...

July 8, 2015, 6:25 p.m. (2015-07-08 18:25:43 UTC) #4

Thomas can you please have a look, 
we are having an error msg in devenv. which we were having also before current
patch. 
I'll also investigate that problem.

https://codereview.adblockplus.org/29321198/diff/29321199/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29321199/background.js#newco...
background.js:263: global.WhitelistFilter =
modules.filterClasses.WhitelistFilter;
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> This does not need to be in the global namespace. It's already exposed in the
> "filterClasses" object from which you can require it.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/option...
File locale/en-US/options.json (right):

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/option...
locale/en-US/options.json:131: "description": "Option name in Advanced tab",
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> Which option?

For some option names we are specifying for some not :/ 
updated other similar descriptions for consistency.

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/option...
locale/en-US/options.json:135: "description": "Placeholder text in Advanced
tab",
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> Placeholder text for what?

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/option...
locale/en-US/options.json:138: "options_customFilter_edit_btn": {
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> The "_btn" at the end is not necessary for it to be uniquely identifiable.
Same
> goes for the ones below.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/option...
locale/en-US/options.json:139: "description": "Button label in Advanced tab",
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> Suggestion: "Label for editing custom filter in Advanced tab". Same goes for
the
> ones below.

Done, better.

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/option...
locale/en-US/options.json:148: "message": "Save"
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> Why is "Save" upper-case while "list view" is lower-case? Both use the same UI
> element.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:215: case "filters.importRaw":
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> It's easier to search for the right method if the cases are ordered
> alphabetically so could you please move them to the according position?

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:219: var errors = result.errors.filter(function(e)
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> Please rename this variable from "e" to "error". Generally, I'd avoid
shortening
> variables wherever it makes sense to avoid confusion.

Done, Agree

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:221: return e.type != "unexpected-filter-list-header";
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> I'm aware that you didn't write this but what's the reason for excluding this
> kind of error?

It's being fired when the the filter starts with "[".
I can't see reason excluding this error. If it's invalid filter we should
somehow inform the user I guess. Should we remove the check ?

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:248: if (filter instanceof WhitelistFilter &&
/^@@\|\|([^\/:]+)\^\$document$/.test(filter.text))
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> Beware of the 80 characters limit

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:256: case "parse.filter":
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> This name is inconsistent with the other ones which follow the pattern
> `<object>.<action>`. Therefore I'd suggest renaming it to "filters.parse".

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:257: var parseFilter =
require("filterValidation").parseFilter;
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> No need to require it each time it's called so please move this to the
beginning
> of the file where you already resolve all of the other dependencies.
> 
> Also found this in "filters.importRaw".

Agree, was stupid.

https://codereview.adblockplus.org/29321198/diff/29321199/messageResponder.js...
messageResponder.js:258: callback(parseFilter(message.text));
On 2015/06/30 09:23:26, Thomas Greiner wrote:
> "parseFilter" returns an instance of `Filter` (e.g. `WhitelistFilter`).
However,
> those object types don't exist in the context of the options page so you need
to
> convert it to a plain object before using `convertFilter`.
> Same applies to `FilterParsingError` for which, however, there's no conversion
> function yet.
> 
> var result = parseFilter(message.text);
> callback({
>   error: convertFilterParsingError(result.error),
>   filter: convertFilter(result.filter)
> });

Good point, done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.html
File options.html (right):

https://codereview.adblockplus.org/29321198/diff/29321199/options.html#newcod...
options.html:169: <h1><span class="i18n_options_tweaks_title"></span><a
class="i18n_options_readMore tooltip" href="#"></a></h1>
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> While I did intentionally ignore the Advanced tab in the review for #1524, I
> still expect the same issues to be fixed in this one if they occur.
> 
> So please don't squish everything into a single line. This is not legible and
> difficult to maintain.

I didn't touch this part of advanced tab intentionally, I was only working on
the CustomFilters block, were expecting to start with this part with #2377 and
with blocking list part with #2375. Not sure if this fixes goes to this ticket.

https://codereview.adblockplus.org/29321198/diff/29321199/options.html#newcod...
options.html:200: <h4 id="custom-filters-header"
class="i18n_options_customFilters_title"></h4>
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> Where did <h2> and <h3> go?

You are right we don't have h4 in mock up, h2 should go here.

https://codereview.adblockplus.org/29321198/diff/29321199/options.html#newcod...
options.html:207: <div id="custom-filter-add-wrapper" class="controls">
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> I noticed that almost every single element has an ID. For instance, for this
one
> I couldn't even find anywhere in the JavaScript or CSS code where you
reference
> it via its ID.
Referencing here ->
https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...


> Therefore please make sure for all elements that there's no other way to
> uniquely identify them other than an ID (e.g. #custom-filters-add-btn could be
> referenced via `#custom-filters-add button` in which case you can shorten the
ID
> of #custom-filters-add-wrapper to #custom-filters-add).

Fare enough done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.html#newcod...
options.html:207: <div id="custom-filter-add-wrapper" class="controls">
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> Tip: You could avoid that "Enter" key check by using <form> and simply
listening
> to its "submit" event.

Good point, make sense to also handle in consistency related ticket the
whitelisting part. Added to the document.

https://codereview.adblockplus.org/29321198/diff/29321199/options.html#newcod...
options.html:212: <textarea id="custom-filters-textarea"></textarea>
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> As mentioned in the previous review, IDs or class names should not refer to a
> specific UI element because that can change at some point. Therefore it's more
> flexible if you use something more generic like "custom-filters-raw" or
> "custom-filters-modify".

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode39
options.js:39: this.items.sort(function(a, b)
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> You need to first filter out all empty lines before sorting them or otherwise
> it'll throw an exception if you try to convert a filter to lower case.

I do wander why we even need to submit the empty string ? 
Maybe make sense to handle that during submission ?

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode42
options.js:42: var bValue = (b.title || b.text || a.url).toLowerCase();
On 2015/06/30 09:23:27, Thomas Greiner wrote:
> Be careful: It's `b.url`, not `a.url`

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode279
options.js:279: // TODO: add `filters[i].text` to list of custom filters
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> Remove this comment

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode281
options.js:281: filtersMap[filter.text] = filter;
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> This line is shared across both branches of the if-statement so I'd suggest
> moving it to the end of the function to avoid duplication.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode363
options.js:363: return false;
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> Detail: You could reduce that to one line:
> 
> return (e.key && e.key == "Enter") || (!e.key && e.keyCode == 13);

Done. Please note that this function most probably will be removed with the
followup consistency ticket.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode415
options.js:415: E("custom-filters-add-textbox").setAttribute("placeholder",
ext.i18n.getMessage("options_customFilters_textbox_placeholder"));
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> Split this up into two lines to avoid exceeding the 80 character limit.
> Preferably consistent with how you do it a few lines above.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode416
options.js:416: function addCustomFilter()
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> "addCustomFilters" please

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode443
options.js:443: var customFilterEditButtons =
document.querySelectorAll("#custom-filters-edit-wrapper button");
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> Please avoid setting event listeners individually on each button if you have
to
> distinguish them within the listener anyway. Instead only listen on the
> `#custom-filters-edit-wrapper` element.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode448
options.js:448: E("custom-filters").dataset.view = e.currentTarget.dataset.show;
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> This can be achieved more performantly by toggling a CSS class on that element
> (i.e. `E("custom-filters").classList.toggle("mode-edit");`).

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode545
options.js:545: E("custom-filters-textarea").value = text;
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> This loop is potentially creating a lot of strings. Therefore I'd replace the
> function body with:
> 
> var customFilterItems = collections.customFilters.items;
> var filterTexts = [];
> for (var i = 0; i < customFilterItems.length; i++)
>   filterTexts.push(customFilterItems[i].text);
> E("custom-filters-textarea").value = filterTexts.join("\n");

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode604
options.js:604: function addFilter(filter)
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> There's no need to create a function for that because you're only using it
once.

Done.
Please note that we still have similar methods, that were created initially, I
think they are leftover from previous review:
removeSubscription
removeFilter
onFilterMessage
showAddSubscriptionDialog

Not sure if handling theme here is okey, or make sense to do it separately.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode627
options.js:627: collections.customFilters.removeItem(knownFilter);
On 2015/06/30 09:23:28, Thomas Greiner wrote:
> Note that when I click on "edit view" and right afterwards on "save" without
> making any changes, then the filters for some of the whitelisted domains are
> being removed while some of the filters I wanted to remove remain in the list.
> 
> This must not happen so please test everything carefully.

The problem is that in "filters.importRaw" we were excluding 
"Whitelist" filters before removing custom filters that are not in the raw-text,
but while we are only returning an instance of Filter with the fromText in mock,
we can't differentiate, Updated the mock. 
Thanks for noticing, I was testing in product env. beforehand.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:61: box-sizing: border-box;
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> What about applying `box-sizing:border-box` to every element on the page to
> ensure consistency? Because I guess that's quite a common problem.

I think it make sense to create a separate ticket for that. It will need some
additional fixes on layout.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:548: width: 355px;
On 2015/06/30 09:23:30, Thomas Greiner wrote:
> Could you please try making this more dynamic instead of hard-coding the pixel
> value?

Done, but I do not understand why it's overlapping to the icon when mangin-start
is 14px. Do you have idea why it's overlapping ?

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:595: -webkit-margin-start: 20px;
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> What about `padding: 0px 20px;` instead? We want to have the same distance on
> either side anyway.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:607: #custom-filters[data-view="list"]
#custom-filters-list-wrapper
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> You could combine this, the block above and the block below into this cleaner
> construct:
> 
> #custom-filters-textarea-controls
> {
>   display: flex;
> }
> 
> #custom-filters:not(.mode-edit) #custom-filters-textarea-controls,
> #custom-filters:not(.mode-edit) #custom-filters-textarea,
> #custom-filters.mode-edit #custom-filters-edit-btn,
> #custom-filters.mode-edit #custom-filters-list-wrapper
> {
>   display: none;
> }

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:620: -webkit-margin-start: 16px;
On 2015/06/30 09:23:30, Thomas Greiner wrote:
> Again, what about `padding: 0px 16px;` instead?

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:645: height: 328px;
On 2015/06/30 09:23:30, Thomas Greiner wrote:
> I assume you'll make this pixel value more dynamic with the tabs
implementation
> in #2375?

Couldn't find better way to achieve this, currently it's getting the height of
the wrapper, but please note that the list view table's height is hardcoded.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:649: #custom-filters input[type="text"]:focus
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> Please split up those two styles. I assume `:focus` only needs `outline:0px`.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:668: font-size: 13px;
On 2015/06/30 09:23:30, Thomas Greiner wrote:
> Missing background color which is specified in design mockup.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:673: font-weight: bold;
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> Actually, we don't have fonts for bold (= `font-weight:700`), only for
semi-bold
> (= `font-weight:600`) so I'd generally suggest to exclusively use integers to
> specify font weights.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:685: #custom-filters-add-btn
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> The add-button doesn't occupy the entire block it's contained in. Therefore if
> you click slightly above the text, the action is not registered.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:688: position: absolute;
On 2015/06/30 09:23:29, Thomas Greiner wrote:
> Doesn't this also work without `position:absolute` and `bottom:11px`?  Because
> it seems like those don't do anything.

Exactly and they are fixing comment above :)

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/options.json File locale/en-US/options.json (right): https://codereview.adblockplus.org/29321198/diff/29321199/locale/en-US/options.json#newcode131 locale/en-US/options.json:131: "description": "Option name in Advanced tab", On 2015/07/08 18:25:39, ...

July 9, 2015, 11:07 a.m. (2015-07-09 11:07:56 UTC) #5

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js#newcode233 messageResponder.js:233: if (filter instanceof WhitelistFilter && Nit: Isn't |filter instanceof ...

July 9, 2015, 12:12 p.m. (2015-07-09 12:12:15 UTC) #6

saroyanm

Please note guys, as discussed we will need to also implement error listener. I didn't ...

July 9, 2015, 4:31 p.m. (2015-07-09 16:31:43 UTC) #7

Please note guys, as discussed we will need to also implement error listener. 
I didn't made it into this review, so make sense to handle that separately.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode39
options.js:39: this.items.sort(function(a, b)
On 2015/07/09 11:07:54, Thomas Greiner wrote:
> On 2015/07/08 18:25:42, saroyanm wrote:
> > On 2015/06/30 09:23:29, Thomas Greiner wrote:
> > > You need to first filter out all empty lines before sorting them or
> otherwise
> > > it'll throw an exception if you try to convert a filter to lower case.
> > 
> > I do wander why we even need to submit the empty string ? 
> > Maybe make sense to handle that during submission ?
> 
> Yep, the earlier we can filter those out the better.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/options.js#newcode604
options.js:604: function addFilter(filter)
On 2015/07/09 11:07:54, Thomas Greiner wrote:
> On 2015/07/08 18:25:41, saroyanm wrote:
> > On 2015/06/30 09:23:29, Thomas Greiner wrote:
> > > There's no need to create a function for that because you're only using it
> > once.
> > 
> > Done.
> > Please note that we still have similar methods, that were created initially,
I
> > think they are leftover from previous review:
> > removeSubscription
> > removeFilter
> > onFilterMessage
> > showAddSubscriptionDialog
> > 
> > Not sure if handling theme here is okey, or make sense to do it separately.
> > 
> 
> Good point, feel free to include that in this review. I think for
> "onFilterMessage", however, it might be good to have it separate to avoid
> nesting switch-statements.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:548: width: 355px;
On 2015/07/09 11:07:55, Thomas Greiner wrote:
> On 2015/07/08 18:25:42, saroyanm wrote:
> > On 2015/06/30 09:23:30, Thomas Greiner wrote:
> > > Could you please try making this more dynamic instead of hard-coding the
> pixel
> > > value?
> > 
> > Done, but I do not understand why it's overlapping to the icon when
> mangin-start
> > is 14px. Do you have idea why it's overlapping ?
> 
> It seems that #whitelisting-add-icon's width is lower than 18px for some
reason
> so you need to specify min-width on it.

Done.

https://codereview.adblockplus.org/29321198/diff/29321199/skin/options.css#ne...
skin/options.css:685: #custom-filters-add-btn
On 2015/07/09 11:07:54, Thomas Greiner wrote:
> On 2015/07/08 18:25:42, saroyanm wrote:
> > On 2015/06/30 09:23:29, Thomas Greiner wrote:
> > > The add-button doesn't occupy the entire block it's contained in.
Therefore
> if
> > > you click slightly above the text, the action is not registered.
> > 
> > Done.
> 
> It still doesn't register it when you click on the area to the right side of
the
> button. Adding `-x-padding-end` should be sufficient for that.

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:233: if (filter instanceof WhitelistFilter &&
On 2015/07/09 12:12:14, Sebastian Noack wrote:
> Nit: Isn't |filter instanceof WhilelistFilter| redundant, as the regexp
already
> excludes other kind of filters, by requiring the filter text to start with
"@@"?

Yes this redundant, but it's the way it was implemented in old version of option
page, I thought maybe there is a reason of doing so, similar as you can see my
answer below.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:242: FilterStorage.removeFilter(remove[i]);
On 2015/07/09 12:12:14, Sebastian Noack wrote:
> Any reason to collect the filters to be removed in an array, removing them in
a
> seperate loop, instead simply calling FilterStorage.removeFilter() above?

In that case we will have object relation issue, because we will modify array
each time we are removing item, we faced this issue in previous. Thomas just
found the issue with the solution. Currently I'm slicing the array.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:250: case "filter.parse":
On 2015/07/09 11:07:55, Thomas Greiner wrote:
> Plural for consistency: "filters.parse"

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:252: if (result.filter)
On 2015/07/09 12:12:14, Sebastian Noack wrote:
> Note that if the normalized filter text is empty result.filter will be null,
but
> there won't be an error. So the logic here should probably look like that:
> 
> if (result.error)
>   callback({error: convertFilterParsingError(result.error)});
> else
>   callback({filter: result.filter && convertFilter(result.filter)});

We changed the implementation here, we will need to implement error.listener and
for that I'm not sending the error msd through callback method.
I'll suggest to implement error handling in separate ticket.

https://codereview.adblockplus.org/29321198/diff/29321453/options.html
File options.html (right):

https://codereview.adblockplus.org/29321198/diff/29321453/options.html#newcod...
options.html:202: <div id="custom-filters" data-view="list">
On 2015/07/09 11:07:55, Thomas Greiner wrote:
> The "data-view" attribute is no longer used.

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.html#newcod...
options.html:211: <form id="custom-filter-add" class="controls">
On 2015/07/09 11:07:56, Thomas Greiner wrote:
> Plural for consistency: "custom-filters-add"

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.html#newcod...
options.html:219: <button id="custom-filters-edit-btn" data-show="edit">
On 2015/07/09 11:07:55, Thomas Greiner wrote:
> The "data-show" attribute is no longer used here and below.

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.html#newcod...
options.html:219: <button id="custom-filters-edit-btn" data-show="edit">
On 2015/07/09 11:07:55, Thomas Greiner wrote:
> Again, please avoid referring to specific UI elements from class names. Same
> applies for "custom-filters-list-btn" and "custom-filters-save-btn" below.

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.js
File options.js (left):

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#oldcode407
options.js:407: delete document.body.dataset.dialog;
On 2015/07/09 12:12:15, Sebastian Noack wrote:
> I wasn't involved in the initial review, but just realized that you use
dataset,
> which isn't supported in older Safari versions. So we should get rid of it,
> probably with a followup patch.

According to MDN dataset is supported since Safari 6, should be sufficient I
think -> https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset

https://codereview.adblockplus.org/29321198/diff/29321453/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#newcode357
options.js:357: return (e.key && e.key == "Enter") || (!e.key && e.keyCode ==
13);
On 2015/07/09 12:12:15, Sebastian Noack wrote:
> Please don't duplicate the check for presence of e.key:
> 
> if ("key" in e)
>   return e.key == "Enter";
> return e.keyCode == 13;

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#newcode459
options.js:459: if (e.target.tagName == "BUTTON")
On 2015/07/09 11:07:56, Thomas Greiner wrote:
> Use `e.target.localName == "button"` instead because its output is more
> consistent.

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#newcode464
options.js:464: return;
On 2015/07/09 11:07:56, Thomas Greiner wrote:
> The most flexible way to achieve this is to go up the hierarchy until you find
a
> matching element.
> 
> Note that I implemented this in the review for #2357 (see
>
https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode319)
> so it would be great if we could merge those together into a single listener
> later on.

Should we keep it as it is now and add merge it in 2357 ?

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#newcode467
options.js:467: E("custom-filters").classList.toggle("mode-edit");
On 2015/07/09 12:12:15, Sebastian Noack wrote:
> classList.toggle() isn't supported in older Safari versions.

According to MDN it's supported from 5.1, but webkit has a bug which is
currently fixed:
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Specification

Can you please confirm that toggle is not supported in Safari 6 ?

https://codereview.adblockplus.org/29321198/diff/29321453/skin/options.css
File skin/options.css (right):

https://codereview.adblockplus.org/29321198/diff/29321453/skin/options.css#ne...
skin/options.css:436: font-weight: bold;
On 2015/07/09 11:07:56, Thomas Greiner wrote:
> You still specify "bold" here so make sure to consistently use number values
> across the entire stylesheet.

I didn't changed initially because it wasn't part of the advanced tab and though
it was changed in sidebar review, but yes, changed anyway so will take into
consideration during merging.

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js#newcode211 messageResponder.js:211: alert(errors.join("\n")); On 2015/07/09 11:07:55, Thomas Greiner wrote: > This ...

July 10, 2015, 7:31 a.m. (2015-07-10 07:31:01 UTC) #8

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:211: alert(errors.join("\n"));
On 2015/07/09 11:07:55, Thomas Greiner wrote:
> This won't work because this code is running in the background page. Just like
> when adding individual filters we need two separate messages: one for parsing
> the filters and one for adding them.

What could go wrong when adding filters? Currently, only parsing errors are
handled. But yeah, alert() from within the background page is inappropriate as
explained in my other comment.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:233: if (filter instanceof WhitelistFilter &&
On 2015/07/09 16:31:41, saroyanm wrote:
> On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > Nit: Isn't |filter instanceof WhilelistFilter| redundant, as the regexp
> already
> > excludes other kind of filters, by requiring the filter text to start with
> "@@"?
> 
> Yes this redundant, but it's the way it was implemented in old version of
option
> page, I thought maybe there is a reason of doing so, similar as you can see my
> answer below.

Just because the previous code is doing something, isn't reason enough to keep
doing it like that if there is a better option. And I cannot think of any reason
why the redundant instanceof check here is necessary.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:242: FilterStorage.removeFilter(remove[i]);
On 2015/07/09 16:31:40, saroyanm wrote:
> On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > Any reason to collect the filters to be removed in an array, removing them
in
> a
> > seperate loop, instead simply calling FilterStorage.removeFilter() above?
> 
> In that case we will have object relation issue, because we will modify array
> each time we are removing item, we faced this issue in previous. Thomas just
> found the issue with the solution. Currently I'm slicing the array.

What one would usually do in this case, is simply iterating over the array in
reverse order, starting at the last item towards the first:

  for (var i = FilterStorage.subscriptions.length - 1; i >= 0; i--)
  {
    var subscription = FilterStorage.subscriptions[i];

    for (var j = subscription.filters.length - 1; j >= 0; j--)
    {
      var filter = subscription.filters[j];

      ...

      FilterStorage.removeFilter(filter);
    }
  }

That way, if an item gets removed, it doesn't change the index of the previous
items to be processed next. This will be simpler and faster than creating a
separate array.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:252: if (result.filter)
On 2015/07/09 16:31:40, saroyanm wrote:
> On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > Note that if the normalized filter text is empty result.filter will be null,
> but
> > there won't be an error. So the logic here should probably look like that:
> > 
> > if (result.error)
> >   callback({error: convertFilterParsingError(result.error)});
> > else
> >   callback({filter: result.filter && convertFilter(result.filter)});
> 
> We changed the implementation here, we will need to implement error.listener
and
> for that I'm not sending the error msd through callback method.
> I'll suggest to implement error handling in separate ticket.

This sounds rather complicated and backwards to me. Simply returning the
corresponding error with the API that it occurred with, sounds more appropriate
and simpler to implement to me.

https://codereview.adblockplus.org/29321198/diff/29321453/options.js
File options.js (left):

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#oldcode407
options.js:407: delete document.body.dataset.dialog;
On 2015/07/09 16:31:41, saroyanm wrote:
> On 2015/07/09 12:12:15, Sebastian Noack wrote:
> > I wasn't involved in the initial review, but just realized that you use
> dataset,
> > which isn't supported in older Safari versions. So we should get rid of it,
> > probably with a followup patch.
> 
> According to MDN dataset is supported since Safari 6, should be sufficient I
> think -> https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset

See
https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333

https://codereview.adblockplus.org/29321198/diff/29321453/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#newcode467
options.js:467: E("custom-filters").classList.toggle("mode-edit");
On 2015/07/09 16:31:42, saroyanm wrote:
> On 2015/07/09 12:12:15, Sebastian Noack wrote:
> > classList.toggle() isn't supported in older Safari versions.
> 
> According to MDN it's supported from 5.1, but webkit has a bug which is
> currently fixed:
>
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Specification
> 
> Can you please confirm that toggle is not supported in Safari 6 ?

It might be that I confused this with the second parameter of toggle() not being
supported before Safari 7 (http://caniuse.com/#feat=classlist). So the way we
use toggle() here should be fine.

https://codereview.adblockplus.org/29321198/diff/29321545/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321545/messageResponder.js...
messageResponder.js:250: alert(result.error);
Please no alert() or similar calls in the background page. These will block the
whole extension until user action occurs.

Not to mention, that I thought that the new option page considers validation
errors in a nicer way than using alert().

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js#newcode211 messageResponder.js:211: alert(errors.join("\n")); On 2015/07/10 07:31:00, Sebastian Noack wrote: > On ...

July 10, 2015, 12:38 p.m. (2015-07-10 12:38:54 UTC) #9

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:211: alert(errors.join("\n"));
On 2015/07/10 07:31:00, Sebastian Noack wrote:
> On 2015/07/09 11:07:55, Thomas Greiner wrote:
> > This won't work because this code is running in the background page. Just
like
> > when adding individual filters we need two separate messages: one for
parsing
> > the filters and one for adding them.
> 
> What could go wrong when adding filters? Currently, only parsing errors are
> handled. But yeah, alert() from within the background page is inappropriate as
> explained in my other comment.

We're talking about the same issue: the placement of the alert().

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:242: FilterStorage.removeFilter(remove[i]);
On 2015/07/10 07:31:00, Sebastian Noack wrote:
> On 2015/07/09 16:31:40, saroyanm wrote:
> > On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > > Any reason to collect the filters to be removed in an array, removing them
> in
> > a
> > > seperate loop, instead simply calling FilterStorage.removeFilter() above?
> > 
> > In that case we will have object relation issue, because we will modify
array
> > each time we are removing item, we faced this issue in previous. Thomas just
> > found the issue with the solution. Currently I'm slicing the array.
> 
> What one would usually do in this case, is simply iterating over the array in
> reverse order, starting at the last item towards the first:
> 
>   for (var i = FilterStorage.subscriptions.length - 1; i >= 0; i--)
>   {
>     var subscription = FilterStorage.subscriptions[i];
> 
>     for (var j = subscription.filters.length - 1; j >= 0; j--)
>     {
>       var filter = subscription.filters[j];
> 
>       ...
> 
>       FilterStorage.removeFilter(filter);
>     }
>   }
> 
> That way, if an item gets removed, it doesn't change the index of the previous
> items to be processed next. This will be simpler and faster than creating a
> separate array.

However, it's just as confusing as the current method. Therefore I'd still
recommend creating a copy of the array because, if possible, you shouldn't
remove elements from the array you're iterating over.

Performance optimizations at this point are probably premature optimization
unless it turns out that the lag is noticeably bigger.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:252: if (result.filter)
On 2015/07/10 07:31:00, Sebastian Noack wrote:
> On 2015/07/09 16:31:40, saroyanm wrote:
> > On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > > Note that if the normalized filter text is empty result.filter will be
null,
> > but
> > > there won't be an error. So the logic here should probably look like that:
> > > 
> > > if (result.error)
> > >   callback({error: convertFilterParsingError(result.error)});
> > > else
> > >   callback({filter: result.filter && convertFilter(result.filter)});
> > 
> > We changed the implementation here, we will need to implement error.listener
> and
> > for that I'm not sending the error msd through callback method.
> > I'll suggest to implement error handling in separate ticket.
> 
> This sounds rather complicated and backwards to me. Simply returning the
> corresponding error with the API that it occurred with, sounds more
appropriate
> and simpler to implement to me.

It would be more inline with how we return filters that were added using
"filters.add" and note that we'll need the ability to listen to errors later on
anyway.

https://codereview.adblockplus.org/29321198/diff/29321453/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#newcode464
options.js:464: return;
On 2015/07/09 16:31:42, saroyanm wrote:
> On 2015/07/09 11:07:56, Thomas Greiner wrote:
> > The most flexible way to achieve this is to go up the hierarchy until you
find
> a
> > matching element.
> > 
> > Note that I implemented this in the review for #2357 (see
> >
>
https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode319)
> > so it would be great if we could merge those together into a single listener
> > later on.
> 
> Should we keep it as it is now and add merge it in 2357 ?

Yes, either in #2357 or a later review.

https://codereview.adblockplus.org/29321198/diff/29321545/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321545/messageResponder.js...
messageResponder.js:250: alert(result.error);
On 2015/07/10 07:31:01, Sebastian Noack wrote:
> Not to mention, that I thought that the new option page considers validation
> errors in a nicer way than using alert(). 

Unfortunately, this didn't make it into the initial version which is only about
having feature-parity with the current one.

saroyanm

Guys sorry for late response, can you please have a look, Hope with Error handling ...

July 13, 2015, 2:05 p.m. (2015-07-13 14:05:35 UTC) #10

Guys sorry for late response, 
can you please have a look,
Hope with Error handling it won't be so confusing anymore.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:211: alert(errors.join("\n"));
On 2015/07/10 12:38:52, Thomas Greiner wrote:
> On 2015/07/10 07:31:00, Sebastian Noack wrote:
> > On 2015/07/09 11:07:55, Thomas Greiner wrote:
> > > This won't work because this code is running in the background page. Just
> like
> > > when adding individual filters we need two separate messages: one for
> parsing
> > > the filters and one for adding them.
> > 
> > What could go wrong when adding filters? Currently, only parsing errors are
> > handled. But yeah, alert() from within the background page is inappropriate
as
> > explained in my other comment.
> 
> We're talking about the same issue: the placement of the alert().

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:233: if (filter instanceof WhitelistFilter &&
On 2015/07/10 07:31:00, Sebastian Noack wrote:
> On 2015/07/09 16:31:41, saroyanm wrote:
> > On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > > Nit: Isn't |filter instanceof WhilelistFilter| redundant, as the regexp
> > already
> > > excludes other kind of filters, by requiring the filter text to start with
> > "@@"?
> > 
> > Yes this redundant, but it's the way it was implemented in old version of
> option
> > page, I thought maybe there is a reason of doing so, similar as you can see
my
> > answer below.
> 
> Just because the previous code is doing something, isn't reason enough to keep
> doing it like that if there is a better option. And I cannot think of any
reason
> why the redundant instanceof check here is necessary.

Done.

https://codereview.adblockplus.org/29321198/diff/29321453/options.js
File options.js (left):

https://codereview.adblockplus.org/29321198/diff/29321453/options.js#oldcode407
options.js:407: delete document.body.dataset.dialog;
On 2015/07/10 07:31:00, Sebastian Noack wrote:
> On 2015/07/09 16:31:41, saroyanm wrote:
> > On 2015/07/09 12:12:15, Sebastian Noack wrote:
> > > I wasn't involved in the initial review, but just realized that you use
> > dataset,
> > > which isn't supported in older Safari versions. So we should get rid of
it,
> > > probably with a followup patch.
> > 
> > According to MDN dataset is supported since Safari 6, should be sufficient I
> > think ->
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dataset
> 
> See
>
https://codereview.adblockplus.org/29321417/diff/29321418/options.js#newcode333

Yes Its should be fixed with the #2357

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322158/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29322158/messageResponder.js#newcode115 messageResponder.js:115: var action = "error"; @Thomas for Error handling, is ...

July 13, 2015, 2:09 p.m. (2015-07-13 14:09:53 UTC) #11

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322158/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29322158/messageResponder.js#newcode115 messageResponder.js:115: var action = "error"; On 2015/07/13 14:09:52, saroyanm wrote: ...

July 13, 2015, 3:54 p.m. (2015-07-13 15:54:24 UTC) #12

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322165/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322165/background.js#newcode173 background.js:173: } On 2015/07/13 15:54:21, Thomas Greiner wrote: > You're ...

July 14, 2015, 10:21 a.m. (2015-07-14 10:21:00 UTC) #13

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322165/options.js File options.js (right): https://codereview.adblockplus.org/29321198/diff/29322165/options.js#newcode727 options.js:727: filter: ["addSubscription"] On 2015/07/14 10:20:59, saroyanm wrote: > On ...

July 14, 2015, 10:58 a.m. (2015-07-14 10:58:55 UTC) #14

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322165/options.js File options.js (right): https://codereview.adblockplus.org/29321198/diff/29322165/options.js#newcode727 options.js:727: filter: ["addSubscription"] On 2015/07/14 10:58:52, Thomas Greiner wrote: > ...

July 14, 2015, 11:19 a.m. (2015-07-14 11:19:28 UTC) #15

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322193/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29322193/messageResponder.js#newcode215 messageResponder.js:215: sendMessage("app", "error", [errors.join("\n")], sender.page); On 2015/07/14 11:19:26, saroyanm wrote: ...

July 14, 2015, 11:40 a.m. (2015-07-14 11:40:09 UTC) #16

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js#newcode242 messageResponder.js:242: FilterStorage.removeFilter(remove[i]); On 2015/07/10 12:38:53, Thomas Greiner wrote: > On ...

July 14, 2015, 11:57 a.m. (2015-07-14 11:57:25 UTC) #17

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:242: FilterStorage.removeFilter(remove[i]);
On 2015/07/10 12:38:53, Thomas Greiner wrote:
> On 2015/07/10 07:31:00, Sebastian Noack wrote:
> > On 2015/07/09 16:31:40, saroyanm wrote:
> > > On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > > > Any reason to collect the filters to be removed in an array, removing
them
> > in
> > > a
> > > > seperate loop, instead simply calling FilterStorage.removeFilter()
above?
> > > 
> > > In that case we will have object relation issue, because we will modify
> array
> > > each time we are removing item, we faced this issue in previous. Thomas
just
> > > found the issue with the solution. Currently I'm slicing the array.
> > 
> > What one would usually do in this case, is simply iterating over the array
in
> > reverse order, starting at the last item towards the first:
> > 
> >   for (var i = FilterStorage.subscriptions.length - 1; i >= 0; i--)
> >   {
> >     var subscription = FilterStorage.subscriptions[i];
> > 
> >     for (var j = subscription.filters.length - 1; j >= 0; j--)
> >     {
> >       var filter = subscription.filters[j];
> > 
> >       ...
> > 
> >       FilterStorage.removeFilter(filter);
> >     }
> >   }
> > 
> > That way, if an item gets removed, it doesn't change the index of the
previous
> > items to be processed next. This will be simpler and faster than creating a
> > separate array.
> 
> However, it's just as confusing as the current method. Therefore I'd still
> recommend creating a copy of the array because, if possible, you shouldn't
> remove elements from the array you're iterating over.

If you keep the performance aspect aside, I suppose it's just personal
preference. I'd certainly prefer to iterate over an array from behind rather
than copying it. Another option would be to simply decrease the counter when you
removed an item.

> Performance optimizations at this point are probably premature optimization
> unless it turns out that the lag is noticeably bigger.

Ever tried to save several thousand of filters in raw mode?

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:252: if (result.filter)
On 2015/07/10 12:38:53, Thomas Greiner wrote:
> On 2015/07/10 07:31:00, Sebastian Noack wrote:
> > On 2015/07/09 16:31:40, saroyanm wrote:
> > > On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > > > Note that if the normalized filter text is empty result.filter will be
> null,
> > > but
> > > > there won't be an error. So the logic here should probably look like
that:
> > > > 
> > > > if (result.error)
> > > >   callback({error: convertFilterParsingError(result.error)});
> > > > else
> > > >   callback({filter: result.filter && convertFilter(result.filter)});
> > > 
> > > We changed the implementation here, we will need to implement
error.listener
> > and
> > > for that I'm not sending the error msd through callback method.
> > > I'll suggest to implement error handling in separate ticket.
> > 
> > This sounds rather complicated and backwards to me. Simply returning the
> > corresponding error with the API that it occurred with, sounds more
> appropriate
> > and simpler to implement to me.
> 
> It would be more inline with how we return filters that were added using
> "filters.add" and note that we'll need the ability to listen to errors later
on
> anyway.

Yes, we need to listen for subscription download errors as well. But these are
unrelated and shown in a different way. So I don't see much a point in reporting
all kind of errors with the same message. I think it makes much more sense and
would be simpler to return validation errors, in the response to the (only)
message that can cause them.

https://codereview.adblockplus.org/29321198/diff/29322207/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322207/messageResponder.js...
messageResponder.js:249: case "filters.parse":
IMO this should be renamed to "filters.add", replacing that message type. We
should never add any filters that haven't been validated.

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js#newcode242 messageResponder.js:242: FilterStorage.removeFilter(remove[i]); On 2015/07/14 11:57:24, Sebastian Noack wrote: > > ...

July 14, 2015, 12:47 p.m. (2015-07-14 12:47:17 UTC) #18

saroyanm

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js#newcode242 messageResponder.js:242: FilterStorage.removeFilter(remove[i]); On 2015/07/14 12:47:16, Thomas Greiner wrote: > On ...

July 14, 2015, 1:15 p.m. (2015-07-14 13:15:48 UTC) #19

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:242: FilterStorage.removeFilter(remove[i]);
On 2015/07/14 12:47:16, Thomas Greiner wrote:
> On 2015/07/14 11:57:24, Sebastian Noack wrote:
> > > Performance optimizations at this point are probably premature
optimization
> > > unless it turns out that the lag is noticeably bigger.
> > 
> > Ever tried to save several thousand of filters in raw mode?
> 
> Remembering what you said that some people copy entire filter lists into this
> field, it may indeed be necessary to optimize for performance here, agreed.

Agree as well, done.

https://codereview.adblockplus.org/29321198/diff/29321453/messageResponder.js...
messageResponder.js:252: if (result.filter)
On 2015/07/14 11:57:24, Sebastian Noack wrote:
> On 2015/07/10 12:38:53, Thomas Greiner wrote:
> > On 2015/07/10 07:31:00, Sebastian Noack wrote:
> > > On 2015/07/09 16:31:40, saroyanm wrote:
> > > > On 2015/07/09 12:12:14, Sebastian Noack wrote:
> > > > > Note that if the normalized filter text is empty result.filter will be
> > null,
> > > > but
> > > > > there won't be an error. So the logic here should probably look like
> that:
> > > > > 
> > > > > if (result.error)
> > > > >   callback({error: convertFilterParsingError(result.error)});
> > > > > else
> > > > >   callback({filter: result.filter && convertFilter(result.filter)});
> > > > 
> > > > We changed the implementation here, we will need to implement
> error.listener
> > > and
> > > > for that I'm not sending the error msd through callback method.
> > > > I'll suggest to implement error handling in separate ticket.
> > > 
> > > This sounds rather complicated and backwards to me. Simply returning the
> > > corresponding error with the API that it occurred with, sounds more
> > appropriate
> > > and simpler to implement to me.
> > 
> > It would be more inline with how we return filters that were added using
> > "filters.add" and note that we'll need the ability to listen to errors later
> on
> > anyway.
> 
> Yes, we need to listen for subscription download errors as well. But these are
> unrelated and shown in a different way. So I don't see much a point in
reporting
> all kind of errors with the same message. I think it makes much more sense and
> would be simpler to return validation errors, in the response to the (only)
> message that can cause them.

Currently we have implemented global error reporting, in case I'll change it to
the callback I'll need to remove current implementation, because we won't use it
in current code, but we are going to use later even if we will implement
callback error handling, so I'll suggest to have this error handling for now and
implement the callback error handling for specific element afterward when we
will have request for that.

https://codereview.adblockplus.org/29321198/diff/29322193/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322193/messageResponder.js...
messageResponder.js:215: sendMessage("app", "error", [errors.join("\n")],
sender.page);
On 2015/07/14 11:40:08, Thomas Greiner wrote:
> On 2015/07/14 11:19:26, saroyanm wrote:
> > On 2015/07/14 10:58:53, Thomas Greiner wrote:
> > > Just a suggestion: Since you're passing an array anyway, what about
sending
> > the
> > > error messages separately and let options.js deal with how to display
them?
> > 
> > Good point. Done.
> 
> This won't work because this would send the FilterParsingError objects so you
> have to convert each error into a string first.

Done.

saroyanm

Forgot msg: Patch uploaded (Patch Set 9) https://codereview.adblockplus.org/29321198/diff/29322207/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29322207/messageResponder.js#newcode249 messageResponder.js:249: case "filters.parse": ...

July 14, 2015, 1:23 p.m. (2015-07-14 13:23:00 UTC) #20

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = I wonder whezther this implementation might go ...

July 14, 2015, 1:36 p.m. (2015-07-14 13:36:32 UTC) #21

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = On 2015/07/14 13:36:31, Sebastian Noack wrote: > ...

July 14, 2015, 1:53 p.m. (2015-07-14 13:53:30 UTC) #22

https://codereview.adblockplus.org/29321198/diff/29322235/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newco...
background.js:156: modules.filterValidation =
On 2015/07/14 13:36:31, Sebastian Noack wrote:
> I wonder whezther this implementation might go a little to far for a mock. How
> about simply making it noop?
> 
> modules.filterValidation = {
>   parseFilter: modules.filterClasses.Filter.fromText,
>   parseFilters: function(text)
>   {
>     return text.split("\n").map(modules.filterClasses.Filter.fromText);
>   }
> };

parseFilter: In this case we wouldn't be able to return FilterParsingError,
which helps testing locally for errors. 
parseFilters: this method should return {filters: filters, errors: errors},
again helps testing locally, at least returning similar format, that we have for
production code.

https://codereview.adblockplus.org/29321198/diff/29322235/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/messageResponder.js...
messageResponder.js:33: var parseFilters =
require("filterValidation").parseFilters;
On 2015/07/14 13:36:31, Sebastian Noack wrote:
> You might want to cache the module into a variable, similar too filterClasses:
> 
>   var filterValidation = require("filterValidation");
> 
> And then, either below:
> 
>   var parseFilters = filterValidation.parseFilters;
>   var parseFilter = filterValidation.parseFilter;
> 
> Or access the functions via the module where needed:
> 
>   var result = filterValidation.parseFilters(..);

Good point, done.

https://codereview.adblockplus.org/29321198/diff/29322235/messageResponder.js...
messageResponder.js:210: var errors = result.errors.filter(function(error)
On 2015/07/14 13:36:31, Sebastian Noack wrote:
> I'd rather go with for-loop, rather than chaining .filter() and .map(). I find
> this simpler to read, beside being faster. But it's not too important, if you
> disagree.
> 
>   var errors = [];
>   for (var i = 0; i < result.errors.length; i++)
>   {
>     var error = result.errors[i];
>     if (error.type != "unexpected-filter-list-header")
>       errors.push(error);
>   }

Good point, done.

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = On 2015/07/14 13:53:28, saroyanm wrote: > On ...

July 14, 2015, 2:10 p.m. (2015-07-14 14:10:10 UTC) #23

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = On 2015/07/14 14:10:09, Sebastian Noack wrote: > ...

July 14, 2015, 2:20 p.m. (2015-07-14 14:20:29 UTC) #24

https://codereview.adblockplus.org/29321198/diff/29322235/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newco...
background.js:156: modules.filterValidation =
On 2015/07/14 14:10:09, Sebastian Noack wrote:
> On 2015/07/14 13:53:28, saroyanm wrote:
> > On 2015/07/14 13:36:31, Sebastian Noack wrote:
> > > I wonder whezther this implementation might go a little to far for a mock.
> How
> > > about simply making it noop?
> > > 
> > > modules.filterValidation = {
> > >   parseFilter: modules.filterClasses.Filter.fromText,
> > >   parseFilters: function(text)
> > >   {
> > >     return text.split("\n").map(modules.filterClasses.Filter.fromText);
> > >   }
> > > };
> > 
> > parseFilter: In this case we wouldn't be able to return FilterParsingError,
> > which helps testing locally for errors. 
> > parseFilters: this method should return {filters: filters, errors: errors},
> > again helps testing locally, at least returning similar format, that we have
> for
> > production code.
> 
> Sorry, I meant:
> 
>   modules.filterValidation = {
>     parseFilter: function(text)
>     {
>       return {filter: modules.filterClasses.Filter.fromText(text)};
>     },
>     parseFilters: function(text)
>     {
>       return {filters:
> text.split("\n").map(modules.filterClasses.Filter.fromText)};
>     }
>   };
> 
> Sure testing validation errors with the UI only wouldn't be possible then. But
I
> was wondering whether testing these (outside of the extension) is worth the
> added complexity and duplicated logic. Thomas?

That's true, for testing purposes we don't need to duplicate any parts of the
validation in the UI. It's still valuable to be able to test error handling in
the test environment so for that we could define some way that always throws a
generic filter validation error in parseFilter(s).

e.g. A URL parameter that causes the parseFilter(s) function to return an error
OR specify a character that - when included in a filter text - causes an error.

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = On 2015/07/14 14:20:29, Thomas Greiner wrote: > ...

July 14, 2015, 2:25 p.m. (2015-07-14 14:25:28 UTC) #25

https://codereview.adblockplus.org/29321198/diff/29322235/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newco...
background.js:156: modules.filterValidation =
On 2015/07/14 14:20:29, Thomas Greiner wrote:
> On 2015/07/14 14:10:09, Sebastian Noack wrote:
> > On 2015/07/14 13:53:28, saroyanm wrote:
> > > On 2015/07/14 13:36:31, Sebastian Noack wrote:
> > > > I wonder whezther this implementation might go a little to far for a
mock.
> > How
> > > > about simply making it noop?
> > > > 
> > > > modules.filterValidation = {
> > > >   parseFilter: modules.filterClasses.Filter.fromText,
> > > >   parseFilters: function(text)
> > > >   {
> > > >     return text.split("\n").map(modules.filterClasses.Filter.fromText);
> > > >   }
> > > > };
> > > 
> > > parseFilter: In this case we wouldn't be able to return
FilterParsingError,
> > > which helps testing locally for errors. 
> > > parseFilters: this method should return {filters: filters, errors:
errors},
> > > again helps testing locally, at least returning similar format, that we
have
> > for
> > > production code.
> > 
> > Sorry, I meant:
> > 
> >   modules.filterValidation = {
> >     parseFilter: function(text)
> >     {
> >       return {filter: modules.filterClasses.Filter.fromText(text)};
> >     },
> >     parseFilters: function(text)
> >     {
> >       return {filters:
> > text.split("\n").map(modules.filterClasses.Filter.fromText)};
> >     }
> >   };
> > 
> > Sure testing validation errors with the UI only wouldn't be possible then.
But
> I
> > was wondering whether testing these (outside of the extension) is worth the
> > added complexity and duplicated logic. Thomas?
> 
> That's true, for testing purposes we don't need to duplicate any parts of the
> validation in the UI. It's still valuable to be able to test error handling in
> the test environment so for that we could define some way that always throws a
> generic filter validation error in parseFilter(s).
> 
> e.g. A URL parameter that causes the parseFilter(s) function to return an
error
> OR specify a character that - when included in a filter text - causes an
error.

+1 (this would also be in line with how we test the data corruption warning on
the first run page, and different languages)

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = On 2015/07/14 14:25:27, Sebastian Noack wrote: > ...

July 14, 2015, 3:08 p.m. (2015-07-14 15:08:27 UTC) #26

https://codereview.adblockplus.org/29321198/diff/29322235/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newco...
background.js:156: modules.filterValidation =
On 2015/07/14 14:25:27, Sebastian Noack wrote:
> On 2015/07/14 14:20:29, Thomas Greiner wrote:
> > On 2015/07/14 14:10:09, Sebastian Noack wrote:
> > > On 2015/07/14 13:53:28, saroyanm wrote:
> > > > On 2015/07/14 13:36:31, Sebastian Noack wrote:
> > > > > I wonder whezther this implementation might go a little to far for a
> mock.
> > > How
> > > > > about simply making it noop?
> > > > > 
> > > > > modules.filterValidation = {
> > > > >   parseFilter: modules.filterClasses.Filter.fromText,
> > > > >   parseFilters: function(text)
> > > > >   {
> > > > >     return
text.split("\n").map(modules.filterClasses.Filter.fromText);
> > > > >   }
> > > > > };
> > > > 
> > > > parseFilter: In this case we wouldn't be able to return
> FilterParsingError,
> > > > which helps testing locally for errors. 
> > > > parseFilters: this method should return {filters: filters, errors:
> errors},
> > > > again helps testing locally, at least returning similar format, that we
> have
> > > for
> > > > production code.
> > > 
> > > Sorry, I meant:
> > > 
> > >   modules.filterValidation = {
> > >     parseFilter: function(text)
> > >     {
> > >       return {filter: modules.filterClasses.Filter.fromText(text)};
> > >     },
> > >     parseFilters: function(text)
> > >     {
> > >       return {filters:
> > > text.split("\n").map(modules.filterClasses.Filter.fromText)};
> > >     }
> > >   };
> > > 
> > > Sure testing validation errors with the UI only wouldn't be possible then.
> But
> > I
> > > was wondering whether testing these (outside of the extension) is worth
the
> > > added complexity and duplicated logic. Thomas?
> > 
> > That's true, for testing purposes we don't need to duplicate any parts of
the
> > validation in the UI. It's still valuable to be able to test error handling
in
> > the test environment so for that we could define some way that always throws
a
> > generic filter validation error in parseFilter(s).
> > 
> > e.g. A URL parameter that causes the parseFilter(s) function to return an
> error
> > OR specify a character that - when included in a filter text - causes an
> error.
> 
> +1 (this would also be in line with how we test the data corruption warning on
> the first run page, and different languages)

Please note that error should be an instance of "FilterParsingError".
also code is not fully duplicated, there was more robust check for different
errors.
parseFilters doesn't return {filters: filters}, but {filters: fiters, errors:
errors}
I would say make sense to change parsing error to anything else while we are
excluding "unexpected-filter-list-header" in filters.importRaw, but it's making
the testing more nautral, just enter "[" in textbox or in textarea several times
to test it.
Updated the code please let me know if you are Okey with current one, otherwise
I can go with your suggestion.

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322235/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newcode156 background.js:156: modules.filterValidation = On 2015/07/14 15:08:26, saroyanm wrote: > On ...

July 14, 2015, 4:48 p.m. (2015-07-14 16:48:41 UTC) #27

https://codereview.adblockplus.org/29321198/diff/29322235/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newco...
background.js:156: modules.filterValidation =
On 2015/07/14 15:08:26, saroyanm wrote:
> On 2015/07/14 14:25:27, Sebastian Noack wrote:
> > On 2015/07/14 14:20:29, Thomas Greiner wrote:
> > > On 2015/07/14 14:10:09, Sebastian Noack wrote:
> > > > On 2015/07/14 13:53:28, saroyanm wrote:
> > > > > On 2015/07/14 13:36:31, Sebastian Noack wrote:
> > > > > > I wonder whezther this implementation might go a little to far for a
> > mock.
> > > > How
> > > > > > about simply making it noop?
> > > > > > 
> > > > > > modules.filterValidation = {
> > > > > >   parseFilter: modules.filterClasses.Filter.fromText,
> > > > > >   parseFilters: function(text)
> > > > > >   {
> > > > > >     return
> text.split("\n").map(modules.filterClasses.Filter.fromText);
> > > > > >   }
> > > > > > };
> > > > > 
> > > > > parseFilter: In this case we wouldn't be able to return
> > FilterParsingError,
> > > > > which helps testing locally for errors. 
> > > > > parseFilters: this method should return {filters: filters, errors:
> > errors},
> > > > > again helps testing locally, at least returning similar format, that
we
> > have
> > > > for
> > > > > production code.
> > > > 
> > > > Sorry, I meant:
> > > > 
> > > >   modules.filterValidation = {
> > > >     parseFilter: function(text)
> > > >     {
> > > >       return {filter: modules.filterClasses.Filter.fromText(text)};
> > > >     },
> > > >     parseFilters: function(text)
> > > >     {
> > > >       return {filters:
> > > > text.split("\n").map(modules.filterClasses.Filter.fromText)};
> > > >     }
> > > >   };
> > > > 
> > > > Sure testing validation errors with the UI only wouldn't be possible
then.
> > But
> > > I
> > > > was wondering whether testing these (outside of the extension) is worth
> the
> > > > added complexity and duplicated logic. Thomas?
> > > 
> > > That's true, for testing purposes we don't need to duplicate any parts of
> the
> > > validation in the UI. It's still valuable to be able to test error
handling
> in
> > > the test environment so for that we could define some way that always
throws
> a
> > > generic filter validation error in parseFilter(s).
> > > 
> > > e.g. A URL parameter that causes the parseFilter(s) function to return an
> > error
> > > OR specify a character that - when included in a filter text - causes an
> > error.
> > 
> > +1 (this would also be in line with how we test the data corruption warning
on
> > the first run page, and different languages)
> 
> Please note that error should be an instance of "FilterParsingError".

Actually, we don't need to implement the FilterParsingError class here. We could
simply use a string or window.Error object instead. It's toString() method will
return the message, and its "type" attribute will be undefined an therefore
won't equal "unexpected-filter-list-header".

> I would say make sense to change parsing error to anything else while we are
> excluding "unexpected-filter-list-header" in filters.importRaw, but it's
making
> the testing more nautral, just enter "[" in textbox or in textarea several
times
> to test it.

This behavior isn't obvious. There are several error scenarios (in the
extension), and you have to look at the code to know which one is implemented
here, to test the validation error scenario outside of the extension. Moreover,
the way you implemented it is inconsistent. In the extension, lines starting
with "[" will be stripped when importing raw filters. With your implementation
it will cause an error.

So we either end up duplicating an unreasonable amount of the logic we have in
the extension here, and the current implementation already goes a little to far
for my taste. Or we return an error here, regardless of the input, but
depdendant on an URL parameter, as greiner suggested, similar to
"seenDataCorruption" and a couple of others we already have for similar purposes
(see the README).

> Updated the code please let me know if you are Okey with current one,
otherwise
> I can go with your suggestion.

You merely slightly simplified the current logic. That is not what we were
talking about.

https://codereview.adblockplus.org/29321198/diff/29322301/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322301/messageResponder.js...
messageResponder.js:190: FilterStorage.addFilter(result.filter);
I pointed this out before: If |text.trim() == ""| then parseFilter() returns
{filter: null}, calling FilterStorage.addFilter(null) here, causing an error.
I've seen that you already bail out on the options page before sending the
filters.add message, if the filter text is an empty string. However if the text
is " " for example you still cause an unhandled error. I'd prefer to keep this
logic entirely to the parseFilter() function, simply handling result.filter
being null.

https://codereview.adblockplus.org/29321198/diff/29322301/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29322301/options.js#newcode438
options.js:438: if (!filterText)
This check is redundant with the logic implemented in parseFilter().

saroyanm

Sebastian can you please have a look, hope I've got your point right. https://codereview.adblockplus.org/29321198/diff/29322235/background.js File ...

July 14, 2015, 6:28 p.m. (2015-07-14 18:28:31 UTC) #28

Sebastian can you please have a look, hope I've got your point right.

https://codereview.adblockplus.org/29321198/diff/29322235/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322235/background.js#newco...
background.js:156: modules.filterValidation =
On 2015/07/14 16:48:38, Sebastian Noack wrote:
> On 2015/07/14 15:08:26, saroyanm wrote:
> > On 2015/07/14 14:25:27, Sebastian Noack wrote:
> > > On 2015/07/14 14:20:29, Thomas Greiner wrote:
> > > > On 2015/07/14 14:10:09, Sebastian Noack wrote:
> > > > > On 2015/07/14 13:53:28, saroyanm wrote:
> > > > > > On 2015/07/14 13:36:31, Sebastian Noack wrote:
> > > > > > > I wonder whezther this implementation might go a little to far for
a
> > > mock.
> > > > > How
> > > > > > > about simply making it noop?
> > > > > > > 
> > > > > > > modules.filterValidation = {
> > > > > > >   parseFilter: modules.filterClasses.Filter.fromText,
> > > > > > >   parseFilters: function(text)
> > > > > > >   {
> > > > > > >     return
> > text.split("\n").map(modules.filterClasses.Filter.fromText);
> > > > > > >   }
> > > > > > > };
> > > > > > 
> > > > > > parseFilter: In this case we wouldn't be able to return
> > > FilterParsingError,
> > > > > > which helps testing locally for errors. 
> > > > > > parseFilters: this method should return {filters: filters, errors:
> > > errors},
> > > > > > again helps testing locally, at least returning similar format, that
> we
> > > have
> > > > > for
> > > > > > production code.
> > > > > 
> > > > > Sorry, I meant:
> > > > > 
> > > > >   modules.filterValidation = {
> > > > >     parseFilter: function(text)
> > > > >     {
> > > > >       return {filter: modules.filterClasses.Filter.fromText(text)};
> > > > >     },
> > > > >     parseFilters: function(text)
> > > > >     {
> > > > >       return {filters:
> > > > > text.split("\n").map(modules.filterClasses.Filter.fromText)};
> > > > >     }
> > > > >   };
> > > > > 
> > > > > Sure testing validation errors with the UI only wouldn't be possible
> then.
> > > But
> > > > I
> > > > > was wondering whether testing these (outside of the extension) is
worth
> > the
> > > > > added complexity and duplicated logic. Thomas?
> > > > 
> > > > That's true, for testing purposes we don't need to duplicate any parts
of
> > the
> > > > validation in the UI. It's still valuable to be able to test error
> handling
> > in
> > > > the test environment so for that we could define some way that always
> throws
> > a
> > > > generic filter validation error in parseFilter(s).
> > > > 
> > > > e.g. A URL parameter that causes the parseFilter(s) function to return
an
> > > error
> > > > OR specify a character that - when included in a filter text - causes an
> > > error.
> > > 
> > > +1 (this would also be in line with how we test the data corruption
warning
> on
> > > the first run page, and different languages)
> > 
> > Please note that error should be an instance of "FilterParsingError".
> 
> Actually, we don't need to implement the FilterParsingError class here. We
could
> simply use a string or window.Error object instead. It's toString() method
will
> return the message, and its "type" attribute will be undefined an therefore
> won't equal "unexpected-filter-list-header".
> 
> > I would say make sense to change parsing error to anything else while we are
> > excluding "unexpected-filter-list-header" in filters.importRaw, but it's
> making
> > the testing more nautral, just enter "[" in textbox or in textarea several
> times
> > to test it.
> 
> This behavior isn't obvious. There are several error scenarios (in the
> extension), and you have to look at the code to know which one is implemented
> here, to test the validation error scenario outside of the extension.
Moreover,
> the way you implemented it is inconsistent. In the extension, lines starting
> with "[" will be stripped when importing raw filters. With your implementation
> it will cause an error.
> 
> So we either end up duplicating an unreasonable amount of the logic we have in
> the extension here, and the current implementation already goes a little to
far
> for my taste. Or we return an error here, regardless of the input, but
> depdendant on an URL parameter, as greiner suggested, similar to
> "seenDataCorruption" and a couple of others we already have for similar
purposes
> (see the README).
> 
> > Updated the code please let me know if you are Okey with current one,
> otherwise
> > I can go with your suggestion.
> 
> You merely slightly simplified the current logic. That is not what we were
> talking about.

Done.

https://codereview.adblockplus.org/29321198/diff/29322301/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322301/messageResponder.js...
messageResponder.js:190: FilterStorage.addFilter(result.filter);
On 2015/07/14 16:48:40, Sebastian Noack wrote:
> I pointed this out before: If |text.trim() == ""| then parseFilter() returns
> {filter: null}, calling FilterStorage.addFilter(null) here, causing an error.
> I've seen that you already bail out on the options page before sending the
> filters.add message, if the filter text is an empty string. However if the
text
> is " " for example you still cause an unhandled error. I'd prefer to keep this
> logic entirely to the parseFilter() function, simply handling result.filter
> being null.

Done.

https://codereview.adblockplus.org/29321198/diff/29322301/options.js
File options.js (right):

https://codereview.adblockplus.org/29321198/diff/29322301/options.js#newcode438
options.js:438: if (!filterText)
On 2015/07/14 16:48:40, Sebastian Noack wrote:
> This check is redundant with the logic implemented in parseFilter().

Done.

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322325/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newcode161 background.js:161: updateFromURL(filterError); Add this parameter to the README. https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newcode163 background.js:163: ...

July 15, 2015, 8:32 a.m. (2015-07-15 08:32:39 UTC) #29

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322325/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newcode162 background.js:162: if (filterError) This will always be true. It should ...

July 15, 2015, 9 a.m. (2015-07-15 09:00:46 UTC) #30

saroyanm

Sorry for the confusion with separate review. New patch uploaded, feels like we are near. ...

July 15, 2015, 10:47 a.m. (2015-07-15 10:47:12 UTC) #31

Sorry for the confusion with separate review. 
New patch uploaded, feels like we are near.

https://codereview.adblockplus.org/29321198/diff/29322325/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newco...
background.js:161: updateFromURL(filterError);
On 2015/07/15 08:32:35, Thomas Greiner wrote:
> Add this parameter to the README.

Done.

https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newco...
background.js:162: if (filterError)
On 2015/07/15 09:00:44, Sebastian Noack wrote:
> This will always be true. It should be |filterError.filterError|. Or to make
it
> less confusing, you might want to rename the variable to "params":
> 
>   var params = {filterError: false};
>   updateFromURL(params);
>   if (params.filterError)
>     ...

Done.

https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newco...
background.js:163: return {error: "Filter-parsing-error-occurred"};
On 2015/07/15 08:32:36, Thomas Greiner wrote:
> I'd suggest to use all lower-case for consistency and to remove the "-occured"
> at the end because it's an ID, not a statement.

Done.

https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newco...
background.js:168: return {filters:
text.split("\n").map(modules.filterClasses.Filter.fromText)};
On 2015/07/15 09:00:44, Sebastian Noack wrote:
> You don't consider the filter filterError URL parameter here.

Done.

https://codereview.adblockplus.org/29321198/diff/29322325/background.js#newco...
background.js:168: return {filters:
text.split("\n").map(modules.filterClasses.Filter.fromText)};
On 2015/07/15 08:32:35, Thomas Greiner wrote:
> This line exceeds the 80 characters limit.

Done.

https://codereview.adblockplus.org/29321198/diff/29322325/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322325/messageResponder.js...
messageResponder.js:187: if (result.error && result.error.type !=
"empty-filter")
On 2015/07/15 09:00:45, Sebastian Noack wrote:
> There is no error type "empty-filter". As I said, the result will be {filter:
> null} in this case. So the logic here should look like:
> 
>   if (result.error)
>     sendMessage("app", "error", [result.error.toString()], sender.page);
>   else if (result.filter)
>     FilterStorage.addFilter(result.filter);

I think it was misunderstanding, what I understood from your previouse message
that you would like to keep that entire logic in parseFilter, which is in
filterValidation module, so I've created a separate review where I've added new
"empty-filter" error to parseFilter function.
Should have aligned with you before implementing.
Done.

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322325/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29321198/diff/29322325/messageResponder.js#newcode187 messageResponder.js:187: if (result.error && result.error.type != "empty-filter") On 2015/07/15 10:47:11, ...

July 15, 2015, 11:20 a.m. (2015-07-15 11:20:01 UTC) #32

https://codereview.adblockplus.org/29321198/diff/29322325/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29321198/diff/29322325/messageResponder.js...
messageResponder.js:187: if (result.error && result.error.type !=
"empty-filter")
On 2015/07/15 10:47:11, saroyanm wrote:
> On 2015/07/15 09:00:45, Sebastian Noack wrote:
> > There is no error type "empty-filter". As I said, the result will be
{filter:
> > null} in this case. So the logic here should look like:
> > 
> >   if (result.error)
> >     sendMessage("app", "error", [result.error.toString()], sender.page);
> >   else if (result.filter)
> >     FilterStorage.addFilter(result.filter);
> 
> I think it was misunderstanding, what I understood from your previouse message
> that you would like to keep that entire logic in parseFilter, which is in
> filterValidation module, so I've created a separate review where I've added
new
> "empty-filter" error to parseFilter function.
> Should have aligned with you before implementing.
> Done.

What I meant is, that you should merely leave the check for empty filters to the
filterValidation module, which is already implemented there as
!Filter.normlize(text), and returns {filter: null} in that case.

https://codereview.adblockplus.org/29321198/diff/29322358/README.md
File README.md (right):

https://codereview.adblockplus.org/29321198/diff/29322358/README.md#newcode75
README.md:75: * `filterError=true`: this parameter should trigger filter parsing
error
IMO better: "causes filter validation to fail, showing validation errors when
adding new filters on the options page"

https://codereview.adblockplus.org/29321198/diff/29322358/background.js
File background.js (right):

https://codereview.adblockplus.org/29321198/diff/29322358/background.js#newco...
background.js:156: var validation = {filterError: false};
I wonder whether it would make more sense meanwhile (to reduce code and number
of variables) to have only one object with all params? Thomas? 

  var params = {
    blockedURLs: "",
    seenDataCorruption: false,
    filterlistsReinitialized: false,
    addSubscription: false,
    filterError: false
  };
  updateFromURL(params);

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322358/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322358/background.js#newcode156 background.js:156: var validation = {filterError: false}; On 2015/07/15 11:20:00, Sebastian ...

July 15, 2015, 11:31 a.m. (2015-07-15 11:31:41 UTC) #33

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322358/README.md File README.md (right): https://codereview.adblockplus.org/29321198/diff/29322358/README.md#newcode75 README.md:75: * `filterError=true`: this parameter should trigger filter parsing error ...

July 15, 2015, 12:06 p.m. (2015-07-15 12:06:54 UTC) #34

Thomas Greiner

https://codereview.adblockplus.org/29321198/diff/29322403/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322403/background.js#newcode225 background.js:225: modules.info = { On 2015/07/15 12:06:53, saroyanm wrote: > ...

July 15, 2015, 12:29 p.m. (2015-07-15 12:29:36 UTC) #35

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322403/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322403/background.js#newcode225 background.js:225: modules.info = { On 2015/07/15 12:29:35, Thomas Greiner wrote: ...

July 15, 2015, 12:34 p.m. (2015-07-15 12:34:59 UTC) #36

Sebastian Noack

https://codereview.adblockplus.org/29321198/diff/29322403/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322403/background.js#newcode41 background.js:41: updateFromURL(params); Nit: There should be an empty line below. ...

July 15, 2015, 2:30 p.m. (2015-07-15 14:30:09 UTC) #37

saroyanm

https://codereview.adblockplus.org/29321198/diff/29322403/background.js File background.js (right): https://codereview.adblockplus.org/29321198/diff/29322403/background.js#newcode41 background.js:41: updateFromURL(params); On 2015/07/15 14:30:08, Sebastian Noack wrote: > Nit: ...

July 15, 2015, 2:36 p.m. (2015-07-15 14:36:06 UTC) #38

LGTM

Issue 29321198: Issue 2376 - Implement custom filters in new options page (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed initial comments #

Patch Set 3 : Addressed comments, moved error message to background page for consistency #

Patch Set 4 : Rebase #

Patch Set 5 : Error handling #

Patch Set 6 : removed the console.log #

Patch Set 7 : Error handling improvements #

Patch Set 8 : Small fixes #

Patch Set 9 : #

Patch Set 10 : Sebastian requested fixes #

Patch Set 11 : updated mockup #

Patch Set 12 : Mockup simplification and validation update #

Patch Set 13 : Added param to readme and some fixes #

Patch Set 14 : Small fixes #

Patch Set 15 : Nit fixes #

Messages