[webkit-reviews] review denied: [Bug 188696] beforeunload interoperability issues with a throwing return : [Attachment 347442] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied PhistucK <phistuck at chromium.org>'s
request for review:
Bug 188696: beforeunload interoperability issues with a throwing return
https://bugs.webkit.org/show_bug.cgi?id=188696

Attachment 347442: Patch

https://bugs.webkit.org/attachment.cgi?id=347442&action=review




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 347442
  --> https://bugs.webkit.org/attachment.cgi?id=347442
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347442&action=review

Thanks for contributing this patch. Seems like a good change to make! Almost
there, but needs a little work. Need to add a test, and there’s a mistake in
the patch that a good test would find.

> Source/WebCore/ChangeLog:8
> +	   No new tests .

Normally we’d say *why* there are no new tests. If there is a change in
behavior, the project requires we add a test to demonstrate that the patch
works. Can we construct a test? I think we should be able to do this by passing
an object that shows an exception in its valueOf function. We should look at
our existing beforeunload tests to see if one can be adapted for this purpose.
Ideally another test will cover the case where the valueOf function does not
return an exception and ensure that it is called exactly once. That would have
caught the mistake in the patch that I mention below.

> Source/WebCore/bindings/js/JSEventListener.cpp:194
> +		       reportException(exec, exception);

We might need something like the call to uncaughtExceptionInEventHandler above.
Not sure if we need it. Might require a little research.

> Source/WebCore/bindings/js/JSEventListener.cpp:196
> +		      
handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event),
convert<IDLNullable<IDLDOMString>>(*exec, retval));

This needs to pass returnedString, and not compute it a second time.