Bug 147603

Summary: [INTL] Implement Intl.DateTimeFormat.prototype.resolvedOptions ()
Product: WebKit Reporter: Andy VanWagoner <andy>
Component: JavaScriptCoreAssignee: Andy VanWagoner <andy>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, benjamin, buildbot, commit-queue, dbates, keith_miller, mark.lam, mcatanzaro, mrobinson, msaboff, ossy, peavo, rniwa, ryanhaddad, saam, sukolsak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 90906    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch reworked to align with Collator
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Patch
none
Patch
none
Patch none

Andy VanWagoner
Reported 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.
Attachments
Patch (110.40 KB, patch)
2015-12-11 16:56 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (110.91 KB, patch)
2015-12-12 11:47 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (122.93 KB, patch)
2015-12-14 16:13 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (120.68 KB, patch)
2015-12-15 15:54 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (144.52 KB, patch)
2015-12-16 15:16 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (143.72 KB, patch)
2015-12-16 18:05 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (145.19 KB, patch)
2015-12-16 18:58 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch reworked to align with Collator (224.68 KB, patch)
2015-12-18 07:18 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (225.61 KB, patch)
2015-12-18 17:14 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Patch (233.35 KB, patch)
2015-12-21 15:38 PST, Andy VanWagoner 		no flags
DetailsFormatted DiffDiff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.
Andy VanWagoner
Comment 1 2015-12-11 16:56:24 PST
Created attachment 267202 [details] Patch
WebKit Commit Bot
Comment 2 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.
Andy VanWagoner
Comment 3 2015-12-12 11:47:25 PST
Created attachment 267238 [details] Patch
WebKit Commit Bot
Comment 4 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.
Andy VanWagoner
Comment 5 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.
Andy VanWagoner
Comment 6 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
Andy VanWagoner
Comment 7 2015-12-14 16:13:22 PST
Created attachment 267326 [details] Patch
WebKit Commit Bot
Comment 8 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.
Andy VanWagoner
Comment 9 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?
Darin Adler
Comment 10 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.
Andy VanWagoner
Comment 11 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.
Andy VanWagoner
Comment 12 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.
Andy VanWagoner
Comment 13 2015-12-15 15:54:11 PST
Created attachment 267406 [details] Patch
WebKit Commit Bot
Comment 14 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.
Andy VanWagoner
Comment 15 2015-12-15 16:00:24 PST
I just realized that timeZone is poorly tested. I will add more tests.
Alex Christensen
Comment 16 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.
Andy VanWagoner
Comment 17 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?
Alex Christensen
Comment 18 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.
Andy VanWagoner
Comment 19 2015-12-16 15:16:46 PST
Created attachment 267497 [details] Patch
WebKit Commit Bot
Comment 20 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.
Andy VanWagoner
Comment 21 2015-12-16 18:01:19 PST
FeatureList.pm change did not disable INTL on Windows EWS. Reverting change.
Andy VanWagoner
Comment 22 2015-12-16 18:05:25 PST
Created attachment 267517 [details] Patch
WebKit Commit Bot
Comment 23 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.
Alex Christensen
Comment 24 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.
Alex Christensen
Comment 25 2015-12-16 18:35:11 PST
And it might not show on EWS because changing feature enable flags sometimes requires a clean build.
Andy VanWagoner
Comment 26 2015-12-16 18:58:03 PST
Created attachment 267522 [details] Patch
Andy VanWagoner
Comment 27 2015-12-16 18:59:21 PST
Thanks Alex!
WebKit Commit Bot
Comment 28 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.
Darin Adler
Comment 29 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
Alex Christensen
Comment 30 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.
Alex Christensen
Comment 31 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.
Alex Christensen
Comment 32 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.
Andy VanWagoner
Comment 33 2015-12-18 07:18:09 PST
Created attachment 267627 [details] Patch reworked to align with Collator
WebKit Commit Bot
Comment 34 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.
Build Bot
Comment 35 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
Build Bot
Comment 36 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
Andy VanWagoner
Comment 37 2015-12-18 08:42:49 PST
Created attachment 267631 [details] Patch
Andy VanWagoner
Comment 38 2015-12-18 08:43:28 PST
Comment on attachment 267631 [details] Patch Cacnonicalized the default time zone. Other minor fixups.
WebKit Commit Bot
Comment 39 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.
Andy VanWagoner
Comment 40 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.
Alex Christensen
Comment 41 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.
Andy VanWagoner
Comment 42 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.
Andy VanWagoner
Comment 43 2015-12-18 17:14:24 PST
Created attachment 267662 [details] Patch
WebKit Commit Bot
Comment 44 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.
Benjamin Poulain
Comment 45 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.
Andy VanWagoner
Comment 46 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.
Andy VanWagoner
Comment 47 2015-12-21 15:38:13 PST
Created attachment 267764 [details] Patch
WebKit Commit Bot
Comment 48 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.
Benjamin Poulain
Comment 49 2015-12-23 02:07:00 PST
Comment on attachment 267764 [details] Patch Quick second round, I did not find any issue.
WebKit Commit Bot
Comment 50 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>
WebKit Commit Bot
Comment 51 2015-12-23 02:48:59 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 52 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
Csaba Osztrogonác
Comment 53 2015-12-23 06:36:32 PST
And it bróker the Apple Windows build too.
Andy VanWagoner
Comment 54 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?
Ryan Haddad
Comment 55 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>
Andy VanWagoner
Comment 56 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?
Alexey Proskuryakov
Comment 57 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?
Andy VanWagoner
Comment 58 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.
Alex Christensen
Comment 59 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
Michael Catanzaro
Comment 60 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.
Michael Catanzaro
Comment 61 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
Andy VanWagoner
Comment 62 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.
Alexey Proskuryakov
Comment 63 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.
Andy VanWagoner
Comment 64 2015-12-24 12:42:41 PST
Thanks Alexey!
Andy VanWagoner
Comment 65 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.
Michael Catanzaro
Comment 66 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....
Andy VanWagoner
Comment 67 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.
Andy VanWagoner
Comment 68 2015-12-24 14:29:59 PST
https://bugs.webkit.org/show_bug.cgi?id=152550
Note You need to log in before you can comment on or make changes to this bug.