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

Issue 29348698: NoIssue - Use localStorage for storing filters in current Windows Store version of ABP for Edge (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 8 months ago by Oleksandr
Modified:
3 years, 8 months ago
Reviewers:
Sebastian Noack, kzar
CC:
Wladimir Palant, Felix Dahlke
Visibility:
Public.

Description

NoIssue - Use localStorage for storing filters in current Windows Store version of ABP for Edge

Patch Set 1 #

Total comments: 3

Patch Set 2 : Display an alert when trying to save to full localStorage #

Total comments: 4

Patch Set 3 : Address the nits #

Patch Set 4 : Fallback to chrome.storage.local to read initialization data #

Total comments: 16

Patch Set 5 : Address the comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -19 lines) Patch
M lib/adblockplus.js View 1 2 3 4 1 chunk +35 lines, -18 lines 0 comments Download
M manifest.json View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 18
Oleksandr
3 years, 8 months ago (2016-07-27 08:18:54 UTC) #1
kzar
https://codereview.adblockplus.org/29348698/diff/29348699/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348699/lib/adblockplus.js#newcode364 lib/adblockplus.js:364: if (dataInJson != null) Perhaps just check if dataInJson ...
3 years, 8 months ago (2016-07-27 08:44:22 UTC) #2
Oleksandr
All valid points. Addressed.
3 years, 8 months ago (2016-07-27 09:31:23 UTC) #3
kzar
https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#newcode363 lib/adblockplus.js:363: entry = localStorage.getItem(key); You missed the `var`. https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#newcode382 lib/adblockplus.js:382: ...
3 years, 8 months ago (2016-07-27 10:08:15 UTC) #4
Oleksandr
3 years, 8 months ago (2016-07-27 10:34:07 UTC) #5
shoniko
https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348711/lib/adblockplus.js#newcode383 lib/adblockplus.js:383: callback(new Error("Subscription storage is full. Please remove some subscriptions ...
3 years, 8 months ago (2016-07-27 10:46:51 UTC) #6
kzar
LGTM What do you think Sebastian, it worth localising the message that's displayed when storage ...
3 years, 8 months ago (2016-07-27 10:47:47 UTC) #7
Sebastian Noack
That should be good enough, for the temporary version we currently have on the store. ...
3 years, 8 months ago (2016-07-27 12:23:37 UTC) #8
Oleksandr
3 years, 8 months ago (2016-07-27 16:12:12 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#newcode363 lib/adblockplus.js:363: var fileDoesNotExistMessage = "File doesn't exist"; Any reason to ...
3 years, 8 months ago (2016-07-27 16:18:16 UTC) #10
kzar
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#newcode363 lib/adblockplus.js:363: var fileDoesNotExistMessage = "File doesn't exist"; Nit: Don't see ...
3 years, 8 months ago (2016-07-27 16:22:03 UTC) #11
Oleksandr
Done. Man you guys are in sync :) https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#newcode369 lib/adblockplus.js:369: // ...
3 years, 8 months ago (2016-07-27 16:33:30 UTC) #12
kzar
LGTM
3 years, 8 months ago (2016-07-27 16:37:51 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#newcode369 lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 16:33:30, Oleksandr ...
3 years, 8 months ago (2016-07-27 16:45:43 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json File manifest.json (right): https://codereview.adblockplus.org/29348698/diff/29348738/manifest.json#newcode74 manifest.json:74: "version": "0.9.6.0", Don't you have to update the AppXManifest ...
3 years, 8 months ago (2016-07-27 17:08:52 UTC) #15
Oleksandr
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#newcode369 lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 16:45:43, Sebastian ...
3 years, 8 months ago (2016-07-27 18:23:40 UTC) #16
Sebastian Noack
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js File lib/adblockplus.js (right): https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#newcode369 lib/adblockplus.js:369: // Also check the chrome.storage.local On 2016/07/27 18:23:40, Oleksandr ...
3 years, 8 months ago (2016-07-27 18:29:43 UTC) #17
Sebastian Noack
3 years, 8 months ago (2016-07-27 21:33:16 UTC) #18
https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js
File lib/adblockplus.js (right):

https://codereview.adblockplus.org/29348698/diff/29348738/lib/adblockplus.js#...
lib/adblockplus.js:369: // Also check the chrome.storage.local
On 2016/07/27 18:29:43, Sebastian Noack wrote:
> On 2016/07/27 18:23:40, Oleksandr wrote:
> > On 2016/07/27 16:45:43, Sebastian Noack wrote:
> > > On 2016/07/27 16:33:30, Oleksandr wrote:
> > > > On 2016/07/27 16:18:16, Sebastian Noack wrote:
> > > > > Just to clarify, so this is for when migrating from a version that had
> > some
> > > > data
> > > > > previously stored in chrome.storage.local, or what's the scenario?
> > > > 
> > > > No, this is for the first run when there is no patterns.ini at all.  
> > > 
> > > Then I don't understand what this workaround does. If the file just
doesn't
> > > exist you get an error either way. So what's the difference?
> > 
> > I misdiagnosed it, you are right. It is only needed to make IO.readFile to
> > respond asynchronously, so setTimeout() would in fact work too. However this
> > does fix the issue as well, and it tries to not deviate from the original
code
> > too much, so I think this is fine as well. 
> 
> Well, using setTimeout() would be much simpler. Also note that currently, you
> only respond asynchronously if the data aren't found in localStorage which
still
> might cause similar issues under slightly different circumstances, I suppose.

(Not quite) LGTM but since the code as-is already passed QA, I don't want to
have yet another QA round for this, since we also run out of time.
Sign in to reply to this message.

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