Issue 5309182173511680: Issue 2211 - Implemented subscribe.adblockplus.org subscription links (Closed)
|
Created:
April 1, 2015, 11:56 a.m. by Thomas Greiner Modified:
July 20, 2015, 2:39 p.m. Visibility:
Public. |
DescriptionIssue 2211 - Implemented subscribe.adblockplus.org subscription links
Patch Set 1 #
Total comments: 4
Patch Set 2 : Check URL-object properties instead of regular expression #
Total comments: 2
Patch Set 3 : Removed redundant URL scheme check #
Total comments: 4
Patch Set 4 : Removed code related to "linkTarget" parameter #
Total comments: 3
Patch Set 5 : #Patch Set 6 : Rebased to b7c6ed7c2137 #MessagesTotal messages: 15
http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... lib/ui.js:922: let match = /^(?:abp:\/*subscribe|https?:\/*subscribe\.adblockplus\.org)\/*\?(.*)/i.exec(linkTarget); I think we should use DOM API's (i.e. the anchor element's "protocol", "host", "pathname" and "search" property) to parse HTTP/HTTPS URLs like we do on Chrome. This will be more efficient and IMO cleaner than using a regexp, if consistency with the implementation for Chrome isn't reason enough.
http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... lib/ui.js:922: let match = /^(?:abp:\/*subscribe|https?:\/*subscribe\.adblockplus\.org)\/*\?(.*)/i.exec(linkTarget); On 2015/04/01 12:29:49, Sebastian Noack wrote: > I think we should use DOM API's (i.e. the anchor element's "protocol", "host", > "pathname" and "search" property) to parse HTTP/HTTPS URLs like we do on Chrome. > This will be more efficient and IMO cleaner than using a regexp, if consistency > with the implementation for Chrome isn't reason enough. It's a different situation than on Chrome since we also have to consider the optional "linkTarget" parameter but I guess by creating a new instance of URL it could be made more similar to the Chrome implementation. I'll work on that.
http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... lib/ui.js:922: let match = /^(?:abp:\/*subscribe|https?:\/*subscribe\.adblockplus\.org)\/*\?(.*)/i.exec(linkTarget); On 2015/04/01 12:41:15, Thomas Greiner wrote: > On 2015/04/01 12:29:49, Sebastian Noack wrote: > > I think we should use DOM API's (i.e. the anchor element's "protocol", "host", > > "pathname" and "search" property) to parse HTTP/HTTPS URLs like we do on > Chrome. > > This will be more efficient and IMO cleaner than using a regexp, if > consistency > > with the implementation for Chrome isn't reason enough. > > It's a different situation than on Chrome since we also have to consider the > optional "linkTarget" parameter but I guess by creating a new instance of URL it > could be made more similar to the Chrome implementation. I'll work on that. I'm not sure what you mean. Since you already use link.href above, and the old code also used link.protocol, I suppose you could also use link.host, link.pathname and link.search?
http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5629499534213120/lib/... lib/ui.js:922: let match = /^(?:abp:\/*subscribe|https?:\/*subscribe\.adblockplus\.org)\/*\?(.*)/i.exec(linkTarget); On 2015/04/01 12:44:20, Sebastian Noack wrote: > On 2015/04/01 12:41:15, Thomas Greiner wrote: > > On 2015/04/01 12:29:49, Sebastian Noack wrote: > > > I think we should use DOM API's (i.e. the anchor element's "protocol", > "host", > > > "pathname" and "search" property) to parse HTTP/HTTPS URLs like we do on > > Chrome. > > > This will be more efficient and IMO cleaner than using a regexp, if > > consistency > > > with the implementation for Chrome isn't reason enough. > > > > It's a different situation than on Chrome since we also have to consider the > > optional "linkTarget" string parameter but I guess by creating a new instance of URL > it > > could be made more similar to the Chrome implementation. I'll work on that. > > I'm not sure what you mean. Since you already use link.href above, and the old > code also used link.protocol, I suppose you could also use link.host, > link.pathname and link.search? There are two ways to get a link target here. One is through an event (as we do in Chrome) and the other way is through the "linkTarget" parameter of the "onBrowserClick" function. If we only check for "subscribe.adblockplus.org" in the above if-statement we'd ignore cases where "subscribe.adblockplus.org" is passed through the "linkTarget" parameter.
http://codereview.adblockplus.org/5309182173511680/diff/5639274879778816/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5639274879778816/lib/... lib/ui.js:941: else if (link.protocol == "abp:") Note that in the Chrome implementation - for simplicity - we omit that check, running the regexp on all non-HTTP(S) URLs. This removes duplication, and non-HTTP(s) URLs are a case not frequent enough to be worth optimizing, IMO.
http://codereview.adblockplus.org/5309182173511680/diff/5639274879778816/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5639274879778816/lib/... lib/ui.js:941: else if (link.protocol == "abp:") On 2015/04/01 14:41:58, Sebastian Noack wrote: > Note that in the Chrome implementation - for simplicity - we omit that check, > running the regexp on all non-HTTP(S) URLs. This removes duplication, and > non-HTTP(s) URLs are a case not frequent enough to be worth optimizing, IMO. Done.
LGTM
http://codereview.adblockplus.org/5309182173511680/diff/5733935958982656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5733935958982656/lib/... lib/ui.js:899: * either with an event object or with the link target (if it is the former then Nit: Please stick to the 80 characters line limit. http://codereview.adblockplus.org/5309182173511680/diff/5733935958982656/lib/... lib/ui.js:902: onBrowserClick: function (/**Window*/ window, /**Event*/ event, /**String*/ linkTarget) It seems that the linkTarget parameter is no longer used. I traced that parameter back to https://hg.adblockplus.org/adblockplus/rev/96ef08f7d6ea - it was added to support E10S in Firefox Mobile. With the click being intercepted by a content script there was no event object. While we might have to add something like this again eventually, for now it should be better to remove dead code and simplify the logic. Particularly given that there is currently no way to test this code path...
http://codereview.adblockplus.org/5309182173511680/diff/5733935958982656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5309182173511680/diff/5733935958982656/lib/... lib/ui.js:899: * either with an event object or with the link target (if it is the former then On 2015/04/01 17:49:31, Wladimir Palant wrote: > Nit: Please stick to the 80 characters line limit. Done. I also added a preference to my editor to notice that in the future. :) http://codereview.adblockplus.org/5309182173511680/diff/5733935958982656/lib/... lib/ui.js:902: onBrowserClick: function (/**Window*/ window, /**Event*/ event, /**String*/ linkTarget) On 2015/04/01 17:49:31, Wladimir Palant wrote: > It seems that the linkTarget parameter is no longer used. I traced that > parameter back to https://hg.adblockplus.org/adblockplus/rev/96ef08f7d6ea - it > was added to support E10S in Firefox Mobile. With the click being intercepted by > a content script there was no event object. > > While we might have to add something like this again eventually, for now it > should be better to remove dead code and simplify the logic. Particularly given > that there is currently no way to test this code path... Done.
Even more LGTM.
https://codereview.adblockplus.org/5309182173511680/diff/5700735861784576/lib... File lib/ui.js (right): https://codereview.adblockplus.org/5309182173511680/diff/5700735861784576/lib... lib/ui.js:900: onBrowserClick: function (/**Window*/ window, /**Event*/ event) It seems that currently we will always get an event, so this change is ok. We'll need to fix E10S compatibility here again soon, but maybe we will come up with a better solution this time. https://codereview.adblockplus.org/5309182173511680/diff/5700735861784576/lib... lib/ui.js:919: if (link.host == "subscribe.adblockplus.org" || link.pathname == "/") Shouldn't this be && rather than ||?
Addressed comment and rebased to current revision https://codereview.adblockplus.org/5309182173511680/diff/5700735861784576/lib... File lib/ui.js (right): https://codereview.adblockplus.org/5309182173511680/diff/5700735861784576/lib... lib/ui.js:919: if (link.host == "subscribe.adblockplus.org" || link.pathname == "/") On 2015/07/20 11:23:10, Wladimir Palant wrote: > Shouldn't this be && rather than ||? Done.
LGTM 
|
