Issue 2801 - Create 'Whitelisted websites' screen and add link to 'Ad blocking' screen

Issue 2801 - Create 'Whitelisted websites' screen and add link to 'Ad blocking' screen

[Thomas Greiner, 2017-09-15 15:08:06]

I was asked to provide feedback regarding the extension interface so I added
some comments.

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm
File adblockplus/Api.jsm (left):

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm...
adblockplus/Api.jsm:104: let uriObject = Services.io.newURI(url, null, null);
It's not a big deal whether or not we're extracting the host here or in the Java
code but I noticed that we're now manually normalizing the URL in
`UrlUtils.formatUrl()` rather than using an existing API for that like we do
here.

This may cause issues with specially crafted URLs such as
"http://example.com/?url=http://www.example.com/". That's why we're relying on
native APIs wherever possible so that we can safely ignore such special cases.

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm
File adblockplus/Api.jsm (right):

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm...
adblockplus/Api.jsm:199: let whitelistMatch = filter.text ?
filter.text.match(/@@\|\|(.*)\^\$document/) : null;
We're using a slightly different regular expression for the extension's options
page (see https://hg.adblockplus.org/adblockpluschrome/file/tip/options.js#l75)
so I'd recommend using that one instead to avoid any issues and to ensure
consistent behavior.

[jens, 2017-09-15 15:23:36]

On 2017/09/13 17:04:14, diegocarloslima wrote:

Except Thomas remarks, LGTM

[diegocarloslima, 2017-09-18 14:16:37]

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm
File adblockplus/Api.jsm (left):

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm...
adblockplus/Api.jsm:104: let uriObject = Services.io.newURI(url, null, null);
On 2017/09/15 15:08:06, Thomas Greiner wrote:
> It's not a big deal whether or not we're extracting the host here or in the
Java
> code but I noticed that we're now manually normalizing the URL in
> `UrlUtils.formatUrl()` rather than using an existing API for that like we do
> here.
> 
> This may cause issues with specially crafted URLs such as
> "http://example.com/?url=http://www.example.com/". That's why we're relying on
> native APIs wherever possible so that we can safely ignore such special cases.

I do agree with you, the intention of the formatUrl() was just to add the HTTP
protocol on some cases which it's missing in order to properly parse the URL. I
will reimplement it by relying in the URL class in order to provide a more
robust solution

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm
File adblockplus/Api.jsm (right):

https://codereview.adblockplus.org/29543774/diff/29543775/adblockplus/Api.jsm...
adblockplus/Api.jsm:199: let whitelistMatch = filter.text ?
filter.text.match(/@@\|\|(.*)\^\$document/) : null;
On 2017/09/15 15:08:06, Thomas Greiner wrote:
> We're using a slightly different regular expression for the extension's
options
> page (see
https://hg.adblockplus.org/adblockpluschrome/file/tip/options.js#l75)
> so I'd recommend using that one instead to avoid any issues and to ensure
> consistent behavior.

Acknowledged.

[anton, 2017-09-26 06:24:08]

LGTM, taking into account 2 minor notes

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/base...
File mobile/android/base/locales/adblockbrowser/ar/android_strings.dtd (right):

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/base...
mobile/android/base/locales/adblockbrowser/ar/android_strings.dtd:42: <!ENTITY
abb_whitelisted_websites_loading "Loading whitelisted websites...">
shouldn't it be translated first (same for other DTDs)?

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/UrlUtils.java (right):

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlUtils.java:25: 
the line seems to be not required

[Thomas Greiner, 2017-09-26 11:25:21]

LGTM

[diegocarloslima, 2017-09-26 12:24:44]

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/base...
File mobile/android/base/locales/adblockbrowser/ar/android_strings.dtd (right):

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/base...
mobile/android/base/locales/adblockbrowser/ar/android_strings.dtd:42: <!ENTITY
abb_whitelisted_websites_loading "Loading whitelisted websites...">
On 2017/09/26 06:24:07, anton wrote:
> shouldn't it be translated first (same for other DTDs)?

We usually first add the feature without translations, because it might take a
while to get them and before releasing, we push all the relevant translations
for all introduced/changed strings.

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/thir...
File mobile/android/thirdparty/org/adblockplus/browser/UrlUtils.java (right):

https://codereview.adblockplus.org/29543774/diff/29549803/mobile/android/thir...
mobile/android/thirdparty/org/adblockplus/browser/UrlUtils.java:25: 
On 2017/09/26 06:24:07, anton wrote:
> the line seems to be not required

Acknowledged.