-
Notifications
You must be signed in to change notification settings - Fork 279
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
Update MouseManagerRemote.js #919
base: master
Are you sure you want to change the base?
Conversation
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't move this line. You need to wrap the entire module in a function like this.
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 have fixed the code and made the indentation, can you check and get back to me please
@@ -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) { | |||
|
|||
var testSessionStorage = "testSessionStorage"; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
No description provided.