Update MouseManagerRemote.js #919
Conversation
humphd
left a comment
humphd
left a comment
There was a problem hiding this comment.
I've left some feedback. @gideonthomas can you look at this too?
| @@ -1,6 +1,21 @@ | |||
| /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, forin: true, maxerr: 50, regexp: true, bitwise: true */ | |||
| /* global addEventListener, removeEventListener, sessionStorage */ | |||
| (function(transport) { | |||
There was a problem hiding this comment.
Don't move this line. You need to wrap the entire module in a function like this.
There was a problem hiding this comment.
I have fixed the code and made the indentation, can you check and get back to me please
| /* global addEventListener, removeEventListener, sessionStorage */ | ||
| (function(transport) { | ||
|
|
||
| var testSessionStorage = "testSessionStorage"; |
There was a problem hiding this comment.
All these lines will need to be indented 4-spaces after you restore that function above.
| sessionStorage = window.sessionStorage.bind(window); | ||
| } catch(e) { | ||
| console.warn("[Bramble] Session storage not accessible for MouseManager."); | ||
|
|
There was a problem hiding this comment.
You are missing a closing }. It looks like you've never tested this code, since this would never work. You need to make sure your code a) works; b) passes linting before you submit a PR.
| setItem: noop | ||
| }; | ||
|
|
||
| try { |
There was a problem hiding this comment.
Let's add a comment above this that indicates that we're doing this in order to deal with security issues in browsers that don't allow third-party frames to use storage.
|
@teddypee, let us know when this is ready for a review. It looks like there are still some review comments that need to be addressed |
|
@teddypee this is still not right: |
|
@humphd Hey Dave any suggestions on what to do no next |
3 participants
No description provided.