Category Archives: Code Reviews

The Joy of Coding (Ep. 20): Reviewin’ and Mystery Solvin’

After a two week hiatus, we’re back with Episode 20!

In this episode, I start off by demonstrating my new green screen1, and then dive right into reviewing some code to make the Lightweight Theme web installer work with e10s.

After that, I start investigating a mystery that my intern ran into a few days back, where for some reason, preloaded about:newtab pages were behaving really strangely when they were loaded in the content process. Strangely, as in, the pages wouldn’t do simple things, like reload when the user pressed the Reload button.

Something strange was afoot.

Do we solve the mystery? Do we figure out what’s going on? Do we find a solution? Tune in and find out!

Episode agenda.

References

Bug 653065 – Make the lightweight theme web installer ready for e10s
Bug 1181601 – Unable to receive messages from preloaded, remote newtab page – Notes
@mrrrgn hacks together a WebSocket server implementation in Go. To techno!

  1. Although throughout the video, the lag between the audio and the video gets worse and worse – sorry about that. I’ll see what I can do to fix that for next time. ↩

Things I’ve Learned This Week (May 25 – May 29, 2015)

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:

  1. Manually setting the r+ or r- flags in Bugzilla for the MozReview attachments
  2. Marking Ship It on the child review requests, and letting the reviewee take care of setting the reviewer flags in the commit message
  3. 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!

See gps’s blog post for the nitty gritty details, and some other cool MozReview announcements!

The Joy of Coding (Ep. 11): Cleaning up the View Source Patch

For this episode, Richard Milewski and I figured out the syncing issue I’d been having in Episode 9, so I had my head floating in the bottom right corner while I hacked. Now you can see what I do with my face while hacking, if that’s a thing you had been interested in.

I’ve also started mirroring the episodes to YouTube, if YouTube is your choice platform for video consumption.

So, like last week, I was under a bit of time pressure because of a meeting scheduled for 2:30PM (actually the meeting I was supposed to have the week before – it just got postponed), so that gave me 1.5 hours to move forward with the View Source work we’d started back in Episode 8.

I started the episode by explaining that the cache key stuff we’d figured out in Episode 9 was really important, and that a bug had been filed by the Necko team to get the issue fixed. At the time of the video, there was a patch up for review in that bug, and when we applied it, we were able to retrieve source code out of the network cache after POST requests! Success!

Now that we had verified that our technique was going to work, I spent the rest of the episode cleaning up the patches we’d written. I started by doing a brief self-code-review to smoke out any glaring problems, and then started to fix those problems.

We got a good chunk of the way before I had to cut off the camera.

I know back when I started working on this particular bug, I had said that I wanted to take you through right to the end on camera – but the truth of the matter is, the priority of the bug went up, and I was moving too slowly on it, since I was restricting myself to a few hours on Wednesdays. So unfortunately, after my meeting, I went back to hacking on the bug off-camera, and yesterday I put up a patch for review. Here’s the review request, if you’re interested in seeing where I got to!

I felt good about the continuity experiment, and I think I’ll try it again for the next few episodes – but I think I’ll choose a lower-priority bug; that way, I think it’s more likely that I can keep the work contained within the episodes.

How did you feel about the continuity between episodes? Did it help to engage you, or did it not matter? I’d love to hear your comments!

Episode Agenda

References

Bug 1025146 – [e10s] Never load the source off of the network when viewing source – Notes

The Joy of Coding (Ep. 8): View Source Hacking

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!

Episode Agenda

References

Bug 1148807 – Method moveToAlertPosition in dialog.xml should check if opener is not null

Bug 1110887 – With e10s, plugin crash submit UI is broken – Notes

Bug 1025146 – [e10s] Never load the source off of the network when viewing source – Notes

  1. I say brand new, except that, as I explain in the video, I had already attacked this bug early on in my e10s work, and had only recently come back to it. ↩

  2. The View Source tool sometimes re-retrieves the source off of the network when opened from an e10s-browser ↩

  3. My longest episode ever, clocking in at over 2.5 hours. ↩

The Joy of Coding (Episode 7): Code review, and a Regression

In this episode, I started with some code review. I was reviewing a patch to make the Findbar (particularly, the Find As You Type feature) e10s-friendly.

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!

Episode Agenda

References:
Bug 1133981 – [e10s] Stop sending unsafe CPOWs after the findbar has been closed in a remote browser

Bug 1110887 – With e10s, plugin crash submit UI is broken – Notes

Bug 1147521 – Cannot type into comment area of plugin crash UI