Bug 188174

Created attachment 346093 [details]
Patch

Comment on attachment 346093 [details]
Patch

Is there a way we could do this without using CF?

We should be able to save them elsewhere rather than CF, will update.

Created attachment 346178 [details]
Patch

Updated to save search terms to registry rather than using CF.

Comment on attachment 346178 [details]
Patch

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

New failing tests:
imported/blink/transitions/unprefixed-transform.html
legacy-animation-engine/imported/blink/transitions/unprefixed-transform.html

Created attachment 346220 [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

WK1 and WK2 of Cocoa port shares saveRecentSearches.

Unfortunately, WK1 of Windows port is using CF.
https://github.com/WebKit/webkit/blob/7ba360682b81ec26924a602eaabc6f038e0140f1/Source/WebCore/platform/win/SearchPopupMenuWin.cpp#L58

Comment on attachment 346178 [details]
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-regsetvalueexw

> 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.

Created attachment 346656 [details]
Patch

Comment on attachment 346656 [details]
Patch

I think in the Windows registry is a bad place to save recent searches.  It makes it hard to clear.  Shouldn't we just put it in a file on disk somewhere?

I think WK1 and WK2 of WinCairo port should share the implementation of saveRecentSearches as well as Cocoa port does.

(In reply to Alex Christensen from comment #11)
> Comment on attachment 346656 [details]
> Patch
> 
> I think in the Windows registry is a bad place to save recent searches.  It
> makes it hard to clear.  Shouldn't we just put it in a file on disk
> somewhere?

I wasn't entirely sure that sticking a file in the user's local application data would be better, but it seems like there's already data there for indexeddb and localstorage so that seems fine.

(In reply to Fujii Hironori from comment #12)
> I think WK1 and WK2 of WinCairo port should share the implementation of
> saveRecentSearches as well as Cocoa port does.

Should we do that as part of this, or make the new non-CF implementation and then update WK1 separately?

Comment on attachment 346656 [details]
Patch

application data directory is much better for this sort of thing than the registry.  Implementations ought to be shared, and ideally wouldn't use CF.

Created attachment 352094 [details]
Save search inputs to filesystem

Sorry about the long delay, had internal items then vacation.

Switching to using filesystem for the storage and replacing the SearchPopupMenuWin implementation as well.

Need to see if this is happy for win.

Comment on attachment 352094 [details]
Save search inputs to filesystem

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

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:25
> +#include <WebCore/UserAgent.h>

Do you need to include <WebCore/UserAgent.h> in this file?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:58
> +    file.append(name);

Can `name` contain '/', '\', or '..'? How much is the maximum length? 
How do you think the idea using single SQLite database, for example 'search.db' instead of many files?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:72
> +void saveRecentSearches(const String& name, const Vector<WebCore::RecentSearch>& searchItems)

I think you shouldn't use 'WebCore::' in WebCore.

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:91
> +        auto nullTerminatedSearchItem = WTF::stringToNullTerminatedWChar(searchItems[i].string);

Remove 'WTF::' because there is using statement.
https://github.com/WebKit/webkit/blob/2fde85661b4bf2a3ad6344694401d91e9220e99e/Source/WTF/wtf/text/win/WCharStringExtras.h#L68

Created attachment 352625 [details]
Save windows search inputs to sqlite db

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


ERROR: Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:301:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1 in 5 files


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

Created attachment 352626 [details]
Save windows search inputs to sqlite db fixed newline

Created attachment 352627 [details]
Save windows search inputs to sqlite db fixed newline

Created attachment 352647 [details]
Save windows search inputs to sqlite db

A version that saves to a sqlite db. I'd initially thought it might be overkill vs plain file, but it actually wasn't too bad.

Comment on attachment 352647 [details]
Save windows search inputs to sqlite db

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

> Source/WebCore/PlatformWin.cmake:105
> +    platform/win/SearchPopupMenuWinDB.cpp

This is almost platform independent. Remove 'Win'. It is OK for me to put this file in this platform/win/ dir until other ports reuse this.

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:25
> +#include <WebCore/FileSystem.h>

Do you use <WebCore/FileSystem.h> in this file?

> Source/WebCore/platform/win/SearchPopupMenuWin.cpp:27
> +#include <wtf/text/win/WCharStringExtras.h>

Do you use <wtf/text/win/WCharStringExtras.h> in this file?

> Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:35
> +static const String createSearchTableSQL {

String ctor calls fastMalloc. Do not define String global variables.
Use WTF::ASCIILiteral instead.
> static constexpr auto createSearchTableSQL = "..."_s;

> Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:38
> +    "    ind INTEGER NOT NULL,"

ind -> index
https://webkit.org/code-style-guidelines/#names-full-words

> Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:58
> +    static SearchPopupMenuDB inst;

inst -> instance
https://webkit.org/code-style-guidelines/#names-full-words

> Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:81
> +    SQLiteTransaction xact(m_database, false);

xact -> transaction
https://webkit.org/code-style-guidelines/#names-full-words

> Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:82
> +    auto rem = getPreparedStatement(removeSearchTermsForNameSQL);

https://webkit.org/code-style-guidelines/#names-full-words

> Source/WebCore/platform/win/SearchPopupMenuWinDB.h:43
> +    static WEBCORE_EXPORT SearchPopupMenuDB &instance();

instance -> singleton
https://webkit.org/code-style-guidelines/#singleton-static-member

> Source/WebCore/platform/win/SearchPopupMenuWinDB.h:63
> +    HashMap<String, std::unique_ptr<SQLiteStatement>> m_statements;

Do you really need this HashMap? 
https://trac.webkit.org/wiki/InvestigatingLeaksAndBloat

> m_loadSearchTermsForNameStatement = createPreparedStatement(loadSearchTermsForNameSQL);

Comment on attachment 352647 [details]
Save windows search inputs to sqlite db

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

> Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:119
> +        // We are choosing not to use or store search times on Windows at thistime, so for now it's OK to use a "distant past" time as a placeholder.

thistime -> this time

(In reply to Fujii Hironori from comment #23)
> Comment on attachment 352647 [details]
> Save windows search inputs to sqlite db
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352647&action=review

Just updating for most of these, but a few comments.

> > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:25
> > +#include <WebCore/FileSystem.h>
> 
> Do you use <WebCore/FileSystem.h> in this file?

Yes, I'm using pathByAppendingComponent and localUserSpecificStorageDirectory to generate the database file location.

> > Source/WebCore/platform/win/SearchPopupMenuWinDB.cpp:38
> > +    "    ind INTEGER NOT NULL,"
> 
> ind -> index
> https://webkit.org/code-style-guidelines/#names-full-words

Index is a reserved word in SQL, so we cannot name a column index (we could use "index" with the double quotes, but that becomes a bit of a mess). I'll change the name to position or something like that.

> > Source/WebCore/platform/win/SearchPopupMenuWinDB.h:63
> > +    HashMap<String, std::unique_ptr<SQLiteStatement>> m_statements;
> 
> Do you really need this HashMap? 
> https://trac.webkit.org/wiki/InvestigatingLeaksAndBloat

Since there's only a few specific statements, we can probably do without it as you mention. If this somehow ended up with a ton of statements it might be a problem, but that seems unlikely.

(In reply to Stephan Szabo from comment #25)
> (In reply to Fujii Hironori from comment #23)
> > Comment on attachment 352647 [details]
> > Save windows search inputs to sqlite db
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=352647&action=review
> 
> Just updating for most of these, but a few comments.
> 
> > > Source/WebCore/platform/win/SearchPopupMenuWin.cpp:25
> > > +#include <WebCore/FileSystem.h>
> > 
> > Do you use <WebCore/FileSystem.h> in this file?
> 
> Yes, I'm using pathByAppendingComponent and
> localUserSpecificStorageDirectory to generate the database file location.

Sorry, looked back now and had misremembered that I moved the file location out to the DB cpp as well. Will kill that too.

Created attachment 352735 [details]
Save windows search inputs to sqlite db

Removed extra includes
Renamed the db file source to remove win
Switched to explicit statements rather than hashmap
Updated variable names
Switched ind -> position in sql database
Fixed missing space in comment

Comment on attachment 352735 [details]
Save windows search inputs to sqlite db

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

Look good to me. Please add ChangeLog entry. I need to review it, too, to mark r+.

> Source/WebCore/platform/win/SearchPopupMenuDB.cpp:89
> +    for (const auto &search : searches) {

Nit: auto&
https://webkit.org/code-style-guidelines/#pointers-cpp

Comment on attachment 352735 [details]
Save windows search inputs to sqlite db

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

> Source/WebCore/platform/win/SearchPopupMenuDB.h:43
> +    static WEBCORE_EXPORT SearchPopupMenuDB &singleton();

Nit: SearchPopupMenuDB&
https://webkit.org/code-style-guidelines/#pointers-cpp

Created attachment 352810 [details]
Save windows search inputs to sqlite db

Comment on attachment 352810 [details]
Save windows search inputs to sqlite db

Clearing flags on attachment: 352810

Committed r237308: <https://trac.webkit.org/changeset/237308>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/45421883>