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...
Created attachment 75782 [details] Propose the QWebObjectBinder API This is a proposal for an API that might be useful for hybrid applications...
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.
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.
Hmm, wouldn't it be simpler to add a boolean property to QWebFrame that adds support for this feature right in QWebFrame itself? :)
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.
(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.
(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).
(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.
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.
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
Created attachment 91804 [details] Patch Add new function to QWebFrame
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.
(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).
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.
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)
9-year-old review request. Qt folks? Please r- or close if this is no longer needed.
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 :)
Sounds like we should close this and re-open if Holger feels strongly.
WebKit1 API is the passed, it would have been nice to not require our API users to write this by hand all the time. :)