Bug 30640

Summary: Combine local and session storage tests into one and use script-tests properly
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 30535    
Attachments:
Description Flags
Patch v1
none
Patch v1
none
Patch v1 darin: review+

Description Jeremy Orlow 2009-10-21 11:27:28 PDT
In https://bugs.webkit.org/show_bug.cgi?id=30535 Darin pointed out that I'm not using "script-tests" correctly.  Apparently the tool assumes that all layout tests that correspond to a test in a script-tests directory is under its control.  The dom storage layout tests require a small modification to the .html in order to have session storage and local storage tests share the same JavaScript file.

I think the best solution for now is to move them back to a resources directory.  Darin has suggested that maybe we shouldn't have seprate tests for session storage and local storage, but I'm not positive whether this is the best route to take yet since there are some places where session and local storage diverge (like in quota support).
Comment 1 Jeremy Orlow 2009-10-21 11:30:26 PDT
Created attachment 41590 [details]
Patch v1
Comment 2 Ojan Vafai 2009-10-21 11:55:47 PDT
Why doesn't using a TEMPLATE.html file in domstorage/script-tests work for this?
Comment 3 Jeremy Orlow 2009-10-21 12:10:56 PDT
(In reply to comment #2)
> Why doesn't using a TEMPLATE.html file in domstorage/script-tests work for
> this?

I have no idea.

Where's documentation on how to use this script-tests stuff?
Comment 4 Jeremy Orlow 2009-10-21 12:14:27 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Why doesn't using a TEMPLATE.html file in domstorage/script-tests work for
> > this?
> 
> I have no idea.
> 
> Where's documentation on how to use this script-tests stuff?

Oh...well if the template is tied to the script-tests directory it's not going to work because the customization is passing in either localStorage or sessionStorage.

If the template is tied to the directory where the HTML exists then we might be able to make this work.  The thing is that the directory structure is as follows:

domstorage/script-tests
domstorage/localstorage
domstorage/sessionstorage

The latter 2 directories are where the .html files exist.  The html files pass in either "localStorage" or "sessionStorage" to the runTest function in the .js files.  I suspect that this is different enough from the rest of the script tests that we can't trivially re-use the infrastructure.  And, unless this is going to be useful elsewhere, I don't see a point to adapting the infrastructure to this use case.
Comment 5 Jeremy Orlow 2009-10-21 13:46:22 PDT
After talking to Ojan, it sounded like it was best to simply do what Darin originally suggested.
Comment 6 Jeremy Orlow 2009-10-21 13:46:56 PDT
Created attachment 41607 [details]
Patch v1
Comment 7 Jeremy Orlow 2009-10-21 13:48:39 PDT
Created attachment 41608 [details]
Patch v1
Comment 8 Ojan Vafai 2009-10-21 15:34:15 PDT
One nit:

In your template, <script src="../../fast/js/resources/js-test-post-function.js"></script> should go after the runTest call. Is there a problem with that?
Comment 9 Darin Adler 2009-10-21 15:35:39 PDT
Comment on attachment 41608 [details]
Patch v1

Looks good. r=me
Comment 10 Jeremy Orlow 2009-10-21 15:43:01 PDT
Ojan: I (In reply to comment #8)
> One nit:
> 
> In your template, <script
> src="../../fast/js/resources/js-test-post-function.js"></script> should go
> after the runTest call. Is there a problem with that?

This won't work since the runTest call ends up calling it before the parser will load that file.
Comment 11 Ojan Vafai 2009-10-21 15:56:08 PDT
(In reply to comment #10)
> Ojan: I (In reply to comment #8)
> > One nit:
> > 
> > In your template, <script
> > src="../../fast/js/resources/js-test-post-function.js"></script> should go
> > after the runTest call. Is there a problem with that?
> 
> This won't work since the runTest call ends up calling it before the parser
> will load that file.

That seems fine.

Now that you don't need to pass in the storage type from the TEMPLATE.html file, you could reduce the boilerplate needed for new storage JS tests, and be more consistent with most other JS tests.

There's no need to have a runTest function at all. Just run the actual JS at the end of the test and set window.successfullyParsed = true. Then instead of using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js.

<html>
<head>
<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
<script src="../../fast/js/resources/js-test-pre.js"></script>
</head>
<body>
<p id="description"></p>
<div id="console"></div>
<script src="YOUR_JS_FILE_HERE"></script>
<script src="../../fast/js/resources/js-test-post.js"></script>
</body>
</html>
Comment 12 Jeremy Orlow 2009-10-21 16:43:45 PDT
(In reply to comment #11)
> There's no need to have a runTest function at all. Just run the actual JS at
> the end of the test

I've changed my code to do it this way.

> and set window.successfullyParsed = true. Then instead of
> using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js.

This is not possible since half the local storage tests involve events.  This is why I need to use js-test-post-function.  But I need to include it first because the other half don't and make the call before the page is fully loaded.
Comment 13 Jeremy Orlow 2009-10-21 16:48:00 PDT
Submitted because this is blocking my other change, but Ojan let me know if you have any further suggestions and I'll address them in future patches.
Comment 14 Ojan Vafai 2009-10-21 18:11:35 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > and set window.successfullyParsed = true. Then instead of
> > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js.
> 
> This is not possible since half the local storage tests involve events.  This
> is why I need to use js-test-post-function.  But I need to include it first
> because the other half don't and make the call before the page is fully loaded.

Ah. I looked at the patch for cases like this and didn't see any. That makes sense then. Thanks for humoring my nits.
Comment 15 Jeremy Orlow 2009-10-21 18:21:20 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > and set window.successfullyParsed = true. Then instead of
> > > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js.
> > 
> > This is not possible since half the local storage tests involve events.  This
> > is why I need to use js-test-post-function.  But I need to include it first
> > because the other half don't and make the call before the page is fully loaded.
> 
> Ah. I looked at the patch for cases like this and didn't see any. That makes
> sense then. Thanks for humoring my nits.

The change that needs it is the one that depends on this.  :-)
Comment 16 Jeremy Orlow 2009-10-21 18:21:46 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > and set window.successfullyParsed = true. Then instead of
> > > > using js-test-post-function.js in the TEMPLATE.html file, use js-test-post.js.
> > > 
> > > This is not possible since half the local storage tests involve events.  This
> > > is why I need to use js-test-post-function.  But I need to include it first
> > > because the other half don't and make the call before the page is fully loaded.
> > 
> > Ah. I looked at the patch for cases like this and didn't see any. That makes
> > sense then. Thanks for humoring my nits.
> 
> The change that needs it is the one that depends on this.  :-)

Er...the _test_ that needs this is in the _bug_ that depends on this.