Summary

Delete Firefox installation codebase

Review Request #9832 — Created March 25, 2018 and discarded Sept. 14, 2021, 11:17 p.m.

Information

Owner

ridum

Repository

rb-screenshot

Branch

master

Bugs

Depends On

Reviewers

Groups

rb-extension-pack, students

People

Description

Since Firefox can install Chrome extensions directly, a seperate code base for Firefox is not required. Therefore I deleted the XPI code;

Testing Done

Try User test on Firefox and Chrome to make sure all features still works after the deletion.

Issues

Description	From	Last Updated
Typos/changes in description: noe -> not chrome -> Chrome Also, I'm not sure what "deleted most of them" means. Can …	david	March 27, 2018, 10:41 p.m.
Testing done?	david	March 27, 2018, 10:43 p.m.
Col: 17 'row' is already defined.	reviewbot	March 25, 2018, 11:26 a.m.
The changes to this file seem unrelated to the intent of this review request. Please fix up this change to …	david	March 28, 2018, 12:44 a.m.
I think we can avoid doing the UA string detection here, and instead use something like: if (!browser) { var …	mike_conley	April 3, 2018, 4:54 p.m.
Let's call this callbackFn instead.	mike_conley	April 3, 2018, 4:56 p.m.
I'm reasonably certain that the Firefox implementation of the API also can take a callback function, can it not?	mike_conley	April 3, 2018, 5:03 p.m.
Let's call this callbackFun instead.	mike_conley	April 3, 2018, 5:05 p.m.
Same as above, re: captureVisibleTab.	mike_conley	April 3, 2018, 5:05 p.m.
As before, I think we can skip the UA detection here.	mike_conley	April 3, 2018, 5:05 p.m.
Again, I'm reasonably sure that the Firefox implementation can take a callback as well, and only optionally returns a Promise.	mike_conley	April 3, 2018, 5:06 p.m.
Same as above. Also, there are some general formatting things that you need to watch out for here (indentation, spacing …	mike_conley	April 3, 2018, 5:11 p.m.
Same as before, re: UA detection	mike_conley	April 3, 2018, 5:11 p.m.
Same as before, re: UA detection	mike_conley	April 3, 2018, 5:11 p.m.
trailing whitespace	mike_conley	April 3, 2018, 5:11 p.m.
Why is this needed?	mike_conley	April 3, 2018, 5:19 p.m.
Busted indentation here, and some trailing whitespace	mike_conley	April 3, 2018, 5:20 p.m.

flake8 passed.

JSHint failed.

JSHint

crx/js/modify_users.js (Diff revision 1)
Show all issues
```
Col: 17
 'row' is already defined.
```

Commit:

-	d4fadb3c02e3df7b83928089acc24d82d923d019
+	5c658e6b95e4bc15f48524879b5f440229e4355d

Diff:

Revision 2 (+70 -808)

Show changes

	.gitignore
	gulpfile.js
	crx/js/background.js
	xpi/README.md
	xpi/chrome.manifest
	xpi/index.js
	xpi/package.json
	xpi/js/capture.js
	6 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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?

ridum March 28, 2018, 12:44 a.m.

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

Show all issues
```
Testing done?
```

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.

Description:

~		Since Firefox can noe install chrome extensions directly, a seperate code base for Firefox is not required. Therefore I deleted most of them;
	~	Since Firefox can not install Chrome extensions directly, a seperate code base for Firefox is not required. Therefore I deleted the XPI code;

Testing Done:

Try User test on Firefox and Chrome to make sure all features still works after the deletion.

Commit:

-	5c658e6b95e4bc15f48524879b5f440229e4355d
+	188d2615fa8e939d2d78f317a4ee6cc325a43462

Diff:

Revision 3 (+98 -821)

Show changes

	.gitignore
	gulpfile.js
	crx/manifest.json
	crx/js/background.js
	xpi/README.md
	xpi/chrome.manifest
	xpi/index.js
	xpi/package.json
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	188d2615fa8e939d2d78f317a4ee6cc325a43462
+	23bd0572d5b040c365ca6ae402b504dd02fb0fe6

Diff:

Revision 4 (+97 -821)

Show changes

	.gitignore
	gulpfile.js
	crx/manifest.json
	crx/js/background.js
	xpi/README.md
	xpi/chrome.manifest
	xpi/index.js
	xpi/package.json
	7 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

Commit:

-	23bd0572d5b040c365ca6ae402b504dd02fb0fe6
+	2d476ce0a249f2cffe4ed89d75bc07e2e1354786

Diff:

Revision 5 (+105 -824)

Show changes

	.gitignore
	gulpfile.js
	crx/manifest.json
	crx/js/background.js
	xpi/README.md
	xpi/chrome.manifest
	xpi/index.js
	xpi/package.json
	7 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

Commit:

-	2d476ce0a249f2cffe4ed89d75bc07e2e1354786
+	68c43e1667ba3a4297ff286f140d62ad39c7771c

Diff:

Revision 6 (+156 -840)

Show changes

	.gitignore
	gulpfile.js
	crx/manifest.json
	crx/js/background.js
	crx/js/modify_users.js
	crx/js/popup.js
	crx/js/save_user.js
	xpi/README.md
	10 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

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?

ridum April 3, 2018, 5:23 p.m.

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.

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;
}

ridum April 3, 2018, 5:23 p.m.

I might still need the UA string detection here since it is needed to determine whether using callback or promise.

crx/js/background.js (Diff revision 6)
Show all issues
```
Let's call this callbackFn instead.
```

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?

ridum April 3, 2018, 5:23 p.m.

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.

ridum April 3, 2018, 5:26 p.m.

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

crx/js/background.js (Diff revision 6)
Show all issues
```
Let's call this callbackFun instead.
```

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

Show all issues

Same as above, re: captureVisibleTab.

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

Show all issues

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

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.

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

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

Show all issues

Same as before, re: UA detection

crx/js/save_user.js (Diff revision 6)
Show all issues
```
Same as before, re: UA detection
```
crx/js/save_user.js (Diff revision 6)
Show all issues
```
trailing whitespace
```

crx/manifest.json (Diff revision 6)

Show all issues

Why is this needed?

ridum April 3, 2018, 5:23 p.m.

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

crx/manifest.json (Diff revision 6)

Show all issues

Busted indentation here, and some trailing whitespace

Description:

~		Since Firefox can not install Chrome extensions directly, a seperate code base for Firefox is not required. Therefore I deleted the XPI code;
	~	Since Firefox can install Chrome extensions directly, a seperate code base for Firefox is not required. Therefore I deleted the XPI code;

Commit:

-	68c43e1667ba3a4297ff286f140d62ad39c7771c
+	df7f385553c8faf2010574c6b805888760dd3ee9

Diff:

Revision 7 (+149 -846)

Show changes

	.gitignore
	gulpfile.js
	crx/manifest.json
	crx/js/background.js
	crx/js/capture.js
	crx/js/modify_users.js
	crx/js/popup.js
	crx/js/save_user.js
	13 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

Commit:

-	df7f385553c8faf2010574c6b805888760dd3ee9
+	77a3f8991ba73c97f9ee00595e7da6e131a6f5e6

Diff:

Revision 8 (+132 -847)

Show changes

	.gitignore
	AUTHORS.md
	gulpfile.js
	crx/manifest.json
	crx/js/background.js
	crx/js/capture.js
	crx/js/modify_users.js
	crx/js/popup.js
	14 more

Checks run (1 succeeded, 1 failed with error)

flake8 passed.

JSHint internal error.

Review Board 6.0.1 alpha 0 (dev)

Delete Firefox installation codebase

Information

Reviewers

JSHint

Commit:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 timed out)

Commit:

Diff:

Checks run (2 timed out)

Commit:

Diff:

Checks run (2 timed out)

Description:

Commit:

Diff:

Checks run (2 timed out)

Commit:

Diff:

Checks run (1 succeeded, 1 failed with error)

Status: Discarded