[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