Notes re: review of Page Shot for landing in Firefox


(Ian Bicking) #1

We had a meeting with Dave Townsend today to discuss code review of Page
Shot for the purpose of landing it in Firefox. This is the process Dave
suggested:

  1. Once we get the add-on in a reviewable state, we should start
    reviews, and not wait until we are ready to actually import the code.
  2. I believe the WebExtension portion is very close to ready for
    review. There will be a small bootstrap.js wrapper on the extension, but
    that has not been written. That won’t have much effect the WebExtension
    itself.
  3. Dave suggested he could review the bootstrap portion of the add-on,
    and suggested Kris Maglione as a reviewer for the WebExtension portion.
  4. If the review goes well we’ll discuss having subsequent updates
    reviewed inside the team.
  5. The testing plan remains that we’ll have functional tests in the
    Firefox tree, with server interactions mocked out.

Here’s what I’d like to do before reviews:

  1. Freshen up some of the architectural documentation. I’m only
    expecting to have a couple pages total of documentation: (a) a basic code
    map, (b) a description of the error handling process, and © a description
    of the communication between the different processes/workers/server.
    Please note if there’s something more that would be helpful.
  2. I have a small number of code cleanup tasks to finish.
  3. For other reasons I am working on converting to an Embedded Extension
    right now, but I don’t expect to have a complete bootstrap.js worth
    reviewing until a little later.

I think the best way to do the review is to import the entirety of the
code into reviewboard. We have about 3000 lines of code in the extension,
half of it related to the selection interface.

Note that security review is currently happening, and is separate from
these reviews.

BTW, I’ve CC’d testpilot+page-shot-dev@mozilla-community.org which goes to
a public Discourse group
https://discourse.mozilla-community.org/c/test-pilot/page-shot-dev. I
don’t know how well this will work, Discourse may barf on people’s replies.


(Leo McArdle) #2