[websql] Filenames are utf-8, not latin1.

[websql] Filenames are utf-8, not latin1.

From WTFString.h:
  // Construct a string with latin1 data, from a null-terminated source.
  String(const char* characters);
This is ... not right for this case.  Why do we even HAVE that lever?
String::fromUTF8() works better.

Bug was basically that the above-SQLite code in the webdatabase module
converted filenames differently from the below-SQLite code in
chromium_vfs, so the above-level code caused the tracker to create
things, but the below-level code couldn't find the matching record.

BUG=680410
R=jsbell@chromium.org
TBR=michaeln@chromium.org

Review-Url: https://codereview.chromium.org/2634473002
Cr-Commit-Position: refs/heads/master@{#444435}
Committed: https://chromium.googlesource.com/chromium/src/+/713773b46f90f18b194cd10c68a8f9ebacb2e49f

[Scott Hess - ex-Googler, 2017-01-13 00:19:44]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 00:21:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-13 00:22:21]

I didn't add a test because ... we've gotten this far without testing.

The chromiumOpenInternal() instance was the obvious blocker.  The other cases
are all necessary for correct operation.  Surprisingly, most things "worked"
without those other changes, but they were wrong (for instance, SQLite would
never notice a hot journal).  The other three cases are essentially untestable,
I individually tracked them down with logging and tweaking.  I never managed to
get the truncate to fire, but I believe it's correct.

[commit-bot: I haz the power, 2017-01-13 01:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 01:54:09]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2017-01-13 02:52:33]

Also, reviewers chosen as the union of OWNERS and people-on-the-bug-report. 
Feel free to train me to redirect.

[jsbell, 2017-01-13 17:21:37]

lgtm

It seems like this should be pretty easy to test with a layout test - something
like
third_party/WebKit/LayoutTests/storage/websql/open-database-creation-callback.html
- should we give that a try?

[Scott Hess - ex-Googler, 2017-01-14 06:46:24]

layout test

[Scott Hess - ex-Googler, 2017-01-14 06:46:32]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 06:46:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-14 06:50:27]

On 2017/01/13 17:21:37, jsbell wrote:
> lgtm
> 
> It seems like this should be pretty easy to test with a layout test -
something
> like
>
third_party/WebKit/LayoutTests/storage/websql/open-database-creation-callback.html
> - should we give that a try?

Oooookaaaaay.  Though I added a new test instead.  Mostly I worry about whether
adding a test to woefully-untested code is worth the time (also, layout tests
honestly aren't what I would call sufficient for this infrastructure).

PTAL, then?

[commit-bot: I haz the power, 2017-01-14 08:49:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-14 08:49:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Scott Hess - ex-Googler, 2017-01-14 22:27:59]

WAT?  Windows has WebSQL, too?

[Scott Hess - ex-Googler, 2017-01-14 22:28:06]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 22:28:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-14 22:30:09]

On 2017/01/14 06:50:27, Scott Hess wrote:
> On 2017/01/13 17:21:37, jsbell wrote:
> > lgtm
> > 
> > It seems like this should be pretty easy to test with a layout test -
> something
> > like
> >
>
third_party/WebKit/LayoutTests/storage/websql/open-database-creation-callback.html
> > - should we give that a try?
> 
> Oooookaaaaay.  Though I added a new test instead.  Mostly I worry about
whether
> adding a test to woefully-untested code is worth the time (also, layout tests
> honestly aren't what I would call sufficient for this infrastructure).
> 
> PTAL, then?

Well, _that_ was embarrassing.  I was focussed on a specific case, and it
completely slipped my mind that there were two implementations of this code.

[commit-bot: I haz the power, 2017-01-14 23:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-14 23:54:36]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-01-17 17:29:53]

lgtm again with a couple nits

> Mostly I worry about whether adding a test to
> woefully-untested code is worth the time (also, layout
> tests honestly aren't what I would call sufficient for
> this infrastructure).

Did its job this time. :) Mostly I was worried about regressions, e.g. due to a
refactor that eliminates one of the newly added conversions, reintroducing the
bug.

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html
(right):

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html:2:
<head>
Can you declare the file encoding? This should be sufficient:

<meta charset=utf-8> 

(Otherwise the HTML parser defaults to Windows-1252 unless there's other
information available, like sniffing the stream.)

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html:25:
// Open database with utf-8 name.
nit: Non-ASCII

(it's not UTF-8 encoded by the time JavaScript makes the openDatabase call)

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html:30:
log("Database creation failed");
finishTest() and return here?

[Scott Hess - ex-Googler, 2017-01-17 20:41:24]

final changes?

[Scott Hess - ex-Googler, 2017-01-17 20:41:37]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2017-01-17 20:41:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2634473002/#ps60001
(title: "final changes?")

[commit-bot: I haz the power, 2017-01-17 20:42:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-17 20:42:36]

OK, rolling with that, as my changes were minor enough that the waterfall can be
my reviewer.

[I'd test your assertion about Windows default encoding, but I'm in the midst of
a multiyear mission to successfully reimage my Windows desktop.]

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html
(right):

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html:2:
<head>
On 2017/01/17 17:29:52, jsbell wrote:
> Can you declare the file encoding? This should be sufficient:
> 
> <meta charset=utf-8> 
> 
> (Otherwise the HTML parser defaults to Windows-1252 unless there's other
> information available, like sniffing the stream.)

So the Windows version was a stress test, then?

I'm putting the line after <head> based on web-reading.  Other layout tests
don't seem to do particularly consistent things, AFAICT.

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html:25:
// Open database with utf-8 name.
On 2017/01/17 17:29:52, jsbell wrote:
> nit: Non-ASCII
> 
> (it's not UTF-8 encoded by the time JavaScript makes the openDatabase call)

Acknowledged.

https://codereview.chromium.org/2634473002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/websql/open-database-utf8-name.html:30:
log("Database creation failed");
On 2017/01/17 17:29:53, jsbell wrote:
> finishTest() and return here?

Acknowledged.  Restructured the if() to take advantage of the early return.

[jsbell, 2017-01-17 21:04:52]

On 2017/01/17 20:42:36, Scott Hess wrote:
> [I'd test your assertion about Windows default encoding, but I'm in the midst
of
> a multiyear mission to successfully reimage my Windows desktop.]

Nothing specific to Windows-the-OS. The default encoding for HTML is a
windows-1252 which is variant of ISO-8859-1. This falls out of
https://html.spec.whatwg.org/multipage/syntax.html#determining-the-character-...
if layers upon layers of hints and hacks and everything else fails. I think our
layout tests are served with an encoding header so they get UTF-8 but it's a bit
sketchy, e.g. if you load them via file: or open them in another browser or ...

Anyway, to be safe, if your file is not plain ASCII specify an encoding.

[commit-bot: I haz the power, 2017-01-18 00:54:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 00:54:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Scott Hess - ex-Googler, 2017-01-18 01:11:21]

On 2017/01/18 00:54:04, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

I don't see how that could relate, unless my code is poisoning the well,
somehow?  Ima pusha button again.

[Scott Hess - ex-Googler, 2017-01-18 01:11:24]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2017-01-18 01:12:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 01:21:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 01:21:36]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Scott Hess - ex-Googler, 2017-01-18 19:11:37]

On 2017/01/18 01:21:36, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

LayoutTests/virtual/mojo-localstorage/external/wpt/webstorage/README.txt is
missing (each virtual suite must have one).

Again, does not seem related.  Third time is the charm.

[Scott Hess - ex-Googler, 2017-01-18 19:11:46]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2017-01-18 19:12:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 19:16:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484766706782720,
"parent_rev": "284d5abdceb308ce4fd96d6301a160f7c0f0fe4f", "commit_rev":
"713773b46f90f18b194cd10c68a8f9ebacb2e49f"}

[commit-bot: I haz the power, 2017-01-18 19:17:29]

Description was changed from

==========
[websql] Filenames are utf-8, not latin1.

From WTFString.h:
  // Construct a string with latin1 data, from a null-terminated source.
  String(const char* characters);
This is ... not right for this case.  Why do we even HAVE that lever?
String::fromUTF8() works better.

Bug was basically that the above-SQLite code in the webdatabase module
converted filenames differently from the below-SQLite code in
chromium_vfs, so the above-level code caused the tracker to create
things, but the below-level code couldn't find the matching record.

BUG=680410
R=jsbell@chromium.org
TBR=michaeln@chromium.org
==========

to

==========
[websql] Filenames are utf-8, not latin1.

From WTFString.h:
  // Construct a string with latin1 data, from a null-terminated source.
  String(const char* characters);
This is ... not right for this case.  Why do we even HAVE that lever?
String::fromUTF8() works better.

Bug was basically that the above-SQLite code in the webdatabase module
converted filenames differently from the below-SQLite code in
chromium_vfs, so the above-level code caused the tracker to create
things, but the below-level code couldn't find the matching record.

BUG=680410
R=jsbell@chromium.org
TBR=michaeln@chromium.org

Review-Url: https://codereview.chromium.org/2634473002
Cr-Commit-Position: refs/heads/master@{#444435}
Committed:
https://chromium.googlesource.com/chromium/src/+/713773b46f90f18b194cd10c68a8...
==========

[commit-bot: I haz the power, 2017-01-18 19:17:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/713773b46f90f18b194cd10c68a8...