Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(114)

Issue 5468762555809792: Issue 1601 - Generate blocking filters for all URLs associated with the selected element (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 2 months ago by Sebastian Noack
Modified:
5 years, 2 months ago
Reviewers:
Wladimir Palant, kzar
Visibility:
Public.

Description

Issue 1601 - Generate blocking filters for all URLs associated with the selected element

Patch Set 1 : #

Total comments: 13

Patch Set 2 : Also consider "poster" attribute #

Total comments: 2

Patch Set 3 : Addressed comments #

Patch Set 4 : Simplified code #

Total comments: 13

Patch Set 5 : Rebased, addressed comments, and restructured code #

Total comments: 5

Patch Set 6 : Fixed wrong comparision operator #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -44 lines) Patch
M include.postload.js View 1 2 3 4 5 8 chunks +117 lines, -44 lines 0 comments Download

Messages

Total messages: 17
Sebastian Noack
5 years, 2 months ago (2014-11-24 15:22:54 UTC) #1
kzar
Not finished looking through this one yet (finding it quite hard to grok everything) but ...
5 years, 2 months ago (2014-11-25 18:10:10 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js#newcode395 include.postload.js:395: var addSelector = function(selector) On 2014/11/25 18:10:11, kzar wrote: ...
5 years, 2 months ago (2014-11-25 19:01:15 UTC) #3
Sebastian Noack
http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js#newcode481 include.postload.js:481: urls.splice(i--, 1); On 2014/11/25 19:01:15, Sebastian Noack wrote: > ...
5 years, 2 months ago (2014-11-25 19:03:11 UTC) #4
kzar
http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js#newcode544 include.postload.js:544: function hasFilters(element) Maybe a name like "isFilterable" would be ...
5 years, 2 months ago (2014-11-26 10:51:22 UTC) #5
Sebastian Noack
http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5718998062727168/include.postload.js#newcode544 include.postload.js:544: function hasFilters(element) On 2014/11/26 10:51:22, kzar wrote: > Maybe ...
5 years, 2 months ago (2014-11-26 11:08:14 UTC) #6
kzar
LGTM apart from the accidentally included bubbling / capturing changes from the other review. I ...
5 years, 2 months ago (2014-11-26 11:36:17 UTC) #7
Sebastian Noack
http://codereview.adblockplus.org/5468762555809792/diff/5697982787747840/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5697982787747840/include.postload.js#newcode312 include.postload.js:312: document.removeEventListener("keydown", clickHide_keyDown, true); On 2014/11/26 11:36:17, kzar wrote: > ...
5 years, 2 months ago (2014-11-26 11:42:48 UTC) #8
Wladimir Palant
Frankly, I don't think that this effort is justified. To get all the possible URLs ...
5 years, 2 months ago (2014-11-26 13:19:17 UTC) #9
Sebastian Noack
I agree that associating webRequest calls with elements would be a better way, however as ...
5 years, 2 months ago (2014-11-26 13:43:49 UTC) #10
Sebastian Noack
http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js#newcode509 include.postload.js:509: A fair amount of complexity here, only came from ...
5 years, 2 months ago (2014-11-26 14:01:21 UTC) #11
kzar
On 2014/11/26 13:19:17, Wladimir Palant wrote: > Frankly, I don't think that this effort is ...
5 years, 2 months ago (2014-11-26 14:33:23 UTC) #12
Wladimir Palant
http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js#newcode187 include.postload.js:187: var url = element.getAttribute("data"); For reference: <object> also has ...
5 years, 2 months ago (2014-12-08 11:43:00 UTC) #13
Sebastian Noack
Moreover, I restructured getURLsFromElement() into multiple functions for better readability. http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js#newcode187 ...
5 years, 2 months ago (2014-12-08 16:45:04 UTC) #14
Wladimir Palant
http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js#newcode187 include.postload.js:187: var url = element.getAttribute("data"); On 2014/12/08 16:45:04, Sebastian Noack ...
5 years, 2 months ago (2014-12-08 20:31:09 UTC) #15
Sebastian Noack
http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/5468762555809792/diff/5196828823781376/include.postload.js#newcode187 include.postload.js:187: var url = element.getAttribute("data"); On 2014/12/08 20:31:09, Wladimir Palant ...
5 years, 2 months ago (2014-12-08 21:12:49 UTC) #16
Wladimir Palant
5 years, 2 months ago (2014-12-09 08:27:21 UTC) #17
LGTM

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

http://codereview.adblockplus.org/5468762555809792/diff/5703128158568448/incl...
include.postload.js:246: urls.push.apply(urls, getURLsFromAttributes(child));
On 2014/12/08 21:12:49, Sebastian Noack wrote:
> If I didn't misread the standard, a <track> element can't have <track> or
> <source> child elements. Neither did I find any examples doing that. So why do
> you think so?

It seems that I indeed misunderstood the standard, MDN clearly says that <track>
is an empty element.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5