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

Issue 29761597: Issue 6538, 6781 - Implement script parsing for snippets (Closed)

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

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Move parseScript to top level #

Patch Set 3 : Support single character escape sequences #

Patch Set 4 : Support Unicode escape sequences #

Total comments: 1

Patch Set 5 : Rebase #

Total comments: 13

Patch Set 6 : Move singleCharacterEscapes to outer scope #

Patch Set 7 : Rename literal to withinQuotes #

Patch Set 8 : Add some comments #

Total comments: 3

Patch Set 9 : Revert Patch Set 8 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -1 line) Patch
M lib/snippets.js View 1 2 3 4 5 6 8 2 chunks +78 lines, -0 lines 0 comments Download
M test/snippets.js View 1 2 3 4 2 chunks +64 lines, -1 line 0 comments Download

Messages

Total messages: 20
Manish Jethani
2 months, 3 weeks ago (2018-04-25 17:25:39 UTC) #1
Manish Jethani
Patch Set 1 This builds on https://codereview.adblockplus.org/29737558/ See issue description for specification.
2 months, 3 weeks ago (2018-04-26 12:22:53 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29761597/diff/29761598/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761597/diff/29761598/lib/snippets.js#newcode83 lib/snippets.js:83: * @return {Array.<string[]>} I had to use Array.<string[]> instead ...
2 months, 3 weeks ago (2018-04-26 13:25:17 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29761597/diff/29761598/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29761597/diff/29761598/test/snippets.js#newcode152 test/snippets.js:152: checkParsedScript("Script with argument containing an escaped space", Also note ...
2 months, 3 weeks ago (2018-04-26 13:27:34 UTC) #4
Manish Jethani
Patch Set 3: Support single character escape sequences Since filters can't contain non-space whitespace because ...
2 months, 3 weeks ago (2018-04-26 15:36:39 UTC) #5
Manish Jethani
On 2018/04/26 15:36:39, Manish Jethani wrote: > Patch Set 3: Support single character escape sequences ...
2 months, 3 weeks ago (2018-04-26 15:45:12 UTC) #6
Manish Jethani
Patch Set 4: Support Unicode escape sequences Now you can write a filter like "example.com#$#foo ...
2 months, 3 weeks ago (2018-04-26 16:22:09 UTC) #7
Manish Jethani
On 2018/04/26 16:22:09, Manish Jethani wrote: > Patch Set 4: Support Unicode escape sequences > ...
2 months, 3 weeks ago (2018-04-26 16:23:16 UTC) #8
Manish Jethani
Patch Set 5: Rebase
1 month, 3 weeks ago (2018-05-23 04:14:28 UTC) #9
kzar
Copying in Erik and Sebastian since I think as many people as possible should review ...
1 week, 1 day ago (2018-07-10 13:55:21 UTC) #10
Manish Jethani
On 2018/07/10 13:55:21, kzar wrote: > Copying in Erik and Sebastian since I think as ...
1 week ago (2018-07-11 13:19:30 UTC) #11
Manish Jethani
Patch Set 6: Move singleCharacterEscapes to outer scope https://codereview.adblockplus.org/29761597/diff/29787583/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761597/diff/29787583/lib/snippets.js#newcode84 lib/snippets.js:84: function ...
1 week ago (2018-07-11 19:04:42 UTC) #12
Manish Jethani
Patch Set 7: Rename literal to withinQuotes Patch Set 8: Add some comments
1 week ago (2018-07-12 08:37:36 UTC) #13
kzar
Once you remove those extra comments this LGTM. I would like someone (ideally Sebastian) to ...
6 days, 22 hours ago (2018-07-12 10:30:37 UTC) #14
Manish Jethani
Patch Set 9: Revert Patch Set 8 On 2018/07/12 10:30:37, kzar wrote: > Once you ...
6 days, 22 hours ago (2018-07-12 10:57:49 UTC) #15
Sebastian Noack
I wonder if the implementation would be simpler (and more robust) if we'd just handle ...
2 days, 17 hours ago (2018-07-16 16:28:05 UTC) #16
Manish Jethani
On 2018/07/16 16:28:05, Sebastian Noack wrote: > I wonder if the implementation would be simpler ...
1 day, 22 hours ago (2018-07-17 10:47:23 UTC) #17
Sebastian Noack
On 2018/07/17 10:47:23, Manish Jethani wrote: > On 2018/07/16 16:28:05, Sebastian Noack wrote: > > ...
1 day, 21 hours ago (2018-07-17 12:24:13 UTC) #18
Manish Jethani
On 2018/07/17 12:24:13, Sebastian Noack wrote: > On 2018/07/17 10:47:23, Manish Jethani wrote: > > ...
1 day, 20 hours ago (2018-07-17 12:50:10 UTC) #19
Sebastian Noack
1 day, 18 hours ago (2018-07-17 14:56:26 UTC) #20
On 2018/07/17 12:50:10, Manish Jethani wrote:
> If you're suggesting that every argument, after adding the surrounding quotes,
> could be parsed as a JSON string, that somewhat makes sense.

Yes, this what I'm suggesting.

> But in order to
> extract the arguments we would still have to go through the entire script,
> character by character. If we're going to do that, we might as well do the
> parsing of the arguments based on our own spec?
> 
> There is more to JSON strings [1] than what we need in a snippet filter. JSON
is
> more strict. For example, a sole "\x" is an error, whereas in our spec we
simply
> treat it as an "x". We are more forgiving in what input we accept, which is
how
> it should be, since this is not a programming language and is going to be used
> by non-programmers (so it's more like HTML than JavaScript).
> 
> One more good reason not to implement JSON is that we're going to have to port
> this to C++ at some point and it would be really nice if we did not have the
> overhead of JSON.

Fair enough, LGTM.
Sign in to reply to this message.

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