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

Issue 29796555: Issue 6621 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by piscoi.georgiana
Modified:
1 month, 1 week ago
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue 6621 - Use IndexDB for filter storage on Edge

Patch Set 1 #

Total comments: 40

Patch Set 2 : Most of the changes from patch 1 #

Patch Set 3 : Comment improvements and a rename #

Total comments: 19

Patch Set 4 : Most of the changes from P3 #

Total comments: 6

Patch Set 5 : Remaining change from p3 and changes from p4 #

Patch Set 6 : Fixed tests #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -116 lines) Patch
M qunit/tests/ioIndexedDB.js View 1 2 3 4 5 6 chunks +137 lines, -116 lines 1 comment Download

Messages

Total messages: 17
piscoi.georgiana_gmail.com
2 months, 1 week ago (2018-06-01 14:46:16 UTC) #1
kzar
Thanks Geo, I'll take a look through this next week! In the mean time please ...
2 months, 1 week ago (2018-06-01 14:53:26 UTC) #2
Sebastian Noack
Hi Geo, thanks for the patch! The overall implementation looks great, but I have some ...
2 months, 1 week ago (2018-06-01 19:15:46 UTC) #3
kzar
Good work Geo, my comments are mostly "nits" (small stylistic things). https://codereview.adblockplus.org/29796555/diff/29796556/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): ...
2 months, 1 week ago (2018-06-04 11:06:08 UTC) #4
kzar
https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newcode17 metadata.edge:17: qunit/tests.js += qunit/tests/ioIndexedDB.js On 2018/06/04 11:06:07, kzar wrote: > ...
2 months, 1 week ago (2018-06-04 18:06:56 UTC) #5
piscoi.georgiana_gmail.com
Thank you for reviewing my initial code. I've replied to most of the comments. The ...
2 months, 1 week ago (2018-06-05 07:15:56 UTC) #6
kzar
https://codereview.adblockplus.org/29796555/diff/29799557/lib/io.js File lib/io.js (left): https://codereview.adblockplus.org/29796555/diff/29799557/lib/io.js#oldcode85 lib/io.js:85: * Copies a file. Nice, always good to remove ...
2 months, 1 week ago (2018-06-05 12:38:54 UTC) #7
piscoi.georgiana_gmail.com
https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge File metadata.edge (right): https://codereview.adblockplus.org/29796555/diff/29796556/metadata.edge#newcode17 metadata.edge:17: qunit/tests.js += qunit/tests/ioIndexedDB.js On 2018/06/04 18:06:56, kzar wrote: > ...
2 months ago (2018-06-11 07:02:45 UTC) #8
kzar
https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#newcode50 lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/11 07:02:45, geo wrote: > On ...
2 months ago (2018-06-12 10:46:17 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#newcode50 lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/12 10:46:16, kzar wrote: > On ...
2 months ago (2018-06-13 00:55:13 UTC) #10
piscoi.georgiana_gmail.com
https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js File lib/ioIndexedDB.js (right): https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#newcode50 lib/ioIndexedDB.js:50: return openDB(dbConfig) On 2018/06/13 00:55:11, Sebastian Noack wrote: > ...
2 months ago (2018-06-14 13:41:27 UTC) #11
Oleksandr
On 2018/06/14 13:41:27, geo wrote: > https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js > File lib/ioIndexedDB.js (right): > > https://codereview.adblockplus.org/29796555/diff/29799557/lib/ioIndexedDB.js#newcode50 > ...
2 months ago (2018-06-14 20:57:05 UTC) #12
Sebastian Noack
Also LGTM from my side.
2 months ago (2018-06-14 21:05:29 UTC) #13
kzar
LGTM, nice work. Please email a (squashed) patch to Sebastian or myself when you get ...
2 months ago (2018-06-15 08:30:42 UTC) #14
Sebastian Noack
I went ahead, downloading the patch from Rietveld and pushing it: https://hg.adblockplus.org/adblockpluschrome/rev/d97edcab2c18 Thanks for your ...
1 month, 4 weeks ago (2018-06-15 18:03:30 UTC) #15
kzar
Please could you open a new review for the test fix? This one should be ...
1 month, 1 week ago (2018-07-04 15:45:59 UTC) #16
piscoi.georgiana_gmail.com
1 month, 1 week ago (2018-07-05 09:37:39 UTC) #17
On 2018/07/04 15:45:59, kzar wrote:
> Please could you open a new review for the test fix? This one should be closed
> now since the change already landed.
> 
> Anyway I have one idea for an alternative approach, see the inline comment
> below.
> 
>
https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex...
> File qunit/tests/ioIndexedDB.js (right):
> 
>
https://codereview.adblockplus.org/29796555/diff/29822560/qunit/tests/ioIndex...
> qunit/tests/ioIndexedDB.js:21: const isEdge = info.platform == "edgehtml";
> How about something like this?
> 
>   let testEdge = require("info").platform == "edgehtml" ? QUnit.test :
> QUnit.skip;
> 
> Then we could replace the `test` calls with `testEdge`.

Yes, here is the new review https://codereview.adblockplus.org/29823569/
Sign in to reply to this message.

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