[webkit-reviews] review denied: [Bug 25990] [GTK] DumpRenderTree needs eventSender object and implementation : [Attachment 39297] Partial implementation of EventSender

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 9 13:38:12 PDT 2009


Gustavo Noronha (kov) <gns at gnome.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 25990: [GTK] DumpRenderTree needs eventSender object and implementation
https://bugs.webkit.org/show_bug.cgi?id=25990

Attachment 39297: Partial implementation of EventSender
https://bugs.webkit.org/attachment.cgi?id=39297&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +#include "DumpRenderTree.h"
> +
> +#include <gdk/gdk.h>
> +#include <gdk/gdkkeysyms.h>
> +
> +#include <wtf/ASCIICType.h>
> +#include <wtf/Platform.h>
> +#include <JavaScriptCore/JSObjectRef.h>
> +#include <JavaScriptCore/JSRetainPtr.h>
> +#include <JavaScriptCore/JSStringRef.h>
> +#include <webkit/webkitwebframe.h>
> +#include <webkit/webkitwebview.h>
> +
> +#include <string.h>

The first two includes are OK, but the others look a bit weirdly organized. I
think we want to follow our standards, which we use elsewhere, even if
DumpRenderTree has it this way somewhere, so let's do DumpRenderTree.h, and all

the other webkit-related includes in one block, and the system inclues in a
second block.

> +#define ZoomMultiplierRatio	   1.2f

like we discussed on IRC, make this all-caps, or turn it into a static const
float, and Z->z.

> +    WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame);
> +    ASSERT(view);
> +
> +    webkit_web_frame_layout(mainFrame);
> +
> +    gboolean return_val;

Both view and return_val are declared many lines before they are actually used.

Please put their declaration close to where they are used for the first time.

> +    if (!msgQueue[endOfQueue].delay) {
> +	   webkit_web_frame_layout(mainFrame);
> +	   WebKitWebView* view = webkit_web_frame_get_web_view(mainFrame);
> +	   ASSERT(view);

I suggest an empty line between the _layout call, and the declaration, but feel

free to ignore this, I just feel it feels better.

> +	   if (modifiersArray) {
> +	       int modifiersCount = JSValueToNumber(context,
JSObjectGetProperty(context, modifiersArray, lengthProperty, 0), 0);
> +	       for (int i = 0; i < modifiersCount; ++i) {

Since modifiersCount is not used anywhere else, I'd just add the
JSValueToNumber call inside the for, where the variable is now.

> +    JSStringRef character = JSValueToStringCopy(context, arguments[0],
exception);
> +    ASSERT(!*exception);

I don't think ASSERT is what we want, in DRT. If this fails, we want the test
to fail in release builds, right? So I suggest returning with undefined in
here, if the asserted condition is not met.

> +    else if (JSStringIsEqualToUTF8CString(character, "delete"))
> +	   gdkKeySym = GDK_BackSpace;

Interesting. I would expect delete here instead of backspace =). Sounds like
those compatibility options from gnome terminal would be required even for the
web, eh?

Thanks for working on this, r- for now because of the stuff mentioned above.


More information about the webkit-reviews mailing list