[Webkit-unassigned] [Bug 133730] [GTK] Introduce a new WebKitUserContent API

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Adrian Perez <aperez at igalia.com>  2014-06-23 16:31:24 PST ---
(In reply to comment #5)
> (From update of attachment 232952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232952&action=review
> 
> Looks good so far. Removing the review flags since this is a wip patch.

\o/

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:83
> > +    CString source;
> > +    Vector<String> whitelist;
> > +    Vector<String> blacklist;
> > +    WebKitUserContentInjectedFrames injectedFrames;
> 
> hmm, the problem of having this common class is that we can't wrap the WebCore classes directly, or maybe we could use an union or something like that.

I have changed this to use an union in _WebKitUserContentPrivate, and
then wrapping the WebCore classes directly.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:97
> > +void webkit_user_content_set_source(WebKitUserContent* user_content, const gchar* source)
> 
> I'm not sure about having set/get for all members, some of them (if not all) should probably be construct only, passed to the new method and then readonly. You should not be able to modify them once the object is created, I think.

You are right, it seems sensible that WebKitUser{Script,StyleSheet} are
are created and then they can be inspected (e.g. they have getters) but
not modified (e.g. they do not have setters). The WebCore classes are done
that way and I have modified our public API to have only getters as well.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:202
> > + * @source: (transfer none): Source code of the user script.
> 
> I think you don't need to use the transfer none annotation for parameters that are const

True, you are right.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:231
> > +UserScript webkitWebKitUserScriptToUserScript(WebKitUserScript* script, guint64 scriptId)
> 
> Why do we need a script id?

The script identifier is used internally in WebCore to build a fake URI
that can then be used to reference the user script. In practice that is
used only to remove an specific script (or style sheet), but there are
no IPC messages for that operation, so there is not much point in trying
to provide different URIs for each script — if I am understanding the
WebCore code correctly.

As it seems we will not be needing the URIs, I have gotten rid of the
script identifiers (same for style sheets) and all the URIs are set to
empty values.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUserContent.cpp:237
> > +    StringBuilder urlStringBuilder;
> > +    urlStringBuilder.append("user-script:");
> > +    urlStringBuilder.appendNumber(scriptId);
> 
> Why? Shouldn't the baseURL be provided by the user as well? The first append should be appendLiteral instead

No, it is used internally. Read the comment above.

> ScriptMessageClientGtk(const GRefPtr<WebKitUserContentManager>& manager, const String& handlerName)
>     : m_handlerName(handlerName)
>     , m_manager(manager)
> {
> }

The new version of the patch has this now implemented, but I am not very
happy of using webkit_web_context_get_default() in the code... Probably
I will try to come up with something that won't choke if we ever have
support for multiple contexts.

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