This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/admin/forms.py
    reviewboard/settings.py
    reviewboard/extensionbrowser/base.py
    reviewboard/extensionbrowser/forms.py
    reviewboard/extensionbrowser/views.py
    reviewboard/urls.py
    reviewboard/extensionbrowser/urls.py
  Ignored Files:
    reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
    reviewboard/static/rb/js/extensionbrowser.js
    reviewboard/extensionbrowser/templates/extensionbrowser/extension_details.html
    reviewboard/static/rb/css/extensionbrowser.css

Col: 21
 E128 continuation line under-indented for visual indent

Col: 5
 E128 continuation line under-indented for visual indent

Col: 80
 E501 line too long (84 > 79 characters)

Col: 80
 E501 line too long (81 > 79 characters)

Surya:

This is really coming along nicely. Just some high-level suggestions / notes, along with some typos I noticed.

There are a few style things also worth fixing, but we'll bring those up when you're out of WIP.

Let me know if you have any questions. Thanks,

-Mike

Er, isn't this a change from one of your original easyfix bugs?

Just curious - why are we making extension_manager optional?

typo: corresponding

If an extension manager wasn't passed to the constructor, self.installed_extensions will be None, which is not iterable.

typo: package

Wouldn't we want to pass the error on to the caller instead of swallowing it silently?

All of the strings in here should be wrapped in trans blocks

Trans blocks for the strings in here.

typo: sanitizing

typo: obtaining

Did it? If we catch an Exception on lines 57-58, we'll store the error string in response, but the status will be 204? That doesn't sound right.

You might want to check out how we use LessCSS to compile our CSS and use things like variables to use common colours, etc.

This is looking cool, but there's a lot of stylistic stuff to fix up, and some more important architectural stuff.

We're moving all our JavaScript to be Model/View based, using Backbone.js. I want to see all new JavaScript fit this. Start thinking in terms of objects that contain data/core logic, and objects that render (using templates, building jQuery elements, etc.).

You can look at some of our other code to see how we're doing this, and of course talk to us if you need help structuring it.

Imports are always in up to 3 groups:

1) Python-bundled modules
2) Third-party modules (like simplejson)
3) Project's modules (like reviewboard)

Separated by blank lines.

Also, never import from reviewboard.settings. This should be using:

    from django.conf import settings

They're too very different things. One is our settings file, another is a wrapper that may be modified dynamically (by siteconfig).

Shouldn't use simplejson. Instead, for now, use django.utils.simplejson. (This is going away, but let's be consistent for now.)

We now have multiple things named Extension. Let's change this one to StoreExtensionInfo.

This is very technical and too specific to the current workflow. This level of code should not know how it will be used (as that may change). Instead, it should just say what it is and what, in general, it's meant to represent.

This is the kind of thing that should be handled by the rendering, not core logic. I'd say just nuke this function and do this in the template.

The JSON response part is an implementation detail, and not something that needs to be known to consumers of the API.

Remove the blank line.

Generally when a list comprehension spans multiple lines, we like to do:

    self.foo = [
        ext.dist.project_name.lower()
        for ext in ....
    ]

Use format strings. They're cheaper for Python.

Same.

In fact, combine this with the previous line, too.

What happens when/if this fails?

Make sure to change the "Extension" part when renaming the class.

Since we're using a dictionary, does this mean we can't rely on any certain ordering from the server?

Given how many arguments we have, and that this will likely grow, can you instead use keyword arguments for this? And then wrap the values so they're indented 1 level from the previous line, with one keyword argument per line.

It may not be a zip file. It could be an egg, or tar.gz, or whatever. Best not to be specific here.

We're repeating the whole HTTP code a couple times. Can this be consolidated into a wrapper function?

One-line summaries shouldn't have both """ on the same line.

In this function, though, there's no documentation on what this extension information is.

Shouldn't have the ":" in the label. That's a rendering detail.

See about using {% static %} for this, since {% admin_media_prefix %} is going away.

Shouldn't use either of these. Instead, use {% compressed_css %} with some name, which you'll register in settings.py in PIPELINE_CSS. This will list all the CSS files you use.

This ties into our build system, so that 3 files can be turned into one minified file.

Should be scripts-post, so that we load this after rendering the page.

Blank line before this.

Do we not have one of these already? (Not sure, asking.)

Always use {% url %} instead of hard-coding URLs.

Indentation is screwy.

Use {% trans %}

Any IDs we don't need should be removed.

Any we do should be properly namespaced.

Use {% trans %} for all these strings.

Go through the files and make sure you're always localizing.

Seem to have some tab vs. space problems. Make sure this file has no tabs, and no trailing whitespace.

Classes always use -, not _

Never use <center>. Any styling is up to a stylesheet.

Never use <b>, for the same reason as <center>.

Don't use <b>. Should use <label>.

Why do you need this? It feels wrong having it.

Two things URLs should always have:

1) A trailing /
2) A name= parameter.

Should be separated by a blank line.

No blank line.

Would you mind maybe putting a "_ajax" suffix on any functions like this?

Actually though, I think this function would be best done as an API resource. Maybe a next step on top of this change.

That way, we can continue to use our API as the one true way to talk to Review Board for most things, and admins can script installation of extensions.

No blank line.

We do this more than once. Best to pull it out in the beginning.

No blank line.

A good thing of polish would be to change these error strings. They should be more human-readable, less technical, and say roughly what went wrong, maybe even what to do about it.

This is kind of funky.

I'd suggest getting the extension manager first, then operating on it.

If you still need to wrap, wrap at the argument level.

Remove the blank line.

Best to use format strings.

Remove the blank line.

Some of the same comments.

Go through and apply the same blank line removal as the previous change.

Blank lines are good, but there shouldn't be one in the following cases:

1) At the start of a function.
2) Before an else/elif block.

This should default to None. You can override it in your own settings_local.py.

As Mike mention, this should be a .less file.

Also make sure ou are following our style. At a quick glance:

1) Always have a space before {
2) Indent with 2 spaces.

"Listener"

Same elsewhere.

Note that you should be using 4 space indentation (no tabs!) everywhere.

Space before {

Same throughout the file.

Also, you're fetching .item_actions more than once. This will traverse the entire DOM.

Best to fetch that only once (and within some known parent element), and then fetch whatever else you need from it.

jQuery does chaining, so you can do:

$('#' + id)
    .hide()
    .after(...)

Note that you shouldn't be building HTML though.

Should have a trailing slash after "browser"

I would change "browser" to "browse" in the URL, though.

Also, this is longer than 80 chars, so wrap the include to the next line.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/settings.py
    reviewboard/webapi/resources.py
    reviewboard/extensionbrowser/base.py
    reviewboard/extensionbrowser/forms.py
    reviewboard/extensionbrowser/views.py
    reviewboard/urls.py
    reviewboard/extensionbrowser/urls.py
  Ignored Files:
    reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
    reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
    reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
    reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
    reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
    reviewboard/static/rb/css/extensionbrowser.css
    reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
    reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
    reviewboard/static/rb/css/jquery-ui-1.10.1.custom.css
    reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
    reviewboard/static/rb/js/models/extensionInformationModel.js
    reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
    reviewboard/static/rb/js/extensionbrowser.js
    reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
    reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
    reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
    reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
    reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
    reviewboard/static/rb/js/views/extensionBrowserView.js
    reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
    reviewboard/static/rb/css/images/animated-overlay.gif
    reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png

Col: 80
 E501 line too long (83 > 79 characters)

Col: 45
 E128 continuation line under-indented for visual indent

Col: 45
 E128 continuation line under-indented for visual indent

Col: 45
 E128 continuation line under-indented for visual indent

Col: 45
 E128 continuation line under-indented for visual indent

Col: 45
 E128 continuation line under-indented for visual indent

Col: 45
 E128 continuation line under-indented for visual indent

Col: 80
 E501 line too long (95 > 79 characters)

Col: 5
 E128 continuation line under-indented for visual indent

Col: 8
 W292 no newline at end of file

Col: 35
 W291 trailing whitespace

Col: 1
 E302 expected 2 blank lines, found 1

Col: 21
 E126 continuation line over-indented for hanging indent

Col: 21
 E126 continuation line over-indented for hanging indent

Col: 17
 E123 closing bracket does not match indentation of opening bracket's line

Col: 80
 E501 line too long (80 > 79 characters)

One thing that strikes me is that when you switch from tab to tab, the size of the modal changes, and so the tabs move.

This is not ideal. What we should do is size the modal to fit the largest tab content, and stick to that size, so that we're not resizing on the fly.

Detail - tabs should touch their content.

reviewboard.extensionbrowser isn't being recognized as a module because there's no __init__.py under reviewboard/extensionbrowser. You probably have one locally, but haven't included it. You need to include it.

"Keywords" should maybe be wrapped in an underscore function.

Also, not sure "Keywords" is the right word to use here. Makes me think of AOL. Let's leave it for now, but maybe worth revisiting later on.

"Search" should be in a trans tag

Mind if I ask why a <table> wasn't used here instead? I know we didn't want it to *look* like a table, but a table, semantically speaking, is what it is...

We use 2 spaces indentation in our .css, and I think you've used tabs.

Also, please put your properties in alphabetical order, and always make sure there is a space before {.

I'm a bit worried that some of these hard-coded widths and heights might be a little fragile...

Also, it might be worth looking into converting this into a .less file. See http://lesscss.org/

id:, as opposed to id :

Space before {

Please remove this log

Modals are not usually the best way to handle things. We have a thing that drops down from the top of the screen that says "Loading" when doing an AJAX request, and if something goes wrong, that Loading banner becomes an error that stays put. Maybe use that instead.

Space before { after function() - true for the rest of these Javascript files too.

I wonder if instead of tossing this into the view, we render this as a hidden node in the page. That'd give us trans tags too.

remove the newline

remove the newline

No newline.

Yes please. :)

See my other comment, re the "Loading" banner.

No newline.

var targets = ["ext-information", "ext-screenshots", etc];

is preferable over new Array.

Again, I wonder if these should be in the rendered HTML instead, in something like this:

<script type="text/template" id="itemViewTemplate">
  <div class="extension_info_wrapper">
    ...
  </div>
</script>

Instead, for translation, and to keep our HTML (even our templates) a little more separated from our JS.

[] as opposed to new Array.

Also, this looks related to the array on line 91.

If we end up going with the embedded template route, maybe this object have an object property, like:

target_templates: {
  'ext-information': 'info_template',
  'ext-screenshots': 'screenshots'
}

etc, where 'info_template' and 'screenshots' could be the template IDs. Just an idea, to reduce duplication.

These should be in alphabetical order, so DOES_NOT_EXIST, EXTENSION_INSTALLED, INVALID_FORM_DATA, NOT_LOGGED_IN, PERMISSION_DENIED.

EXTENSION_INSTALLED does not seem to exist in either this patch, or in the patch in /r/3973

Needs documentation - see ServerInfoResource, for example.

Needs documentation. I guess that's what this handy newline is supposed to indicate. :)

Might be worth logging the error details too.

Needs documentation

Needs documentation

We should probably log this error.

Does this need to get wrapped in an underscore translation function?

We should definitely log this, and maybe throw a new INSTALL_EXTENSION_FAILED error?

Documentation needed.