Bug 15255

Summary: GTK+ WebKit should allow the application to execute Javascript on it.
Product: WebKit Reporter: Sean Egan <seanegan>
Component: WebKit APIAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: christian, jmalonzo, seanegan, zecke
Priority: P2 Keywords: Gtk
Version: 523.x (Safari 3)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
allow application to call javascript on a WebkitPage
none
Call Javascript from gtkwebkitpage
aroben: review-
Fixes style issues
mjs: review-
Moved functions from Page to Frame
aroben: review-
Adds String::fromUtf8 around the script parameter, per request alp: review+

Description Sean Egan 2007-09-21 21:29:27 PDT
I'm implementing AdiumX "chat styles" (http://adiumxtras.com) in Pidgin (http://pidgin.im). These chat styles require the application to call a javascript function defined by the style.

This patch allows the GTK+ bindings to call arbitrary javascript on the WebkitPage.

An alternative is to have webkit_gtk_page_open call the change_location method, which special cases javascript: uri's and use that method with javascript: uri's, but this seemed like the better solution to me.

Just to show off a bit, some screenshots of what this allows: http://pidgin.im/~seanegan/webkit3.png http://pidgin.im/~seanegan/webkit4.png http://pidgin.im/~seanegan/webkit5.png http://pidgin.im/~seanegan/webkit6.gif
Comment 1 Sean Egan 2007-09-21 21:30:24 PDT
Created attachment 16345 [details]
allow application to call javascript on a WebkitPage
Comment 2 Sean Egan 2007-09-22 10:02:49 PDT
Created attachment 16347 [details]
Call Javascript from gtkwebkitpage
Comment 3 Sean Egan 2007-09-22 10:03:28 PDT
Whoops! My first patch still had some test code in it. Removed.
Comment 4 Adam Roben (:aroben) 2007-09-22 13:21:20 PDT
Comment on attachment 16347 [details]
Call Javascript from gtkwebkitpage

As per <http://webkit.org/coding/coding-style.html>, please put asterisks next to the type name, not the variable name.

+    if (FrameLoader *loader = frameData->frame->loader())
+        loader->executeScript(script, true);
+	
+}

Looks like you've got an extra line of whitespace after the if statement.

I also think it would make more sense for this API to be on WebKitGtkFrame, rather than having it implicitly operate on the main frame.

r- for now so that the above changes can be made, but I definitely agree this is a useful API to have!
Comment 5 Sean Egan 2007-09-22 14:35:33 PDT
Created attachment 16350 [details]
Fixes style issues

I fixed the style issues, but kept the function in gtkwebkitpage for consistency. Right now, the frame just has some accessor functions, and all the functions that act on the frame live in the page. It seems weird to do this differently, but I can change it if you still like.
Comment 6 Adam Roben (:aroben) 2007-09-22 14:41:44 PDT
Comment on attachment 16350 [details]
Fixes style issues

I think ultimately we want all those functions that implicitly work on the main frame to move to WebKitGtkFrame so that callers can execute them on any frame. I think we can set a good example here by putting execute_script on WebKitGtkFrame. Otherwise everything looks good!
Comment 7 Sean Egan 2007-09-22 15:28:35 PDT
I'll go ahead and move everything into Frame, then. Should I keep convenience functions in Page that call the new functions on the main Frame, or just move it all outright?
Comment 8 Adam Roben (:aroben) 2007-09-22 15:33:09 PDT
(In reply to comment #7)
> I'll go ahead and move everything into Frame, then. Should I keep convenience
> functions in Page that call the new functions on the main Frame, or just move
> it all outright?

We have no such convenience methods on Mac/Windows, and I don't think they're all that necessary, given that

webkit_gtk_frame_execute_script(webkit_gtk_page_get_main_frame(page), script)

is pretty readable/simple. It also makes it clearer what's going on, especially since the DOM window object (which is the global object) is different for each frame, so the behavior of the script could be different depending on which frame it executes on.
Comment 9 Sean Egan 2007-09-22 17:30:07 PDT
Created attachment 16353 [details]
Moved functions from Page to Frame
Comment 10 Adam Roben (:aroben) 2007-09-22 17:36:01 PDT
Comment on attachment 16353 [details]
Moved functions from Page to Frame

Would you be willing to split the changes up into two patch (move the functions, then add the new one)? If not, I can r+ this one, but it would make for slightly cleaner history.
Comment 11 Adam Roben (:aroben) 2007-09-23 14:05:50 PDT
Comment on attachment 16353 [details]
Moved functions from Page to Frame

Holger has brought it to my attention that the Mac/Windows APIs do in fact put these methods on WebView (the equivalent of WebKitGtkPage), not on WebFrame (the equivalent of WebKitGtkFrame). Given this, I think it makes sense to stick with attachment 16350 [details]. Sorry for the trouble, and thanks for sticking with it!
Comment 12 Adam Roben (:aroben) 2007-09-23 14:06:22 PDT
Comment on attachment 16350 [details]
Fixes style issues

Putting this back up for review so that Holger can comment on it.
Comment 13 Holger Freyther 2007-09-23 14:16:06 PDT
(In reply to comment #5)
> Created an attachment (id=16350) [edit]

As Adam pointed out I have one question/concern/remark. executeScript(WebCore::String, bool) gets invoked with executeScript(const gchar*, bool), this means auto-boxing is going to take place.

Now WebCore::String (WebCore/platform/PlatformString.h) will not do any charset conversion. So only ascii/latin1 is a valid input. I think we should follow Gtk+ examples and make all strings utf-8. So I propose adding a String::fromUtf8 around the script parameter.
Comment 14 Maciej Stachowiak 2007-09-29 20:53:11 PDT
Comment on attachment 16350 [details]
Fixes style issues

r- for Holger's last comment - make sure to convert from utf8. Please resubmit when this is fixed.
Comment 15 Sean Egan 2007-10-01 19:46:46 PDT
Created attachment 16492 [details]
Adds String::fromUtf8 around the script parameter, per request

Sorry for the delay; I was out of town. Feel free to make such fixes yourselves in the future ;)
Comment 16 Holger Freyther 2007-10-02 15:02:57 PDT
(In reply to comment #15)
> Created an attachment (id=16492) [edit]
> Adds String::fromUtf8 around the script parameter, per request
> 
> Sorry for the delay; I was out of town. Feel free to make such fixes yourselves
> in the future ;)

Hehe, it could have been that you wanted to comment on the moving of the functions or that you wanted to have ascii/latin1 encoding for a certain reason. I think Adam is going to review this patch soon and I will land it then.

Comment 17 Alp Toker 2007-10-02 18:47:24 PDT
s/programatically/programmatically but otherwise looks good.
Comment 18 Alp Toker 2007-10-02 18:49:06 PDT
Comment on attachment 16492 [details]
Adds String::fromUtf8 around the script parameter, per request

s/programatically/programmatically  and maybe s/method/function
Comment 19 Holger Freyther 2007-10-03 09:40:42 PDT
Landed in r26019.