[webkit-reviews] review granted: [Bug 24747] preloading logic caused the same resource was loaded and reloaded.(sent two requests for same resource) : [Attachment 30712] patch for test of this issue

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted johnnyding
<johnnyding.webkit at gmail.com>'s request for review:
Bug 24747: preloading logic caused the same resource was loaded and
reloaded.(sent two requests for same resource)
https://bugs.webkit.org/show_bug.cgi?id=24747

Attachment 30712: patch for test of this issue
https://bugs.webkit.org/attachment.cgi?id=30712&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   Reviewed by NOBODY (OOPS!).
> +	   https://bugs.webkit.org/show_bug.cgi?id=24747
> +
> +	   Add a test case for multiple requests for same sub-resource due to
preload.

We usually put an empty line after "Reviewed by" line, not between bug
reference and title.

> +
> +	   * http\tests\loading\preload-img-test-expected.txt: Added.
> +	   * http\tests\loading\preload-img-test.html: Added.
> +	   * http\tests\loading\resources\preload-test.jpg: Added.
> +	   * http\tests\resources\network-simulator.php:

One day we should fix prepare-ChangeLog to not emit Windows paths.

> +main frame - didStartProvisionalLoadForFrame
> +main frame - didCommitLoadForFrame
> +main frame - didFinishDocumentLoadForFrame
> +main frame - didHandleOnloadEventsForFrame
> +main frame - didFinishLoadForFrame

These lines are useless - but I know that they are generated because the test
in in a loading/ directory. Maybe we just need a new directory for preloading
tests.

> +	  document.getElementById("outputPanel").innerHTML = "FALIED";

Typo: "FAILED".

+  <span id="outputPanel">PASSED</span>

It's not good to have "PASSED" output from the beginning - if the test doesn't
run to completion, that could mask the failure.

These are all extremely minor nitpicks, just wanted to mention them for the
future. No need to fix them to land this patch.

It is really great to have a framework for preloading tests, and thanks for
refactoring network-simulator!

r=me