[webkit-reviews] review denied: [Bug 188174] [WinCairo] Search terms are not saved for <input type="search"> : [Attachment 346178] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Stephan Szabo
<stephan.szabo at sony.com>'s request for review:
Bug 188174: [WinCairo] Search terms are not saved for <input type="search">
https://bugs.webkit.org/show_bug.cgi?id=188174

Attachment 346178: Patch

https://bugs.webkit.org/attachment.cgi?id=346178&action=review




--- Comment #9 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 346178
  --> https://bugs.webkit.org/attachment.cgi?id=346178
Patch

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

> Source/WebKit/UIProcess/win/WebPageProxyWin.cpp:62
> +    if (size) {

You don't need "if (size)" because the following 'for' skips if size is zero.

> Source/WebKit/UIProcess/win/WebPageProxyWin.cpp:65
> +	       if (::RegSetValueEx(key, std::to_wstring(i).c_str(), 0, REG_SZ,
reinterpret_cast<const BYTE*>(searchItems[i].string.characters16()),
searchItems[i].string.length() * sizeof(UChar)) != ERROR_SUCCESS)

According to the spec, you should include the size of the terminating null
character.
You should use stringToNullTerminatedWChar.

https://docs.microsoft.com/en-us/windows/desktop/api/winreg/nf-winreg-regsetval
ueexw

> cbData
> The size of the information pointed to by the lpData parameter, in bytes. If
the data is of type REG_SZ, REG_EXPAND_SZ, or REG_MULTI_SZ, cbData must include
the size of the terminating null character or characters.

> Source/WebKit/UIProcess/win/WebPageProxyWin.cpp:80
> +    if (::RegCreateKeyEx(HKEY_CURRENT_USER, keyName.characters16(), 0,
nullptr, REG_OPTION_NON_VOLATILE, KEY_ALL_ACCESS, nullptr, &key, nullptr) !=
ERROR_SUCCESS)

You should use stringToNullTerminatedWChar.

> Source/WebKit/UIProcess/win/WebPageProxyWin.cpp:101
> +	   auto buff = std::make_unique<BYTE[]>(size);

WTF has makeUniqueArray which is using fastMalloc.

> Source/WebKit/UIProcess/win/WebPageProxyWin.cpp:104
> +

Why do you want to remove the comment? I think it should be preserved to
explain WallTime::infinity. 
// We are choosing not to use or store search times on Windows at this time, so
for now it's OK to use a "distant past" time as a placeholder.