Issue 2212 - Implement http://subscribe.adblockplus.org/?... subscription links

Issue 2212 - Implement http://subscribe.adblockplus.org/?... subscription links

[kzar, 2015-03-25 14:09:29]

LGTM

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:621: if (link.protocol == "http:" || link.protocol ==
"https:")
(Do we even need to support http? I guess we will always use https for
subscribe.adblockplus.org links?)

[Sebastian Noack, 2015-03-25 14:21:03]

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:621: if (link.protocol == "http:" || link.protocol ==
"https:")
On 2015/03/25 14:09:29, kzar wrote:
> (Do we even need to support http? I guess we will always use https for
> http://subscribe.adblockplus.org links?)

We probably will. But others linking to our website might not. And since we
aren't going to intercept requests but just link clicking here, we won't see the
redirect.

[Thomas Greiner, 2015-03-25 15:05:36]

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:626: else if
(!/^abp:\/*subscribe\/*\?(.*)/i.test(link.href))
The `(.*)` at the end of the regular expression is redundant now.

[Wladimir Palant, 2015-03-25 15:07:16]

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:626: else if
(!/^abp:\/*subscribe\/*\?(.*)/i.test(link.href))
Nit: Please remove the parentheses in this regexp if you aren't going to use the
group.

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:634: var params = link.search.substr(1).split("&");
Note that this change will make the approach inconsistent between Firefox and
Chrome - Firefox (unlike Chrome and Safari) won't extract the query string for
unknown protocols.

[Sebastian Noack, 2015-03-25 15:23:26]

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:626: else if
(!/^abp:\/*subscribe\/*\?(.*)/i.test(link.href))
On 2015/03/25 15:05:36, Thomas Greiner wrote:
> The `(.*)` at the end of the regular expression is redundant now.

Done.

http://codereview.adblockplus.org/5246523029848064/diff/5629499534213120/incl...
include.postload.js:634: var params = link.search.substr(1).split("&");
On 2015/03/25 15:07:17, Wladimir Palant wrote:
> Note that this change will make the approach inconsistent between Firefox and
> Chrome - Firefox (unlike Chrome and Safari) won't extract the query string for
> unknown protocols.

Well, the alternative would be to make things inconsistent between abp: and
http: URLs, introducing redundant logic here.

[Wladimir Palant, 2015-03-25 15:29:21]

LGTM