Require HTTPS before trusting publisher payment information#109
Require HTTPS before trusting publisher payment information#109da2x wants to merge 2 commits intohaystack:masterfrom da2x:https-policy
Conversation
| } | ||
| } if (req.status == 304) { | ||
| info = localStorage.getItem(document.domain); | ||
| if (info !== undefined || info !== '') { |
There was a problem hiding this comment.
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.
(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.
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) |
2 participants
Resolves issue #104 (which has the details about this).
(I also snuck in support for HTTP 304 caching.)