-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Require HTTPS before trusting publisher payment information #109
base: master
Are you sure you want to change the base?
Conversation
@@ -109,6 +111,15 @@ export function parseTxt() { | |||
serialInfo.prevTime = Date.now().toString(); | |||
localStorage.setItem(document.domain, JSON.stringify(serialInfo)); | |||
} | |||
} if (req.status == 304) { | |||
info = localStorage.getItem(document.domain); | |||
if (info !== undefined || info !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too nested here. In the last else clause, there is another info = localStorage.getItem(document.domain)
, which also needs a check if info
is not empty. Actually, these steps could be packaged into {getInfo
, updateInfo
, initInfo
} (local operation) and syncInfo
(remote operation). The if (info != null)
check afterward can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don’t get emails from this repo, so I’m not notified of the review.) You’re complaining about existing code and not my code changes, right? I’m checking to make sure there is a locally stored item (cached information) before updating the prevTime
. All other uses of setItem
replaces the existing item where as this updates the item already in storage. I can refactor everything here, but that would be a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, to be precise. The diff algo / git-blame couldn't capture that the lines are being moved instead of rewritten. Yes, it was a cleanup request, and I could do it later as well once this PR is merged.
LGTM for https, LGTM for 304 (while the verbification stuff can be added later) |
(needs a rebase) |
Resolves issue #104 (which has the details about this).
(I also snuck in support for HTTP 304 caching.)