Summary

Browser extension for posting screenshots to Review Board

Review Request #7741 — Created Oct. 27, 2015 and discarded Jan. 29, 2019, 1:40 p.m.

Information

Owner

jwu

Repository

rb-extension-pack

Branch

master

Bugs

Depends On

Reviewers

Groups

rb-extension-pack

People

Description

Browser extension for Firefox and Chrome that allows the user to take screenshots of their browser's web content and post it to a Review Board server.

The extension adds the Review Board logo as an icon to the browser's toolbar. Clicking on the icon allows you to select different options of capturing web content. Currently you can captural all visible web content and crop a portion of the visible web content.

Chrome

Can take screenshot of all visible web content
Can crop a section of visible web content
Can send screenshot to a Review Board user's review request
Can add/delete/modify users from the user.html page

Firefox

Can take screenshot of all visible web content
Can crop a section of visible web content
Can send screenshot to a Review Board user's review request
Can add/delete/modify users from the user.html page
Multiprocessing friendly

TO DO LIST

Asana Link

Basic Validation on table cell editing
Fix infinite loading when requesting to some servers
Fill in all table cells alert message
Make Crop option overlay current page and open new tab after cropping (Chrome)
Organize javascript folder into import libraries and non
Alert user if user.html page has unsaved information

Testing Done


Sent a screenshot to a review request on a local server (Firefox)
Sent a screenshot to a review request on the Review Board server (Firefox)
Tested addon in nightly build with multiprocessing enabled (Firefox)
Sent a screenshot to a review request on a local server (Chrome)
Sent a screenshot to a review request on the Review Board server (Chrome)

Files

Issues

Description	From	Last Updated
It's possible that you'll be one of several people who eventually contribute to this project. Might be best to say: …	mike_conley	Oct. 28, 2015, 9:05 p.m.
You can pull this out :P	brennie	Nov. 5, 2015, 6:20 a.m.
Please put a space after : for each of these.	mike_conley	Oct. 28, 2015, 9:05 p.m.
Busted indentation.	mike_conley	Nov. 7, 2015, 10:43 a.m.
Yeah, pretty sure you can get away with just a 90% height and width on the #screenshot element instead of …	mike_conley	Nov. 5, 2015, 6:20 a.m.
Can you explain this 0.9 thing?	mike_conley	Nov. 24, 2015, 4:02 p.m.
We should definitely do something more explicit to alert the user that the upload succeeded. Might also be worthwhile to …	mike_conley	Nov. 5, 2015, 6:18 a.m.
I think this can be simplified to: return $("#rr-select").val();	mike_conley	Nov. 5, 2015, 6:20 a.m.
Alternatively, instead of generic <div>'s, you could use the new <section> element that is part of HTML5.	mike_conley	Oct. 28, 2015, 9:13 p.m.
For <button>'s, you don't need type="button".	mike_conley	Oct. 28, 2015, 9 p.m.
Nit - space before = Also, it's usually best to use <label>'s for these things. Example: <label for="account-select">Account:</label> <select id="account-select"></select>	mike_conley	Oct. 28, 2015, 9:04 p.m.
Again, no need for type="button".	mike_conley	Oct. 28, 2015, 9:04 p.m.
Nit - single quotes.	mike_conley	Oct. 28, 2015, 9:05 p.m.
Mixed tabs and spaces.	brennie	Nov. 5, 2015, 6:13 a.m.
This can't be shared?	mike_conley	Nov. 5, 2015, 6:19 a.m.
Can you reindent your work with 4 spaces instead of tabs?	brennie	Oct. 28, 2015, 7:08 p.m.
I believe it's gulp style to namespace tasks with colons, e.g. firefox:js.	brennie	Nov. 5, 2015, 6:16 a.m.
Put this on previous line.	brennie	Nov. 5, 2015, 6:16 a.m.
Probably not necessary.	mike_conley	Nov. 5, 2015, 6:19 a.m.
This is, again, using synchronous access to the content to do something. We'll need to update this at some point …	mike_conley	Nov. 16, 2015, 5:24 p.m.
All of this, and anything that manipulates content directly like this, should probably go into an additional contentScript that sends …	mike_conley	Nov. 16, 2015, 5:24 p.m.
Wonky indentation.	brennie	Nov. 5, 2015, 6:17 a.m.
Pretty certain this can be shared, no?	mike_conley	Nov. 5, 2015, 6:19 a.m.
Can you have this file use 4 spaces instead of tabs.	brennie	Nov. 14, 2015, 6:16 a.m.
Can we just include the minified source?	brennie	Nov. 14, 2015, 6:23 a.m.
Single space indentation for HTML.	brennie	Nov. 14, 2015, 6:35 a.m.
Missing <meta charset="utf-8"> Also, indentation.	brennie	Nov. 14, 2015, 6:37 a.m.
Likewise.	brennie	Nov. 14, 2015, 6:37 a.m.
This should be the first element in <head/>	brennie	Nov. 14, 2015, 6:38 a.m.
Sorry to be pedantic, but I thought we were going to go with 4 spaces?	mike_conley	Nov. 15, 2015, 7 a.m.
4 space indentation in here please	mike_conley	Nov. 15, 2015, 7:06 a.m.
Leftover tab?	mike_conley	Nov. 15, 2015, 7:06 a.m.
What if there's no tabindex? Do we even really need a tabindex in this case?	mike_conley	Nov. 15, 2015, 7:07 a.m.
We probably shouldn't alert. Logging to the console is fine, but alerting is usually a bad idea.	mike_conley	Nov. 15, 2015, 7:07 a.m.
4 space indentation please	mike_conley	Nov. 15, 2015, 7:08 a.m.
Is this file still being used?	mike_conley	Nov. 15, 2015, 7:08 a.m.
Why? Is this leftovers? Why are we using both?	mike_conley	Nov. 24, 2015, 4:02 p.m.
I think I'd prefer: if (users && users.length && users.length != table.rows.length && !modified)	mike_conley	Nov. 15, 2015, 7:13 a.m.
Is this still being used?	mike_conley	Nov. 15, 2015, 7:13 a.m.
Each of these functions should have a quick bit of documentation above to describe what exactly they do, what parameters …	mike_conley	Nov. 16, 2015, 3:36 p.m.
I'm curious why we're sticking with a numerical ID... doesn't that make it (cognitively) more difficult to map it to …	mike_conley	Nov. 16, 2015, 3:44 p.m.
space after if	mike_conley	Nov. 16, 2015, 3:45 p.m.
Best to use textContent here instead of innerHTML.	mike_conley	Nov. 16, 2015, 3:46 p.m.
This can be simplified to: option.value = serverDropdown.options.length; Even better though is to set the value to the URL itself? …	mike_conley	Nov. 21, 2015, 10:26 p.m.
Space before (	mike_conley	Nov. 21, 2015, 10:28 p.m.
Space before (	mike_conley	Nov. 21, 2015, 10:28 p.m.
Might as well simplify this: var allFields = $([apiKey, username, server]);	mike_conley	Nov. 21, 2015, 10:28 p.m.
This doesn't appear to be used.	mike_conley	Nov. 21, 2015, 10:28 p.m.
This should be indented 1 space. Also, replace "Reviewboard" with "Review Board"	mike_conley	Nov. 21, 2015, 10:28 p.m.
Instead of adding an event listener to every anchor element, you should add an event listener on a parent element …	mike_conley	Nov. 21, 2015, 10:28 p.m.
Bit strange to have a switch for just one option. Should probably just be an if condition.	mike_conley	Nov. 21, 2015, 10:28 p.m.
"review board" -> "Review Board"	mike_conley	Nov. 21, 2015, 10:28 p.m.
What calls this?	mike_conley	Nov. 21, 2015, 9:34 a.m.
This is not going to work with multi-process Firefox, as you'll have no access to the contentWindow.	mike_conley	Nov. 21, 2015, 9:32 a.m.
Same as above, re: adding an event listener to a parent and inspecting the event target id.	mike_conley	Nov. 21, 2015, 10:28 p.m.
"review board" -> "Review Board". Maybe take a TODO down to have all of this project metadata come from one …	mike_conley	Nov. 21, 2015, 10:29 p.m.

Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/jquery-ui.css
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/xpi/popup.html
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/crx/popup.html
    browser_screenshot/content/images/ui-icons_222222_256x240.png
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/screenshot.html
    browser_screenshot/AUTHORS.md
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/gulpfile.js
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/.gitignore
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/xpi/README.md
    browser_screenshot/LICENSE
    browser_screenshot/xpi/index.js
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/screenshot.less



Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/jquery-ui.css
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/xpi/popup.html
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/crx/popup.html
    browser_screenshot/content/images/ui-icons_222222_256x240.png
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/screenshot.html
    browser_screenshot/AUTHORS.md
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/gulpfile.js
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/.gitignore
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/xpi/README.md
    browser_screenshot/LICENSE
    browser_screenshot/xpi/index.js
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/screenshot.less

Can you attach the images you're including? They don't show up in the diff.

browser_screenshot/gulpfile.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you reindent your work with 4 spaces instead of tabs?
```

Added Files:

This is a really great start, Justin!

I'll have more comments soon, once I've had another few nights to look at this.

browser_screenshot/LICENSE (Diff revision 1)

The issue has been resolved. Show all issues

It's possible that you'll be one of several people who eventually contribute to this project. Might be best to say:

and then put your name in the AUTHORS file.

Also, we should start this with "The MIT License (MIT)"

browser_screenshot/content/css/popup.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Please put a space after : for each of these.
```
browser_screenshot/content/css/screenshot.less (Diff revision 1)
The issue has been resolved. Show all issues
```
Busted indentation.
```
1. JW
  
  jwu Oct. 28, 2015, 9:14 p.m.
  Sorry, what do you mean by busted indentation?
2. mike_conley Nov. 3, 2015, 5:07 p.m.
  I hopefully made this clear in Slack the other day.

browser_screenshot/content/js/screenshot.js (Diff revision 1)

The issue has been dropped. Show all issues

Can you explain this 0.9 thing?

jwu Oct. 28, 2015, 9:14 p.m.

Since the screenshot will be the size of the browser's web content, it would be too tall to fit on the screenshot page. This resizes the image down to 90% of the original size so it can fit on the screen.

mike_conley Nov. 3, 2015, 5:07 p.m.

Ah, I see. Might be better than to set the width and height of the #screenshot element to 90% in the CSS file.

jwu Nov. 5, 2015, 6:44 a.m.

It also helped for the cropped image as well since if we resized the element to 90% in the CSS file then the image would be resized in the browser but the actual image size would still be the same. This caused the coordinates obtained from the crop of the (smaller) image to be different than the actual image.
I would like to make it so the crop function doesn't open a new tab and just puts a crop overlay over the current webcontent, I believe this is how you envisioned it in the hackpad. This would solve the * 0.9 problem and then we could set the element to 90%.

browser_screenshot/content/screenshot.html (Diff revision 1)

The issue has been resolved. Show all issues

Alternatively, instead of generic <div>'s, you could use the new <section> element that is part of HTML5.

browser_screenshot/content/screenshot.html (Diff revision 1)
The issue has been resolved. Show all issues
```
For <button>'s, you don't need type="button".
```

browser_screenshot/content/screenshot.html (Diff revision 1)

The issue has been resolved. Show all issues

Nit - space before =
Also, it's usually best to use <label>'s for these things. Example:

<label for="account-select">Account:</label>
<select id="account-select"></select>

browser_screenshot/content/screenshot.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Again, no need for type="button".
```
browser_screenshot/content/screenshot.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit - single quotes.
```

browser_screenshot/README.md (Diff revision 1)
The issue has been resolved. Show all issues
```
You can pull this out :P
```
browser_screenshot/crx/js/popup.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Mixed tabs and spaces.
```
browser_screenshot/gulpfile.js (Diff revision 1)
The issue has been resolved. Show all issues
```
I believe it's gulp style to namespace tasks with colons, e.g. firefox:js.
```
browser_screenshot/gulpfile.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Put this on previous line.
```
browser_screenshot/xpi/js/save_user.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Wonky indentation.
```

browser_screenshot/content/js/screenshot.js (Diff revision 1)

The issue has been resolved. Show all issues

Yeah, pretty sure you can get away with just a 90% height and width on the #screenshot element instead of actually resizing the image.

JW

jwu Nov. 5, 2015, 6:44 a.m.
```
See above.
```

browser_screenshot/content/js/screenshot.js (Diff revision 1)

The issue has been resolved. Show all issues

We should definitely do something more explicit to alert the user that the upload succeeded.

Might also be worthwhile to emit progress data somewhere with a progress bar while the upload occurs.

jwu Nov. 5, 2015, 6:44 a.m.

There is currently a GIF loading image when AJAX requests are made and a "toast" is shown for alerting the user (as seen in the demo video).

browser_screenshot/content/js/screenshot.js (Diff revision 1)
The issue has been resolved. Show all issues
```
I think this can be simplified to:
return $("#rr-select").val();
```

browser_screenshot/crx/popup.html (Diff revision 1)

The issue has been resolved. Show all issues

This can't be shared?

JW

jwu Nov. 5, 2015, 6:44 a.m.
```
See below.
```

browser_screenshot/xpi/index.js (Diff revision 1)

So right away, I can tell this add-on is not going to be multi-process friendly out of the box, because this script (which runs in the parent process) is synchronously accessing the contentWindow for the selected browser.

That's probably okay for now, but in the future, we're probably going to need to send a message down to the page content, and wait for the dataURL to come back from it.

browser_screenshot/xpi/index.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Probably not necessary.
```

browser_screenshot/xpi/index.js (Diff revision 1)

The issue has been resolved. Show all issues

This is, again, using synchronous access to the content to do something. We'll need to update this at some point to be multiprocess friendly.

browser_screenshot/xpi/index.js (Diff revision 1)

The issue has been resolved. Show all issues

All of this, and anything that manipulates content directly like this, should probably go into an additional contentScript that sends messages up to the parent to do things like persistence.

browser_screenshot/xpi/popup.html (Diff revision 1)

The issue has been resolved. Show all issues

Pretty certain this can be shared, no?

jwu Nov. 5, 2015, 6:44 a.m.

Previously, there was an error with having the same file as Firefox needs to set the popup.html script with panels.Panel() while Chrome needs to set the script in the actual html file. This caused a bug where some user information was being added twice in Firefox since the script was linked both in panels.Panel() and in popup.html. However, I just tried reproducing the bugs and I can't seem to reproduce them so I've merged the files for now.

Description:

		Browser extension for Firefox and Chrome that allows the user to take screenshots of their browser's web content and post it to a Review Board server.

		The extension adds the Review Board logo as an icon to the browser's toolbar. Clicking on the icon allows you to select different options of capturing web content. Currently you can captural all visible web content and crop a portion of the visible web content.

		Chrome

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
	+	Can add/delete/modify users from the user.html page

		Firefox

		Can take screenshot of all visible web content
~		Can send screenshot to a Review Board user's review request
	~	Can crop a section of visible web content
	+	Can send screenshot to a Review Board user's review request
	+
	+	TO DO LIST
	+
	+	Asana Link
	+
	+	Basic Validation on table cell editing
	+	Install Font-Awesome using just LESS
	+	Fix copy and paste style (does not remove text styling)
	+	Fix infinite loading when requesting to some servers
	+	Fill in all table cells alert message
	+	Link up table file with browser extension (Firefox)
	+	Fix CSS issues
	+	Make Crop option overlay current page and open new tab after cropping
	+	Firefox --> multiprocess friendly

Diff:

Revision 2 (+8280)

Show changes

	browser_screenshot/.eslintrc
	browser_screenshot/.gitignore
	browser_screenshot/AUTHORS.md
	browser_screenshot/CONTRIBUTING.md
	browser_screenshot/LICENSE
	browser_screenshot/README.md
	browser_screenshot/gulpfile.js
	browser_screenshot/content/popup.html
	71 more

Added Files:

Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/font-awesome/scss/_rotated-flipped.scss
    browser_screenshot/content/css/jquery-ui.css
    browser_screenshot/content/font-awesome/less/font-awesome.less
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/font-awesome/scss/_core.scss
    browser_screenshot/content/font-awesome/less/fixed-width.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff2
    browser_screenshot/xpi/package.json
    browser_screenshot/content/font-awesome/scss/_animated.scss
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/js/users.js
    browser_screenshot/xpi/popup.html
    browser_screenshot/content/images/ajax-loader.gif
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.svg
    browser_screenshot/content/font-awesome/less/variables.less
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/font-awesome/scss/_path.scss
    browser_screenshot/content/font-awesome/less/bordered-pulled.less
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/font-awesome/fonts/FontAwesome.otf
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/crx/popup.html
    browser_screenshot/crx/js/popup.js
    browser_screenshot/content/images/ui-icons_222222_256x240.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/popup.html
    browser_screenshot/AUTHORS.md
    browser_screenshot/content/font-awesome/scss/_mixins.scss
    browser_screenshot/content/font-awesome/scss/_variables.scss
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/gulpfile.js
    browser_screenshot/content/font-awesome/less/animated.less
    browser_screenshot/content/font-awesome/scss/_fixed-width.scss
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/font-awesome/scss/_stacked.scss
    browser_screenshot/crx/js/all_content.js
    browser_screenshot/content/font-awesome/scss/font-awesome.scss
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/font-awesome/less/mixins.less
    browser_screenshot/content/font-awesome/less/rotated-flipped.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/xpi/index.js
    browser_screenshot/content/font-awesome/scss/_icons.scss
    browser_screenshot/crx/manifest.json
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/content/font-awesome/less/larger.less
    browser_screenshot/content/font-awesome/scss/_bordered-pulled.scss
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/font-awesome/less/path.less
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/font-awesome/less/list.less
    browser_screenshot/content/font-awesome/less/stacked.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.ttf
    browser_screenshot/README.md
    browser_screenshot/content/font-awesome/less/core.less
    browser_screenshot/content/font-awesome/scss/_larger.scss
    browser_screenshot/xpi/README.md
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.eot
    browser_screenshot/LICENSE
    browser_screenshot/content/font-awesome/less/icons.less
    browser_screenshot/content/font-awesome/scss/_list.scss
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff
    browser_screenshot/content/font-awesome/css/font-awesome.css
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/content/font-awesome/css/font-awesome.min.css
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/css/users.css
    browser_screenshot/content/css/screenshot.less



Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/font-awesome/scss/_rotated-flipped.scss
    browser_screenshot/content/css/jquery-ui.css
    browser_screenshot/content/font-awesome/less/font-awesome.less
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/font-awesome/scss/_core.scss
    browser_screenshot/content/font-awesome/less/fixed-width.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff2
    browser_screenshot/xpi/package.json
    browser_screenshot/content/font-awesome/scss/_animated.scss
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/js/users.js
    browser_screenshot/xpi/popup.html
    browser_screenshot/content/images/ajax-loader.gif
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.svg
    browser_screenshot/content/font-awesome/less/variables.less
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/font-awesome/scss/_path.scss
    browser_screenshot/content/font-awesome/less/bordered-pulled.less
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/font-awesome/fonts/FontAwesome.otf
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/crx/popup.html
    browser_screenshot/crx/js/popup.js
    browser_screenshot/content/images/ui-icons_222222_256x240.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/popup.html
    browser_screenshot/AUTHORS.md
    browser_screenshot/content/font-awesome/scss/_mixins.scss
    browser_screenshot/content/font-awesome/scss/_variables.scss
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/gulpfile.js
    browser_screenshot/content/font-awesome/less/animated.less
    browser_screenshot/content/font-awesome/scss/_fixed-width.scss
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/font-awesome/scss/_stacked.scss
    browser_screenshot/crx/js/all_content.js
    browser_screenshot/content/font-awesome/scss/font-awesome.scss
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/font-awesome/less/mixins.less
    browser_screenshot/content/font-awesome/less/rotated-flipped.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/xpi/index.js
    browser_screenshot/content/font-awesome/scss/_icons.scss
    browser_screenshot/crx/manifest.json
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/content/font-awesome/less/larger.less
    browser_screenshot/content/font-awesome/scss/_bordered-pulled.scss
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/font-awesome/less/path.less
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/font-awesome/less/list.less
    browser_screenshot/content/font-awesome/less/stacked.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.ttf
    browser_screenshot/README.md
    browser_screenshot/content/font-awesome/less/core.less
    browser_screenshot/content/font-awesome/scss/_larger.scss
    browser_screenshot/xpi/README.md
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.eot
    browser_screenshot/LICENSE
    browser_screenshot/content/font-awesome/less/icons.less
    browser_screenshot/content/font-awesome/scss/_list.scss
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff
    browser_screenshot/content/font-awesome/css/font-awesome.css
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/content/font-awesome/css/font-awesome.min.css
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/css/users.css
    browser_screenshot/content/css/screenshot.less

Hey Justin,

I think if we can, we should try to land what you've currently got into rb-extension-pack, and then continue building on top of it.

So can you take this out of WIP and try polishing it up for landing? We can do your TODOs in follow-up review requests. Sound good?

jwu Nov. 10, 2015, 6:39 a.m.

Sounds good. I'll work on polishing it up this week then :)

browser_screenshot/.eslintrc (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you have this file use 4 spaces instead of tabs.
```
browser_screenshot/content/font-awesome/css/font-awesome.css (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we just include the minified source?
```
browser_screenshot/content/screenshot.html (Diff revision 2)
The issue has been resolved. Show all issues
```
Single space indentation for HTML.
```
browser_screenshot/content/screenshot.html (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing <meta charset="utf-8">

Also, indentation.
```
browser_screenshot/content/users.html (Diff revision 2)
The issue has been resolved. Show all issues
```
Likewise.
```
browser_screenshot/content/users.html (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be the first element in <head/>
```

Description:

		Browser extension for Firefox and Chrome that allows the user to take screenshots of their browser's web content and post it to a Review Board server.

		The extension adds the Review Board logo as an icon to the browser's toolbar. Clicking on the icon allows you to select different options of capturing web content. Currently you can captural all visible web content and crop a portion of the visible web content.

		Chrome

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
		Can add/delete/modify users from the user.html page

		Firefox

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
	+	Can add/delete/modify users from the user.html page

		TO DO LIST

		Asana Link

		Basic Validation on table cell editing
~		Install Font-Awesome using just LESS
~		Fix copy and paste style (does not remove text styling)
~		Fix infinite loading when requesting to some servers
~		Fill in all table cells alert message
	~	Fix infinite loading when requesting to some servers
	~	Fill in all table cells alert message
	~	Make Crop option overlay current page and open new tab after cropping
	~	Firefox --> multiprocess friendly
-		Link up table file with browser extension (Firefox)
-		Fix CSS issues
-		Make Crop option overlay current page and open new tab after cropping
-		Firefox --> multiprocess friendly

Diff:

Revision 3 (+9272)

Show changes

	browser_screenshot/.eslintrc
	browser_screenshot/.gitignore
	browser_screenshot/AUTHORS.md
	browser_screenshot/CONTRIBUTING.md
	browser_screenshot/LICENSE
	browser_screenshot/README.md
	browser_screenshot/gulpfile.js
	browser_screenshot/content/popup.html
	97 more

Removed Files:

Summary:

-	[WIP] Browser extension for posting screenshots to Review Board
+	Browser extension for posting screenshots to Review Board

Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/css/jquery-ui.css
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/font-awesome/less/font-awesome.less
    browser_screenshot/content/font-awesome/scss/_core.scss
    browser_screenshot/content/font-awesome/less/fixed-width.less
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/crx/js/background.js
    browser_screenshot/crx/js/all_content.js
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/font-awesome/less/rotated-flipped.less
    browser_screenshot/.eslintrc
    browser_screenshot/.gitignore
    browser_screenshot/crx/manifest.json
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/js/users.js
    browser_screenshot/content/font-awesome/less/bordered-pulled.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.svg
    browser_screenshot/content/font-awesome/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/crx/popup.html
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/AUTHORS.md
    browser_screenshot/content/font-awesome/scss/_variables.scss
    browser_screenshot/content/font-awesome/less/animated.less
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/content/font-awesome/scss/_icons.scss
    browser_screenshot/content/users.html
    browser_screenshot/content/font-awesome/less/list.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.ttf
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/font-awesome/less/core.less
    browser_screenshot/content/font-awesome/scss/_larger.scss
    browser_screenshot/content/font-awesome/scss/_list.scss
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/font-awesome/scss/_animated.scss
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/xpi/popup.html
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/images/ui-icons_222222_256x240.png
    browser_screenshot/content/font-awesome/less/path.less
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/content/images/ajax-loader.gif
    browser_screenshot/content/font-awesome/scss/_stacked.scss
    browser_screenshot/gulpfile.js
    browser_screenshot/content/font-awesome/scss/font-awesome.scss
    browser_screenshot/content/font-awesome/less/mixins.less
    browser_screenshot/content/font-awesome/less/larger.less
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/README.md
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/content/font-awesome/less/icons.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/crx/js/popup.js
    browser_screenshot/content/font-awesome/less/stacked.less
    browser_screenshot/content/css/screenshot.less
    browser_screenshot/content/font-awesome/scss/_rotated-flipped.scss
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/LICENSE
    browser_screenshot/xpi/package.json
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/font-awesome/less/variables.less
    browser_screenshot/content/font-awesome/scss/_path.scss
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/images/logo.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/font-awesome/scss/_fixed-width.scss
    browser_screenshot/content/popup.html
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/font-awesome/scss/_mixins.scss
    browser_screenshot/content/font-awesome/scss/_bordered-pulled.scss
    browser_screenshot/xpi/README.md
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.eot
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/content/font-awesome/css/font-awesome.css
    browser_screenshot/content/font-awesome/css/font-awesome.min.css
    browser_screenshot/content/css/users.css
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png



Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/css/jquery-ui.css
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/font-awesome/less/font-awesome.less
    browser_screenshot/content/font-awesome/scss/_core.scss
    browser_screenshot/content/font-awesome/less/fixed-width.less
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/crx/js/background.js
    browser_screenshot/crx/js/all_content.js
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/font-awesome/less/rotated-flipped.less
    browser_screenshot/.eslintrc
    browser_screenshot/.gitignore
    browser_screenshot/crx/manifest.json
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/js/users.js
    browser_screenshot/content/font-awesome/less/bordered-pulled.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.svg
    browser_screenshot/content/font-awesome/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/crx/popup.html
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/AUTHORS.md
    browser_screenshot/content/font-awesome/scss/_variables.scss
    browser_screenshot/content/font-awesome/less/animated.less
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/content/font-awesome/scss/_icons.scss
    browser_screenshot/content/users.html
    browser_screenshot/content/font-awesome/less/list.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.ttf
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/font-awesome/less/core.less
    browser_screenshot/content/font-awesome/scss/_larger.scss
    browser_screenshot/content/font-awesome/scss/_list.scss
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/font-awesome/scss/_animated.scss
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/xpi/popup.html
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/images/ui-icons_222222_256x240.png
    browser_screenshot/content/font-awesome/less/path.less
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/content/images/ajax-loader.gif
    browser_screenshot/content/font-awesome/scss/_stacked.scss
    browser_screenshot/gulpfile.js
    browser_screenshot/content/font-awesome/scss/font-awesome.scss
    browser_screenshot/content/font-awesome/less/mixins.less
    browser_screenshot/content/font-awesome/less/larger.less
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/README.md
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/content/font-awesome/less/icons.less
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.woff
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/crx/js/popup.js
    browser_screenshot/content/font-awesome/less/stacked.less
    browser_screenshot/content/css/screenshot.less
    browser_screenshot/content/font-awesome/scss/_rotated-flipped.scss
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/LICENSE
    browser_screenshot/xpi/package.json
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/font-awesome/less/variables.less
    browser_screenshot/content/font-awesome/scss/_path.scss
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/images/logo.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/font-awesome/scss/_fixed-width.scss
    browser_screenshot/content/popup.html
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/font-awesome/scss/_mixins.scss
    browser_screenshot/content/font-awesome/scss/_bordered-pulled.scss
    browser_screenshot/xpi/README.md
    browser_screenshot/content/font-awesome/fonts/fontawesome-webfont.eot
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/content/font-awesome/css/font-awesome.css
    browser_screenshot/content/font-awesome/css/font-awesome.min.css
    browser_screenshot/content/css/users.css
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png

I'll look into this more deeply tomorrow, but for now, I'm wondering about this FontAwesome library that I see in here. It looks like you've brought in the entire FontAwesome source code... I was under the impression that you'd just need the font itself and the CSS? If so, can we get rid of the rest of the files?

jwu Nov. 15, 2015, 7:22 a.m.

It seems I forgot to delete some extraneous files in my rb-extension-pack directory and copy over the hidden files. Sorry about that and please check the updated request.

(Updated Request does not have users.css, and the extra popup.html files.

browser_screenshot/.eslintrc (Diff revision 3)
The issue has been resolved. Show all issues
```
Sorry to be pedantic, but I thought we were going to go with 4 spaces?
```
browser_screenshot/content/css/users.css (Diff revision 3)
The issue has been resolved. Show all issues
```
4 space indentation in here please
```
browser_screenshot/content/css/users.css (Diff revision 3)
The issue has been resolved. Show all issues
```
Leftover tab?
```

browser_screenshot/content/popup.html (Diff revision 3)

The issue has been resolved. Show all issues

What if there's no tabindex? Do we even really need a tabindex in this case?

jwu Nov. 15, 2015, 7:22 a.m.

If there is no tabindex, focus is automatically given to the first element without a tabindex.

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

The issue has been resolved. Show all issues

We probably shouldn't alert. Logging to the console is fine, but alerting is usually a bad idea.

browser_screenshot/crx/manifest.json (Diff revision 3)
The issue has been resolved. Show all issues
```
4 space indentation please
```
browser_screenshot/crx/popup.html (Diff revision 3)
The issue has been resolved. Show all issues
```
Is this file still being used?
```

browser_screenshot/gulpfile.js (Diff revision 3)

The issue has been dropped. Show all issues

Why? Is this leftovers? Why are we using both?

jwu Nov. 15, 2015, 7:22 a.m.

The standalone imports in the first browserify task allows js/screenshot.js to have the standalone package screenshot, every time I've tried to combine the two tasks into one, screenshot.js loses its standalone pacakge.
I haven't been able to figure a way to combine them so that's why both are being used.

browser_screenshot/xpi/js/modify_users.js (Diff revision 3)

The issue has been resolved. Show all issues

I think I'd prefer:

if (users && users.length && users.length != table.rows.length && !modified)

browser_screenshot/xpi/popup.html (Diff revision 3)
The issue has been resolved. Show all issues
```
Is this still being used?
```

Diff:

Revision 4 (+2660)

Show changes

	browser_screenshot/.eslintrc
	browser_screenshot/.gitignore
	browser_screenshot/AUTHORS.md
	browser_screenshot/CONTRIBUTING.md
	browser_screenshot/LICENSE
	browser_screenshot/README.md
	browser_screenshot/gulpfile.js
	browser_screenshot/content/popup.html
	56 more

Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less



Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less

Still reviewing, but some more feedback here.

Do we also not need a package.json so that npm knows what other packages are required?

browser_screenshot/content/js/screenshot.js (Diff revision 4)

The issue has been resolved. Show all issues

Each of these functions should have a quick bit of documentation above to describe what exactly they do, what parameters they take, what they return, etc.

Example:

/**
 * Adds a server to the list of servers that the user
 * can attach the image to.
 *
 * @param server (String)
 *        The URL of the server to add.
 */

browser_screenshot/content/popup.html (Diff revision 4)

The issue has been resolved. Show all issues

I'm curious why we're sticking with a numerical ID... doesn't that make it (cognitively) more difficult to map it to the right action?

Perhaps change "1" to be "capture_visible_content", "2" to be "capture_area", "3" to be "accounts", etc.

browser_screenshot/xpi/js/modify_users.js (Diff revision 4)
The issue has been resolved. Show all issues
```
space after if
```
browser_screenshot/xpi/js/modify_users.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Best to use textContent here instead of innerHTML.
```

Diff:

Revision 5 (+3102)

Show changes

	browser_screenshot/.eslintrc
	browser_screenshot/.gitignore
	browser_screenshot/AUTHORS.md
	browser_screenshot/CONTRIBUTING.md
	browser_screenshot/LICENSE
	browser_screenshot/README.md
	browser_screenshot/gulpfile.js
	browser_screenshot/package.json
	59 more

Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/xpi/js/cs_screenshot.js
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/package.json
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/xpi/js/capture.js
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less



Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/xpi/js/cs_screenshot.js
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/package.json
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/xpi/js/capture.js
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less

Description:

		Browser extension for Firefox and Chrome that allows the user to take screenshots of their browser's web content and post it to a Review Board server.

		The extension adds the Review Board logo as an icon to the browser's toolbar. Clicking on the icon allows you to select different options of capturing web content. Currently you can captural all visible web content and crop a portion of the visible web content.

		Chrome

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
		Can add/delete/modify users from the user.html page

		Firefox

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
		Can add/delete/modify users from the user.html page
	+	Multiprocessing friendly

		TO DO LIST

		Asana Link

		Basic Validation on table cell editing
		Fix infinite loading when requesting to some servers
		Fill in all table cells alert message
		Make Crop option overlay current page and open new tab after cropping
-		Firefox --> multiprocess friendly

Testing Done:

		Sent a screenshot to a review request on a local server (Firefox)
		Sent a screenshot to a review request on the Review Board server (Firefox)
~		Sent a screenshot to a review request on a local server (Chrome)
~		Sent a screenshot to a review request on the Review Board server (Chrome)
	~	Tested addon in nightly build with multiprocessing enabled (Firefox)
	~	Sent a screenshot to a review request on a local server (Chrome)
	+	Sent a screenshot to a review request on the Review Board server (Chrome)

Diff:

Revision 6 (+3101)

Show changes

	browser_screenshot/.eslintrc
	browser_screenshot/.gitignore
	browser_screenshot/AUTHORS.md
	browser_screenshot/CONTRIBUTING.md
	browser_screenshot/LICENSE
	browser_screenshot/README.md
	browser_screenshot/gulpfile.js
	browser_screenshot/package.json
	59 more

Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/xpi/js/cs_screenshot.js
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/package.json
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/xpi/js/capture.js
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less



Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/xpi/js/cs_screenshot.js
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/package.json
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/xpi/js/capture.js
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less

As a first landing, I think this is pretty good! I think there are some UX and UI improvements to make, perhaps some simplifications in architecture[1], and perhaps there some more cleverness we can do to reduce the duplication between the crx/xpi folders, but as an initial landing I'm digging this.
[1]: At some point, we might want to try to combine some of these modules, unless there's a good reason to keep them separate. We might be able to reduce some of the functions that seem to just add another level of abstraction. We might also want to opt for top-level objects with function properties instead of top level functions. But I don't think we should worry about that for now - let's just stand this thing up and see how it feels, and then start filing bugs against it so we can start making improvements.

browser_screenshot/content/js/screenshot.js (Diff revision 6)

I guess this kind of magic is needed for jQuery selectmenus? Because traditional <select>'s normally size themselves to their largest child.

browser_screenshot/content/js/screenshot.js (Diff revision 6)
```
TIL this is a quick way of clearing out the list!
```

browser_screenshot/content/js/screenshot.js (Diff revision 6)

The issue has been resolved. Show all issues

This can be simplified to:

option.value = serverDropdown.options.length;

Even better though is to set the value to the URL itself? Or is there an advantage to the value being the index?

browser_screenshot/content/js/screenshot.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Space before (
```
browser_screenshot/content/js/screenshot.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Space before (
```

browser_screenshot/content/js/user_form.js (Diff revision 6)

The issue has been resolved. Show all issues

Might as well simplify this:

var allFields = $([apiKey, username, server]);

browser_screenshot/content/js/user_form.js (Diff revision 6)
The issue has been resolved. Show all issues
```
This doesn't appear to be used.
```

browser_screenshot/content/screenshot.html (Diff revision 6)

The issue has been resolved. Show all issues

This should be indented 1 space.

Also, replace "Reviewboard" with "Review Board"

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

The issue has been resolved. Show all issues

Instead of adding an event listener to every anchor element, you should add an event listener on a parent element (like the buttons div), and then have your handler use the event.target.id to decide what to do.

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

The issue has been resolved. Show all issues

Bit strange to have a switch for just one option. Should probably just be an if condition.

browser_screenshot/xpi/README.md (Diff revision 6)
The issue has been resolved. Show all issues
```
"review board" -> "Review Board"
```
browser_screenshot/xpi/index.js (Diff revision 6)
The issue has been resolved. Show all issues
```
What calls this?
```

browser_screenshot/xpi/index.js (Diff revision 6)

The issue has been resolved. Show all issues

This is not going to work with multi-process Firefox, as you'll have no access to the contentWindow.

browser_screenshot/xpi/js/popup.js (Diff revision 6)

The issue has been resolved. Show all issues

Same as above, re: adding an event listener to a parent and inspecting the event target id.

browser_screenshot/xpi/package.json (Diff revision 6)

The issue has been resolved. Show all issues

"review board" -> "Review Board".

Maybe take a TODO down to have all of this project metadata come from one central place and get injected at build-time for the addons.

Description:

		Browser extension for Firefox and Chrome that allows the user to take screenshots of their browser's web content and post it to a Review Board server.

		The extension adds the Review Board logo as an icon to the browser's toolbar. Clicking on the icon allows you to select different options of capturing web content. Currently you can captural all visible web content and crop a portion of the visible web content.

		Chrome

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
		Can add/delete/modify users from the user.html page

		Firefox

		Can take screenshot of all visible web content
		Can crop a section of visible web content
		Can send screenshot to a Review Board user's review request
		Can add/delete/modify users from the user.html page
		Multiprocessing friendly

		TO DO LIST

		Asana Link

		Basic Validation on table cell editing
		Fix infinite loading when requesting to some servers
		Fill in all table cells alert message
~		Make Crop option overlay current page and open new tab after cropping
	~	Make Crop option overlay current page and open new tab after cropping (Chrome)
	+	Organize javascript folder into import libraries and non
	+	Alert user if user.html page has unsaved information

Diff:

Revision 7 (+3365)

Show changes

	browser_screenshot/.eslintrc
	browser_screenshot/.gitignore
	browser_screenshot/AUTHORS.md
	browser_screenshot/CONTRIBUTING.md
	browser_screenshot/LICENSE
	browser_screenshot/README.md
	browser_screenshot/gulpfile.js
	browser_screenshot/package.json
	64 more

Added Files:

Tool: PEP8 Style Checker
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/xpi/js/cs_screenshot.js
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/exit.png
    browser_screenshot/xpi/js/crop.js
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/xpi/js/capture-area.js
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/package.json
    browser_screenshot/content/images/crop.png
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/content/css/jcrop-rb-style.css
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/xpi/js/capture.js
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less



Tool: Pyflakes
Ignored Files:
    browser_screenshot/content/images/icons/icon64.png
    browser_screenshot/content/css/font-awesome.min.css
    browser_screenshot/xpi/js/cs_screenshot.js
    browser_screenshot/content/fonts/fontawesome-webfont.woff
    browser_screenshot/content/fonts/FontAwesome.otf
    browser_screenshot/content/css/images/ui-icons_ef8c08_256x240.png
    browser_screenshot/content/js/user_form.js
    browser_screenshot/content/css/jquery-ui.min.js
    browser_screenshot/xpi/package.json
    browser_screenshot/xpi/js/save_user.js
    browser_screenshot/content/images/exit.png
    browser_screenshot/xpi/js/crop.js
    browser_screenshot/content/css/images/ui-icons_ffffff_256x240.png
    browser_screenshot/content/js/users.js
    browser_screenshot/crx/js/save_user.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_100_eeeeee_1x100.png
    browser_screenshot/content/js/jquery.Jcrop.min.js
    browser_screenshot/content/css/images/ui-bg_glass_65_ffffff_1x400.png
    browser_screenshot/content/popup.html
    browser_screenshot/content/css/toastr.less
    browser_screenshot/content/js/jquery-2.1.4.min.js
    browser_screenshot/content/images/icons/icon16.png
    browser_screenshot/content/images/logo.png
    browser_screenshot/xpi/js/modify_users.js
    browser_screenshot/xpi/index.js
    browser_screenshot/content/css/images/ui-icons_222222_256x240.png
    browser_screenshot/xpi/js/capture-area.js
    browser_screenshot/content/css/images/ui-bg_glass_100_fdf5ce_1x400.png
    browser_screenshot/content/js/toastr.min.js
    browser_screenshot/content/images/icons/icon48.png
    browser_screenshot/content/css/images/ui-icons_ffd27a_256x240.png
    browser_screenshot/content/css/images/ui-icons_228ef1_256x240.png
    browser_screenshot/AUTHORS.md
    browser_screenshot/package.json
    browser_screenshot/content/images/crop.png
    browser_screenshot/crx/js/modify_users.js
    browser_screenshot/content/js/screenshot.js
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_20_666666_40x40.png
    browser_screenshot/content/css/jcrop-rb-style.css
    browser_screenshot/crx/js/popup.js
    browser_screenshot/crx/js/background.js
    browser_screenshot/content/css/images/ui-bg_highlight-soft_75_ffe45c_1x100.png
    browser_screenshot/gulpfile.js
    browser_screenshot/content/js/jquery-ui.min.js
    browser_screenshot/content/screenshot.html
    browser_screenshot/content/css/users.less
    browser_screenshot/content/images/icons/icon32.png
    browser_screenshot/content/images/icons/icon128.png
    browser_screenshot/xpi/chrome.manifest
    browser_screenshot/content/fonts/fontawesome-webfont.ttf
    browser_screenshot/xpi/js/capture.js
    browser_screenshot/content/fonts/fontawesome-webfont.svg
    browser_screenshot/.eslintrc
    browser_screenshot/xpi/js/popup.js
    browser_screenshot/content/users.html
    browser_screenshot/.gitignore
    browser_screenshot/content/css/images/ui-bg_flat_10_000000_40x100.png
    browser_screenshot/crx/manifest.json
    browser_screenshot/README.md
    browser_screenshot/content/css/images/ui-bg_gloss-wave_35_f6a828_500x100.png
    browser_screenshot/content/css/jquery-ui.min.css
    browser_screenshot/LICENSE
    browser_screenshot/content/css/images/ui-bg_glass_100_f6f6f6_1x400.png
    browser_screenshot/CONTRIBUTING.md
    browser_screenshot/content/fonts/fontawesome-webfont.woff2
    browser_screenshot/content/css/jquery.Jcrop.min.css
    browser_screenshot/xpi/README.md
    browser_screenshot/content/images/filler.png
    browser_screenshot/content/css/popup.less
    browser_screenshot/content/fonts/fontawesome-webfont.eot
    browser_screenshot/content/css/images/ui-bg_diagonals-thick_18_b81900_40x40.png
    browser_screenshot/content/css/screenshot.less

Ship it!

As a foundation, I think this is good stuff. Thanks Justin!

I think we can build off of this over time - although it'd be good to get a polished architecture diagram somewhere for where you ended up, like on hackpad.

For now, just keep working off this in your GitHub, and don't change this underlying commit. Mark any future work as depending on this change. We're eventually going to get a repository for this thing set up, and then we'll get this landed in it.

Great work!

Review Board 7.0 alpha 0 (dev)

Browser extension for posting screenshots to Review Board

Information

Reviewers

Chrome

Firefox

TO DO LIST

Added Files:

Description:

Chrome

Firefox

TO DO LIST

Diff:

Added Files:

Description:

Chrome

Firefox

TO DO LIST

Diff:

Removed Files:

Summary:

Diff:

Diff:

Description:

Chrome

Firefox

TO DO LIST

Testing Done:

Diff:

Description:

Chrome

Firefox

TO DO LIST

Diff:

Added Files:

Status: Discarded