Bug 89356 – [Gtk] Implement dumpFrameScrollPosition in DumpRenderTree

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[Gtk] Implement dumpFrameScrollPosition in DumpRenderTree

[Zan Dobersek]

Created attachment 148115 [details]
Patch

[Martin Robinson]

Comment on attachment 148115 [details]
Patch

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

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:140
> +    if (abs(x) > 0 || abs(y) > 0) {
> +        if (webkit_web_frame_get_parent(frame))
> +            printf("frame '%s' ", webkit_web_frame_get_name(frame));
> +        printf("scrolled to %d,%d\n", x, y);
> +    }
> +
> +    if (gLayoutTestController->dumpChildFrameScrollPositions()) {
> +        GSList* children = DumpRenderTreeSupportGtk::getFrameChildren(frame);
> +        for (GSList* child = children; child; child = g_slist_next(child))
> +            dumpFrameScrollPosition(static_cast<WebKitWebFrame*>(child->data));
> +        g_slist_free(children);
> +    }

Hrm. Isn't it possible to get this with the DOM bindings instead of using DumpRenderTreeSupport?

[Zan Dobersek]

Created attachment 148273 [details]
Patch

Sorry for the new API! :>

[Martin Robinson]

Comment on attachment 148273 [details]
Patch

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

Looks good to me! We just need another GTK+ reviewer to approve the API.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:1188
> + * Returns: (transfer none): the #WebKitDOMDocument currently loaded in the @frame

Should say "or %NULL if no document is loaded."

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:136
> +    glong x, y;
> +    x = webkit_dom_dom_window_get_page_x_offset(domWindow);
> +    y = webkit_dom_dom_window_get_page_y_offset(domWindow);

Just collapse these into

glong x = webkit_dom_dom_window_get_page_x_offset(domWindow);
glong y = webkit_dom_dom_window_get_page_y_offset(domWindow);

[Zan Dobersek]

Created attachment 148294 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 148294 [details]
Patch

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

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:1191
> + * Since: 2.0

We are not sure yet, whether we will release 2.0 or 1.10, so I'd use 1.10 for now just in case.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:1194
> +webkit_web_frame_get_dom_document(WebKitWebFrame* frame)

Maybe we could also update webkit_web_view_get_dom_document) to use this method now with priv->mainFrame.

[Gustavo Noronha (kov)]

Comment on attachment 148294 [details]
Patch

New API LGTM for the record

[Zan Dobersek]

Created attachment 151292 [details]
Patch

[Martin Robinson]

Comment on attachment 151292 [details]
Patch

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

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:1194
> +WebKitDOMDocument*
> +webkit_web_frame_get_dom_document(WebKitWebFrame* frame)

One nit here is that breaking this line doesn't follow WebKit style.

[Zan Dobersek]

(In reply to comment #9)
> (From update of attachment 151292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151292&action=review
> 
> > Source/WebKit/gtk/webkit/webkitwebframe.cpp:1194
> > +WebKitDOMDocument*
> > +webkit_web_frame_get_dom_document(WebKitWebFrame* frame)
> 
> One nit here is that breaking this line doesn't follow WebKit style.

Fixed and committed in r122147
http://trac.webkit.org/changeset/122147