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

Issue 29761612: Issue 6538, 6781 - Implement script compilation for snippets (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by Manish Jethani
Modified:
10 hours, 49 minutes ago
Reviewers:
kzar, Sebastian Noack
CC:
sergei, hub, Erik
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

This builds on https://codereview.adblockplus.org/29761597/

Patch Set 1 #

Patch Set 2 : Clean up test #

Patch Set 3 : Test script execution as well #

Total comments: 5

Patch Set 4 : Move compileScript to top level #

Patch Set 5 : Rebase #

Total comments: 20
Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -1 line) Patch
M lib/snippets.js View 1 2 3 4 1 chunk +34 lines, -0 lines 17 comments Download
M test/snippets.js View 1 2 3 4 2 chunks +74 lines, -1 line 3 comments Download

Messages

Total messages: 16
Manish Jethani
2 months, 3 weeks ago (2018-04-25 20:03:48 UTC) #1
Manish Jethani
Patch Set 1 This builds on https://codereview.adblockplus.org/29737558/ and https://codereview.adblockplus.org/29761597/ See issue description for specification.
2 months, 3 weeks ago (2018-04-26 12:23:26 UTC) #2
Manish Jethani
Patch Set 2: Clean up test Patch Set 3: Test script execution as well
2 months, 3 weeks ago (2018-04-26 12:24:06 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29761612/diff/29761623/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29761623/lib/snippets.js#newcode149 lib/snippets.js:149: let imports = Object.create(null); There's an imports object and ...
2 months, 3 weeks ago (2018-04-26 13:46:01 UTC) #4
Manish Jethani
Patch Set 5: Rebase
1 month, 3 weeks ago (2018-05-23 04:18:40 UTC) #5
kzar
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#newcode174 lib/snippets.js:174: new Function("exports", library)(imports); If I got it right, `compileScript` ...
1 week, 1 day ago (2018-07-10 14:35:15 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#newcode174 lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/10 14:35:15, kzar wrote: > ...
6 days, 23 hours ago (2018-07-12 09:48:32 UTC) #7
kzar
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#newcode174 lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/12 09:48:32, Manish Jethani wrote: ...
6 days, 23 hours ago (2018-07-12 10:21:43 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#newcode174 lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/12 10:21:43, kzar wrote: > ...
6 days, 22 hours ago (2018-07-12 11:05:29 UTC) #9
kzar
LGTM, would you mind also taking a look at `compileScript` Sebastian? https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): ...
6 days, 22 hours ago (2018-07-12 11:19:56 UTC) #10
Manish Jethani
Thanks! I'll wait for Sebastian's feedback. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#newcode174 lib/snippets.js:174: new Function("exports", library)(imports); ...
6 days, 21 hours ago (2018-07-12 11:35:16 UTC) #11
Sebastian Noack
Can someone explain to me the concept of libraries, and how/where it will be used ...
2 days, 16 hours ago (2018-07-16 16:37:36 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#newcode174 lib/snippets.js:174: new Function("exports", library)(imports); So we are generating code that ...
2 days, 14 hours ago (2018-07-16 19:14:53 UTC) #13
Manish Jethani
On 2018/07/16 16:37:36, Sebastian Noack wrote: > Can someone explain to me the concept of ...
1 day, 13 hours ago (2018-07-17 19:57:15 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js#newcode223 test/snippets.js:223: test.equal(expected, actual); On 2018/07/17 19:57:15, Manish Jethani wrote: > ...
1 day, 13 hours ago (2018-07-17 20:10:26 UTC) #15
Manish Jethani
10 hours, 49 minutes ago (2018-07-18 22:42:50 UTC) #16
It looks like Sebastian won't have time for this.

Since Dave and I are both satisfied with this change, I'll close this now. If we
need to make any improvements, we can still do it before the next release as a
separate issue/patch.

Thanks!
Sign in to reply to this message.

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