Chromium JavaScript Style guide: 'const'

182 views
Skip to first unread message

Eric Lawrence

unread,
Mar 21, 2017, 9:16:47 PM3/21/17
to Chromium-dev
The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?

thanks!

Tim Sergeant

unread,
Mar 21, 2017, 11:27:38 PM3/21/17
to elaw...@chromium.org, Chromium-dev, Dan Beam
For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/8d219f95-6a13-44d5-bab8-a7d34340d110%40chromium.org.

Dan Beam

unread,
Mar 22, 2017, 1:31:28 AM3/22/17
to Tim Sergeant, Eric Lawrence, Chromium-dev, Adam Klein
On Wed, Mar 22, 2017 at 2:26 PM, Tim Sergeant <tser...@chromium.org> wrote:
For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

Yeah, I suspect tooling and feature support are likely more the issue these days.
 

On 22 March 2017 at 12:16, Eric Lawrence <elaw...@chromium.org> wrote:
The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?
It likely depends on where and how you're using it.  It's probably fine but you should talk with somebody that works on v8 / turbofan to be sure.  +adamk@ 

-- Dan

Adam Klein

unread,
Mar 22, 2017, 11:49:47 AM3/22/17
to Dan Beam, Tim Sergeant, Eric Lawrence, Chromium-dev
On Tue, Mar 21, 2017 at 10:30 PM, Dan Beam <db...@chromium.org> wrote:
On Wed, Mar 22, 2017 at 2:26 PM, Tim Sergeant <tser...@chromium.org> wrote:
For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

Yeah, I suspect tooling and feature support are likely more the issue these days.
 

On 22 March 2017 at 12:16, Eric Lawrence <elaw...@chromium.org> wrote:
The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?
It likely depends on where and how you're using it.  It's probably fine but you should talk with somebody that works on v8 / turbofan to be sure.  +adamk@ 

If worried about deopt issues, the best strategy would be to wait until we're fully on the ignition/turbofan pipeline, which is targeting M59. The deopt sometimes seen with const (and let) is due to Crankshaft, which will be retired once the new pipeline is the default.
 
- Adam

John Williams

unread,
Jun 13, 2017, 7:16:49 PM6/13/17
to Chromium-dev, db...@chromium.org, tser...@chromium.org, elaw...@chromium.org
Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

henderson...@gmail.com

unread,
Jun 13, 2017, 7:34:41 PM6/13/17
to chromi...@chromium.org

Email that pays.

From Stewart “bowlofstew” Henderson:

Thanks for emailing me! No, I haven’t been hacked :)

I signed up for a spam filtering service called BitBounce. To deliver your email to my inbox, please click the link below and pay the small Bitcoin fee. Thanks!

Select One:

I know you personally.

I have an email Promo Code.

We've never met. I'll pay your BitBounce fee to deliver my email

BitBounce is a product by:

Turing Cloud

Turing-Cloud.com/bitbounce

Redwood City, CA

BitBounce integrates with:

CoinBase

CoinBase.com

San Francisco, CA

henderson...@gmail.com

unread,
Jun 13, 2017, 7:48:24 PM6/13/17
to chromi...@chromium.org

Dan Beam

unread,
Jun 14, 2017, 1:33:06 PM6/14/17
to John Williams, Chromium-dev, Tim Sergeant, Eric Lawrence, Demetrios Papadopoulos
On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.


(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

-- Dan


On Wednesday, March 22, 2017 at 8:49:47 AM UTC-7, Adam Klein wrote:
On Tue, Mar 21, 2017 at 10:30 PM, Dan Beam <db...@chromium.org> wrote:
On Wed, Mar 22, 2017 at 2:26 PM, Tim Sergeant <tser...@chromium.org> wrote:
For ES6/Harmony features like const, the style guide is superseded by the ES6 Support document, which lists const as 'To Be Discussed'.

Some extra concerns besides performance are whether it is supported by uglifyjs (it seems like it is) and by iOS (seemingly iOS 10+ only).

Yeah, I suspect tooling and feature support are likely more the issue these days.
 

On 22 March 2017 at 12:16, Eric Lawrence <elaw...@chromium.org> wrote:
The Chromium JavaScript style guide no longer mentions that 'const' is not permitted in JavaScript. 

That prohibition was dropped in October 2016 in Revision #50. The V8 Issue cited as the original justification was marked as "Won't Fix" with the
note that the behavior of 'const' had changed and should not have the performance impact that led to its original ban.

In a recent CL, I was told that the DevTools still do not use const and existing usage is being removed due to V8 deoptimization concerns.

Should the ban (or a warning) be added back to the JavaScript style guide? Will this concern go away in a future version of V8?
It likely depends on where and how you're using it.  It's probably fine but you should talk with somebody that works on v8 / turbofan to be sure.  +adamk@ 

If worried about deopt issues, the best strategy would be to wait until we're fully on the ignition/turbofan pipeline, which is targeting M59. The deopt sometimes seen with const (and let) is due to Crankshaft, which will be retired once the new pipeline is the default.
 
- Adam

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

John Williams

unread,
Jun 27, 2017, 4:18:19 PM6/27/17
to Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, Demetrios Papadopoulos
On Wed, Jun 14, 2017 at 10:31 AM, Dan Beam <db...@chromium.org> wrote:
On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.

I'll do that.
 


(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

I think maybe I'm missing something. Are you saying the Chromium tree contains JS code that needs to run in Safari on iOS 9, and that's a sufficient reason to ban const in the entire Chromium tree?

John Williams

unread,
Jun 27, 2017, 4:30:26 PM6/27/17
to Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, Demetrios Papadopoulos
Looking at the style guide, I see quite a few ES6 features we use that are in the "to be discussed" category. I'm worried this means I'll need to either get a whole lot of ES6 features approved, or make a lot of changes to the code I'm releasing that will be generally detrimental to the quality of the code. That seems like a high barrier to meet just to include some code in the Chromium tree that is already in production as a de-facto part of Chrome.

Nico Weber

unread,
Jun 27, 2017, 5:48:37 PM6/27/17
to John Williams, Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, Demetrios Papadopoulos
On Tue, Jun 27, 2017 at 4:28 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
Looking at the style guide, I see quite a few ES6 features we use that are in the "to be discussed" category. I'm worried this means I'll need to either get a whole lot of ES6 features approved, or make a lot of changes to the code I'm releasing that will be generally detrimental to the quality of the code. That seems like a high barrier to meet just to include some code in the Chromium tree that is already in production as a de-facto part of Chrome.

Which code are you referring to?

As soon as it's in the chrome repo, chrome team is responsible for maintaining it. It doesn't seem very surprising that we should have unified requirements on our code.
 

Demetrios Papadopoulos

unread,
Jun 28, 2017, 3:16:32 AM6/28/17
to Nico Weber, John Williams, Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence
One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

Now regardless of the JS toolchain ES6 support, the other reason we've setup a process of proposing ES6 features, one by one, is because not necessarily all ES6 (or ES7) features will be non-controversial to use (similar to the C++11 feature proposal process). The ones that come to mind that fall in this category are generators, and async/await.

You can find more context about the thinking behind adding such a process at the original discussion.

Thanks,
Demetrios

Sylvain Defresne

unread,
Jun 28, 2017, 5:42:40 AM6/28/17
to j...@google.com, Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, Demetrios Papadopoulos
On Tue, Jun 27, 2017 at 10:16 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
On Wed, Jun 14, 2017 at 10:31 AM, Dan Beam <db...@chromium.org> wrote:
On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.

I'll do that.
 


(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

I think maybe I'm missing something. Are you saying the Chromium tree contains JS code that needs to run in Safari on iOS 9, and that's a sufficient reason to ban const in the entire Chromium tree?

Some of the javascript code is run by Chrome on iOS. This includes some code for webui, but also some code from components (like translate code, or distiller code). This javascript will be run in a WKWebView that uses the same JS engine as Safari on iOS. That shared code still need to run on iOS and the version of the OS supported is iOS 9 and higher.

So if Safari does not support "const", then we should ensure that all javascript run on Chrome on iOS (from the Chromium repository) does not use features unsupported. I don't think we should ban "const" just for that reason if we can find a way to make the code work on iOS (maybe we can transpile from ES6 to ES5 or something else).
-- Sylvain
 

John Williams

unread,
Jun 28, 2017, 8:48:01 PM6/28/17
to Sylvain Defresne, Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, Demetrios Papadopoulos, mfo...@chromium.org
On Wed, Jun 28, 2017 at 2:41 AM, Sylvain Defresne <sdef...@chromium.org> wrote:
On Tue, Jun 27, 2017 at 10:16 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
On Wed, Jun 14, 2017 at 10:31 AM, Dan Beam <db...@chromium.org> wrote:
On Tue, Jun 13, 2017 at 4:16 PM, 'John Williams' via Chromium-dev <chromi...@chromium.org> wrote:
Ping.

I'm working on migrating about 20k lines of JS code from Google's repository to Chromium, and it uses const all over the place. Replacing every usage of const with the linter's suggestion of "/*const*/ var" seems like a really terrible idea to me. Is there any way to disable this feature, at least for a particular directory?

I don't see anything about const in the markdown version of the web styleguide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md

However, it is an ES6 feature, and there's a separate doc about what we've vetted so far:

It'd make sense to cross-link or combine these docs eventually.

Many Chromium .js files are run through tools that parse source code.  Examples: closure compiler, vulcanize/polymer-bundler, eslint, uglify.  It's generally preferred that they all handle new syntaxes well before allowing everywhere (because it's hard to express/follow "this feature is allowed but only for some directories").  I'm also leary of potential performance problems, so we've been consulting the v8 team where possible (which is why I added adamk@ on your previous email).

Uglify was the most recent tool holding us back and was very recently updated to better handle ES6 features.  Doing compatibility research is the fastest and safest way to introduce your feature.

tl;dr - if we want to use const, I think it should be proposed to chromium-dev@ like I did with => (which we can probably finally start using now).  This matches what we did for C++, as described here.  Might make sense to propose let and const together.

I'll do that.
 


(If it matters, the code in question is only ever meant to run in Chromium, and only on desktop platforms.)

That simplifies things, but I believe Chrome for iOS still supports iOS 9 which has dodgy ES6 support.  If you use ES6 features (like const) in src/ui/webui/ it could break stuff.  https://kangax.github.io/compat-table/es6/ says that iOS9 supports only a small part of const.

I think maybe I'm missing something. Are you saying the Chromium tree contains JS code that needs to run in Safari on iOS 9, and that's a sufficient reason to ban const in the entire Chromium tree?

Some of the javascript code is run by Chrome on iOS. This includes some code for webui, but also some code from components (like translate code, or distiller code). This javascript will be run in a WKWebView that uses the same JS engine as Safari on iOS. That shared code still need to run on iOS and the version of the OS supported is iOS 9 and higher.

Thanks for clarifying.
 

So if Safari does not support "const", then we should ensure that all javascript run on Chrome on iOS (from the Chromium repository) does not use features unsupported. I don't think we should ban "const" just for that reason if we can find a way to make the code work on iOS (maybe we can transpile from ES6 to ES5 or something else).
-- Sylvain

The code in question is a Chrome extension that relies heavily on Chrome APIs, so if it somehow ends up being executed by Safari, something would have to have gone very, very wrong.

Also, I'm not sure it matters, but our build process depends on JSCompiler, so we're already converting to ES5 (not for compatibility, but just because JSCompiler doesn't support ES6 as a target language).

John Williams

unread,
Jun 28, 2017, 9:04:24 PM6/28/17
to Demetrios Papadopoulos, Nico Weber, Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, mfo...@chromium.org
On Wed, Jun 28, 2017 at 12:14 AM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:
One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

My team's codebase has been developed from the start with the Closure compiler, so I know we're not using any features it doesn't support well. It's a Chrome extension, so there's no scenario in which it could execute outside of Chrome proper. Is there any why to distinguish code in the repo that needs to run in Safari vs. code that doesn't?
 
Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

How would I go about testing that? So far we know for sure it works the the Closure compiler and linter. Are there other tools that need to be tested?
 
Now regardless of the JS toolchain ES6 support, the other reason we've setup a process of proposing ES6 features, one by one, is because not necessarily all ES6 (or ES7) features will be non-controversial to use (similar to the C++11 feature proposal process). The ones that come to mind that fall in this category are generators, and async/await.

Fair enough. We're not using either of those features. Out of the features listed in the style guide, the ones we care about are:
  • (Block-Scoped Variables)
  • (Block-Scoped Constants)
  • Default Function Parameters
  • Rest Parameters
  • Spread Operators
  • Object Literal Extensions (maybe)
  • Template Literals
  • Destructuring Assignment
  • for...of Loops
  • String Static & Prototype methods
  • Array Static & Prototype Methods

Demetrios Papadopoulos

unread,
Jun 28, 2017, 9:51:20 PM6/28/17
to John Williams, Nico Weber, Dan Beam, Chromium-dev, Tim Sergeant, Eric Lawrence, mfo...@chromium.org
On Wed, Jun 28, 2017 at 6:02 PM, John Williams <j...@google.com> wrote:
On Wed, Jun 28, 2017 at 12:14 AM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:
One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

My team's codebase has been developed from the start with the Closure compiler, so I know we're not using any features it doesn't support well. It's a Chrome extension, so there's no scenario in which it could execute outside of Chrome proper. Is there any why to distinguish code in the repo that needs to run in Safari vs. code that doesn't?

The distinction is not very clear, without digging into .grd files and figure out what is included in which platform. In general code under 
can be used by iOS (not exhaustive list).

 
 
Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

How would I go about testing that? So far we know for sure it works the the Closure compiler and linter. Are there other tools that need to be tested? 

Chromium includes different types of JS code. For example there is JS code for WebUI pages (chrome:// pages) and JS code for extensions (like chromevox). Even within the same type of JS code, not all JS files go through the same tools. For example Polymer WebUI code is passed to vulcanize, uglify, crisper, closure compiler, eslint, clang-format, GRIT, other parts go through a different set of tools (most likely a subset of those).

Having a single styleguide for all of Chrome's JS is restricting some of the code, because of how other parts of the code are used. If your code is a leaf in the dependency graph of Chromium, then I acknowledge that restricting JS usage can seem unfortunate/unnecessary. On the other hand, having different style guides for different parts of the codebase can also be problematic (and a whole other discussion).

Moving large a codebase developed outside of Chromium into Chromium, is challenging all the points made above. Have you considered other approaches? Maybe including your extension as a third_party dependency and having the code reside on some Git repository could solve a lot of those issues (decoupling Chromium's restrictions from your extension?). Just a suggestion, I don't have knowledge on how feasible that is.

dpapad

unread,
Jun 28, 2017, 10:58:56 PM6/28/17
to Chromium-dev, j...@google.com, tha...@chromium.org, db...@chromium.org, tser...@chromium.org, elaw...@chromium.org, mfo...@chromium.org


On Wednesday, June 28, 2017 at 6:51:20 PM UTC-7, dpapad wrote:


On Wed, Jun 28, 2017 at 6:02 PM, John Williams <j...@google.com> wrote:
On Wed, Jun 28, 2017 at 12:14 AM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:
One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

My team's codebase has been developed from the start with the Closure compiler, so I know we're not using any features it doesn't support well. It's a Chrome extension, so there's no scenario in which it could execute outside of Chrome proper. Is there any why to distinguish code in the repo that needs to run in Safari vs. code that doesn't?

The distinction is not very clear, without digging into .grd files and figure out what is included in which platform. In general code under 
can be used by iOS (not exhaustive list).

 
 
Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

How would I go about testing that? So far we know for sure it works the the Closure compiler and linter. Are there other tools that need to be tested? 

Chromium includes different types of JS code. For example there is JS code for WebUI pages (chrome:// pages) and JS code for extensions (like chromevox). Even within the same type of JS code, not all JS files go through the same tools. For example Polymer WebUI code is passed to vulcanize, uglify, crisper, closure compiler, eslint, clang-format, GRIT, other parts go through a different set of tools (most likely a subset of those).

Clarifying a bit my answer to your question. The best way to test that all the tools support a proposed feature would be to put up a CL that uses that feature in a part of the codebase that uses all those tools (to my knowledge this would be one of MD Settings, History, Downloads, Bookmarks) and see if anything breaks (either the build itself, or an automated test, or doing sanity checking on the page itself). That is the process I have been following so far as part of addressing issue 671426.

dpapad

unread,
Jul 6, 2017, 6:56:10 PM7/6/17
to Chromium-dev, j...@google.com, tha...@chromium.org, db...@chromium.org, tser...@chromium.org, elaw...@chromium.org, mfo...@chromium.org


On Wednesday, June 28, 2017 at 7:58:56 PM UTC-7, dpapad wrote:


On Wednesday, June 28, 2017 at 6:51:20 PM UTC-7, dpapad wrote:


On Wed, Jun 28, 2017 at 6:02 PM, John Williams <j...@google.com> wrote:
On Wed, Jun 28, 2017 at 12:14 AM, Demetrios Papadopoulos <dpa...@chromium.org> wrote:
One of the reasons we did not decide to blindly allow all ES6 features is because various JS tools (for example Closure Complier and Uglify) used to not support them well (either crashing or producing erroneous output). The effort to ensure that all our tools support ES6 is tracked by issue 671426. All currently whitelisted ES6 features have been tested with those tools. Also IOS adds a layer of complication, since features that are supported in Chrome's ToT, are not necessarily support in all IOS versions.

My team's codebase has been developed from the start with the Closure compiler, so I know we're not using any features it doesn't support well. It's a Chrome extension, so there's no scenario in which it could execute outside of Chrome proper. Is there any why to distinguish code in the repo that needs to run in Safari vs. code that doesn't?

The distinction is not very clear, without digging into .grd files and figure out what is included in which platform. In general code under 
can be used by iOS (not exhaustive list).

 
 
Part of the process of proposing a new feature (like let/const) is verifying that nothing in the toolchain crashes as a result of that. So, since I own that bug, I'll take this as an action item to verify let/const are well supported and put up a CL to update the styleguide.

How would I go about testing that? So far we know for sure it works the the Closure compiler and linter. Are there other tools that need to be tested? 

Chromium includes different types of JS code. For example there is JS code for WebUI pages (chrome:// pages) and JS code for extensions (like chromevox). Even within the same type of JS code, not all JS files go through the same tools. For example Polymer WebUI code is passed to vulcanize, uglify, crisper, closure compiler, eslint, clang-format, GRIT, other parts go through a different set of tools (most likely a subset of those).

Clarifying a bit my answer to your question. The best way to test that all the tools support a proposed feature would be to put up a CL that uses that feature in a part of the codebase that uses all those tools (to my knowledge this would be one of MD Settings, History, Downloads, Bookmarks) and see if anything breaks (either the build itself, or an automated test, or doing sanity checking on the page itself). That is the process I have been following so far as part of addressing issue 671426.

FYI, I am experimenting with let/const within WebUI code at https://codereview.chromium.org/2975503002, to see what breaks (if anything).

dpapad

unread,
Jul 13, 2017, 6:39:26 PM7/13/17
to Chromium-dev, j...@google.com, tha...@chromium.org, db...@chromium.org, tser...@chromium.org, elaw...@chromium.org, mfo...@chromium.org

FYI, I am experimenting with let/const within WebUI code at https://codereview.chromium.org/2975503002, to see what breaks (if anything).

The JS compiler issue with let/const has been fixed as of the latest roll. The new experimental CL that declares "const/let" as OK to use is here. If everything goes well I am planning to land it soon. Note that const and let are not fully supported on IOS Safari 9, according to https://caniuse.com/#feat=let and https://caniuse.com/#feat=const, so I will make a note in the styleguide similar to this note for arrow functions.

Let me know if there are any concerns/objections.

Thank you,
Demetrios

吳中銘

unread,
Jul 14, 2017, 1:20:05 AM7/14/17
to chromium-dev...@chromium.org, Chromium-dev
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/3fd2b5f0-8e57-4515-b5b4-fa7fbf9fbeba%40chromium.org.

Reply all
Reply to author
Forward
0 new messages