MozReview will now create individual attachments for child commits
Up until recently, anytime you pushed a patch series to MozReview, a single attachment would be created on the bug associated with the push.
That single attachment would link to the “parent” or “root” review request, which contains the folded diff of all commits.
We noticed a lot of MozReview users were (rightfully) confused about this mapping from Bugzilla to MozReview. It was not at all obvious that Ship It on the parent review request would cause the attachment on Bugzilla to be r+’d. Consequently, reviewers used a number of workarounds, including, but not limited to:
Manually setting the r+ or r- flags in Bugzilla for the MozReview attachments
Marking Ship It on the child review requests, and letting the reviewee take care of setting the reviewer flags in the commit message
Just writing “r+” in a MozReview comment
Anyhow, this model wasn’t great, and caused a lot of confusion.
So it’s changed! Now, when you push to MozReview, there’s one attachment created for every commit in the push. That means that when different reviewers are set for different commits, that’s reflected in the Bugzilla attachments, and when those reviewers mark “Ship It” on a child commit, that’s also reflected in an r+ on the associated Bugzilla attachment!
I think this makes quite a bit more sense. Hopefully you do too!
In this episode, I again started with some code review. I reviewed this patch for this bug by fellow Firefox hacker Gijs, and refreshed my memory on var hoisting. I’ve been using let for so long that it was really, really weird to see how var worked.
After that, I quickly gave an update on my plugin crash UI bug I had been working on the last episode – the patches are up, and are currently undergoing review, so there wasn’t much to do there.
Next, I started on a brand new bug1, explained the bug2, and then laid out my plan for attacking it.
Specifically, I’m going to try an experiment: I will only be working on that bug during Joy of Coding sessions. That way, there is continuity from video to video, and you won’t miss any of the development that goes on between episodes.
We sliced off a chunk to get done, and hit some minor roadblocks (as expected). The View Source code is old and crufty, and I have to do my best to make sure I don’t break any of the other applications that depend on it (like Thunderbird and SeaMonkey).
So that was the name of the game – looking to see how other applications use View Source, and trying to come up with a plan for making sure we don’t break them, while at the same time refactoring View Source to be easier to code against (and work with a frame script and messages).
It was a long slog3, but we got to a good point by the end. Let’s see how far we get next week!
With that review out of the way, I had to swap a bunch of information about the plugin crash UI for e10s in my head – and in particular, some non-determinism that we have to handle. I explained that stuff (and hopefully didn’t spend too much time on it).
Then, I showed how far I’d gotten with the plugin crash UI for e10s. I was able to submit a crash report, but I found I wasn’t able to type into the comment text area.
After a while, I noticed that I couldn’t type into the comment text area on Nightly, even without my patch. And then I reproduced it in Aurora. And then in Beta. Luckily, I couldn’t reproduce it in Release – but with Beta transitioning to Release in only a few days, I didn’t have a lot of time to get a bug on file to shine some light on it.
Luckily, our brilliant Steven Michaud was on the case, and has just landed a patch to fix this. Talk about fast work!
In this episode, I took the feedback of my audience, and did a bit of code review, but also a little bit of work on a bug. Specifically, I was figuring out the relationship between NPAPI plugins and Gecko Media Plugins, and how to crash the latter type (which is necessary for me in order to work on the crash report submission UI).
A minor goof – for the first few minutes, I forgot to switch my camera to my desktop, so you get prolonged exposure to my mug as I figure out how I’m going to review a patch. I eventually figured it out though. Phew!