Delete Firefox installation codebase

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

Information

rb-screenshot
master

Reviewers

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. Show all issues

    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. Show all issues

    Testing done?

  4. crx/js/background.js (Diff revision 2)
     
     
    Show all issues

    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)
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Let's call this callbackFn instead.

  4. crx/js/background.js (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Let's call this callbackFun instead.

  6. crx/js/background.js (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    Same as above, re: captureVisibleTab.

  7. crx/js/modify_users.js (Diff revision 6)
     
     
     
     
     
    Show all issues

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

  8. crx/js/modify_users.js (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
     
     
    Show all issues

    Same as before, re: UA detection

  11. crx/js/save_user.js (Diff revision 6)
     
     
     
     
    Show all issues

    Same as before, re: UA detection

  12. crx/js/save_user.js (Diff revision 6)
     
     
    Show all issues

    trailing whitespace

  13. crx/manifest.json (Diff revision 6)
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    Busted indentation here, and some trailing whitespace

  15. 
      
RI
RI
david
Review request changed
Status:
Discarded