Tip:
Highlight text to annotate it
X
In this video I wanted to talk about reviewing other people's code.
So when someone wants you to review their code, you'll get an email.
And that email will look something like this.
There will be a title of review requested, subject of the bug, and then it'll just give you a link for the review, and a link to the bug.
So if I go to that bug right here, and I scroll down.
You can see that I already reviewed these 2 patches, but I'm just going to click on review just to show you.
So this is what the interface looks like, you have this overview right here.
You have this review box right here.
It'll initially be set to a question mark with your email there.
And if you agree with it, you'll just press +, and if you don't agree with it, you'd hit -.
And then you'll scroll down and you'd press publish.
Now before you actually press +, you probably want to do the review.
And to do that you click on each file, and you just scroll through it, and you see if everything makes sense.
Now before you even look at the code, you should ask yourself if the task that you're being asked to review is actually something that we want in our code base.
Because it's possible that a community member, or even an employee, will try to add code that think is important, but really shouldn't be in there.
Once you determine it is a good task to do, then you want to go through the code.
And just kind of remember that reviewing code is more of a discussion, it's not a judgement.
And also you're not expected to know everything.
So if I'm going through here, and I really don't know what this means, I can just double click here and I can say:
why would you want to make this change? It doesn't make sense to me.
And then the person that you're reviewing code for will then respond to you and you can have a discussion and a dialog back and forth.
You usually also want to pull down the code into your local repository.
You don't always have time for that, but it's usually a good idea
And then you can build it and test it yourself.
When you're done reviewing a certain file, you can just click on this reviewed button right here.
And that's not actually required. It's just to help you, because sometimes a patch has 20-30 files in it, so it gets confusing which one is reviewed or not.
So that box doesn't actually do anything except for help you.
Usually you want to get back to someone within a day, so 24 hours.
And if you can't review the patch within a day, you want to tell them that you won't be able to review the patch for another week, or another couple days.
Or just give them a heads up.
And that basically just gives them a chance to find a different reviewer.
You might not always be the best person for the review as well.
So let's say I wasn't the best person here, I would just put that as a question mark and I'd put someone else's email.
Let's say rstrong, a different person who's better for this review.
I would just put a question mark, his email, and say:
Robert would be better to handle this patch review.
And then just click on submit.
And that just forwards the request to him.
Let's say I am reviewing this patch, so I'm just going to clear that out.
And some things you want to look out for.
One is coding style, make sure it's consistent with the rest of the file, and make sure it is consistent with Mozilla standards.
Try not to review based on what you would do.
If what they're doing is just as good.
And always try to be polite.
So thank the person that submitted a patch.
often times they are contributors and they are not being paid for the patch.
Other things you want to look out for are security issues, privacy, leaks, make sure it's not affecting performance.
Particularly if it's going to affect startup performance.
Often times you want to ask the person to write a test.
Sometimes what they're actually changing is not really testable.
So in those cases you don't need to.
But for most front end code it's usually testable.
So you should always ask for a test from people.
You don't always have to hold up the review, but it's OK to sometimes hold up the review to ask the person to write a test.
Especially if it's new functionality, or even a bug.
So I covered a lot of things, but a good idea is to go look for the Reviewers Checklist on MDN
Reviewer Checklist MDN. And it's the first list there.
And this will go into a lot more detail.
And it'll cover a lot of the things that I already talked about, but it will give you more depth.
And that's all I wanted to cover for this video.
Subtitles by the Amara.org community