[Webkit-unassigned] [Bug 25990] [GTK] DumpRenderTree needs eventSender object and implementation

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


https://bugs.webkit.org/show_bug.cgi?id=25990


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #39297|review?                     |review-
               Flag|                            |




--- Comment #4 from Gustavo Noronha (kov) <gns at gnome.org>  2009-09-09 13:38:13 PDT ---
(From update of attachment 39297)
> +#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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list