Reviewers must understand what you approved.

[Carlos Pizano]

I don't know exactly how to begin explaining this so I am going to give two examples of a hypothetical changes :

========

Add the ready images

BUG=none

TEST=none

A src/chrome/app/images/ready.png

A src/chrome/app/images/not_ready.png

========

Add libfoo to third party

Library licensed has been OKed.

BUG=none

TEST=chrome still compiles

A src/third_party/libfoo.hh

A src/third_party/libfoo.cc

=========

As a reviewer before allowing this kind of changes you need to have at least basic understanding of that feature this change belongs to and that understanding must

be accessible (obtainable) by any other chromium developer that happens to read the CL, for example a tree sheriff.

Please demand:

- An appropriate description, does not need to be too long.

- In the vast majority of the cases a bug number. As a reviewer you need to open the bug and grok the bug description which should be again something

that a random chrome developer can understand at least at a basic level. For example the bug cannot just be "Add xxx to chrome". 

- Just reading the bug you should be able to tell what this is fixing or if it is a new feature you should be able to trace it back to the launch bug. The bug should

also have reasonable tags, like operating system, milestone, feature or area, etc, this ensures that relevant PMs and TLs are in the loop.

- In the case of a 3rd party library or tool, having an approved license is not enough, again what feature this is tied to and a way to get to the launch bug

for the feature is needed. Is it another xml library? why? has it been approved by the security team?

[Peter Kasting]

+1 to every point you made.  And authors/reviewers, remember that you're not the only audience for your change.  Future spelunkers may look at it too.

Recently I saw a change go by that confused me (removing OVERRIDE from some stuff) so I pinged the reviewer, jamesr.  (Sorry to single you out James, this just came to mind!)  James explained very clearly what was going on.  But ideally the patch author would have linked to a bug that had that kind of full explanation on it (if there was a bug link in this case, the bug was really vague; I don't recall).  That way if I'd seen this a year later, I wouldn't have had to ping someone (who maybe by that point wouldn't be working on Chromium anymore) to understand what was going on.

PK

[Pam Greene]

+1. Furthermore, a basic understanding should come from the log message without the need to look up a bug. Someone may be scrolling through the log trying to figure out which change caused a regression, and having to click through to a bug for every change is painful. So I would say that...

- Just reading the log message you should be able to tell what this is fixing, or what new feature it adds.

- Pam

-- 

Google Germany GmbH Dienerstr. 12 80331 München

AG Hamburg, HRB 86891  Sitz der Gesellschaft: Hamburg

Geschäftsführer: Graham Law, Katherine Stephens

Tax ID: 48/725/00206  VAT ID: DE813741370

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Thiago Farina]

On Mon, Jan 7, 2013 at 6:47 AM, Pam Greene <p...@chromium.org> wrote:
>
> +1. Furthermore, a basic understanding should come from the log message
> without the need to look up a bug.

I think that doesn't happen because reviewers (that is just my
observation) doesn't review the CL description as they review the
code. Many commit messages have spelling errors, missing bug numbers,
poor writing, lack of information (justification - some just say "add
Y"), etc.

Just compare a commit message from chromium with one from git project
(for example) and you will see the difference.

--
Thiago

[Torne (Richard Coles)]

Indeed.. Maybe this would occur to people more often if rietveld supported leaving comments on the commit message the way gerrit does? (It is listed in the commit as if it were a file, and you can leave per-line comments on it the same as any other new file..)

[Avi Drissman]

Thiago, you make an excellent point. The commit message is just as much a part of a CL as the code, and should be able to withstand the same level of scrutiny.

Avi

[Scott Hess]

Let's run before we walk, here. I routinely receive reviews where the
description is like "Implement OSX version of bug 12345", and where
bug 12345 is a one-line "Implement chewbacca mode". No mocks, no
descriptive text of any sort, no indicator of how one would even
manage to provoke the UI being implemented if I were to want to patch
it locally to see it in action.

Basically, when writing your CL description, pretend you're writing it
for the very newest batch of Chromium developers, who haven't been
spending the past six months hip-deep in your code. They/I have no
idea what super-extended-app-mode is, they've never seen a
right-handed smoke-shifter, much less a left-handed one. If your
UX/tech lead hasn't provided decent mocks/design-docs, sorry that your
life is so tough, but that doesn't justify making other people's lives
tough.

-scott

[Justin Schuh]

I wanted to add that the third_party case is a particularly painful one. There's the simple issue of bloating checkouts and having multiple implementations of common code (bloating the binaries). However, the bigger issue is that third party libraries incur a serious maintenance burden on the project as a whole.

Take the case of a security vulnerabilities in third party dependencies. If the vulnerability is in Chromium code then we almost always get a report emailed to us or filed in our tracker. But for third-party it's a different story, because each project has its own mechanisms for tracking bugs, and some projects are not exactly diligent about notifying their consumers. As a result, we've had situations where we've fallen behind upstream on security patches because we were never informed, or something slipped through or tracking mechanisms.

And even when we are abreast of all the changes upstream, there's still the issue of integrating the changes into our local checkout. That's often a non-trivial job best performed by the person who originally checked it in. However, you don't have an OWNERS file for third-party code and people generally just don't approach it with the same sense of custodianship that they do for code they've written. So the reviewer really needs to make sure the third party code is appropriate, and that there's a longterm maintenance plan.

-j

[Erik Wright]

On this subject, today I ran into a browser test in base/ ! In chrome_tests.gypi there is a crbug link indicating that the test should be a unittest, but can't be because it can hang upon failure - there are also notes about work in progress to fix this.

There is no explanation for why it hangs or how the reporter intended it to be fixed. The last two messages are a query from another developer seeking background, and the original reporter noting that they have now left the Chromium project and have no idea what the original problem was.

In this case, I think the original committer and the reviewers could have done a better job ensuring that adequate documentation existed for future developers encountering this bit of cruft.

[Pam Greene]

Filed as issue 169249.

- Pam

-- 

Google Germany GmbH Dienerstr. 12 80331 München

AG Hamburg, HRB 86891  Sitz der Gesellschaft: Hamburg

Geschäftsführer: Graham Law, Katherine Stephens

Tax ID: 48/725/00206  VAT ID: DE813741370