Bug 35351 - [GTK] DRT implement execCommand()
Summary: [GTK] DRT implement execCommand()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 19456
Blocks:
  Show dependency tree / graph
 
Reported: 2010-02-24 11:24 PST by Philippe Normand
Modified: 2010-08-03 05:32 PDT (History)
5 users (show)

See Also:

Attachments
Patch for this issue (21.40 KB, patch)
2010-07-30 18:53 PDT, Martin Robinson 		xan.lopez: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-02-24 11:24:18 PST 
At least editing/execCommand/delete-image-in-anchor.html fails because of that.
Comment 1 Christian Dywan 2010-02-24 12:33:50 PST 
See bug 19456 implementing webkit_web_view_execute_command().
Comment 2 Martin Robinson 2010-07-08 13:32:47 PDT 
Committed r62827: <http://trac.webkit.org/changeset/62827>
Comment 3 Martin Robinson 2010-07-30 18:53:36 PDT 
Created attachment 63136 [details]
Patch for this issue
Comment 4 Martin Robinson 2010-07-30 18:54:21 PDT 
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.
Comment 5 Christian Dywan 2010-08-01 12:28:56 PDT 
(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.
Comment 6 Martin Robinson 2010-08-02 09:12:21 PDT 
It should be simple to expose this as a public API once we have it in and test it via the DRT then.
Comment 7 Xan Lopez 2010-08-02 14:12:55 PDT 
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.
Comment 8 Martin Robinson 2010-08-02 19:20:29 PDT 
Committed r64524: <http://trac.webkit.org/changeset/64524>
Comment 9 Martin Robinson 2010-08-02 19:37:28 PDT 
(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!