JSHint
-
crx/js/modify_users.js (Diff revision 1) Show all issues
Review Request #9832 — Created March 25, 2018 and discarded
Since Firefox can install Chrome extensions directly, a seperate code base for Firefox is not required. Therefore I deleted the XPI code;
Try User test on Firefox and Chrome to make sure all features still works after the deletion.
Description | From | Last Updated |
---|---|---|
Typos/changes in description: noe -> not chrome -> Chrome Also, I'm not sure what "deleted most of them" means. Can … |
|
|
Testing done? |
|
|
Col: 17 'row' is already defined. |
![]() |
|
The changes to this file seem unrelated to the intent of this review request. Please fix up this change to … |
|
|
I think we can avoid doing the UA string detection here, and instead use something like: if (!browser) { var … |
|
|
Let's call this callbackFn instead. |
|
|
I'm reasonably certain that the Firefox implementation of the API also can take a callback function, can it not? |
|
|
Let's call this callbackFun instead. |
|
|
Same as above, re: captureVisibleTab. |
|
|
As before, I think we can skip the UA detection here. |
|
|
Again, I'm reasonably sure that the Firefox implementation can take a callback as well, and only optionally returns a Promise. |
|
|
Same as above. Also, there are some general formatting things that you need to watch out for here (indentation, spacing … |
|
|
Same as before, re: UA detection |
|
|
Same as before, re: UA detection |
|
|
trailing whitespace |
|
|
Why is this needed? |
|
|
Busted indentation here, and some trailing whitespace |
|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+70 -808) |
Typos/changes in description:
noe -> not
chrome -> ChromeAlso, I'm not sure what "deleted most of them" means. Can you explain that you've deleted the XPI code?
crx/js/background.js (Diff revision 2) |
---|
The changes to this file seem unrelated to the intent of this review request. Please fix up this change to not include them.
Description: |
|
||||||
---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||
Commit: |
|
||||||
Diff: |
Revision 3 (+98 -821) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+97 -821) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+105 -824) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+156 -840) |
Hey Ridum,
Since Firefox can not install Chrome extensions directly
I think you mean "Since Firefox can install Chrome extensions directly".
I have a bunch of notes here, mainly around UA detection and some formatting / stylistic stuff. Also, I think we can probably get rid of the crx folder and move everything into the root folder, can we not? Or perhaps just have a generic "extension" folder?
crx/js/background.js (Diff revision 6) |
---|
I think we can avoid doing the UA string detection here, and instead use something like:
if (!browser) { var browser = chrome; }
crx/js/background.js (Diff revision 6) |
---|
I'm reasonably certain that the Firefox implementation of the API also can take a callback function, can it not?
crx/js/modify_users.js (Diff revision 6) |
---|
Again, I'm reasonably sure that the Firefox implementation can take a callback as well, and only optionally returns a Promise.
crx/js/modify_users.js (Diff revision 6) |
---|
Same as above. Also, there are some general formatting things that you need to watch out for here (indentation, spacing around braces, trailing whitespace).
Description: |
|
||||||
---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 7 (+149 -846) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+132 -847) |