Issue 5225119261655040: Issue 1282 - Don't generate filters conflicting with existing exception rules (Closed)
|
Created:
Dec. 13, 2014, 2:11 p.m. by Sebastian Noack Modified:
March 3, 2015, 8:11 p.m. Visibility:
Public. |
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Renamed addElemHideFilter() and simplified addStyleAttributeFilter() #Patch Set 3 : Rebased #Patch Set 4 : Rebased #Patch Set 5 : Rebased #
Total comments: 15
Patch Set 6 : Rebased and addressed comments #Patch Set 7 : Addressed comment #
Total comments: 3
MessagesTotal messages: 17
http://codereview.adblockplus.org/5225119261655040/diff/5629499534213120/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5629499534213120/incl... include.postload.js:552: if (!whitelisted[url] && /^https?:/i.test(url)) The code below this line only changed due to indentation.
http://codereview.adblockplus.org/5225119261655040/diff/5724160613416960/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5724160613416960/incl... include.postload.js:548: if (!whitelisted[url] && /^https?:/i.test(url)) The code below this line only changed due to indentation.
I guess your comment about indentation changes no longer apply? I couldn't see the changes you mentioned. LGTM apart from include.preload.js change I commented on. I really think palant should check this one though, the changes are quite involved and I don't understand some of the details. http://codereview.adblockplus.org/5225119261655040/diff/5733935958982656/incl... File include.preload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5733935958982656/incl... include.preload.js:23: "picture": "IMAGE", Are these changes relevant to the issue?
On 2014/12/15 15:45:40, kzar wrote: > I guess your comment about indentation changes no longer apply? I couldn't see > the changes you mentioned. Yes, this comment were on an outdated patch set which I have deleted meanwhile. http://codereview.adblockplus.org/5225119261655040/diff/5733935958982656/incl... File include.preload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5733935958982656/incl... include.preload.js:23: "picture": "IMAGE", On 2014/12/15 15:45:40, kzar wrote: > Are these changes relevant to the issue? Yes, they are. Initially this mapping were only used by the element collapsing code, which isn't used for <obect> and <embed> elements. However, the new code stripping whitelisted filters, using this mapping as well, also considers those elements. The <picture> element is relevant for both use cases, and were just missing, since this element is rather new.
I've uploaded a new patch set based on #1853. Having the filter generation in the background page clearly simplifies things.
A little hard to review since the rebase but that's more a fault of rietveld. Otherwise LGTM, I agree it looks better now after the changes in #1853.
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:465: if (!clickHide_activated || currentElement != e.target) Wouldn't e.target not match for image maps where the element we're blocking is the image but the mouse events register for the area elements as explained above?
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:465: if (!clickHide_activated || currentElement != e.target) On 2015/03/02 18:30:08, kzar wrote: > Wouldn't e.target not match for image maps where the element we're blocking is > the image but the mouse events register for the area elements as explained > above? currentElement is the element we are highlighting (the image in that case), and the this code unhighlights it, when moving the mouse out of that element. I don't see any issues here.
LGTM http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:465: if (!clickHide_activated || currentElement != e.target) On 2015/03/02 18:40:48, Sebastian Noack wrote: > On 2015/03/02 18:30:08, kzar wrote: > > Wouldn't e.target not match for image maps where the element we're blocking is > > the image but the mouse events register for the area elements as explained > > above? > > currentElement is the element we are highlighting (the image in that case), and > the this code unhighlights it, when moving the mouse out of that element. I > don't see any issues here. Whoops missed the return there, OK.
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... background.js:559: msg.baseURL, sender.page, sender.frame Ten positional parameters in a function is certainly a reason to worry. Normally you would turn the parameters into a dictionary but in this particular case - maybe pass in msg and sender directly? http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:401: getBlockableElementOrAncestor(element.parentElement, callback); This seems to be the right recipe to error out with a "too much recursion" error, e.g. when this function is applied to an element deep inside the SVG hierarchy. Chrome seems to allow really massive recursion (my test function crashed only after 17800 recursive calls) but that isn't something to be relied upon. Whenever we can simply iterate we should do that rather than making recursive calls. http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:448: unhighlightElement(currentElement); I don't think it is that simple. Mouse events can be generated very quickly, sometimes even synchronously (e.g. if the handler on the webpage changes layout in such a way that another element is moved under the mouse pointer). So by the time you get a response from the background page it might be outdated already. One solution I see here is making only the most recent request count: a global variable is used as counter for mouseOver requests. Before calling getBlockableElementOrAncestor() this function stores a copy of that counter - after receiving a response it compares the global variable with its own copy. If they don't match the response is discarded, it means that a newer request has been submitted already. http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/lib/... File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/lib/... lib/filterComposer.js:72: return !(filter instanceof WhitelistFilter); For sake of consistency, it might make sense to move that functionality into whitelisting.js - e.g. isUrlWhitelisted(). http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/lib/... lib/filterComposer.js:94: { A call to isFrameWhitelisted(..., "DOCUMENT") is missing here - nothing can be blocked or hidden in a whitelisted frame.
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... background.js:559: msg.baseURL, sender.page, sender.frame On 2015/03/02 20:14:19, Wladimir Palant wrote: > Ten positional parameters in a function is certainly a reason to worry. Normally > you would turn the parameters into a dictionary but in this particular case - > maybe pass in msg and sender directly? How about reducing the number of arguments by grouping the element's attributes into an object? http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... File include.postload.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:401: getBlockableElementOrAncestor(element.parentElement, callback); On 2015/03/02 20:14:19, Wladimir Palant wrote: > This seems to be the right recipe to error out with a "too much recursion" > error, e.g. when this function is applied to an element deep inside the SVG > hierarchy. Chrome seems to allow really massive recursion (my test function > crashed only after 17800 recursive calls) but that isn't something to be relied > upon. Whenever we can simply iterate we should do that rather than making > recursive calls. Done. http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/incl... include.postload.js:448: unhighlightElement(currentElement); On 2015/03/02 20:14:19, Wladimir Palant wrote: > I don't think it is that simple. Mouse events can be generated very quickly, > sometimes even synchronously (e.g. if the handler on the webpage changes layout > in such a way that another element is moved under the mouse pointer). So by the > time you get a response from the background page it might be outdated already. > > One solution I see here is making only the most recent request count: a global > variable is used as counter for mouseOver requests. Before calling > getBlockableElementOrAncestor() this function stores a copy of that counter - > after receiving a response it compares the global variable with its own copy. If > they don't match the response is discarded, it means that a newer request has > been submitted already. I have a hard time verifying that the suggested logic with the counter is correct. How about just recording the last mouse event, bailing out in the callback if it changed? http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/lib/... File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/lib/... lib/filterComposer.js:72: return !(filter instanceof WhitelistFilter); On 2015/03/02 20:14:19, Wladimir Palant wrote: > For sake of consistency, it might make sense to move that functionality into > whitelisting.js - e.g. isUrlWhitelisted(). This function no longer exist. The code has been inlined, and some of it's variables are also used for other purposes now. http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/lib/... lib/filterComposer.js:94: { On 2015/03/02 20:14:19, Wladimir Palant wrote: > A call to isFrameWhitelisted(..., "DOCUMENT") is missing here - nothing can be > blocked or hidden in a whitelisted frame. Done.
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... background.js:559: msg.baseURL, sender.page, sender.frame On 2015/03/03 14:29:00, Sebastian Noack wrote: > How about reducing the number of arguments by grouping the element's attributes > into an object? That's eight parameters now - still way too many to be passed by position for my taste.
http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... File background.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5669283816275968/back... background.js:559: msg.baseURL, sender.page, sender.frame On 2015/03/03 14:40:31, Wladimir Palant wrote: > On 2015/03/03 14:29:00, Sebastian Noack wrote: > > How about reducing the number of arguments by grouping the element's > attributes > > into an object? > > That's eight parameters now - still way too many to be passed by position for my > taste. Fair enough.
LGTM http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/... File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/... lib/filterComposer.js:73: * @param {Frame} details.frame The frame containing the element I wonder whether JsDoc Toolkit will actually accept that parameter list if we ever let it work on this code...
http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/... File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/... lib/filterComposer.js:73: * @param {Frame} details.frame The frame containing the element On 2015/03/03 19:17:01, Wladimir Palant wrote: > I wonder whether JsDoc Toolkit will actually accept that parameter list if we > ever let it work on this code... Why not? This is official JSDoc syntax: http://usejsdoc.org/tags-param.html#parameters-with-properties
http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/... File lib/filterComposer.js (right): http://codereview.adblockplus.org/5225119261655040/diff/5647308616105984/lib/... lib/filterComposer.js:73: * @param {Frame} details.frame The frame containing the element On 2015/03/03 19:18:54, Sebastian Noack wrote: > Why not? This is official JSDoc syntax: > http://usejsdoc.org/tags-param.html#parameters-with-properties Yes, seems to be supported by our version as well: https://code.google.com/p/jsdoc-toolkit/wiki/TagParam 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
