[webkit-reviews] review denied: [Bug 30535] Storage events should use Document::url() rather than documentURI() : [Attachment 41459] Patch v1

Mon Oct 19 17:27:52 PDT 2009

Darin Adler <darin at apple.com> has denied Jeremy Orlow <jorlow at chromium.org>'s
request for review:
Bug 30535: Storage events should use Document::url() rather than documentURI()
https://bugs.webkit.org/show_bug.cgi?id=30535

Attachment 41459: Patch v1
https://bugs.webkit.org/attachment.cgi?id=41459&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.