Delete Firefox installation codebase

Review Request #9832 — Created March 25, 2018 and updated

ridum
rb-screenshot
master
77a3f89...
rb-extension-pack, students

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 ...

daviddavid

Testing done?

daviddavid

Col: 17 'row' is already defined.

reviewbotreviewbot

The changes to this file seem unrelated to the intent of this review request. Please fix up this change to ...

daviddavid

I think we can avoid doing the UA string detection here, and instead use something like: if (!browser) { var ...

mike_conleymike_conley

Let's call this callbackFn instead.

mike_conleymike_conley

I'm reasonably certain that the Firefox implementation of the API also can take a callback function, can it not?

mike_conleymike_conley

Let's call this callbackFun instead.

mike_conleymike_conley

Same as above, re: captureVisibleTab.

mike_conleymike_conley

As before, I think we can skip the UA detection here.

mike_conleymike_conley

Again, I'm reasonably sure that the Firefox implementation can take a callback as well, and only optionally returns a Promise.

mike_conleymike_conley

Same as above. Also, there are some general formatting things that you need to watch out for here (indentation, spacing ...

mike_conleymike_conley

Same as before, re: UA detection

mike_conleymike_conley

Same as before, re: UA detection

mike_conleymike_conley

trailing whitespace

mike_conleymike_conley

Why is this needed?

mike_conleymike_conley

Busted indentation here, and some trailing whitespace

mike_conleymike_conley
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

RI
david
  1. 
      
  2. Typos/changes in description:

    noe -> not
    chrome -> Chrome

    Also, I'm not sure what "deleted most of them" means. Can you explain that you've deleted the XPI code?

    1. I said I deleted most of them bacause I am not sure if I have deleted all of them (there might be some configuration remaining XPI staff hiding in the gulp that I did not found.). But I am sure all the installation and parsing materials of XPI are deleted

  3. Testing done?

  4. 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.

  5. 
      
RI
RI
RI
RI
mike_conley
  1. 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?

    1. I put everything in the content folder now.
      I do feel like a content folder is needed to seperate the extension contents from git/building folders.

  2. 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;
    }
    
    1. I might still need the UA string detection here since it is needed to determine whether using callback or promise.

  3. crx/js/background.js (Diff revision 6)
     
     

    Let's call this callbackFn instead.

  4. 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?

    1. So I did a research about this :
      According to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/captureVisibleTab, browser.tabs.captureVisibleTab() only return a promise and cannot take a call back function as an argument.

      However, according to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API,
      "As a porting aid, the Firefox implementation of WebExtensions APIs supports chrome and callbacks as well as browser and promises."

      So ideally for porting purpose if we let browser = chrome we could use callback function, but I feel weird for letting a Firefox running API under chrome namespace (despite that Firefox allow it). I could switch back if it is not the case.

    2. It is
      https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/captureVisibleTab ,
      https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API ,respectively,
      The UI seem include the last common in the URL

  5. crx/js/background.js (Diff revision 6)
     
     

    Let's call this callbackFun instead.

  6. crx/js/background.js (Diff revision 6)
     
     
     
     
     
     

    Same as above, re: captureVisibleTab.

  7. crx/js/modify_users.js (Diff revision 6)
     
     
     
     
     

    As before, I think we can skip the UA detection here.

  8. 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.

  9. 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).

  10. crx/js/popup.js (Diff revision 6)
     
     
     
     
     

    Same as before, re: UA detection

  11. crx/js/save_user.js (Diff revision 6)
     
     
     
     

    Same as before, re: UA detection

  12. crx/js/save_user.js (Diff revision 6)
     
     

    trailing whitespace

  13. crx/manifest.json (Diff revision 6)
     
     

    Why is this needed?

    1. My bad, I changed it to testing the event but forget to change it back.

  14. crx/manifest.json (Diff revision 6)
     
     
     
     

    Busted indentation here, and some trailing whitespace

  15. 
      
RI
RI
Review request changed

Commit:

-df7f385553c8faf2010574c6b805888760dd3ee9
+77a3f8991ba73c97f9ee00595e7da6e131a6f5e6

Diff:

Revision 8 (+132 -847)

Show changes

Checks run (1 succeeded, 1 failed with error)

flake8 passed.
JSHint internal error.
Loading...