Converging with Chromium style

[Jeffrey Yasskin]

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project. We'll adopt some of the most-loved parts of Blink style in Chromium, especially the parts that allow writing less code, while adopting most of Chromium's naming and formatting guidelines in Blink. Specifically:

• Inline virtuals will be allowed, with some restrictions on when classes can have inline constructors and destructors.
• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
• We're pursuing google3 style changes to allow default arguments and mutable references, which would carry over to Chromium. If google3 rejects these, we'll discuss Chromium allowing them independently. Blink will continue allowing default arguments and mutable references at least until that's resolved.
• We'll use Chromium naming guidelines for most of the codebase: file_names, member_names_, local_variable_names, ClassNames, etc. For functions, Chromium uses a mix of names_with_underscores() and CamelCase(), but Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().
• Blink will adopt Chromium's 80-column limit and the rest of clang-format's Chromium formatting. The 80-column limit does not exempt us from the rule against abbreviation.
• Blink will maintain a short style guide for refinements to and differences from the overall Chromium style, and we'll aim to make this shorter over time.

We understand that many people will be upset about parts of these decisions. None of them were made lightly and this was the best compromise we could find based off the desires people expressed. Particularly, there was enough desire to end up with a shared style, even weighted by the number of Blink commits, that we had to find some compromise.

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Jeffrey

[Peter Kasting]

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project. We'll adopt some of the most-loved parts of Blink style in Chromium, especially the parts that allow writing less code, while adopting most of Chromium's naming and formatting guidelines in Blink.

Thanks for this work; I imagine the haggling wasn't fun and neither will be the irked comments you're bound to get, but it looks to me like a good compromise list.  Two clarification questions:

• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
  

The Google style guide says these may be omitted "only if the parameter is unused and its purpose is obvious" (for both declarations and definitions), and since Chromium style doesn't add further notes, this is also the rule in Chromium.  Blink is more lenient and allows even used parameters to have omitted names in the declaration, which I always found hard to read.  Which policy will be used going forward, or will these continue to differ?

• We'll use Chromium naming guidelines for most of the codebase: file_names, member_names_, local_variable_names, ClassNames, etc. For functions, Chromium uses a mix of names_with_underscores() and CamelCase(), but Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().
  

If you're renaming files to file_name style, are you going to rename C++ source files from .cpp to .cc at the same time?  Seems like if you're trying to be congruent with existing Chromium practice as well as the explicit Google style guide rules, this should happen, but you didn't explicitly mention it.

PK

[Daniel Cheng]

Two questions about the file renaming:

1) Is .cpp -> .cc part of the renaming change?

2) When files are renamed, should we ensure that #includes that change use a fully qualified path? I don't think there will be a lot of naming collisions, but there would be some, and having it read unambiguously would be nice.

Daniel

 

Jeffrey

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 3:22 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project. We'll adopt some of the most-loved parts of Blink style in Chromium, especially the parts that allow writing less code, while adopting most of Chromium's naming and formatting guidelines in Blink.

Thanks for this work; I imagine the haggling wasn't fun and neither will be the irked comments you're bound to get, but it looks to me like a good compromise list.  Two clarification questions:

• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
  

The Google style guide says these may be omitted "only if the parameter is unused and its purpose is obvious" (for both declarations and definitions), and since Chromium style doesn't add further notes, this is also the rule in Chromium.  Blink is more lenient and allows even used parameters to have omitted names in the declaration, which I always found hard to read.  Which policy will be used going forward, or will these continue to differ?

The idea is that in a declaration, the parameters aren't used, so they can be omitted if their purpose is obvious. Blink will continue to require omitting them when they duplicate their type name, while Chromium just allows it.

• We'll use Chromium naming guidelines for most of the codebase: file_names, member_names_, local_variable_names, ClassNames, etc. For functions, Chromium uses a mix of names_with_underscores() and CamelCase(), but Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().
  

If you're renaming files to file_name style, are you going to rename C++ source files from .cpp to .cc at the same time?  Seems like if you're trying to be congruent with existing Chromium practice as well as the explicit Google style guide rules, this should happen, but you didn't explicitly mention it.

Yes, sorry. Dana mentioned that just after I sent the email. ClassName.cpp goes to class_name.cc.

Jeffrey

[Jeffrey Yasskin]

Yes

 

2) When files are renamed, should we ensure that #includes that change use a fully qualified path? I don't think there will be a lot of naming collisions, but there would be some, and having it read unambiguously would be nice.

I think so. Anyone have a reason not to fully-qualify #includes during the rename?

[Nico Weber]

For (2): Having qualified include paths (relative to third_party/WebKit) is generally what we want, and we updated most of the include paths right after the fork (https://code.google.com/p/chromium/issues/detail?id=234793). I think we skipped the files in the WebKit public API in fear of requiring three-sided patches for renames or something like that, but now that we're in one repo we should run the script we used for qualifying includes on the WebKit API too. (...now to find that script...)

 

Daniel

 

Jeffrey

[Peter Kasting]

On Thu, Dec 10, 2015 at 3:31 PM, Jeffrey Yasskin <jyas...@google.com> wrote:

On Thu, Dec 10, 2015 at 3:22 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project. We'll adopt some of the most-loved parts of Blink style in Chromium, especially the parts that allow writing less code, while adopting most of Chromium's naming and formatting guidelines in Blink.

Thanks for this work; I imagine the haggling wasn't fun and neither will be the irked comments you're bound to get, but it looks to me like a good compromise list.  Two clarification questions:

• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
  

The Google style guide says these may be omitted "only if the parameter is unused and its purpose is obvious" (for both declarations and definitions), and since Chromium style doesn't add further notes, this is also the rule in Chromium.  Blink is more lenient and allows even used parameters to have omitted names in the declaration, which I always found hard to read.  Which policy will be used going forward, or will these continue to differ?

The idea is that in a declaration, the parameters aren't used, so they can be omitted if their purpose is obvious.

Huh.  Is that how the Google style guide was always intended to be read, or is this an intentional deviation from the Google style guide?

If the former, that would certainly diverge from how I've always read this, and it would be nice to hear clarification from the C-style arbiters.  I thought the Google rule was supposed to mean that "used" referred to in the function definition, and the declaration and definition should agree on which params were and were not omitted.

PK 

[TAMURA, Kent]

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

--

TAMURA Kent
Software Engineer, Google

[TAMURA, Kent]

> 2) When files are renamed, should we ensure that #includes that change use a fully qualified path? I don't think there will be a lot of naming collisions, but there would be some, and having it read unambiguously would be nice.

I recommend to do it after moving Blink from third_party/WebKit to somewhere.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qxA6mIVi-4I

[Elliott Sprehn]

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Yeah... Can we just make the code generator capitalize the method names so it's ParentNode() or leave stuff alone?

I worry about us ending up with name based variation too like parentNode() for web exposed, and ParentNode() for internal, where they have different return types.

- E

[Kentaro Hara]

On Fri, Dec 11, 2015 at 11:04 AM, Elliott Sprehn <esp...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Yeah... Can we just make the code generator capitalize the method names so it's ParentNode() or leave stuff alone?

We can do that, and I think we should. Only WebIDL uses parentNode(). All C++ methods use ParentNode().

I worry about us ending up with name based variation too like parentNode() for web exposed, and ParentNode() for internal, where they have different return types.

- E

--

Kentaro Hara, Tokyo, Japan

[Kentaro Hara]

About the overall plan:

 

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*.

This sounds reasonable to me.

 

The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

How much are you going to be pushy about fixing the coding style in each directory? I'm asking this because this is going to distribute a ton of work to many teams and the opportunity cost is extremely high.

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

Is the only inconsistency you're worried about that some functions (js implementations) will still use a different naming convention, or are there others?

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Some folks saw this as a good thing: web-exposed functions are different from C++-only functions, both in that you can't change them easily, and in that you can rely on your knowledge of the web platform to figure out their meaning.

That said, if Blink folks in general want to uniformly start functions with capital letters and update the bindings generator, that's fine with me. I agree with Elliott that we wouldn't want to have functions named both parentNode() and ParentNode().

On Thu, Dec 10, 2015 at 5:46 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> 2) When files are renamed, should we ensure that #includes that change use a fully qualified path? I don't think there will be a lot of naming collisions, but there would be some, and having it read unambiguously would be nice.

I recommend to do it after moving Blink from third_party/WebKit to somewhere.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qxA6mIVi-4I

Or maybe as part of moving out of third_party/WebKit? If a lot of files are being renamed anyway, this might be a good opportunity to get it all out of the way. We'll need similar tooling to help other developers migrate their patches across the change.

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 6:13 PM, Kentaro Hara <har...@chromium.org> wrote:

About the overall plan:

 

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*.

This sounds reasonable to me.

 

The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

How much are you going to be pushy about fixing the coding style in each directory? I'm asking this because this is going to distribute a ton of work to many teams and the opportunity cost is extremely high.

Not very pushy. If it's done in 6 months, I'd be pleasantly surprised. It'd also be best to get a smallish set of volunteers to make the changes, and let the directory owners tell them when's most convenient. That way the few volunteers can learn the pitfalls instead of each directory's owners having to learn independently. We need enough volunteers to avoid burning them out though.

Jeffrey

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 6:37 PM, Jeffrey Yasskin <jyas...@google.com> wrote:

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

Is the only inconsistency you're worried about that some functions (js implementations) will still use a different naming convention, or are there others?

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Some folks saw this as a good thing: web-exposed functions are different from C++-only functions, both in that you can't change them easily, and in that you can rely on your knowledge of the web platform to figure out their meaning.

That said, if Blink folks in general want to uniformly start functions with capital letters and update the bindings generator, that's fine with me. I agree with Elliott that we wouldn't want to have functions named both parentNode() and ParentNode().

Elliott pointed out that we should probably add a feature to our clang plugin to block function names that differ only in case.

[Jeffrey Yasskin]

Dana's checking with the Google style folks.

Jeffrey

[Elliott Sprehn]

On Thu, Dec 10, 2015 at 6:44 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 6:37 PM, Jeffrey Yasskin <jyas...@google.com> wrote:

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

Is the only inconsistency you're worried about that some functions (js implementations) will still use a different naming convention, or are there others?

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Some folks saw this as a good thing: web-exposed functions are different from C++-only functions, both in that you can't change them easily, and in that you can rely on your knowledge of the web platform to figure out their meaning.

That said, if Blink folks in general want to uniformly start functions with capital letters and update the bindings generator, that's fine with me. I agree with Elliott that we wouldn't want to have functions named both parentNode() and ParentNode().

Elliott pointed out that we should probably add a feature to our clang plugin to block function names that differ only in case.

I'm fine with having lowerCase names for JS and UpperCase names for C++ as long as we don't end up in a case variation issue, it does mean exposing an already existing method requires you to go change all the internal users, but that doesn't seem too crazy to me since it makes sure you know what your'e about to do. :)

- E

[Mike Lawther]

tl,dr; 

 - is this a proposal, or a 'happening thing?'

 - are compromises being made in other Chrome projects?

 - communication about this upcoming impact hasn't been effective

On Fri, Dec 11, 2015 at 10:07 AM 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project.

Is this a 'definitely going ahead' thing? I (just) finished reading the last thread about this and it finished with "I'm trying to work out a proposal with the folks in styleguide/c++/OWNERS, which we'll bring back to here and 

chromium-dev. " (https://groups.google.com/a/chromium.org/d/msg/blink-dev/4oqLbzyMWLE/0HB8MbJqAwAJ). Is this that proposal?

Unifying coding standards is a great goal. This means compromises. I don't see any compromises/changes being suggested for Chromium's style guide? 

For something that seriously affects every single engineer on Chrome projects every single day, I don't think the communication has been effective. For example, today was actually the first day that most engineers in my office heard that Blink was going to move to 80 cols. I understand it's been something of a topic of water cooler discussion in other offices (eg SFO), but that hadn't filtered through to us. Something's not worked here.

I've dug through the archives and found a link to a form in another thread. I heard there were something like 80 responses? For the hundreds of engineers working on the Chrome projects, that kinda indicates it wasn't widely known - it was trying to survey all Chrome engineers yeah? For something of this magnitude, at a least a top level thread (like this one!) with an attention grabbing subject (like this one!) asking for feedback on a concrete proposal (like this one!) would have been great. If that is the purpose of this email, I've misunderstood, and I thank you.

    mike

[Dirk Pranke]

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

My read: somewhere between the two; it's at least a proposal and might've transitioned to a 'happening thing' if there hadn't been any push back :).

 

 - are compromises being made in other Chrome projects?

There are changes being proposed that will affect non-Blink Chrome code: see the first two bullets.

 - communication about this upcoming impact hasn't been effective

On Fri, Dec 11, 2015 at 10:07 AM 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project.

Is this a 'definitely going ahead' thing? I (just) finished reading the last thread about this and it finished with "I'm trying to work out a proposal with the folks in styleguide/c++/OWNERS, which we'll bring back to here and 

chromium-dev. " (https://groups.google.com/a/chromium.org/d/msg/blink-dev/4oqLbzyMWLE/0HB8MbJqAwAJ). Is this that proposal?

Yes.

 

Unifying coding standards is a great goal. This means compromises. I don't see any compromises/changes being suggested for Chromium's style guide? 

See above. 

 

For something that seriously affects every single engineer on Chrome projects every single day, I don't think the communication has been effective. For example, today was actually the first day that most engineers in my office heard that Blink was going to move to 80 cols.

I believe this thread is the first broad discussion of the conclusions, at least it's the first I've seen of it. You're not that far out of the loop :).

 

I understand it's been something of a topic of water cooler discussion in other offices (eg SFO), but that hadn't filtered through to us. Something's not worked here.

I've dug through the archives and found a link to a form in another thread. I heard there were something like 80 responses? For the hundreds of engineers working on the Chrome projects, that kinda indicates it wasn't widely known - it was trying to survey all Chrome engineers yeah? For something of this magnitude, at a least a top level thread (like this one!) with an attention grabbing subject (like this one!) asking for feedback on a concrete proposal (like this one!) would have been great. If that is the purpose of this email, I've misunderstood, and I thank you.

80 responses actually strikes me as pretty good given the average response rate of surveys :).

-- Dirk

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

I'm sorry Sydney doesn't feel like you had a chance to comment, and that the form didn't go out in a top-level message. Please do keep filling out https://goo.gl/forms/ChnS89os70.

Yes, this plan can still change if it looks like we've misjudged the sentiment of Blink overall. I think it's in about the right place, but if lots of people come back saying it's wrong (and the survey's how we quantify that), then it won't happen in the current form. 

We had 50-60 responses before we started looking at how to merge the styles. I was trying to get a feel for Blink's opinion, not all of Chromium. Blink has had ~250 committers since the beginning of November (vs ~550 chromium folks who didn't touch Blink), and I don't expect most of the project to care about style, so that's not particularly out of line. The form originally went out in a thread titled "Reformatting blink with `clang-format -style=Chromium`", which I also expected to catch the attention of folks who cared about style.

The first three bullets of my email are compromises Chromium's making:

• Inline virtuals will be allowed, with some restrictions on when classes can have inline constructors and destructors.
• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
• We're pursuing google3 style changes to allow default arguments and mutable references, which would carry over to Chromium. If google3 rejects these, we'll discuss Chromium allowing them independently. Blink will continue allowing default arguments and mutable references at least until that's resolved.

(The change for default arguments is actually mailed and notionally approved, but it's not submitted yet.) I tried to get a 100 column limit across all of Chromium (to match Java and reduce the number of Blink changes), but too many Blink folks in the survey responses wanted to take Chromium's style wholesale to let me get that through.

Thanks for commenting,

Jeffrey

[Matt Giuca]

It seems a little unfair to be only consulting and informing Blink developers about a change that will affect both the Chromium and Blink style guides (albeit Blink much more). I am primarily a Chromium developer, so I don't feel it is my place to answer your Blink-specific survey.

If Blink is going to adopt to Chromium style guide, then this is fine. But if you are planning to make compromises in the main Chromium code, you should consult us too. (For example, why were only Blink people consulted about changing the entire Chromium code base to 100 column limit?)

[Kentaro Hara]

On Fri, Dec 11, 2015 at 11:42 AM, Jeffrey Yasskin <jyas...@google.com> wrote:

On Thu, Dec 10, 2015 at 6:13 PM, Kentaro Hara <har...@chromium.org> wrote:

About the overall plan:

 

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*.

This sounds reasonable to me.

 

The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

How much are you going to be pushy about fixing the coding style in each directory? I'm asking this because this is going to distribute a ton of work to many teams and the opportunity cost is extremely high.

Not very pushy. If it's done in 6 months, I'd be pleasantly surprised. It'd also be best to get a smallish set of volunteers to make the changes, and let the directory owners tell them when's most convenient. That way the few volunteers can learn the pitfalls instead of each directory's owners having to learn independently. We need enough volunteers to avoid burning them out though.

This approach makes a lot of more sense to me. I will be sad if each team starts setting up a code-style-fix project into their OKRs.

Looking at other reactions in this thread, it looks a bit too premature to get people to fill in the ownership spreadsheet.

(Either way, I appreciate you moving the discussion forward. I do understand the difficulty to make this kind of thing happen :-)

 

Jeffrey

 

On Fri, Dec 11, 2015 at 11:07 AM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 11:04 AM, Elliott Sprehn <esp...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Yeah... Can we just make the code generator capitalize the method names so it's ParentNode() or leave stuff alone?

We can do that, and I think we should. Only WebIDL uses parentNode(). All C++ methods use ParentNode().

I worry about us ending up with name based variation too like parentNode() for web exposed, and ParentNode() for internal, where they have different return types.

- E

--

Kentaro Hara, Tokyo, Japan

--

Kentaro Hara, Tokyo, Japan

[Shane Stephens]

On Fri, Dec 11, 2015 at 2:24 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

My read: somewhere between the two; it's at least a proposal and might've transitioned to a 'happening thing' if there hadn't been any push back :).

 

 - are compromises being made in other Chrome projects? 

There are changes being proposed that will affect non-Blink Chrome code: see the first two bullets.

"will affect non-Blink Chrome code" is not the same as compromises. In particular, allowing new features (inline virtuals, optional argument names) doesn't actually require anyone to change the way they write code. The second bullet (optional argument names) was even already allowed by the Chromium guide, but just isn't used - which is precisely what is likely to happen with inline virtuals unless people already want to use them.

In contrast, Blink engineers are being asked to conform to a restrictive new set of rules, and to change a number of familiar coding practices. *That's* a compromise. The asymmetry of requirements on Blink engineers vs Chromium engineers here is actually getting me quite upset.

Cheers,

    -Shane

 

 - communication about this upcoming impact hasn't been effective

On Fri, Dec 11, 2015 at 10:07 AM 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project.

Is this a 'definitely going ahead' thing? I (just) finished reading the last thread about this and it finished with "I'm trying to work out a proposal with the folks in styleguide/c++/OWNERS, which we'll bring back to here and 

chromium-dev. " (https://groups.google.com/a/chromium.org/d/msg/blink-dev/4oqLbzyMWLE/0HB8MbJqAwAJ). Is this that proposal?

Yes.

 

Unifying coding standards is a great goal. This means compromises. I don't see any compromises/changes being suggested for Chromium's style guide? 

See above. 

 

For something that seriously affects every single engineer on Chrome projects every single day, I don't think the communication has been effective. For example, today was actually the first day that most engineers in my office heard that Blink was going to move to 80 cols.

I believe this thread is the first broad discussion of the conclusions, at least it's the first I've seen of it. You're not that far out of the loop :).

 

I understand it's been something of a topic of water cooler discussion in other offices (eg SFO), but that hadn't filtered through to us. Something's not worked here.

I've dug through the archives and found a link to a form in another thread. I heard there were something like 80 responses? For the hundreds of engineers working on the Chrome projects, that kinda indicates it wasn't widely known - it was trying to survey all Chrome engineers yeah? For something of this magnitude, at a least a top level thread (like this one!) with an attention grabbing subject (like this one!) asking for feedback on a concrete proposal (like this one!) would have been great. If that is the purpose of this email, I've misunderstood, and I thank you.

80 responses actually strikes me as pretty good given the average response rate of surveys :).

-- Dirk

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Marshall Greenblatt]

On Dec 10, 2015, at 11:20 PM, 'Shane Stephens' via blink-dev <blin...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 2:24 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

My read: somewhere between the two; it's at least a proposal and might've transitioned to a 'happening thing' if there hadn't been any push back :).

 

 - are compromises being made in other Chrome projects? 

There are changes being proposed that will affect non-Blink Chrome code: see the first two bullets.

"will affect non-Blink Chrome code" is not the same as compromises. In particular, allowing new features (inline virtuals, optional argument names) doesn't actually require anyone to change the way they write code. The second bullet (optional argument names) was even already allowed by the Chromium guide, but just isn't used - which is precisely what is likely to happen with inline virtuals unless people already want to use them.

In contrast, Blink engineers are being asked to conform to a restrictive new set of rules, and to change a number of familiar coding practices. *That's* a compromise. The asymmetry of requirements on Blink engineers vs Chromium engineers here is actually getting me quite upset.

Google has a style guide that all Google (and many third-party) developers should already be familiar with. Chromium (mostly) follows the Google style guide currently. Blink does not. Both projects are "owned" by Google. Consequently both projects should follow the Google style guide unless there are good (read: technical and not personal opinion) reasons to diverge. Do you disagree?

[Shane Stephens]

On Fri, Dec 11, 2015 at 3:51 PM Marshall Greenblatt <magree...@gmail.com> wrote:

On Dec 10, 2015, at 11:20 PM, 'Shane Stephens' via blink-dev <blin...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 2:24 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

My read: somewhere between the two; it's at least a proposal and might've transitioned to a 'happening thing' if there hadn't been any push back :).

 

 - are compromises being made in other Chrome projects? 

There are changes being proposed that will affect non-Blink Chrome code: see the first two bullets.

"will affect non-Blink Chrome code" is not the same as compromises. In particular, allowing new features (inline virtuals, optional argument names) doesn't actually require anyone to change the way they write code. The second bullet (optional argument names) was even already allowed by the Chromium guide, but just isn't used - which is precisely what is likely to happen with inline virtuals unless people already want to use them.

In contrast, Blink engineers are being asked to conform to a restrictive new set of rules, and to change a number of familiar coding practices. *That's* a compromise. The asymmetry of requirements on Blink engineers vs Chromium engineers here is actually getting me quite upset.

Google has a style guide that all Google (and many third-party) developers should already be familiar with.

Counter-point: many Blink engineers were hired directly onto the Blink project and are far more familiar with the existing Blink style guide. Many other third-party developers have no reason to be familiar with the Google style guide (though I appreciate that you do).

 

Chromium (mostly) follows the Google style guide currently. Blink does not. Both projects are "owned" by Google. Consequently both projects should follow the Google style guide unless there are good (read: technical and not personal opinion) reasons to diverge. Do you disagree?

Well, yes. Blink is an open source project that multiple different companies collaborate on. There is no strong reason for it to conform to Google's guide.

There's also a strong argument for not making breaking changes without good technical reasons.

Cheers,

    -Shane

[TAMURA, Kent]

On Fri, Dec 11, 2015 at 11:37 AM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 5:42 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().

Can we change it so that we just use jsStyle() as ever?

The proposed rule doesn't look good to me because

 - We need to pay huge amount of cost to adopt the proposed rule, but it won't achieve consistency with Chromium.

Is the only inconsistency you're worried about that some functions (js implementations) will still use a different naming convention, or are there others?

Yes for the former.

 

 - We'll see inconsistency in each of files.  e.g. Node::parentElement() (web-exposed) vs. Node::ParentElementOrShadowRoot() (internal)

Some folks saw this as a good thing: web-exposed functions are different from C++-only functions, both in that you can't change them easily, and in that you can rely on your knowledge of the web platform to figure out their meaning.

That said, if Blink folks in general want to uniformly start functions with capital letters and update the bindings generator, that's fine with me. I agree with Elliott that we wouldn't want to have functions named both parentNode() and ParentNode().

The idea of CaptitalLetterStartingFunctionNames + adjusting in the binding layer is reasonable.  The rule would be very simple.

 

On Thu, Dec 10, 2015 at 5:46 PM, TAMURA, Kent <tk...@chromium.org> wrote:

> 2) When files are renamed, should we ensure that #includes that change use a fully qualified path? I don't think there will be a lot of naming collisions, but there would be some, and having it read unambiguously would be nice.

I recommend to do it after moving Blink from third_party/WebKit to somewhere.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/qxA6mIVi-4I

Or maybe as part of moving out of third_party/WebKit? If a lot of files are being renamed anyway, this might be a good opportunity to get it all out of the way. We'll need similar tooling to help other developers migrate their patches across the change.

On Fri, Dec 11, 2015 at 8:07 AM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project. We'll adopt some of the most-loved parts of Blink style in Chromium, especially the parts that allow writing less code, while adopting most of Chromium's naming and formatting guidelines in Blink. Specifically:

• Inline virtuals will be allowed, with some restrictions on when classes can have inline constructors and destructors.
• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
• We're pursuing google3 style changes to allow default arguments and mutable references, which would carry over to Chromium. If google3 rejects these, we'll discuss Chromium allowing them independently. Blink will continue allowing default arguments and mutable references at least until that's resolved.
• We'll use Chromium naming guidelines for most of the codebase: file_names, member_names_, local_variable_names, ClassNames, etc. For functions, Chromium uses a mix of names_with_underscores() and CamelCase(), but Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().
• Blink will adopt Chromium's 80-column limit and the rest of clang-format's Chromium formatting. The 80-column limit does not exempt us from the rule against abbreviation.
• Blink will maintain a short style guide for refinements to and differences from the overall Chromium style, and we'll aim to make this shorter over time.

We understand that many people will be upset about parts of these decisions. None of them were made lightly and this was the best compromise we could find based off the desires people expressed. Particularly, there was enough desire to end up with a shared style, even weighted by the number of Blink commits, that we had to find some compromise.

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Jeffrey

--

TAMURA Kent
Software Engineer, Google

[Marshall Greenblatt]

On Dec 11, 2015, at 12:17 AM, Shane Stephens <sh...@google.com> wrote:

On Fri, Dec 11, 2015 at 3:51 PM Marshall Greenblatt <magree...@gmail.com> wrote:

On Dec 10, 2015, at 11:20 PM, 'Shane Stephens' via blink-dev <blin...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 2:24 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

My read: somewhere between the two; it's at least a proposal and might've transitioned to a 'happening thing' if there hadn't been any push back :).

 

 - are compromises being made in other Chrome projects? 

There are changes being proposed that will affect non-Blink Chrome code: see the first two bullets.

"will affect non-Blink Chrome code" is not the same as compromises. In particular, allowing new features (inline virtuals, optional argument names) doesn't actually require anyone to change the way they write code. The second bullet (optional argument names) was even already allowed by the Chromium guide, but just isn't used - which is precisely what is likely to happen with inline virtuals unless people already want to use them.

In contrast, Blink engineers are being asked to conform to a restrictive new set of rules, and to change a number of familiar coding practices. *That's* a compromise. The asymmetry of requirements on Blink engineers vs Chromium engineers here is actually getting me quite upset.

Google has a style guide that all Google (and many third-party) developers should already be familiar with.

Counter-point: many Blink engineers were hired directly onto the Blink project and are far more familiar with the existing Blink style guide.

Granted, it will take a bit of effort for existing Blink engineers to familiarize themselves with any changes. However, once the changes are made, it will be that much easier for new engineers to move into the Blink project from elsewhere at Google/Chromium. I expect that will become more frequent as the "onion soup" refactoring effort gains momentum and many chunks of the current Chromium code base move under the Blink umbrella.

Many other third-party developers have no reason to be familiar with the Google style guide (though I appreciate that you do).

There are actually quite a few companies who use the Google style guide. It's complete and well-reasoned/documented which makes in an easy choice for companies without long code legacy or their own style committees.

 

Chromium (mostly) follows the Google style guide currently. Blink does not. Both projects are "owned" by Google. Consequently both projects should follow the Google style guide unless there are good (read: technical and not personal opinion) reasons to diverge. Do you disagree?

Well, yes. Blink is an open source project that multiple different companies collaborate on. There is no strong reason for it to conform to Google's guide.

Many companies are open sourcing projects these days. If each open source project has its own unique style it quickly becomes a collective headache. Let's do our small part to make it easier instead of harder for developers to work on multiple projects.

There's also a strong argument for not making breaking changes without good technical reasons.

Personally, I'm much more concerned about possible breakage resulting from the upcoming refactoring efforts. Blink/Chromium will be a very different beast in a few years. Any style changes that we make now are likely small fry by comparison.

[Ken Rockot]

On Thu, Dec 10, 2015 at 9:17 PM, 'Shane Stephens' via blink-dev <blin...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 3:51 PM Marshall Greenblatt <magree...@gmail.com> wrote:

On Dec 10, 2015, at 11:20 PM, 'Shane Stephens' via blink-dev <blin...@chromium.org> wrote:

On Fri, Dec 11, 2015 at 2:24 PM Dirk Pranke <dpr...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

My read: somewhere between the two; it's at least a proposal and might've transitioned to a 'happening thing' if there hadn't been any push back :).

 

 - are compromises being made in other Chrome projects? 

There are changes being proposed that will affect non-Blink Chrome code: see the first two bullets.

"will affect non-Blink Chrome code" is not the same as compromises. In particular, allowing new features (inline virtuals, optional argument names) doesn't actually require anyone to change the way they write code. The second bullet (optional argument names) was even already allowed by the Chromium guide, but just isn't used - which is precisely what is likely to happen with inline virtuals unless people already want to use them.

In contrast, Blink engineers are being asked to conform to a restrictive new set of rules, and to change a number of familiar coding practices. *That's* a compromise. The asymmetry of requirements on Blink engineers vs Chromium engineers here is actually getting me quite upset.

Google has a style guide that all Google (and many third-party) developers should already be familiar with.

Counter-point: many Blink engineers were hired directly onto the Blink project and are far more familiar with the existing Blink style guide. Many other third-party developers have no reason to be familiar with the Google style guide (though I appreciate that you do).

Many developers work with both Blink and Chromium code, and this is only going to increase with time. Frequently switching between the two styles imposes some persistent and non-trivial cognitive overhead. On the other hand, switching permanently to a unified style imposes only a one-time cost per developer. Zero cost per new developer.

IMHO for such a large project this stands on its own as an utterly sufficient argument in favor of convergence.

Chromium (mostly) follows the Google style guide currently. Blink does not. Both projects are "owned" by Google. Consequently both projects should follow the Google style guide unless there are good (read: technical and not personal opinion) reasons to diverge. Do you disagree?

Well, yes. Blink is an open source project that multiple different companies collaborate on. There is no strong reason for it to conform to Google's guide.

Yes, Blink is an open source project and it has many contributors outside of Google, but its development is still driven primarily by Google engineers. If you accept the premise that convergence is a worthwhile endeavor, what would be the practical value of converging on any other style?

There's also a strong argument for not making breaking changes without good technical reasons.

What is breaking?

[Mike Lawther]

Hey Jeffrey,

On Fri, Dec 11, 2015 at 2:49 PM Jeffrey Yasskin <jyas...@google.com> wrote:

On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org> wrote:

tl,dr; 

 - is this a proposal, or a 'happening thing?'

 - are compromises being made in other Chrome projects?

 - communication about this upcoming impact hasn't been effective

On Fri, Dec 11, 2015 at 10:07 AM 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project.

Is this a 'definitely going ahead' thing? I (just) finished reading the last thread about this and it finished with "I'm trying to work out a proposal with the folks in styleguide/c++/OWNERS, which we'll bring back to here and 

chromium-dev. " (https://groups.google.com/a/chromium.org/d/msg/blink-dev/4oqLbzyMWLE/0HB8MbJqAwAJ). Is this that proposal?

Unifying coding standards is a great goal. This means compromises. I don't see any compromises/changes being suggested for Chromium's style guide? 

For something that seriously affects every single engineer on Chrome projects every single day, I don't think the communication has been effective. For example, today was actually the first day that most engineers in my office heard that Blink was going to move to 80 cols. I understand it's been something of a topic of water cooler discussion in other offices (eg SFO), but that hadn't filtered through to us. Something's not worked here.

I've dug through the archives and found a link to a form in another thread. I heard there were something like 80 responses? For the hundreds of engineers working on the Chrome projects, that kinda indicates it wasn't widely known - it was trying to survey all Chrome engineers yeah? For something of this magnitude, at a least a top level thread (like this one!) with an attention grabbing subject (like this one!) asking for feedback on a concrete proposal (like this one!) would have been great. If that is the purpose of this email, I've misunderstood, and I thank you.

I'm sorry Sydney doesn't feel like you had a chance to comment, and that the form didn't go out in a top-level message. Please do keep filling out https://goo.gl/forms/ChnS89os70.

Thanks - I'll make sure that everyone here knows that filling it in is still useful.

 

Yes, this plan can still change if it looks like we've misjudged the sentiment of Blink overall. I think it's in about the right place, but if lots of people come back saying it's wrong (and the survey's how we quantify that), then it won't happen in the current form. 

OK cool, that's great to know - thank you! 

 

We had 50-60 responses before we started looking at how to merge the styles. I was trying to get a feel for Blink's opinion, not all of Chromium. Blink has had ~250 committers since the beginning of November (vs ~550 chromium folks who didn't touch Blink), and I don't expect most of the project to care about style, so that's not particularly out of line. The form originally went out in a thread titled "Reformatting blink with `clang-format -style=Chromium`", which I also expected to catch the attention of folks who cared about style.

Yep, I did see that thread, and I saw you'd been appointed, but then it got very long, and what became the lede (the survey) was buried in it. Yep, there is some mea culpa there, and some improvements I think we can make to my own teams informal communications. 

I'd also thought (until I read it) that the survey was intended for all Chrome devs (ie the 250 + 550), which is why 80 seemed quite a low response rate. I see that was not right.

 

The first three bullets of my email are compromises Chromium's making:

• Inline virtuals will be allowed, with some restrictions on when classes can have inline constructors and destructors.
• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
• We're pursuing google3 style changes to allow default arguments and mutable references, which would carry over to Chromium. If google3 rejects these, we'll discuss Chromium allowing them independently. Blink will continue allowing default arguments and mutable references at least until that's resolved.

Thanks for clarifying this and giving some of the color behind it.

 

(The change for default arguments is actually mailed and notionally approved, but it's not submitted yet.) I tried to get a 100 column limit across all of Chromium (to match Java and reduce the number of Blink changes), but too many Blink folks in the survey responses wanted to take Chromium's style wholesale to let me get that through.

If that is the fair dinkum consensus of the Blink hive-mind, then no worries - that is what we should do. My main worry was that a lot of the mind was not actively aware it was being consulted.

 

Thanks for commenting,

And thank you. Really. This is never ever an easy thing.

 

Jeffrey

[Peter Kasting]

On Thu, Dec 10, 2015 at 8:20 PM, 'Shane Stephens' via blink-dev <blin...@chromium.org> wrote:

In contrast, Blink engineers are being asked to conform to a restrictive new set of rules, and to change a number of familiar coding practices. *That's* a compromise. The asymmetry of requirements on Blink engineers vs Chromium engineers here is actually getting me quite upset.

So, to be clear:

* When asking for feedback, the process focused on getting feedback from engineers specifically working on Blink, not all of Chromium

* When determining what compromise positions to accept, Jeff has mentioned he was specifically guided by Blink engineer feedback (e.g. in going ahead with 80 columns vs. 100)

And given these:

* You're upset that the result of a process heavily biased towards respecting Blink engineer feedback seems to make more changes to Blink style than Chromium style

Perhaps you should be upset that many of your fellow Blink engineers seem to disagree with your position on what a combined style should look like.  An 80-person response rate, given the number of Blink developers, is pretty darn high.

PK

[Jeffrey Yasskin]

I'll reply in detail to the rest of the thread later, but I wanted to interject that "it's a Google project" wasn't part of any of these discussions. Shane's right: it's far from just a Google project. About 2/3 of new contributors come without Google experience, and we have to welcome them.

The bias in favor of the Chromium style guide (given that folks want to unify in the first place) is because it's the larger part of the project, so migrating Blink is cheaper, and Chromium uses the Google guide for historical reasons and because there's a perception that it reduces style discussions.

Jeffrey

[Ojan Vafai]

I write this hoping that this will be the last message on this thread (unless Jeffrey wants to respond to any specific concerns of course). Please strongly consider the added value of responding to anything else in this thread before doing so. I'm not trying to shut down open discussion, but I'm skeptical further discussion on this thread will be fruitful.

I understand that the previous thread had a "we will not change under any circumstances" tone to it, but in practice what I witnessed was an honest willingness to compromise on both sides and a well-intentioned effort to find the most pragmatic middle ground.

I'll make a few points to try and close the conversation.

1. Jeffrey reached out for broad feedback via the survey before making decisions. There was a mistake of not posting it in a new top-level thread so some folks were lost in the process. It was a reasonable mistake to assume that everyone who cared strongly would continue paying attention to the thread. That said, the responses were actually from a broad swath of engineers all over the world. I don't personally think any group was egregiously underrepresented.
   
2. Jeffrey is open to revising if sufficient new feedback comes in that would affect any of the decisions. You can't really ask for more than that in terms of fairness IMO.
   
3. Making these decisions is really hard. Every decision here will have a large group of people upset. There's just no getting around that. Dimitri appointed a decider specifically to make those hard decisions. From what I observed, Jeffrey was extremely impartial in doing so.
   
4. Discussing the details of these style discussions on an enormous mailing list is mostly just divisive. If there is a specific concern you have, the most effective way to get it considered is to fill out Jeffrey's survey. I think it's really unlikely that you will have a technical point he hasn't already considered, but the actual decision making was almost entirely based off the survey responses, so if the numbers change, then the conclusions might as well. Again, it's hard to be more fair than that.
   
5. On all the semantic issues that affect the bits we actually send to users, Blink was able to keep it's current style and now we get to use those same patterns in all chromium code (modulo the couple things that aren't 100% resolved yet). That's pretty huge and a big benefit for the entire project.
   
6. FWIW, as someone who thinks the Google style is comically bad in it's aesthetic choices, I think this compromise is actually the best pragmatic solution for the long-term health of the project. So this isn't just me defending a style I happen to like.
   

[Daniel Bratell]

On Fri, 11 Dec 2015 00:07:17 +0100, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

 we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*.

If someone renames Melon.h to melon.h case insensitive filesystems will be unhappy (maybe git is better at handling it than in the past, maybe not).

There are files like that: File.h, Blob.h, Editor.h, Event.h, ...

Also, if files are refomatted and renamed at the same time you will kill git's move detection so I would suggest doing the renaming in a separate step.

/Daniel

--

/* Opera Software, Linköping, Sweden: CET (UTC+1) */

[David Benjamin]

On Fri, Dec 11, 2015 at 4:13 AM Daniel Bratell <bra...@opera.com> wrote:

On Fri, 11 Dec 2015 00:07:17 +0100, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

 we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*.

If someone renames Melon.h to melon.h case insensitive filesystems will be unhappy (maybe git is better at handling it than in the past, maybe not).

There are files like that: File.h, Blob.h, Editor.h, Event.h, ...

I've done it in third_party/tlslite before after upstream kindly lowercased all the files. The try job and CQ infrastructure (https://crbug.com/355812) blows up on Windows and Mac bots, but if you land it manually, git copes fine. (At least, it doesn't seem to be public knowledge that I had to do that, so it evidently didn't cause problems! ;-) )

David

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 8:04 PM, Matt Giuca <mgi...@chromium.org> wrote:
> It seems a little unfair to be only consulting and informing Blink
> developers about a change that will affect both the Chromium and Blink style
> guides (albeit Blink much more). I am primarily a Chromium developer, so I
> don't feel it is my place to answer your Blink-specific survey.
>
> If Blink is going to adopt to Chromium style guide, then this is fine. But
> if you are planning to make compromises in the main Chromium code, you
> should consult us too. (For example, why were only Blink people consulted
> about changing the entire Chromium code base to 100 column limit?)

Yep, it would be unfair to change Chromium style while only consulting
Blink, so we didn't do it. :) All of the changes to Chrome style are
going through either chromium-dev
(https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/discussion),
Chromium's style owners, or the Google style list (default arguments,
soon mutable references). A change to the column limit didn't get past
Chromium's style owners, so it didn't need to bother chromium-dev.

Jeffrey

[Jeffrey Yasskin]

Thanks. I'm starting a document of technical constraints on the style
migration at https://docs.google.com/document/d/1k4oo31_KBB1B9uYcrf_4FIlbj4EXAl0ypqV-FflQyKM/edit,
and have added your comments.

[Dirk Pranke]

At the risk of opening another can of worms, would it make sense to move files to //blink as a part of

the grand renaming and reformatting? It seems like it might since you already have to touch build files

and include paths, and it would avoid the case-sensitivity rename-in-place issue,

but maybe it just makes a complicated thing even more complicated.

Thoughts?

-- Dirk

[Jeffrey Yasskin]

tkent mentioned this earlier: I think it makes a lot of sense, but I
also don't know what folks have been thinking about the //blink move
in general. It probably deserves a dedicated thread? Let's let this
one die down a bit first so the stylistic direction is more clear.

Jeffrey

[Nico Weber]

Someone (davidben?) pointed out that reformatting and renaming in one go will confuse git anyhoo, so the grand formatting should happen independently of the grand renaming. We could combine the grand renaming with the grand moving around maybe though.

[Jeffrey Yasskin]

On Thu, Dec 10, 2015 at 8:20 PM, Shane Stephens <sh...@google.com> wrote:
>
>
> On Fri, Dec 11, 2015 at 2:24 PM Dirk Pranke <dpr...@chromium.org> wrote:
>>
>> On Thu, Dec 10, 2015 at 7:03 PM, Mike Lawther <mikel...@chromium.org>
>> wrote:
>>>
>>> tl,dr;
>>> - is this a proposal, or a 'happening thing?'
>>
>>
>> My read: somewhere between the two; it's at least a proposal and might've
>> transitioned to a 'happening thing' if there hadn't been any push back :).
>>
>>>
>>> - are compromises being made in other Chrome projects?
>>
>>
>> There are changes being proposed that will affect non-Blink Chrome code:
>> see the first two bullets.
>
>
> "will affect non-Blink Chrome code" is not the same as compromises. In
> particular, allowing new features (inline virtuals, optional argument names)
> doesn't actually require anyone to change the way they write code. The
> second bullet (optional argument names) was even already allowed by the
> Chromium guide, but just isn't used - which is precisely what is likely to
> happen with inline virtuals unless people already want to use them.
>
> In contrast, Blink engineers are being asked to conform to a restrictive new
> set of rules, and to change a number of familiar coding practices. *That's*
> a compromise. The asymmetry of requirements on Blink engineers vs Chromium
> engineers here is actually getting me quite upset.

Thanks for caring, and for filling out https://goo.gl/forms/ChnS89os70.

Allowing new features requires people to change the way they *read*
code, which is really the point of a style guide. It looks like a pure
improvement from your standpoint, but pkasting, for example, has been
sad about most of the newly-allowed code for Chromium.

I'm going to add you and the other 80-column-lovers-in-the-survey to a
previous thread about the column limit, and we'll see where it goes.

Jeffrey

[Jeffrey Yasskin]

Daniel's point was that we need to do them in separate commits, but we
should try to only make folks use the rebase script once. We do need
to double-check that git doesn't get confused across whatever we plan
to commit.

[Matt Giuca]

Right. My concern (on behalf of Chromium style) is not what I will be *disallowed* from doing, but what I and my colleagues will be *allowed* to do (and therefore, not only what I have to read, but also what I have to accept in my own code as a reviewer).

In particular, allowing non-const references seems like a disaster. It means whenever I call a function, my local variable might be silently mutated. (I know this particular one isn't being proposed explicitly yet, but it was mentioned.) Also, optional argument names was discussed earlier in this thread and it seems it's only allowed by Chromium in a very liberal reading of the style guide.

Shane: I'm aware of the asymmetry, and the fact that under this proposal, Blink engineers will be getting the bulk of the changes. But the changes being proposed to the Chromium style are still significant and warrant further discussion with Chromium engineers before proceeding.

[Dana Jansens]

So, I took our interpretation to c-s...@google.com, and no one had a problem with it. The only reply it received was basically, to make reasonable judgements.

[Peter Kasting]

On Mon, Dec 14, 2015 at 12:06 PM, Dana Jansens <dan...@chromium.org> wrote:

So, I took our interpretation to c-s...@google.com, and no one had a problem with it. The only reply it received was basically, to make reasonable judgements.

You mean, regarding the issue of optional argument names?

PK 

[Dominic Mazzoni]

On Sun, Dec 13, 2015 at 3:21 PM Matt Giuca <mgi...@chromium.org> wrote:

In particular, allowing non-const references seems like a disaster. It means whenever I call a function, my local variable might be silently mutated.

I'd like to better understand what's being proposed here. Maybe I missed it, but in the New Blink Style Guide I only see a proposal to make out-arguments non-const references. The example (MyClass::GetSomeValue) is pretty obvious, but that doesn't seem to me to be the common case. Blink makes heavy use of passing non-const references, and definitely not only for "getter"-type functions. In google3 and Chromium this is disallowed.

I don't have a strong preference for one or the other. The issue is that both codebases evolved for a long time under a different set of assumptions and these assumptions are baked into the style. For example, I think that someone writing Chromium and Blink code to do the same might pick a different function name and argument names because in Chromium the argument types imply mutability whereas in Blink it might need to be conveyed by the function name or argument name.

So my concern is that it will be hard to change code in either direction without impacting readability and safety. Switching Blink to use pointers instead of non-const references would require adding a bunch of null checks, and potentially losing out on some places where the compiler was helping to catch bugs. Going the opposite direction and switching Chromium to allow non-const references everywhere could make existing code a lot harder to read because it won't be obvious which parameters are mutable.

[Peter Kasting]

On Mon, Dec 14, 2015 at 12:23 PM, Dominic Mazzoni <dmaz...@chromium.org> wrote:

Switching Blink to use pointers instead of non-const references would require adding a bunch of null checks,

For clarity, in Chrome you not only don't need to null-check pointers which aren't supposed to be null, you explicitly shouldn't, primarily because it misleads readers into thinking an argument can be non-null.  At most, add a DCHECK to notify the reader the pointer is non-null.

So if code in Blink currently uses non-const refs, that code could be changed to use pointers without adding additional null checks (since all those cases are guaranteed not to be null).

PK

[Dana Jansens]

Yepyep.

[Jeffrey Yasskin]

On Mon, Dec 14, 2015 at 12:23 PM, Dominic Mazzoni <dmaz...@chromium.org> wrote:

We should have the pointers vs references discussion on chromium-dev rather than blink-dev, and probably after it's been re-discussed inside Google as a potential change to the base style guide.

Jeffrey

[Dimitri Glazkov]

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Let's be a bit more organized than per-directory, distributed transition. The obviously failure mode here is that we'll end up with a psychotic code base that is half-pregnant with both styles.

May I propose that, if we decide to do this, we:

1) get a better handle on the total cost of the transition

2) coordinate a scoped/small period of time in the future to do this transition?

:DG<

[Matt Giuca]

On Tue, 15 Dec 2015 at 10:00 Dimitri Glazkov <dgla...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Let's be a bit more organized than per-directory, distributed transition. The obviously failure mode here is that we'll end up with a psychotic code base that is half-pregnant with both styles.

We don't *have* to end up with the exact same style in both parts. I think we should decide on a per-issue basis whether a) Chromium should change to match Blink style, b) Blink should change to match Chromium style, or c) it isn't worth the cost of migration and both parts should retain their historical style (possibly with a recommendation for new code, but old code doesn't have to migrate).

In the case of non-const references, I would suggest that is quite harmful to allow (that is just my opinion, but it is backed up by years of Chromium and Google3 style rules). But I understand this is commonplace in Blink and it would be a tremendous overhead to "fix" all of the non-const references into pointers. So we should go with (c) on this. Perhaps recommend that new code in Blink prefers pointers over non-const refs, but ban it in non-Blink directories and allow it inside Blink for historical reasons.

This doesn't have to be all or nothing.

[Colin Blundell]

On Tue, Dec 15, 2015 at 12:45 AM Matt Giuca <mgi...@chromium.org> wrote:

On Tue, 15 Dec 2015 at 10:00 Dimitri Glazkov <dgla...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Let's be a bit more organized than per-directory, distributed transition. The obviously failure mode here is that we'll end up with a psychotic code base that is half-pregnant with both styles.

We don't *have* to end up with the exact same style in both parts. I think we should decide on a per-issue basis whether a) Chromium should change to match Blink style, b) Blink should change to match Chromium style, or c) it isn't worth the cost of migration and both parts should retain their historical style (possibly with a recommendation for new code, but old code doesn't have to migrate).

If I understood correctly, Dimitri is worried about the failure case where Blink itself ends up being a mix of both styles on a per-directory basis in the long-term.

[Raymond Toy]

On Mon, Dec 14, 2015 at 3:45 PM, Matt Giuca <mgi...@chromium.org> wrote:

On Tue, 15 Dec 2015 at 10:00 Dimitri Glazkov <dgla...@chromium.org> wrote:

On Thu, Dec 10, 2015 at 3:07 PM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Let's be a bit more organized than per-directory, distributed transition. The obviously failure mode here is that we'll end up with a psychotic code base that is half-pregnant with both styles.

We don't *have* to end up with the exact same style in both parts. I think we should decide on a per-issue basis whether a) Chromium should change to match Blink style, b) Blink should change to match Chromium style, or c) it isn't worth the cost of migration and both parts should retain their historical style (possibly with a recommendation for new code, but old code doesn't have to migrate).

If the styles are being merged, I think we *do* have to end up with the exact same style.  If it's too hard today when the styles are pretty distinct, what happens later when they're almost the same but diffeent?  The burden in even higher to know the correct style if they almost look the same but aren't.  Today, it's pretty easy to tell:  2-space indent means Chromium, 4 is blink.

If the styles must converge, I want the style to be truly uniform.  I don't care what, but uniform with no exceptions between the trees. 

In the case of non-const references, I would suggest that is quite harmful to allow (that is just my opinion, but it is backed up by years of Chromium and Google3 style rules). But I understand this is commonplace in Blink and it would be a tremendous overhead to "fix" all of the non-const references into pointers. So we should go with (c) on this. Perhaps recommend that new code in Blink prefers pointers over non-const refs, but ban it in non-Blink directories and allow it inside Blink for historical reasons.

This doesn't have to be all or nothing.

 

May I propose that, if we decide to do this, we:

1) get a better handle on the total cost of the transition

2) coordinate a scoped/small period of time in the future to do this transition?

:DG<

--

[Jeffrey Yasskin]

Sounds good. dcheng has started a tool to rename variables and functions, rather than relying on individual work per directory, so it looks like we'll be able to avoid a transitional period there. 

[TAMURA, Kent]

I noticed that the new style guide didn't mention config.h.  We need some effort to remove it.

On Fri, Dec 11, 2015 at 8:07 AM, 'Jeffrey Yasskin' via blink-dev <blin...@chromium.org> wrote:

Hi blink-dev,

Based on your feedback, Nico, Dana, and I have come up with a plan to converge Blink and Chromium style so that it's easier to work across both parts of our project. We'll adopt some of the most-loved parts of Blink style in Chromium, especially the parts that allow writing less code, while adopting most of Chromium's naming and formatting guidelines in Blink. Specifically:

• Inline virtuals will be allowed, with some restrictions on when classes can have inline constructors and destructors.
• Argument names are optional in headers across all of Chromium. (This was already allowed, but it wasn't clear, so nobody had noticed.) Blink will keep requiring this; individual Chromium teams can adopt it if they want.
• We're pursuing google3 style changes to allow default arguments and mutable references, which would carry over to Chromium. If google3 rejects these, we'll discuss Chromium allowing them independently. Blink will continue allowing default arguments and mutable references at least until that's resolved.

• We'll use Chromium naming guidelines for most of the codebase: file_names, member_names_, local_variable_names, ClassNames, etc. For functions, Chromium uses a mix of names_with_underscores() and CamelCase(), but Blink will standardize on CamelCase(). However, C++ functions that implement JS functions get an exception and will be named in jsStyle().
• Blink will adopt Chromium's 80-column limit and the rest of clang-format's Chromium formatting. The 80-column limit does not exempt us from the rule against abbreviation.
• Blink will maintain a short style guide for refinements to and differences from the overall Chromium style, and we'll aim to make this shorter over time.

We understand that many people will be upset about parts of these decisions. None of them were made lightly and this was the best compromise we could find based off the desires people expressed. Particularly, there was enough desire to end up with a shared style, even weighted by the number of Blink commits, that we had to find some compromise.

These are the long-term plans, but we'll stage the migration in a way that we hope is minimally disruptive to the project. Once https://crbug.com/563793 paves the way, we'd like to get volunteers to make 2 changes up-front: clang-formatting the code to 80 columns, and renaming files from ClassName.* to class_name.*. The rest of the changes are harder to automate, so we'll leave them to individual directory owners/contributors to schedule when it's convenient. Please add yourself to this spreadsheet if you own a directory or want to work on the conversion for one. Please don't "fix" things that aren't prohibited by the Chromium style guide; we want to get into a consistent state with as little churn as possible.

Jeffrey

--

TAMURA Kent
Software Engineer, Google

[Thiago Farina]

On Wed, Dec 16, 2015 at 4:48 AM, TAMURA, Kent <tk...@chromium.org> wrote:

I noticed that the new style guide didn't mention config.h.  We need some effort to remove it.

+1!

wtf/build_config.h to rule it.

--

Thiago Farina