Bug 187712

Summary: TestWTF.WTF.StringConcatenate_Unsigned fails for unsigned short on Windows
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: Web Template FrameworkAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dbates, don.olmstead, ews-watchlist, Hironori.Fujii, jfbastien, pvollan, stephan.szabo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193101
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews205 for win-future
none
Patch
none
Patch for landing
none
Patch for landing none

Description Ross Kirsling 2018-07-16 15:03:55 PDT
On Windows, this API test failure occurs:

> TestWTF.WTF.StringConcatenate_Unsigned
>     
>     ..\..\Tools\TestWebKitAPI\Tests\WTF\StringConcatenate.cpp:84
>     Value of: makeString("hello ", static_cast<unsigned short>(42) , " world")
>       Actual: hello 42 world
>     Expected: "hello * world"
>     Which is: 00007FFAF03F82E8

The issue seems to be a bad template deduction for StringTypeAdapter when UCHAR_TYPE is wchar_t.
Comment 1 Ross Kirsling 2018-07-16 15:14:03 PDT
Created attachment 345123 [details]
Patch
Comment 2 Ross Kirsling 2018-07-16 15:21:48 PDT
The reason for #if OS(WINDOWS) here is because we need a raw `unsigned short` in order to get prioritized above the is_integral && !is_signed case, yet SFINAE only works on template params, so we can't do something like the following:

> template<>
> class StringTypeAdapter<unsigned short, typename std::enable_if_t<std::is_same<UChar, wchar_t>::value>> : public StringTypeAdapter<UChar, void>
Comment 3 Fujii Hironori 2018-07-16 19:58:28 PDT
See following bugs about this issue:

Bug 180720 – makeString: support more integral types
Bug 180753 – makeString: add single-character helpers
Comment 4 EWS Watchlist 2018-07-16 23:01:54 PDT
Comment on attachment 345123 [details]
Patch

Attachment 345123 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8560502

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 5 EWS Watchlist 2018-07-16 23:02:05 PDT
Created attachment 345150 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 6 Ross Kirsling 2018-07-17 09:17:07 PDT
(In reply to Fujii Hironori from comment #3)
> See following bugs about this issue:
> 
> Bug 180720 – makeString: support more integral types
> Bug 180753 – makeString: add single-character helpers

Thanks Fujii-san! JF, do you have an opinion about how this should be handled? (Am I just making Windows behavior worse so I can please a test case?)
Comment 7 Ross Kirsling 2018-07-19 10:19:04 PDT
Rereading Bug 180720 and Bug 180753, would it make the most sense to simply guard this oddly-behaving test with #if !PLATFORM(WIN) for now?
Comment 8 Ross Kirsling 2018-07-19 21:28:18 PDT
Created attachment 345424 [details]
Patch
Comment 9 Fujii Hironori 2018-07-19 21:51:09 PDT
Comment on attachment 345424 [details]
Patch

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

LGTM.

> Tools/ChangeLog:1
> +2018-07-19  Ross Kirsling  <rkirsling@gmail.com>

gmail.com
Comment 10 Ross Kirsling 2018-07-19 22:00:49 PDT
(In reply to Fujii Hironori from comment #9)
> Comment on attachment 345424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345424&action=review
> 
> LGTM.
> 
> > Tools/ChangeLog:1
> > +2018-07-19  Ross Kirsling  <rkirsling@gmail.com>
> 
> gmail.com

Ack, I submitted this from a different computer. Thanks for pointing that out.
Comment 11 Ross Kirsling 2018-07-19 22:03:18 PDT
Created attachment 345425 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-07-19 22:04:42 PDT
Comment on attachment 345425 [details]
Patch for landing

Rejecting attachment 345425 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 345425, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Hironori Fujii found in /Volumes/Data/EWS/WebKit/Tools/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/8595808
Comment 13 Ross Kirsling 2018-07-19 22:05:34 PDT
Created attachment 345426 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2018-07-19 23:10:41 PDT
Comment on attachment 345426 [details]
Patch for landing

Clearing flags on attachment: 345426

Committed r234024: <https://trac.webkit.org/changeset/234024>
Comment 15 WebKit Commit Bot 2018-07-19 23:10:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-07-19 23:12:15 PDT
<rdar://problem/42416633>