Bug 35351 – [GTK] DRT implement execCommand()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

At least editing/execCommand/delete-image-in-anchor.html fails because of that.

[Christian Dywan]

See bug 19456 implementing webkit_web_view_execute_command().

[Martin Robinson]

Committed r62827: <http://trac.webkit.org/changeset/62827>

[Martin Robinson]

Created attachment 63136 [details]
Patch for this issue

[Martin Robinson]

It seems that the needs of LayoutTestController.execCommand may be slightly different than the the exec_command we want to expose in the public API. I've attached a patch which adds private methods we need for unimplemented DRT functionality. This will at least allow us to pass more tests. Eventually this may be made modified and made public for the editing API.

[Christian Dywan]

(In reply to comment #4)
> It seems that the needs of LayoutTestController.execCommand may be slightly different than the the exec_command we want to expose in the public API. I've attached a patch which adds private methods we need for unimplemented DRT functionality. This will at least allow us to pass more tests. Eventually this may be made modified and made public for the editing API.

The difference seems to be an additional string value argument. And in hindsight I actually agree this is useful, for instance for insertHTML. I had not considered that when I wrote the existing patch.

[Martin Robinson]

It should be simple to expose this as a public API once we have it in and test it via the DRT then.

[Xan Lopez]

Comment on attachment 63136 [details]
Patch for this issue

>-bool LayoutTestController::isCommandEnabled(JSStringRef /*name*/)
>+void LayoutTestController::setCacheModel(int)
> {
>     // FIXME: implement
>-    return false;
> }

So in the ChangeLog you say you are only moving this, but you are also adding a return false. Is it needed or just added by mistake?

> 
>+

Not needed?

> void LayoutTestController::setPersistentUserStyleSheetLocation(JSStringRef jsURL)
> {
>     // FIXME: implement

Looks good other than that.

[Martin Robinson]

Committed r64524: <http://trac.webkit.org/changeset/64524>

[Martin Robinson]

(In reply to comment #7)
> (From update of attachment 63136 [details])
> >-bool LayoutTestController::isCommandEnabled(JSStringRef /*name*/)
> >+void LayoutTestController::setCacheModel(int)
> > {
> >     // FIXME: implement
> >-    return false;
> > }
> So in the ChangeLog you say you are only moving this, but you are also adding a return false. Is it needed or just added by mistake?

I think that's just a case of a slightly confusing diff. In this case, I removed the "return false" in this location and replaced it with the result of webkit_web_view_is_command_enabled above.
 
> >+
> Not needed?

Fixed!

[WebKit Review Bot]

http://trac.webkit.org/changeset/64524 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/64517
http://trac.webkit.org/changeset/64518
http://trac.webkit.org/changeset/64519
http://trac.webkit.org/changeset/64520
http://trac.webkit.org/changeset/64521
http://trac.webkit.org/changeset/64522
http://trac.webkit.org/changeset/64523
http://trac.webkit.org/changeset/64524
http://trac.webkit.org/changeset/64525
http://trac.webkit.org/changeset/64526
http://trac.webkit.org/changeset/64527