Bug 30535

Summary:

Storage events should use Document::url() rather than documentURI()

Product:

WebKit

Reporter:

Jeremy Orlow <jorlow>

Component:

New Bugs

Assignee:

Jeremy Orlow <jorlow>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

beidson, darin, fishd

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

30640

Bug Blocks:

Attachments:

Description	Flags
Patch v1	none
Patch v1	none
Patch v1	none
Patch v1	darin: review+

Jeremy Orlow

Reported 2009-10-19 14:17:40 PDT

Storage events should use Document::url() rather than Document::documentURI() per http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-October/023703.html

Attachments
Patch v1 (7.65 KB, patch) 2009-10-19 15:14 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Patch v1 (7.24 KB, patch) 2009-10-19 15:18 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Patch v1 (8.71 KB, patch) 2009-10-19 17:52 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Patch v1 (7.22 KB, patch) 2009-10-21 18:29 PDT, Jeremy Orlow	darin: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jeremy Orlow

Comment 1 2009-10-19 15:14:31 PDT

Created attachment 41458 [details] Patch v1

Jeremy Orlow

Comment 2 2009-10-19 15:18:00 PDT

Created attachment 41459 [details] Patch v1

Darin Adler

Comment 3 2009-10-19 17:27:52 PDT

Comment on attachment 41459 [details] Patch v1 Patch looks fine, but there are no expected results. > Index: LayoutTests/fast/js/resources/js-test-pre.js > =================================================================== > --- LayoutTests/fast/js/resources/js-test-pre.js (revision 49807) > +++ LayoutTests/fast/js/resources/js-test-pre.js (working copy) > @@ -76,18 +76,16 @@ function evalAndLog(_a) > { > if (typeof _a != "string") > debug("WARN: tryAndLog() expects a string argument"); > - var exception; > + > + // Log first in case things go horribly wrong or this causes a sync event. > + debug(_a); > + > var _av; > try { > _av = eval(_a); > } catch (e) { > - exception = e; > + testFailed(_a + " threw exception " + e); > } > - > - if (exception) > - testFailed(_a + " threw exception " + exception); > - else > - debug(_a); > } > > function shouldBe(_a, _b) I believe this will change the results of existing tests that have expected failures. Did you run them to check if that is so? > Index: LayoutTests/storage/domstorage/localstorage/documentURI.html > =================================================================== > --- LayoutTests/storage/domstorage/localstorage/documentURI.html (revision 0) > +++ LayoutTests/storage/domstorage/localstorage/documentURI.html (revision 0) > @@ -0,0 +1,15 @@ > +<html> > +<head> > +<link rel="stylesheet" href="../../../fast/js/resources/js-test-style.css"> > +<script src="../../../fast/js/resources/js-test-pre.js"></script> > +<script src="../../../fast/js/resources/js-test-post-function.js"></script> > +</head> > +<body> > +<p id="description"></p> > +<div id="console"></div> > +<script src="../script-tests/documentURI.js"></script> > +<script> > +runTest("window.localStorage"); > +</script> > +</body> > +</html> You should not make a test in script-tests that requires a non-standard HTML wrapper. Please find a way to make the test work without that. I'm sure you can find some other way to share the tests. How about a single test that checks both using a function call to repeat the tests twice? > Index: LayoutTests/storage/domstorage/sessionstorage/documentURI.html > =================================================================== > --- LayoutTests/storage/domstorage/sessionstorage/documentURI.html (revision 0) > +++ LayoutTests/storage/domstorage/sessionstorage/documentURI.html (revision 0) > @@ -0,0 +1,15 @@ > +<html> > +<head> > +<link rel="stylesheet" href="../../../fast/js/resources/js-test-style.css"> > +<script src="../../../fast/js/resources/js-test-pre.js"></script> > +<script src="../../../fast/js/resources/js-test-post-function.js"></script> > +</head> > +<body> > +<p id="description"></p> > +<div id="console"></div> > +<script src="../script-tests/documentURI.js"></script> > +<script> > +runTest("window.localStorage"); > +</script> > +</body> > +</html> I presume you meant window.sessionStorage here.

Jeremy Orlow

Comment 4 2009-10-19 17:49:10 PDT

(In reply to comment #3) > (From update of attachment 41459 [details]) > Patch looks fine, but there are no expected results. > > > Index: LayoutTests/fast/js/resources/js-test-pre.js > > =================================================================== > > --- LayoutTests/fast/js/resources/js-test-pre.js (revision 49807) > > +++ LayoutTests/fast/js/resources/js-test-pre.js (working copy) > > @@ -76,18 +76,16 @@ function evalAndLog(_a) > > { > > if (typeof _a != "string") > > debug("WARN: tryAndLog() expects a string argument"); > > - var exception; > > + > > + // Log first in case things go horribly wrong or this causes a sync event. > > + debug(_a); > > + > > var _av; > > try { > > _av = eval(_a); > > } catch (e) { > > - exception = e; > > + testFailed(_a + " threw exception " + e); > > } > > - > > - if (exception) > > - testFailed(_a + " threw exception " + exception); > > - else > > - debug(_a); > > } > > > > function shouldBe(_a, _b) > > I believe this will change the results of existing tests that have expected > failures. Did you run them to check if that is so? Yup. Btw, I added this recently so only domstorage is using it at this point which is why there wasn't more fallout. > > Index: LayoutTests/storage/domstorage/localstorage/documentURI.html > > =================================================================== > > --- LayoutTests/storage/domstorage/localstorage/documentURI.html (revision 0) > > +++ LayoutTests/storage/domstorage/localstorage/documentURI.html (revision 0) > > @@ -0,0 +1,15 @@ > > +<html> > > +<head> > > +<link rel="stylesheet" href="../../../fast/js/resources/js-test-style.css"> > > +<script src="../../../fast/js/resources/js-test-pre.js"></script> > > +<script src="../../../fast/js/resources/js-test-post-function.js"></script> > > +</head> > > +<body> > > +<p id="description"></p> > > +<div id="console"></div> > > +<script src="../script-tests/documentURI.js"></script> > > +<script> > > +runTest("window.localStorage"); > > +</script> > > +</body> > > +</html> > > You should not make a test in script-tests that requires a non-standard HTML > wrapper. Please find a way to make the test work without that. I'm sure you can > find some other way to share the tests. How about a single test that checks > both using a function call to repeat the tests twice? That's quite disheartening. I've been doing a lot to clean these up, but it seems like even after every cleanup, the next reviewer still hates the format. Here's one of the cleanup patches: https://bugs.webkit.org/show_bug.cgi?id=28939 I guess maybe I should have run this past webkit-dev or cc'ed people like you, Hyatt, Maciej and such just to be sure? Anyway, how strongly do you feel about this? Can I just move all my tests to resources instead of script-tests? If you want me to change, should I do a webkit-dev mail or just a very large cc list mail to make sure there's consensus on the format? > > Index: LayoutTests/storage/domstorage/sessionstorage/documentURI.html > > =================================================================== > > --- LayoutTests/storage/domstorage/sessionstorage/documentURI.html (revision 0) > > +++ LayoutTests/storage/domstorage/sessionstorage/documentURI.html (revision 0) > > @@ -0,0 +1,15 @@ > > +<html> > > +<head> > > +<link rel="stylesheet" href="../../../fast/js/resources/js-test-style.css"> > > +<script src="../../../fast/js/resources/js-test-pre.js"></script> > > +<script src="../../../fast/js/resources/js-test-post-function.js"></script> > > +</head> > > +<body> > > +<p id="description"></p> > > +<div id="console"></div> > > +<script src="../script-tests/documentURI.js"></script> > > +<script> > > +runTest("window.localStorage"); > > +</script> > > +</body> > > +</html> > > I presume you meant window.sessionStorage here. Oops!

Jeremy Orlow

Comment 5 2009-10-19 17:52:16 PDT

Created attachment 41466 [details] Patch v1

Darin Adler

Comment 6 2009-10-21 07:33:59 PDT

(In reply to comment #4) > Can I just move all my tests to resources instead of script-tests? Sure. The only mechanical problem is that the script to make script test wrappers will overwrite your HTML files unless you do that. > If you want me to change, should I do a > webkit-dev mail or just a very large cc list mail to make sure there's > consensus on the format? I don't really think we need a wider call for consensus here. I was pointing out a specific problem -- your test will be overwritten -- and making some suggestions about how to fit your test into the already-existing test framework. These tests are so close to fitting in as a normal script test that I think it would be easy to make them fit in entirely rather than being a non-standard variant. I sense your frustration here, and you are welcome to just do the minimum of moving out of the script-tests directory. I have read the earlier comments, and I don't agree that this is just more of the same. In fact, I don't "hate the format" at all. I'm just pushing, gently I hope, you to do that last bit of work so that these fit in with the other script tests. I don't think a hand-written wrapper is worth it just so you can have these two tests be the same body with a different string argument passed in.

Darin Adler

Comment 7 2009-10-21 07:35:09 PDT

Comment on attachment 41466 [details] Patch v1 review- because the make-script-test-wrappers script will overwrite storage/domstorage/localstorage/documentURI.html and storage/domstorage/sessionstorage/documentURI.html. Please fix that.

Jeremy Orlow

Comment 8 2009-10-21 18:29:17 PDT

Created attachment 41626 [details] Patch v1

Jeremy Orlow

Comment 9 2009-10-26 13:12:15 PDT

Committed r50088: <http://trac.webkit.org/changeset/50088>

Note You need to log in before you can comment on or make changes to this bug.