551311 - Tracking bug for Jetpack SDK code reviews

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Drew Willcoxon :adw]

As discussed in today's meeting, the code in the SDK 0.1 release hasn't been reviewed, but it needs to be.  We'll use this bug to track that progress.  Make a bug for each review and mark it blocking this one.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Awesome, thanks for filing these Drew!

A couple notes...

We absolutely need to do this, and we shouldn't delay it too long. However, it's not the top priority for the 0.2 cycle, where we should make sure we continue to focus on standing up the initial high-level APIs we've been specing.

Folks who aren't working on 0.2 priorities, however, are welcome to tackle these reviews immediately!

I also want to make sure that all the code we ask folks to review is really on the production train.

Atul: is there anything in the SDK that you consider to still be prototype/experimental code and which you intended to replace with production-quality code at a later date?  If so, we should delay reviews of that code until its replacement is in place (and file bugs on implementing those replacements).

Comment 2

[Atul Varma [:atul]]

Myk, there isn't anything that's prototype/experimental code, but I should mention that when I wrote the SDK, I didn't actually write any tests for any code that wouldn't run on end-user systems.

That means, for instance, that the cfx tool doesn't have any tests, and generally isn't of the same quality as the code in the 'jetpack-core' package that Drew has made bugs for. For that matter, nor does the actual test harness/runner program, contained in the 'test-harness' package.

As hanson mentioned in the Labs meeting yesterday, part of the reason for code review is to improve the "bus factor" for code by ensuring that more folks know how it works, so it might be useful to review the cfx tool and the test-harness package--besides, it would be terrible if our tests were failing but looked like they passed because of a bug in the test harness program.

I've just sent out an email to Ian Bicking and Brian Warner asking them if they'd be interested in either reviewing or writing tests for the cfx tool.

Also, it's imperative that someone review the file at python-lib/cuddlefish/app-extension/components/harness.js, as this is the "bootstrapper" called by Firefox which creates an instance of the Jetpack Runtime and starts the Jetpack-based extension.  I should actually probably document this a bit more before someone does this, though, because I suspect code reviewing it without some docs might be fairly painful.

Comment 3

[Atul Varma [:atul]]

Also, while none of the code is prototype code, I think there are some "TODOs" in the jetpack-core package that we may want to make bugs for. A grep for the string "TODO" should do the trick.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

We still need to come up with a plan for when we're going to do these reviews.  In the meantime, removing the "0.1" piece from the summary, as some of the code we're going to review has landed since 0.1.

Comment 5

[Drew Willcoxon :adw]

Ah, sorry, I meant to ping Dietrich about this again.  Dietrich, would it be OK to roll this up into the JEP implementation goal for fx team?

Comment 6

[Dietrich Ayala (:dietrich)]

Not sure it makes sense to tie it to our JEP implementation plan, but I'm definitely up for helping find the right reviewers here. I'll spend some time looking at it this week.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

We're not going to finish all these reviews for 0.3, nor do I think it's necessary.  I would, however, like us to have these done by the time we release 0.5 in late June, such that all the code in the 0.5 release is reviewed.

Comment 8

[Dietrich Ayala (:dietrich)]

Sounds good. Given how the reviews have gone so far, they definitely seem to have value. And will take a while.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Tracking bug no longer needed.