[webkit-reviews] review denied: [Bug 119791] [Qt] Remove the fix in QWebPage::javaScriptConsoleMessage introduced by (r61433) : [Attachment 209382] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied Arunprasad Rajkumar
<arurajku at cisco.com>'s request for review:
Bug 119791: [Qt] Remove the fix in QWebPage::javaScriptConsoleMessage
introduced by (r61433)
https://bugs.webkit.org/show_bug.cgi?id=119791

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

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209382&action=review


> Source/WebKit/qt/ChangeLog:3
> +	   <https://webkit.org/b/119791> [Qt] Remove the fix in
QWebPage::javaScriptConsoleMessage introduced by (r61433)

There has been some discussion on webkit-dev that we should avoid the new
changelog format of prepare-changelog and edit it manually to use the old
https://bugs.webkit.org/show_bug.cgi?id=119791 URL format on the second line
instead.

The rests are mostly nitpicks, the comment could still use a bit of polishing:

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:162
> +    // Load an empty url to send onunload event to the running page before

*the* onunload event

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:164
> +    // Prior to this fix onunload event would be triggered from '~QWebPage',
but

*the* onunload event

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:165
> +    // it may call virtual functions(e.g. calling a window.alert from
window.onunload)

space before the parenthesis

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:167
> +    // When control is in '~WebPage' the vtable of 'QWebPage' points to the
derived 

"When *the* control is in '~WebPage'" or just "When in '~WebPage'"

> Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:168
> +    // class 'WebPage' and safe to send onunload event.

It was safe to send it, but it wasn't possible to receive virtual calls from
the JS handler. Maybe something like: "and it's possible to receive
javaScriptAlert calls."?