Bug 50615 – [Qt] Introduce API to ease binding objects to a QWebFrame

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Qt

[Holger Freyther]

Adding and re-adding a QObject to a QWebFrame is more difficult than it should be. Maybe an API to take over this can help...

[Holger Freyther]

Created attachment 75782 [details]
Propose the QWebObjectBinder API

This is a proposal for an API that might be useful for hybrid applications...

[WebKit Review Bot]

Attachment 75782 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75782 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75782 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75782 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75782 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Hmm, wouldn't it be simpler to add a boolean property to QWebFrame that adds support for this feature right in QWebFrame itself? :)

[Benjamin Poulain]

I like the idea, I think adding object in hybrid is too complicated at the moment.

I think the binding should depend on the URL. Pierre has a use case where objects are included only for some URL (scheme rekonq://), I do not think that is uncommon.

[Noam Rosenthal]

(In reply to comment #7)
> Hmm, wouldn't it be simpler to add a boolean property to QWebFrame that adds support for this feature right in QWebFrame itself? :)

Yes, that's probably where I'd do it, or add an optional flag argument to addToJavaScriptWindowObject.

[Noam Rosenthal]

(In reply to comment #8)
> I like the idea, I think adding object in hybrid is too complicated at the moment.
> 
> I think the binding should depend on the URL. Pierre has a use case where objects are included only for some URL (scheme rekonq://), I do not think that is uncommon.

That sounds good but very custom. Right now doing something custom like that is relatively easy in my opinion, while doing the mere default (having a persistent bound object) can be made hard. Enabling something like URL-based security can lead to a slippery slope of fine-grained security details we add to this hybrid mechanism.
We should keep it simple where the persistence is opt-in (like today) or opt-out (something like Simon's take on Holger's suggestion, with maybe an ability to remove the object).

[Benjamin Poulain]

(In reply to comment #10)
> That sounds good but very custom. Right now doing something custom like that is relatively easy in my opinion, while doing the mere default (having a persistent bound object) can be made hard. Enabling something like URL-based security can lead to a slippery slope of fine-grained security details we add to this hybrid mechanism.

My comment was in the context of Holger's patch were a new class is introduced. If this is done as convenience in QWebFrame, I agree thinks should be simpler.

[Holger Freyther]

The idea of the extra class (even when not implemented) was that on destruction the bound object might be removed. This way one can/could easily control the lifetime of such binding. The other reason was to avoid the monster uber-classes we have seen in khtml.

Anyway, binding and rebinding should be simpler.

[Simon Hausmann]

Comment on attachment 75782 [details]
Propose the QWebObjectBinder API

From the comments I believe everyone likes the idea of an overload instead of a separate class

[Holger Freyther]

Created attachment 91804 [details]
Patch

Add new function to QWebFrame

[Alexis Menard (darktears)]

Comment on attachment 91804 [details]
Patch

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

> Source/WebKit/qt/Api/qwebframe.cpp:596
> +    \since 4.8

Will not be part of 4.8 I believe. Because 2.2 will be in 4.8 and we already branched 2.2, so we don't put features inside. I add ademar in CC so you can "negociate". Though I don't know how you could flag it.

[Ademar Reis]

(In reply to comment #15)
> (From update of attachment 91804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91804&action=review
> 
> > Source/WebKit/qt/Api/qwebframe.cpp:596
> > +    \since 4.8
> 
> Will not be part of 4.8 I believe. Because 2.2 will be in 4.8 and we already branched 2.2, so we don't put features inside. I add ademar in CC so you can "negociate". Though I don't know how you could flag it.

Alexis is right: we should leave this for 2.3 (unless it's supper critical, my impression is that it's not).

On a side note, we have to rethink these \since macros, as they don't make sense anymore (I remember a discussion on the mailing list a while ago, but we didn't conclude anything).

[Robert Hogan]

Comment on attachment 91804 [details]
Patch

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

> Source/WebKit/qt/Api/qwebframe.cpp:595
> +    The \a object will never be explicitly deleted by QtWebKit.

Is that the case with javaScriptWindowObjectCleared() at the moment? I was under the impression that it was not.

> Source/WebKit/qt/Api/qwebframe.cpp:615
>      javaScriptWindowObjectCleared() signal.

Maybe this part of the documentation is redundant now - you should just use the persistent version of the API instead.

> Source/WebKit/qt/Api/qwebframe.h:133
> +    void addPersistentToJavaScriptWindowObject(const QString &name, QObject *);

I think addPersistentObjectToJavaScriptWindowObject() would be easier to understand at first glance. addPersistentToJavaScriptWindowObject() doesn't make a lot of sense first time round.

[Simon Hausmann]

Comment on attachment 91804 [details]
Patch

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

>> Source/WebKit/qt/Api/qwebframe.h:133
>> +    void addPersistentToJavaScriptWindowObject(const QString &name, QObject *);
> 
> I think addPersistentObjectToJavaScriptWindowObject() would be easier to understand at first glance. addPersistentToJavaScriptWindowObject() doesn't make a lot of sense first time round.

How about addPersistentlyToJavaScriptWindowObject() ?

Alternative is to overload with an enum: addToJavaScriptWindowObject(name, object, RetainBetweenPagesOrSomethingLikeThat)

[Eric Seidel (no email)]

9-year-old review request.  Qt folks?  Please r- or close if this is no longer needed.

[Noam Rosenthal]

Comment on attachment 91804 [details]
Patch

I think people are not convinced that this API is necessary. I think belongs in a higher-level library that uses hybrid, rather than keep a map of objects inside QWebFrame.
In addition, I think it's time we froze the WK1 bridge API, it's good at what it does, it's stable, and doesn't need additional improvements at the moment.
Holger, if you care deeply about this I'm willing to continue this discussion on IRC :)

[Eric Seidel (no email)]

Sounds like we should close this and re-open if Holger feels strongly.

[Holger Freyther]

WebKit1 API is the passed, it would have been nice to not require our API users to write this by hand all the time. :)