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

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