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

Issue 29737561: Issue 6539, 6782 - Implement support for snippets (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 3 weeks ago by Manish Jethani
Modified:
2 months ago
Reviewers:
hub, Sebastian Noack, kzar
CC:
Felix Dahlke, mapx
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Depends on adblockpluscore hg:8e466288a0c3

Patch Set 1 #

Total comments: 10

Patch Set 2 : Use JSON.stringify #

Patch Set 3 : Load snippet library like a module #

Total comments: 4

Patch Set 4 : Add support for remote loading #

Total comments: 4

Patch Set 5 : Load public key off disk #

Patch Set 6 : Add link to Chromium bug #

Patch Set 7 : Check filter against current frame, polish up #

Patch Set 8 : Use adblockpluscore/lib/snippets.js for script parsing #

Patch Set 9 : Move script compilation to Core #

Patch Set 10 : Use top-level compileScript #

Patch Set 11 : Update snippets.js to include just one log function #

Total comments: 2

Patch Set 12 : Wrap tabs.executeScript #

Patch Set 13 : Ignore error when frame is removed #

Patch Set 14 : Update lib/cssInjection.js to use CodeInjectionFilter.code #

Patch Set 15 : Rebase #

Total comments: 5

Patch Set 16 : Rebase, simplify, execute script on "snippets.executeScripts" message #

Total comments: 15

Patch Set 17 : Rebase, use snippets library from core #

Patch Set 18 : Update adblockpluscore dependency #

Patch Set 19 : Merge elemhide and snippets #

Patch Set 20 : Remove elemhideWhitelisted temporary variable #

Total comments: 4

Patch Set 21 : Rename ContentFiltering to Content #

Total comments: 42

Patch Set 22 : Remove dependency update #

Patch Set 23 : Clean up messaging for content.applyFilters #

Patch Set 24 : Rename Content to ContentFiltering #

Patch Set 25 : Rename lib/scriptInjection.js to lib/contentFiltering.js #

Patch Set 26 : Fix formatting for content.applyFilters message #

Patch Set 27 : Add explanatory comment in catch block #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -22 lines) Patch
M composer.postload.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 21 22 23 2 chunks +2 lines, -2 lines 0 comments Download
M include.preload.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 5 chunks +13 lines, -9 lines 0 comments Download
M lib/contentFiltering.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 4 chunks +72 lines, -10 lines 0 comments Download
M metadata.chrome View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +2 lines, -1 line 0 comments Download
M polyfill.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 44
Manish Jethani
5 months, 3 weeks ago (2018-03-30 20:25:44 UTC) #1
Manish Jethani
Patch Set 1 https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#newcode32 lib/snippets.js:32: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) Using the fetch ...
5 months, 3 weeks ago (2018-03-30 20:33:15 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29737561/diff/29737562/snippets.js File snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/snippets.js#newcode2 snippets.js:2: hello() So if you want to invoke this snippet ...
5 months, 3 weeks ago (2018-03-30 20:36:03 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#newcode32 lib/snippets.js:32: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) So this is literally just a ...
5 months, 3 weeks ago (2018-03-30 20:39:25 UTC) #4
Manish Jethani
Patch Set 2: Use JSON.stringify https://codereview.adblockplus.org/29737561/diff/29737562/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/requestBlocker.js#newcode72 lib/requestBlocker.js:72: yield "ELEMHIDE"; Just rearranged ...
5 months, 3 weeks ago (2018-03-31 08:09:40 UTC) #5
Manish Jethani
Patch Set 3: Load snippet library like a module https://codereview.adblockplus.org/29737561/diff/29738565/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29738565/lib/snippets.js#newcode31 lib/snippets.js:31: ...
5 months, 3 weeks ago (2018-03-31 09:36:27 UTC) #6
Manish Jethani
Patch Set 4: Add support for remote loading https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js#newcode32 lib/snippets.js:32: // ...
5 months, 3 weeks ago (2018-03-31 12:06:03 UTC) #7
Manish Jethani
Patch Set 5: Load public key off disk
5 months, 3 weeks ago (2018-03-31 13:53:28 UTC) #8
kzar
(Adding Mapx to Cc at his request.)
5 months, 2 weeks ago (2018-04-03 08:59:16 UTC) #9
Manish Jethani
Patch Set 6: Add link to Chromium bug Patch Set 7: Check filter against current ...
5 months, 2 weeks ago (2018-04-03 10:47:10 UTC) #10
hub
shouldn't snippets.js and lib/snippets.js be in adblockpluscore instead?
5 months ago (2018-04-19 12:27:11 UTC) #11
Manish Jethani
On 2018/04/19 12:27:11, hub wrote: > shouldn't snippets.js and lib/snippets.js be in adblockpluscore instead? I've ...
4 months, 3 weeks ago (2018-04-26 12:24:48 UTC) #12
Manish Jethani
Patch Set 8: Use adblockpluscore/lib/snippets.js for script parsing Patch Set 9: Move script compilation to ...
4 months, 3 weeks ago (2018-04-26 12:26:02 UTC) #13
Manish Jethani
Patch Set 10: Use top-level compileScript Patch Set 11: Update snippets.js to include just one ...
4 months, 3 weeks ago (2018-04-26 16:32:42 UTC) #14
hub
https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection.js#newcode106 lib/scriptInjection.js:106: runAt: "document_start" I am getting multiple occurence of Unchecked ...
4 months, 3 weeks ago (2018-04-27 03:05:21 UTC) #15
Manish Jethani
Patch Set 12: Wrap tabs.executeScript Patch Set 13: Ignore error when frame is removed https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection.js ...
4 months, 3 weeks ago (2018-04-27 11:31:52 UTC) #16
hub
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js#newcode38 lib/scriptInjection.js:38: response => response.ok ? response.text() : "" you should ...
4 months, 1 week ago (2018-05-09 12:23:01 UTC) #17
Manish Jethani
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js#newcode38 lib/scriptInjection.js:38: response => response.ok ? response.text() : "" On 2018/05/09 ...
4 months, 1 week ago (2018-05-09 13:47:39 UTC) #18
hub
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js#newcode38 lib/scriptInjection.js:38: response => response.ok ? response.text() : "" On 2018/05/09 ...
4 months, 1 week ago (2018-05-09 14:27:08 UTC) #19
hub
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js#newcode107 lib/scriptInjection.js:107: }) shouldn't we log that the filter was applied ...
4 months, 1 week ago (2018-05-09 17:19:49 UTC) #20
Manish Jethani
Patch Set 16: Rebase, simplify, execute script on "snippets.executeScripts" message Comments inline. https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js ...
3 months, 4 weeks ago (2018-05-23 05:12:47 UTC) #21
Sebastian Noack
https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js#newcode595 include.preload.js:595: browser.runtime.sendMessage({type: "snippets.executeScripts"}); On 2018/05/23 05:12:45, Manish Jethani wrote: > ...
3 months, 4 weeks ago (2018-05-23 12:32:18 UTC) #22
kzar
https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js#newcode207 lib/scriptInjection.js:207: RegExpFilter.typeMap.DOCUMENT)) On 2018/05/23 05:12:46, Manish Jethani wrote: > Note: ...
2 months, 1 week ago (2018-07-10 14:52:01 UTC) #23
Sebastian Noack
With the current implementation, snippets would be injected as plain content scripts, i.e. run in ...
2 months, 1 week ago (2018-07-11 17:21:09 UTC) #24
Manish Jethani
On 2018/07/11 17:21:09, Sebastian Noack wrote: > With the current implementation, snippets would be injected ...
2 months, 1 week ago (2018-07-12 11:51:48 UTC) #25
Sebastian Noack
On 2018/07/12 11:51:48, Manish Jethani wrote: > Yes, I already ran into this issue when ...
2 months, 1 week ago (2018-07-12 13:40:33 UTC) #26
Manish Jethani
On 2018/07/12 13:40:33, Sebastian Noack wrote: > On 2018/07/12 11:51:48, Manish Jethani wrote: > > ...
2 months, 1 week ago (2018-07-13 15:02:29 UTC) #27
Manish Jethani
Patch Set 17: Rebase, use snippets library from core Patch Set 18: Update adblockpluscore dependency ...
2 months ago (2018-07-19 01:00:54 UTC) #28
Manish Jethani
Patch Set 20: Remove elemhideWhitelisted temporary variable
2 months ago (2018-07-19 01:13:51 UTC) #29
Manish Jethani
Patch Set 21: Rename ContentFiltering to Content
2 months ago (2018-07-19 01:22:53 UTC) #30
kzar
https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js#newcode215 lib/scriptInjection.js:215: }); On 2018/07/19 01:00:53, Manish Jethani wrote: > On ...
2 months ago (2018-07-19 10:37:13 UTC) #31
Manish Jethani
I'll get back after updating #6539 and #6782 with all the details.
2 months ago (2018-07-19 10:39:54 UTC) #32
kzar
> I'll get back after updating #6539 and #6782 with all the details. Thanks, in ...
2 months ago (2018-07-19 12:24:22 UTC) #33
Manish Jethani
https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js#newcode23 composer.postload.js:23: const {checkCollapse, contentFiltering, getURLsFromElement, typeMap} = window; On 2018/07/19 ...
2 months ago (2018-07-19 12:55:27 UTC) #34
kzar
https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js#newcode23 composer.postload.js:23: const {checkCollapse, contentFiltering, getURLsFromElement, typeMap} = window; On 2018/07/19 ...
2 months ago (2018-07-19 13:28:57 UTC) #35
Manish Jethani
Patch Set 22: Remove dependency update Patch Set 23: Clean up messaging for content.applyFilters Patch ...
2 months ago (2018-07-19 14:04:37 UTC) #36
Manish Jethani
Patch Set 26: Fix formatting for content.applyFilters message
2 months ago (2018-07-19 14:10:41 UTC) #37
kzar
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js#newcode199 lib/scriptInjection.js:199: catch (error) On 2018/07/19 14:04:34, Manish Jethani wrote: > ...
2 months ago (2018-07-19 14:52:36 UTC) #38
Manish Jethani
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js#newcode199 lib/scriptInjection.js:199: catch (error) On 2018/07/19 14:52:34, kzar wrote: > On ...
2 months ago (2018-07-19 15:33:39 UTC) #39
kzar
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js#newcode199 lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:33:38, Manish Jethani wrote: > ...
2 months ago (2018-07-19 15:37:46 UTC) #40
Manish Jethani
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js#newcode199 lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:37:45, kzar wrote: > On ...
2 months ago (2018-07-19 15:44:06 UTC) #41
Manish Jethani
Patch Set 27: Add explanatory comment in catch block https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js#newcode199 lib/scriptInjection.js:199: ...
2 months ago (2018-07-19 15:54:09 UTC) #42
kzar
LGTM https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js#newcode199 lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:54:08, Manish Jethani wrote: ...
2 months ago (2018-07-19 16:07:41 UTC) #43
Manish Jethani
2 months ago (2018-07-19 16:18:36 UTC) #44
On 2018/07/19 16:07:41, kzar wrote:
> LGTM

Thanks!

I'll commit this after patch #29833597.

Meanwhile I'll spend all of tomorrow updating all the issues to bring them up to
date with all the information about the current implementation in both core and
the web extension (#6538, #6539, #6781, #6782, and possibly others).
Sign in to reply to this message.

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