[webkit-reviews] review denied: [Bug 35180] [V8] V8LocationCustom.cpp doesn't handle location.hash updates properly : [Attachment 49462] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 35180: [V8] V8LocationCustom.cpp doesn't handle location.hash updates
properly
https://bugs.webkit.org/show_bug.cgi?id=35180

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/LayoutTests/fast/loader/about-blank-hash-change.html
> @@ -0,0 +1,34 @@
> +<script>
> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +}
> +
> +function onload_callback() {
> +  var old_hash = inner.location.hash;
> +  inner.location.hash = 'hash-ref';
> +  var c = document.getElementById('content');
> +  if (c.innerHTML.match(/^No callback/)) {
> +    c.innerHTML = 'PASS: inner.location.hash = "' + inner.location.hash +
'"';
> +  } else if (c.innerHTML.match(/^PASS/)) {
> +    c.innerHTML = c.innerHTML + 'FAIL: inner.location.hash = "' +
inner.location.hash + '"';
> +  }

up top you use 4 space indent, but here you use 2 spaces.  it'd be
good to use consistent indentation.


> +++ b/WebCore/bindings/v8/custom/V8LocationCustom.cpp
> @@ -70,15 +70,23 @@ void V8Location::hashAccessorSetter(v8::Local<v8::String>
name, v8::Local<v8::Va
>      if (!frame)
>	   return;
>  
> -    KURL url = frame->loader()->url();
> +    // We copy the URL to avoid accidentally modifying it in place
> +    // and then returning without actually doing anything. This is
> +    // perhaps overly conservative.
> +    KURL url = frame->loader()->url().copy();

This is unnecessary.  The .copy method is used when you need to copy
a KURL to another thread.  The existing code was using the copy ctor
which is perfect for this single threaded use case.