Bug 95912 - IndexedDB: open-in-upgradeneeded layout test
Summary: IndexedDB: open-in-upgradeneeded layout test
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
Depends on: 90411
  Show dependency treegraph
Reported: 2012-09-05 16:52 PDT by David Grogan
Modified: 2012-10-09 12:18 PDT (History)
5 users (show)

See Also:

Patch (5.97 KB, patch)
2012-09-05 16:53 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2012-09-20 17:50 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2012-10-08 18:23 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (5.94 KB, patch)
2012-10-08 18:26 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-09-05 16:52:50 PDT
IndexedDB: open-in-upgradeneeded layout test
Comment 1 David Grogan 2012-09-05 16:53:27 PDT
Created attachment 162370 [details]
Comment 2 David Grogan 2012-09-05 17:05:29 PDT
This should wait until after 90411 lands.

This currently behaves differently in DRT and content_shell, though I'm not sure which is more correct.  Suppose we call the result of this test in DRT "A" and the result from content_shell "B".  "B" is included in this patch.  It's also what firefox produces.

If the code from 90411 is patched in, the result of this test in DRT is now "B".  But then the result from content_shell ("C") is different from either "A" or "B".  "C" is definitely wrong.

So I'll wait until 90411 lands and sort this out after.
Comment 3 WebKit Review Bot 2012-09-07 12:40:44 PDT
Comment on attachment 162370 [details]

Attachment 162370 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13785566

New failing tests:
Comment 4 Joshua Bell 2012-09-19 16:58:07 PDT
I just tried this patch again, and it passes in DRT and content_shell's output matches.

Did we simplify 90411 enough to eliminate the "B" => "C" change?
Comment 5 David Grogan 2012-09-20 17:50:36 PDT
Created attachment 165022 [details]
Comment 6 David Grogan 2012-09-20 17:51:45 PDT
Yeah, seems to work.  Can you look it over for an LGTM?
Comment 7 Joshua Bell 2012-09-21 12:59:39 PDT
Comment on attachment 165022 [details]

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


> LayoutTests/storage/indexeddb/resources/intversion-open-in-upgradeneeded.js:36
> +    transaction.oncomplete = function() {

Just as a style thing, if this was written as:

transaction.oncomplete = function transactionOnComplete(evt) {

... then you could use preamble(evt); and get the name printed out while still (IMHO) keeping the readability benefits of a function expression.

(I haven't done that anywhere myself, was just pondering it while reading this patch.)

> LayoutTests/storage/indexeddb/resources/intversion-open-in-upgradeneeded.js:55
> +    evalAndLog("transaction = db.transaction('os')");

Is this just to confirm that the connection is still open? Maybe a debug() line explaining it would help.
Comment 8 Joshua Bell 2012-10-02 13:38:18 PDT
Is this waiting on anything?
Comment 9 David Grogan 2012-10-08 18:23:05 PDT
Created attachment 167664 [details]
Comment 10 David Grogan 2012-10-08 18:26:33 PDT
Created attachment 167665 [details]
Comment 11 David Grogan 2012-10-08 18:27:54 PDT
Tony, could you review this test?
Comment 12 WebKit Review Bot 2012-10-09 12:18:27 PDT
Comment on attachment 167665 [details]

Clearing flags on attachment: 167665

Committed r130791: <http://trac.webkit.org/changeset/130791>
Comment 13 WebKit Review Bot 2012-10-09 12:18:32 PDT
All reviewed patches have been landed.  Closing bug.