147603 – [INTL] Implement Intl.DateTimeFormat.prototype.resolvedOptions ()

Bug 147603 - [INTL] Implement Intl.DateTimeFormat.prototype.resolvedOptions ()

Summary: [INTL] Implement Intl.DateTimeFormat.prototype.resolvedOptions ()

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Andy VanWagoner

URL:
Keywords:

Depends on:
Blocks:	90906
	Show dependency tree / graph

Reported:	2015-08-03 16:55 PDT by Andy VanWagoner
Modified:	2015-12-24 14:29 PST (History)
CC List:	17 users (show)

See Also:

Attachments
Patch (110.40 KB, patch) 2015-12-11 16:56 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (110.91 KB, patch) 2015-12-12 11:47 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (122.93 KB, patch) 2015-12-14 16:13 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (120.68 KB, patch) 2015-12-15 15:54 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (144.52 KB, patch) 2015-12-16 15:16 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (143.72 KB, patch) 2015-12-16 18:05 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (145.19 KB, patch) 2015-12-16 18:58 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch reworked to align with Collator (224.68 KB, patch) 2015-12-18 07:18 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (784.73 KB, application/zip) 2015-12-18 08:24 PST, Build Bot	no flags	Details
Patch (225.31 KB, patch) 2015-12-18 08:42 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (225.61 KB, patch) 2015-12-18 17:14 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Patch (233.35 KB, patch) 2015-12-21 15:38 PST, Andy VanWagoner	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (10) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Andy VanWagoner 2015-08-03 16:55:19 PDT

Implement ECMA-402 2.0 12.3.5 Intl.DateTimeFormat.prototype.resolvedOptions ()
http://ecma-international.org/publications/standards/Ecma-402.htm

This function provides access to the locale and formatting options computed during initialization of the object.

The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this DateTimeFormat object (see 12.4): locale, calendar, numberingSystem, timeZone, hour12, weekday, era, year, month, day, hour, minute, second, and timeZoneName. Properties whose corresponding internal slots are not present are not assigned.

NOTE In this version of the ECMAScript 2015 Internationalization API, the timeZone property will be the name of the default time zone if no timeZone property was provided in the options object provided to the Intl.DateTimeFormat constructor. The previous version left the timeZone property undefined in this case.

Comment 1 Andy VanWagoner 2015-12-11 16:56:24 PST

Created attachment 267202 [details]
Patch

Comment 2 WebKit Commit Bot 2015-12-11 16:58:09 PST

Attachment 267202 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 65 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 3 Andy VanWagoner 2015-12-12 11:47:25 PST

Created attachment 267238 [details]
Patch

Comment 4 WebKit Commit Bot 2015-12-12 11:49:20 PST

Attachment 267238 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 65 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 5 Andy VanWagoner 2015-12-13 11:18:17 PST

Incorrectly defaults locale "ar" to numberingSystem "latn" in resolvedOptions, but will still end up formatting with "arab" numberingSystem.

Comment 6 Andy VanWagoner 2015-12-13 11:31:24 PST

ICU provides unumsys, which can be used to determine the default numbering system for a locale, and enumerate available numbering systems. However, it is only available in ICU 52+. 4.6.1 I think is the version for most headers included. What is the actual minimum version we need to interop with?

http://icu-project.org/apiref/icu4c/unumsys_8h.html

Comment 7 Andy VanWagoner 2015-12-14 16:13:22 PST

Created attachment 267326 [details]
Patch

Comment 8 WebKit Commit Bot 2015-12-14 16:15:14 PST

Attachment 267326 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 9 Andy VanWagoner 2015-12-14 17:02:33 PST

The only failure that appears related is that the Windows build cannot find unicode/unumsys.h I assume that means that it is using ICU <52.

I don't know how we go about linking to ICU on any platform. Does this mean that we simply cannot use unumsys?

Comment 10 Darin Adler 2015-12-15 09:08:30 PST

Comment on attachment 267326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267326&action=review

review- because of the lifetime mistake for the StringView named skeleton.

I’m concerned that there is not enough caching here. Looks like some of these operations will be really expensive.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:97
> +String defaultTimeZone()

Can we do any caching here? The user can change the default time zone, but I am worried about doing this work every time this function is called.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:102
> +    UErrorCode status(U_ZERO_ERROR);

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:103
> +    int32_t bufferLength = ucal_getDefaultTimeZone(nullptr, 0, &status);

I suggest using auto for the type here rather than explicitly stating the type.

Is ucal_getDefaultTimeZone implemented properly on the OS X and iOS platforms? I am not sure it is. There is no guarantee we can use ICU for everything on all platforms. Some functions of ICU are not implemented properly on all platforms it exists on.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:106
> +        Vector<UChar> buffer(bufferLength);

This would be more efficient with inline capacity:

    Vector<UChar, 16> buffer(bufferLength);

That’s what Vector is designed for. Doing it that ways means no heap memory allocation at all in the normal case.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:109
> +            return String(buffer.data(), bufferLength);

Is the time zone string guaranteed to be an ASCII or Latin-1 string? If it can be UTF-8 then this code is not quite right.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:122
> +    const String timeZoneNameUpper = timeZoneName.upper();

Seems unfortunate to literally call upper just to compare time zones in a case folding way. We have functions that can do that comparison without allocating a string object.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:125
> +        UErrorCode status(U_ZERO_ERROR);

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:134
> +        String resultString(ianaTimeZone, ianaTimeZoneLength);

Since all we want to do here is compare two time zone strings, this should use StringView, not String, to avoid memory allocation.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:135
> +        if (resultString.upper() != timeZoneNameUpper)

This should use equalIgnoringCase or equalIgnoringASCIICase.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:144
> +        UBool isSystemID(FALSE);

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:148
> +            Vector<UChar> buffer(ianaTimeZoneLength);

This would be more efficient with inline capacity:

    Vector<UChar, 16> buffer(ianaTimeZoneLength);

That’s what Vector is designed for. Doing it that ways means no heap memory allocation at all in the normal case.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:149
> +            UBool isSystemID(FALSE);

Seems unusual to define another isSystemID instead of reusing the one we already have.

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:150
> +            UErrorCode status(U_ZERO_ERROR);

Seems unusual to define another status instead of reusing the one we already have.

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:157
> +            ASSERT(U_SUCCESS(status));
> +            canonical = String(buffer.data(), canonicalLength);
> +        } else {
> +            ASSERT(U_SUCCESS(status));
> +            canonical = String(buffer.data(), canonicalLength);
> +        }

Is the time zone string guaranteed to be an ASCII or Latin-1 string? If it can be UTF-8 then this code is not quite right.

Can we refactor this so we share this code rather than repeating it?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:179
> +        while (const char* availableName = uenum_next(calendars, &nameLength, &status)) {

Not sure why we are using uenum_next here rather than uenum_unext above. It seems arbitrarily different.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:182
> +            String calendar = String(availableName, nameLength);

Is the time zone string guaranteed to be an ASCII or Latin-1 string? If it can be UTF-8 then this code is not quite right.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:219
> +    bool needDefaults(true);

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:319
> +static void setFormatsFromPattern(IntlDateTimeFormat* dateTimeFormat, const String& pattern)

Would be nice to take a StringView here instead of a String.

A parser like this is a tricky thing. We need to test the parser extensively.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:323
> +        UChar ch = pattern[i];

I suggest omitting the local variable here and just doing this inside the switch.

Further, the name "ch" is not right for WebKit coding style so if you kept this I would suggest changing it.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:562
> +            formatOpt.append("EEEEE");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:564
> +            formatOpt.append("EEE");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:566
> +            formatOpt.append("EEEE");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:574
> +            formatOpt.append("GGGGG");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:576
> +            formatOpt.append("GGG");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:578
> +            formatOpt.append("GGGG");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:586
> +            formatOpt.append("yy");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:596
> +            formatOpt.append("MM");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:600
> +            formatOpt.append("MMMMM");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:602
> +            formatOpt.append("MMM");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:604
> +            formatOpt.append("MMMM");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:612
> +            formatOpt.append("dd");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:632
> +                formatOpt.append("jj");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:637
> +                formatOpt.append("hh");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:642
> +                formatOpt.append("HH");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:653
> +            formatOpt.append("mm");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:663
> +            formatOpt.append("ss");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:675
> +            formatOpt.append("zzzz");

appendLiteral

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:693
> +    UErrorCode status(U_ZERO_ERROR);

We normally use "=" for lines like this, not the style that looks like a function call.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:694
> +    UDateTimePatternGenerator* pg = udatpg_open(dataLocale.utf8().data(), &status);

I suggest generator as the name for this local variable.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:696
> +        return throwTypeError(&state, String::format("failed to initialize DateTimeFormat with ICU error %s", u_errorName(status)));

It seems questionable to expose ICU error strings directly to web content. Also, we are trying to deprecate String::format so it would be better not to use that just to concatenate strings.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:698
> +    StringView skeleton(formatOpt.toString());

The object lifetime here is incorrect. The result of toString will be deallocated but the StringView will be left pointing to it. We need to store the result of toString in a local variable.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:703
> +        return throwTypeError(&state, String::format("failed to initialize DateTimeFormat with ICU error %s", u_errorName(status)));

It seems questionable to expose ICU error strings directly to web content. Also, we are trying to deprecate String::format so it would be better not to use that just to concatenate strings.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:707
> +    Vector<UChar> patternBuffer(patternLength);

Need inline capacity here for better efficiency:

    Vector<UChar, 16> patternBuffer(patternLength);

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:711
> +        return throwTypeError(&state, String::format("failed to initialize DateTimeFormat with ICU error %s", u_errorName(status)));

It seems questionable to expose ICU error strings directly to web content. Also, we are trying to deprecate String::format so it would be better not to use that just to concatenate strings.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:713
> +    String pattern(patternBuffer.data(), patternLength);

Is the patternBuffer string guaranteed to be ASCII or Latin-1? This is incorrect if it’s possibly UTF-8.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:732
> +    // 34. If dateTimeFormat has an internal slot [[hour]], then
> +    // a. If hr12 is undefined, then i. Let hr12 be Get(dataLocaleData, "hour12").
> +    // b. Set dateTimeFormat.[[hour12]] to hr12.
> +    // c. If hr12 is true, then
> +    // i. Let hourNo0 be Get(dataLocaleData, "hourNo0").
> +    // ii. Set dateTimeFormat.[[hourNo0]] to hourNo0.
> +    // iii. Let pattern be Get(bestFormat, "pattern12").
> +    // d. Else
> +    // i. Let pattern be Get(bestFormat, "pattern").
> +    // 35. Else
> +    // a. Let pattern be Get(bestFormat, "pattern").
> +    // 36. Set dateTimeFormat.[[pattern]] to pattern.

I don’t understand why these comments are here but there is no code.

> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:304
> +    options->putDirect(vm, vm.propertyNames->locale, jsString(state, dtf->locale()));
> +    options->putDirect(vm, Identifier::fromString(&vm, "calendar"), jsString(state, dtf->calendar()));
> +    options->putDirect(vm, Identifier::fromString(&vm, "numberingSystem"), jsString(state, dtf->numberingSystem()));
> +    options->putDirect(vm, vm.propertyNames->timeZone, jsString(state, dtf->timeZone()));
> +
> +    if (dtf->weekday() != IntlDateTimeFormat::WeekdayStyle::None)
> +        options->putDirect(vm, vm.propertyNames->weekday, jsString(state, weekdayStyleToString(dtf->weekday())));
> +
> +    if (dtf->era() != IntlDateTimeFormat::EraStyle::None)
> +        options->putDirect(vm, vm.propertyNames->era, jsString(state, eraStyleToString(dtf->era())));
> +
> +    if (dtf->year() != IntlDateTimeFormat::YearStyle::None)
> +        options->putDirect(vm, vm.propertyNames->year, jsString(state, yearStyleToString(dtf->year())));
> +
> +    if (dtf->month() != IntlDateTimeFormat::MonthStyle::None)
> +        options->putDirect(vm, vm.propertyNames->month, jsString(state, monthStyleToString(dtf->month())));
> +
> +    if (dtf->day() != IntlDateTimeFormat::DayStyle::None)
> +        options->putDirect(vm, vm.propertyNames->day, jsString(state, dayStyleToString(dtf->day())));
> +
> +    if (dtf->hour() != IntlDateTimeFormat::HourStyle::None) {
> +        options->putDirect(vm, vm.propertyNames->hour, jsString(state, hourStyleToString(dtf->hour())));
> +        options->putDirect(vm, vm.propertyNames->hour12, jsBoolean(dtf->hour12()));
> +    }
> +
> +    if (dtf->minute() != IntlDateTimeFormat::MinuteStyle::None)
> +        options->putDirect(vm, vm.propertyNames->minute, jsString(state, minuteStyleToString(dtf->minute())));
> +
> +    if (dtf->second() != IntlDateTimeFormat::SecondStyle::None)
> +        options->putDirect(vm, vm.propertyNames->second, jsString(state, secondStyleToString(dtf->second())));
>  
> -    // FIXME: Populate object from internal slots.
> +    if (dtf->timeZoneName() != IntlDateTimeFormat::TimeZoneNameStyle::None)
> +        options->putDirect(vm, vm.propertyNames->timeZoneName, jsString(state, timeZoneNameStyleToString(dtf->timeZoneName())));

I believe all these call sites can use jsNontrivialString instead of jsString, it’s a slight optimization that can be done as long as we have a guarantee that none of these strings can be one character long.

> Source/JavaScriptCore/runtime/IntlObject.cpp:925
> +    // unumsys_openAvailableNames appears to only be able usable once per process, so use static list instead.

I’d like to understand more. It seems like a problem to try to be in sync other systems.

> Source/JavaScriptCore/runtime/IntlObject.h:68
>  String removeUnicodeLocaleExtension(const String&);
>  String bestAvailableLocale(const HashSet<String>&, const String&);
> +Vector<String> getNumberingSystemsForLocale(const String&);

I think these arguments need names. I think the strings are all locales, and the HashSet is availableLocales, but it’s not 100% clear from the types alone.

Comment 11 Andy VanWagoner 2015-12-15 09:54:13 PST

Comment on attachment 267326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267326&action=review

Thank you for the detailed review. It is super helpful to quickly get this kind of feedback.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:97
>> +String defaultTimeZone()
> 
> Can we do any caching here? The user can change the default time zone, but I am worried about doing this work every time this function is called.

I don't mind caching this, but where?

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:102
>> +    UErrorCode status(U_ZERO_ERROR);
> 
> We normally use "=" for lines like this, not the style that looks like a function call.

I will fix all of these.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:103
>> +    int32_t bufferLength = ucal_getDefaultTimeZone(nullptr, 0, &status);
> 
> I suggest using auto for the type here rather than explicitly stating the type.
> 
> Is ucal_getDefaultTimeZone implemented properly on the OS X and iOS platforms? I am not sure it is. There is no guarantee we can use ICU for everything on all platforms. Some functions of ICU are not implemented properly on all platforms it exists on.

If there is a more reliable alternative, let me know and I will switch.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:106
>> +        Vector<UChar> buffer(bufferLength);
> 
> This would be more efficient with inline capacity:
> 
>     Vector<UChar, 16> buffer(bufferLength);
> 
> That’s what Vector is designed for. Doing it that ways means no heap memory allocation at all in the normal case.

Should the inline capacity be the expected size? 32 characters would cover most if not all time zone names.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:109
>> +            return String(buffer.data(), bufferLength);
> 
> Is the time zone string guaranteed to be an ASCII or Latin-1 string? If it can be UTF-8 then this code is not quite right.

Time zone names are always ASCII.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:135
>> +        if (resultString.upper() != timeZoneNameUpper)
> 
> This should use equalIgnoringCase or equalIgnoringASCIICase.

I will change this.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:157
>> +        }
> 
> Is the time zone string guaranteed to be an ASCII or Latin-1 string? If it can be UTF-8 then this code is not quite right.
> 
> Can we refactor this so we share this code rather than repeating it?

Time zone strings are always ASCII. I can rework this to share more code.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:179
>> +        while (const char* availableName = uenum_next(calendars, &nameLength, &status)) {
> 
> Not sure why we are using uenum_next here rather than uenum_unext above. It seems arbitrarily different.

_unext() returns UChar, _next() returns char. For ASCII text _next() avoids upconversion, but can fail on non-ASCII. The time zone one above should be changed to _next() as the values are always ASCII.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:182
>> +            String calendar = String(availableName, nameLength);
> 
> Is the time zone string guaranteed to be an ASCII or Latin-1 string? If it can be UTF-8 then this code is not quite right.

Calendar names are always ASCII.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:319
>> +static void setFormatsFromPattern(IntlDateTimeFormat* dateTimeFormat, const String& pattern)
> 
> Would be nice to take a StringView here instead of a String.
> 
> A parser like this is a tricky thing. We need to test the parser extensively.

I thought it more straightforward and faster than the regular expressions v8 uses. ;)
The tricky part of testing is that the generated pattern is not guaranteed to have the same tokens as were passed in, this is only testable indirectly through the style options in the constructor through to resolvedOptions. I've currently tested all of the style options, but could do so in more locales if that will increase our confidence.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:323
>> +        UChar ch = pattern[i];
> 
> I suggest omitting the local variable here and just doing this inside the switch.
> 
> Further, the name "ch" is not right for WebKit coding style so if you kept this I would suggest changing it.

I will move pattern[i] into the switch(), since we don't ever reference it otherwise.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:562
>> +            formatOpt.append("EEEEE");
> 
> appendLiteral

I will change.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:694
>> +    UDateTimePatternGenerator* pg = udatpg_open(dataLocale.utf8().data(), &status);
> 
> I suggest generator as the name for this local variable.

Will do.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:696
>> +        return throwTypeError(&state, String::format("failed to initialize DateTimeFormat with ICU error %s", u_errorName(status)));
> 
> It seems questionable to expose ICU error strings directly to web content. Also, we are trying to deprecate String::format so it would be better not to use that just to concatenate strings.

I can avoid exposing the ICU error name to end users. Is there a way to save it though for internal review? Knowing what error happened makes troubleshooting much easier.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:698
>> +    StringView skeleton(formatOpt.toString());
> 
> The object lifetime here is incorrect. The result of toString will be deallocated but the StringView will be left pointing to it. We need to store the result of toString in a local variable.

k, I will fix this.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:713
>> +    String pattern(patternBuffer.data(), patternLength);
> 
> Is the patternBuffer string guaranteed to be ASCII or Latin-1? This is incorrect if it’s possibly UTF-8.

Here the pattern can be utf8. What is the correct way to initialize this string?

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:732
>> +    // 36. Set dateTimeFormat.[[pattern]] to pattern.
> 
> I don’t understand why these comments are here but there is no code.

These steps are delegated to icu. I can remove the comments if you like.

>> Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:304
>> +        options->putDirect(vm, vm.propertyNames->timeZoneName, jsString(state, timeZoneNameStyleToString(dtf->timeZoneName())));
> 
> I believe all these call sites can use jsNontrivialString instead of jsString, it’s a slight optimization that can be done as long as we have a guarantee that none of these strings can be one character long.

None of these can be one character long. I will make the change.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:925
>> +    // unumsys_openAvailableNames appears to only be able usable once per process, so use static list instead.
> 
> I’d like to understand more. It seems like a problem to try to be in sync other systems.

I had opened the enumeration here, and closed it at the end of the loop as done with calendars and timezones above, but every second run resulted in an invalid memory access. Trying to move it to the JSGlobalObject and caching it didn't fix the issue because opening a new tab or the dev tools would create a new global, and crash when repopulating. My best guess is that ICU doesn't internally handle this enumeration correctly.

>> Source/JavaScriptCore/runtime/IntlObject.h:68
>> +Vector<String> getNumberingSystemsForLocale(const String&);
> 
> I think these arguments need names. I think the strings are all locales, and the HashSet is availableLocales, but it’s not 100% clear from the types alone.

I will add the names.

Comment 12 Andy VanWagoner 2015-12-15 13:07:59 PST

Comment on attachment 267326 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267326&action=review

>>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:713
>>> +    String pattern(patternBuffer.data(), patternLength);
>> 
>> Is the patternBuffer string guaranteed to be ASCII or Latin-1? This is incorrect if it’s possibly UTF-8.
> 
> Here the pattern can be utf8. What is the correct way to initialize this string?

On further investigation, this hits the String(UChar* characters, unsigned length) constructor that is commented to take UTF-16, which I think UChar always represents.

Comment 13 Andy VanWagoner 2015-12-15 15:54:11 PST

Created attachment 267406 [details]
Patch

Comment 14 WebKit Commit Bot 2015-12-15 15:56:00 PST

Attachment 267406 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 15 Andy VanWagoner 2015-12-15 16:00:24 PST

I just realized that timeZone is poorly tested. I will add more tests.

Comment 16 Alex Christensen 2015-12-15 22:09:44 PST

The Windows ICU dependency needs updating, but it will probably take a while to do the update and get the update everywhere it is needed internally.

Comment 17 Andy VanWagoner 2015-12-16 10:16:18 PST

(In reply to comment #16)
> The Windows ICU dependency needs updating, but it will probably take a while
> to do the update and get the update everywhere it is needed internally.

Could I disable the INTL flag for Windows in the interim, or will the update be finished in a matter of only a few days?

Comment 18 Alex Christensen 2015-12-16 11:05:31 PST

The update will not be finished in a matter of a few days.(In reply to comment #17)
> (In reply to comment #16)
> > The Windows ICU dependency needs updating, but it will probably take a while
> > to do the update and get the update everywhere it is needed internally.
> 
> Could I disable the INTL flag for Windows in the interim, or will the update
> be finished in a matter of only a few days?

Updating ICU will take more than a few days.  It's easy to update, but it takes a while to push the update everywhere.

Comment 19 Andy VanWagoner 2015-12-16 15:16:46 PST

Created attachment 267497 [details]
Patch

Comment 20 WebKit Commit Bot 2015-12-16 15:18:56 PST

Attachment 267497 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 21 Andy VanWagoner 2015-12-16 18:01:19 PST

FeatureList.pm change did not disable INTL on Windows EWS. Reverting change.

Comment 22 Andy VanWagoner 2015-12-16 18:05:25 PST

Created attachment 267517 [details]
Patch

Comment 23 WebKit Commit Bot 2015-12-16 18:06:23 PST

Attachment 267517 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 24 Alex Christensen 2015-12-16 18:34:44 PST

The FeatureList.pm change is not necessary, but ENABLE_INTL has two lines in OptionsWin.cmake, the second one is enabling the feature.  Removing the second one disables intl on windows.

Comment 25 Alex Christensen 2015-12-16 18:35:11 PST

And it might not show on EWS because changing feature enable flags sometimes requires a clean build.

Comment 26 Andy VanWagoner 2015-12-16 18:58:03 PST

Created attachment 267522 [details]
Patch

Comment 27 Andy VanWagoner 2015-12-16 18:59:21 PST

Thanks Alex!

Comment 28 WebKit Commit Bot 2015-12-16 19:00:11 PST

Attachment 267522 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 29 Darin Adler 2015-12-16 21:25:40 PST

Comment on attachment 267522 [details]
Patch

Windows build failure with unumsys.h include is still happening:

C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\runtime\IntlObject.cpp(48): fatal error C1083: Cannot open include file: 'unicode/unumsys.h': No such file or directory

Comment 30 Alex Christensen 2015-12-16 23:33:13 PST

(In reply to comment #29)
> Comment on attachment 267522 [details]
> Patch
> 
> Windows build failure with unumsys.h include is still happening:
> 
> C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\runtime\IntlObject.
> cpp(48): fatal error C1083: Cannot open include file: 'unicode/unumsys.h':
> No such file or directory

That is an incremental build failure from disabling a feature (INTL).  I verified that this patch compiles and links successfully in a clean build on Windows.  I haven't been following the INTL development closely enough to r+ this patch, but let me know when it is ready to land and I'll fix any Windows problems that arise.

Comment 31 Alex Christensen 2015-12-17 00:13:29 PST

I got a WinCairo build to compile and link with an updated ICU.  It crashes when I run jsc because I missed something, but it won't be long before we can re-enable INTL on the WinCairo bot so we can catch any compile errors on INTL development on Windows.  AppleWin will take longer to update the ICU libraries, so INTL will need to stay disabled on that port for a while.

Comment 32 Alex Christensen 2015-12-17 15:39:29 PST

I also think that some of Darin's comments have not been resolved, so this is not ready for committing.

Comment 33 Andy VanWagoner 2015-12-18 07:18:09 PST

Created attachment 267627 [details]
Patch reworked to align with Collator

Comment 34 WebKit Commit Bot 2015-12-18 07:21:10 PST

Attachment 267627 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 35 Build Bot 2015-12-18 08:24:04 PST

Comment on attachment 267627 [details]
Patch reworked to align with Collator

Attachment 267627 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/576031

New failing tests:
js/intl-datetimeformat.html

Comment 36 Build Bot 2015-12-18 08:24:09 PST

Created attachment 267629 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

Comment 37 Andy VanWagoner 2015-12-18 08:42:49 PST

Created attachment 267631 [details]
Patch

Comment 38 Andy VanWagoner 2015-12-18 08:43:28 PST

Comment on attachment 267631 [details]
Patch

Cacnonicalized the default time zone. Other minor fixups.

Comment 39 WebKit Commit Bot 2015-12-18 08:44:17 PST

Attachment 267631 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 40 Andy VanWagoner 2015-12-18 10:52:23 PST

Darin, are these still more changes you'd like to see for this patch?

The only thing I know I didn't do is caching. If you could point out the proper mechanism to do it, I can make the change.

Comment 41 Alex Christensen 2015-12-18 15:34:08 PST

(In reply to comment #11)
> >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:97
> >> +String defaultTimeZone()
> > 
> > Can we do any caching here? The user can change the default time zone, but I am worried about doing this work every time this function is called.
> 
> I don't mind caching this, but where?
Something like this:
static const String& defaultTimeZone()
{
    static NeverDestroyed<String> defaultTimeZone;
    if (!defaultTimeZone.get().isEmpty())
        return defaultTimeZone;
...
    defaultTimeZone.get() = String(canonicalBuffer.data(), canonicalLength);
    return defaultTimeZone;
}
> >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:103
> >> +    int32_t bufferLength = ucal_getDefaultTimeZone(nullptr, 0, &status);
> > 
> > I suggest using auto for the type here rather than explicitly stating the type.
> > 
> > Is ucal_getDefaultTimeZone implemented properly on the OS X and iOS platforms? I am not sure it is. There is no guarantee we can use ICU for everything on all platforms. Some functions of ICU are not implemented properly on all platforms it exists on.
> 
> If there is a more reliable alternative, let me know and I will switch.
I don't think this is a problem.  According to http://opensource.apple.com/release/os-x-10105/ OS X 10.10.5 uses ICU-531.48 which is sufficient, isn't it? http://opensource.apple.com/source/ICU/ICU-531.48/icuSources/i18n/ucal.cpp looks like it is implemented just as correctly as it is in the latest from http://site.icu-project.org/download/56
> >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:319
> >> +static void setFormatsFromPattern(IntlDateTimeFormat* dateTimeFormat, const String& pattern)
> > 
> > Would be nice to take a StringView here instead of a String.
> > 
> > A parser like this is a tricky thing. We need to test the parser extensively.
> 
> I thought it more straightforward and faster than the regular expressions v8
> uses. ;)
> The tricky part of testing is that the generated pattern is not guaranteed
> to have the same tokens as were passed in, this is only testable indirectly
> through the style options in the constructor through to resolvedOptions.
> I've currently tested all of the style options, but could do so in more
> locales if that will increase our confidence.
I'm not terribly familiar with what is going on here, but is it possible to test this function with invalid input?
Also, I think it's strange to have special cases for {H, h, K, k} and also have them in the switch.
> >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:696
> >> +        return throwTypeError(&state, String::format("failed to initialize DateTimeFormat with ICU error %s", u_errorName(status)));
> > 
> > It seems questionable to expose ICU error strings directly to web content. Also, we are trying to deprecate String::format so it would be better not to use that just to concatenate strings.
> 
> I can avoid exposing the ICU error name to end users. Is there a way to save
> it though for internal review? Knowing what error happened makes
> troubleshooting much easier.
You could use dataLogF.
> >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:713
> >> +    String pattern(patternBuffer.data(), patternLength);
> > 
> > Is the patternBuffer string guaranteed to be ASCII or Latin-1? This is incorrect if it’s possibly UTF-8.
> 
> Here the pattern can be utf8. What is the correct way to initialize this
> string?
Your approach in a later patch of initializing it from a UChar* and a length works.

Comment 42 Andy VanWagoner 2015-12-18 16:31:59 PST

(In reply to comment #41)
> (In reply to comment #11)
> > >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:97
> > >> +String defaultTimeZone()
> > > 
> > > Can we do any caching here? The user can change the default time zone, but I am worried about doing this work every time this function is called.
> > 
> > I don't mind caching this, but where?
> Something like this:
> static const String& defaultTimeZone()
> {
>     static NeverDestroyed<String> defaultTimeZone;
>     if (!defaultTimeZone.get().isEmpty())
>         return defaultTimeZone;
> ...
>     defaultTimeZone.get() = String(canonicalBuffer.data(), canonicalLength);
>     return defaultTimeZone;
> }

Thanks! This is very helpful. I will do this.

> > >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:103
> > >> +    int32_t bufferLength = ucal_getDefaultTimeZone(nullptr, 0, &status);
> > > 
> > > I suggest using auto for the type here rather than explicitly stating the type.
> > > 
> > > Is ucal_getDefaultTimeZone implemented properly on the OS X and iOS platforms? I am not sure it is. There is no guarantee we can use ICU for everything on all platforms. Some functions of ICU are not implemented properly on all platforms it exists on.
> > 
> > If there is a more reliable alternative, let me know and I will switch.
> I don't think this is a problem.  According to
> http://opensource.apple.com/release/os-x-10105/ OS X 10.10.5 uses ICU-531.48
> which is sufficient, isn't it?
> http://opensource.apple.com/source/ICU/ICU-531.48/icuSources/i18n/ucal.cpp
> looks like it is implemented just as correctly as it is in the latest from
> http://site.icu-project.org/download/56

Yeah, at least my simple testing with switching the time zone in System Settings it worked correctly.

> > >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:319
> > >> +static void setFormatsFromPattern(IntlDateTimeFormat* dateTimeFormat, const String& pattern)
> > > 
> > > Would be nice to take a StringView here instead of a String.
> > > 
> > > A parser like this is a tricky thing. We need to test the parser extensively.
> > 
> > I thought it more straightforward and faster than the regular expressions v8
> > uses. ;)
> > The tricky part of testing is that the generated pattern is not guaranteed
> > to have the same tokens as were passed in, this is only testable indirectly
> > through the style options in the constructor through to resolvedOptions.
> > I've currently tested all of the style options, but could do so in more
> > locales if that will increase our confidence.
> I'm not terribly familiar with what is going on here, but is it possible to
> test this function with invalid input?
> Also, I think it's strange to have special cases for {H, h, K, k} and also
> have them in the switch.

The input to this function will always be the output of udatpg_getBestPattern(), so I don't know of any way to test invalid input. The tests that I added ensuring many locales support the required set of inputs did catch a bug where some Russian patterns used 'c' (standalone weekday) instead of 'E' (weekday), which I later accounted for.

> > >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:696
> > >> +        return throwTypeError(&state, String::format("failed to initialize DateTimeFormat with ICU error %s", u_errorName(status)));
> > > 
> > > It seems questionable to expose ICU error strings directly to web content. Also, we are trying to deprecate String::format so it would be better not to use that just to concatenate strings.
> > 
> > I can avoid exposing the ICU error name to end users. Is there a way to save
> > it though for internal review? Knowing what error happened makes
> > troubleshooting much easier.
> You could use dataLogF.
> > >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:713
> > >> +    String pattern(patternBuffer.data(), patternLength);
> > > 
> > > Is the patternBuffer string guaranteed to be ASCII or Latin-1? This is incorrect if it’s possibly UTF-8.
> > 
> > Here the pattern can be utf8. What is the correct way to initialize this
> > string?
> Your approach in a later patch of initializing it from a UChar* and a length
> works.

Comment 43 Andy VanWagoner 2015-12-18 17:14:24 PST

Created attachment 267662 [details]
Patch

Comment 44 WebKit Commit Bot 2015-12-18 17:15:33 PST

Attachment 267662 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 45 Benjamin Poulain 2015-12-21 03:42:31 PST

Comment on attachment 267662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267662&action=review

Quick review. Some comments/questions.
Everything looks good otherwise, very clean code.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:105
> +    static NeverDestroyed<String> cachedDefaultTimeZone;
> +    if (!cachedDefaultTimeZone.get().isEmpty())
> +        return cachedDefaultTimeZone;

I disagree with Darin here. IMHO, we should only add caching if we add cache invalidation.

Let's this for now, it is not a major bug.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:146
> +    while (canonical.isNull()) {

This should be a do { } while().

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:225
> +static JSObject* toDateTimeOptions(ExecState& state, JSValue originalOptions, const DateTimeOptionRequired required, const DateTimeOptionDefaults defaults)

state->exec for consistency with the existing codebase.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:235
> +        options = constructEmptyObject(&state, state.lexicalGlobalObject()->nullPrototypeObjectStructure());

Shouldn't you use constructEmptyObject(&state/&exec)?
That would use the Structure of a regular object.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:517
> +    } else
> +    // 20. Else,
> +        // a. Let tz be DefaultTimeZone().
> +        tz = defaultTimeZone();

Let's use brackets here. It is a one-statement clause but the comments make it multi-line. It will be clearer with brackets.

> Source/JavaScriptCore/runtime/IntlObject.cpp:928
> +    // unumsys_openAvailableNames appears to only be able usable once per process, so use static list instead.

wow, that's crazy.

Comment 46 Andy VanWagoner 2015-12-21 08:28:10 PST

(In reply to comment #45)
> Comment on attachment 267662 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267662&action=review
> 
> Quick review. Some comments/questions.
> Everything looks good otherwise, very clean code.
> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:105
> > +    static NeverDestroyed<String> cachedDefaultTimeZone;
> > +    if (!cachedDefaultTimeZone.get().isEmpty())
> > +        return cachedDefaultTimeZone;
> 
> I disagree with Darin here. IMHO, we should only add caching if we add cache
> invalidation.
> 
> Let's this for now, it is not a major bug.
> 

My (admittedly not very rigorous) performance testing didn't show this being a noticeable win. I agree that caching default time zone would require a mechanism to invalidate when the time zone changes, which should be better than "restart the browser".

> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:146
> > +    while (canonical.isNull()) {
> 
> This should be a do { } while().
> 

Will do.

> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:225
> > +static JSObject* toDateTimeOptions(ExecState& state, JSValue originalOptions, const DateTimeOptionRequired required, const DateTimeOptionDefaults defaults)
> 
> state->exec for consistency with the existing codebase.
> 
> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:235
> > +        options = constructEmptyObject(&state, state.lexicalGlobalObject()->nullPrototypeObjectStructure());
> 
> Shouldn't you use constructEmptyObject(&state/&exec)?
> That would use the Structure of a regular object.
> 

Considering this object is never exposed outside of initialization, the difference between an Object with the default Object.prototype vs a null prototype is negligible. I can use whichever is faster.

> > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:517
> > +    } else
> > +    // 20. Else,
> > +        // a. Let tz be DefaultTimeZone().
> > +        tz = defaultTimeZone();
> 
> Let's use brackets here. It is a one-statement clause but the comments make
> it multi-line. It will be clearer with brackets.
> 

Will do. I think the style checker will still yell though.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:928
> > +    // unumsys_openAvailableNames appears to only be able usable once per process, so use static list instead.
> 
> wow, that's crazy.

I know, right? Considering I learned how to use NeverDestroyed<> working on this patch set, I can probably make it work now.

Comment 47 Andy VanWagoner 2015-12-21 15:38:13 PST

Created attachment 267764 [details]
Patch

Comment 48 WebKit Commit Bot 2015-12-21 15:39:30 PST

Attachment 267764 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:8:  #ifndef header guard has wrong style, please use: unumsys_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:15:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/unumsys.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:17:  #ifndef header guard has wrong style, please use: udatpg_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:20:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:21:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:152:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:182:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:208:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:244:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:246:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:272:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:271:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:300:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:303:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:331:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:332:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:330:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:357:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:358:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:359:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:372:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:373:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:374:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:390:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:391:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:392:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:405:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:406:  The parameter name "field" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:407:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:431:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:432:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:442:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:443:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:459:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:471:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:501:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:502:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:503:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:500:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:504:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:538:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:537:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:540:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:556:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:569:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:583:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:582:  The parameter name "dtpg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/icu/unicode/udatpg.h:584:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 68 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 49 Benjamin Poulain 2015-12-23 02:07:00 PST

Comment on attachment 267764 [details]
Patch

Quick second round, I did not find any issue.

Comment 50 WebKit Commit Bot 2015-12-23 02:48:52 PST

Comment on attachment 267764 [details]
Patch

Clearing flags on attachment: 267764

Committed r194387: <http://trac.webkit.org/changeset/194387>

Comment 51 WebKit Commit Bot 2015-12-23 02:48:59 PST

All reviewed patches have been landed.  Closing bug.

Comment 52 Csaba Osztrogonác 2015-12-23 04:32:06 PST

(In reply to comment #50)
> Comment on attachment 267764 [details]
> Patch
> 
> Clearing flags on attachment: 267764
> 
> Committed r194387: <http://trac.webkit.org/changeset/194387>

It broke the WinCairo build:

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/52346

..\..\Source\JavaScriptCore\runtime\IntlObject.cpp(48): fatal error C1083: Cannot open include file: 'unicode/unumsys.h': No such file or directory

Comment 53 Csaba Osztrogonác 2015-12-23 06:36:32 PST

And it bróker the Apple Windows build too.

Comment 54 Andy VanWagoner 2015-12-23 07:02:52 PST

Windows should have the INTL flag disabled. Also Alex Christensen was working on updating the ICU version on Windows to fix that problem. Is there more to do to temporarily disable INTL on those bots until the ICU bump happens?

Comment 55 Ryan Haddad 2015-12-23 12:59:52 PST

Looks like some LayoutTests are failing on Windows as a result of this change. Not sure if they depend upon the work you mentioned above.

js/intl-collator.html
js/intl-datetimeformat.html
js/intl-numberformat.html
js/intl.html
js/number-toLocaleString.html
js/string-toLocaleLowerCase.html
js/string-toLocaleUpperCase.html

<https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r194389%20(55698)/results.html>

Comment 56 Andy VanWagoner 2015-12-23 13:04:48 PST

Those all fail now because the INTL flag is disabled. Is there a way to make the tests depend on the state of that flag?

Comment 57 Alexey Proskuryakov 2015-12-23 15:50:46 PST

We normally try to isolate tests from the environment, not massage the bots until the environment is right. Otherwise, the tests will fail when developers run them manually.

We are just using ICU here, why do the tests depend on anything Windows specific? Can code be added to DumpRenderTree to configure ICU the way we need it to be?

Comment 58 Andy VanWagoner 2015-12-23 16:03:18 PST

The problem is the version of ICU in current use on Windows does not include unumsys. All other platforms are current enough. Alex Christensen has been working to update it. In the mean time, I disabled INTL on Windows.

Comment 59 Alex Christensen 2015-12-24 00:14:33 PST

Windows bots just needed a clean build.  Everything is fine.
(In reply to comment #58)
> The problem is the version of ICU in current use on Windows does not include
> unumsys. All other platforms are current enough. Alex Christensen has been
> working to update it. In the mean time, I disabled INTL on Windows.
Let's disable all INTL tests on Windows until icu is updated

Comment 60 Michael Catanzaro 2015-12-24 07:56:53 PST

(In reply to comment #58)
> The problem is the version of ICU in current use on Windows does not include
> unumsys. All other platforms are current enough. Alex Christensen has been
> working to update it. In the mean time, I disabled INTL on Windows.

No, this patch introduced seven JSC test failures for both EFL and GTK as well.

It's not hard to update the version of ICU we depend on for tests, if that's all that needs done...? We will need to add it to the EFL and GTK jhbuild modulesets, but if it affects tests results it ought to go there anyway.

Comment 61 Michael Catanzaro 2015-12-24 08:00:02 PST

It looks like our set of failing tests is different than the ones that are failing on Windows, though:

** The following JSC stress test failures have been introduced:
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-dfg-eager-no-cjit
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-no-cjit
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-cjit
	jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-llint

See: https://build.webkit.org/builders/GTK Linux 64-bit Release (Tests)/builds/12812/steps/jscore-test/logs/stdio

Comment 62 Andy VanWagoner 2015-12-24 09:35:19 PST

It's possible that the GTK failed tests are also due to ICU differences. It's sad that EWS didn't catch any of these problems.

I don't see a way to get the actual text diff from the bots. I don't have a GTK setup. Can someone get the full results containing which assertions are failing, I can make the tests more resilient.

Comment 63 Alexey Proskuryakov 2015-12-24 12:28:15 PST

Andy, if you start from <https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20(Tests)/builds/12812> and click on "view results" in step 15, all the Gtk results will be visible.

Comment 64 Andy VanWagoner 2015-12-24 12:42:41 PST

Thanks Alexey!

Comment 65 Andy VanWagoner 2015-12-24 12:48:58 PST

It looks like some time zone backward links are different, hanidays numbering system isn't supported, and islamic-umalqura calendar isn't supported on that bot, so I shouldn't make them required in the tests. I'll link the new issue with a patch soon.

Comment 66 Michael Catanzaro 2015-12-24 13:08:57 PST

Ugh, the tzdata package is constantly changing timezones. I suspect this test will be the source of much future breakage. :(

What about the numbering systems; any chance you know what system package is responsible for that?

Ideally we would put all packages that influence test results into our jhbuild environment, so we can try to get consistent results regardless of the system packages on the computer running the test. But I'm not sure if it's possible for timezones and locales....

Comment 67 Andy VanWagoner 2015-12-24 13:20:01 PST

I'm getting all of the information through ICU APIs. To get unumsys, it must be at least version 52.

For timezones, I can reduce the tests to just check a few obsolete timezones to make sure backlinks are resolved.

For numbering systems and calendars, I can reduce the set I check to those available on all of our supported configurations. It might be a bit of trial and error though.

Comment 68 Andy VanWagoner 2015-12-24 14:29:59 PST

https://bugs.webkit.org/show_bug.cgi?id=152550