30773 2009-10-26 06:22:06 -0700 [Qt] QWebInspector: improve QWebPage::webInspectorTriggered(QWebElement&) -> QWebInspector::show() connection 2009-11-09 07:46:38 -0800 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) PC Linux RESOLVED FIXED Qt P2 Enhancement --- 29843 1 jturcotte webkit-unassigned benjamin hausmann kenneth laszlo.gombos vestbo oldest_to_newest 157748 0 jturcotte 2009-10-26 06:22:06 -0700 == Currently the way to connect an instantiated QWebInspector to a QWebPage context menu event is: // ... QWebPage *page = new QWebPage; // ... QWebInspector *inspector = new QWebInspector; inspector->setPage(page); connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(show())); == A way to lazily create the inspector on the first context menu event: void UserClass::initialize() { // ... m_page = new QWebPage; // ... connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), userObject, SLOT(createAndShowInspector())); } void UserClass::createAndShowInspector() { if (!m_inspector) { m_inspector = new QWebInspector; m_inspector->setPage(m_page); } m_inspector->show(); } == The problem is that the actual "set inspected element" is happening magically after the webInspectorTriggered signal has been emitted. == So it might be cool to expose this behavior in the api. == Two possible ways: = 1. Add a setInspectedElement property setter = Maybe we will add the getter in later versions, but this would require additional API in WebKit's web inspector. = Also, what is actually the current inspectedElement is not totally obvious neither. // ... QWebPage *page = new QWebPage; // ... QWebInspector *inspector = new QWebInspector; inspector->setPage(page); connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(show())); connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(setInspectedElement(const QWebElement&))); = 2. Add a showElement(QWebElement&) slot = Good thing: simple = Bad thing: The showElement slot however has two responsibilities = This might cause problem for a user that might call this method just to set the inspected element since show() will be called each time. // ... QWebPage *page = new QWebPage; // ... QWebInspector *inspector = new QWebInspector; inspector->setPage(page); connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(showElement(const QWebElement&))); 158170 1 vestbo 2009-10-27 06:32:19 -0700 After discussion, we're still not sure if this is the right API. Areas of concern: - Do we really need a way to provide lazy initialization? If not we don't need the webInspectorTriggered() signal. Also, without the signal you could always override the triggerAction - We want to keep the "magic"/default/99% case behavior of selecting the element you clicked on. The question is how to do it. Currently it's hard-coded, but if we add setInspectedElement() in the future as a slot we could add the webInspectorTriggered() signal and automatically connect it to setInspectedElement in setPage() 158641 2 kenneth 2009-10-28 11:07:35 -0700 webInspectorTriggered? wouldn't inspectorRequested be a better name? No need for web either... we have methods like history() returning QWebHistory. page() returning QWebPage etc. I also like requested more than triggered. For setInspectedElement(), I would just go for inspectElement(). 158646 3 vestbo 2009-10-28 11:20:35 -0700 (In reply to comment #2) > No need for web either... we have methods like history() returning QWebHistory. > page() returning QWebPage etc. I agree with you in general, but in this specific case the "Web Inspector" is a name on its own: http://webkit.org/blog/41/introducing-the-web-inspector/ > I also like requested more than triggered. It's actually triggered, ie it's not a request you can ignore and then nothing will happen. I think that's why we went for triggered. > For setInspectedElement(), I would just go for inspectElement(). That changes the semantics of the function. It's a property setter, not a function that does something (shows the view, highlights, etc). It would not match the getter inspectedElement() either. 158978 4 jturcotte 2009-10-29 11:22:19 -0700 (In reply to comment #3) > (In reply to comment #2) > > I also like requested more than triggered. > > It's actually triggered, ie it's not a request you can ignore and then nothing > will happen. I think that's why we went for triggered. > I think that what makes us feel that its triggered is the inspection that happen "magically". If QWebPage have no direct effect on the inspector beside through this signal's emission, I agree that it feels more like a request than a trigger. Take for example if we have two signals/connections instead of one which connects to the QWebInspector: QWPage::webInspectorShowRequested() ==> QWInspector::show() QWPage::inspectElementRequested(QWE&) ==> QWInspector::setInspectedElement(QWE&) Having one signal connected to both slots might also make sense, still no magic: QWebPage::webInspectorRequested(QWE&) ==> QWebInspector::show() QWebPage::webInspectorRequested(QWE&) ==> QWebInspector::setInspectedElement(QWE&) 158990 5 42119 jturcotte 2009-10-29 11:44:34 -0700 Created attachment 42119 Patch: Remove the signal This patch remove the signal for this release since it is not perfect yet and we think that it's use is not mandatory for now. The only scenario I could think of is if somebody wants to lazily create the QWebInspector object until the first trigger through the context menu. This scenario can still be achieved in the 4.6 release by subclassing QWebPage and override ::triggerAction for InspectElement actions. IMPORTANT: ===> We can remove the signal from 4.6 only if we all agree that in the future the signal connections between QWebPage and QWebInspector will be done automatically in QWebInspector::setPage(QWPage*). This would allow us to change the "magic" behavior of 4.6 by an "automatically connected signals" behavior in 4.7. Right now in trunk, this connection has to be made by the developer, which is not convenient since being able to change it is useless power in 95% of the time. == Explanation: // In 4.6 QWebInspector *inspector = new QWebInspector; inspector->setPage(page) // <-- In there, QWebPage keeps a pointer to the // inspector and in trigger action, calls // insp->show() and sets its element // In 4.7 QWebInspector *inspector = new QWebInspector; inspector->setPage(page) // <-- In there, signals from QWebPage are // connected to the show() and // setInspectedElement(QWE&) slots 159297 6 kenneth 2009-10-30 05:40:44 -0700 > == Explanation: > // In 4.6 > QWebInspector *inspector = new QWebInspector; > inspector->setPage(page) // <-- In there, QWebPage keeps a pointer to the > // inspector and in trigger action, calls > // insp->show() and sets its element > > // In 4.7 > QWebInspector *inspector = new QWebInspector; > inspector->setPage(page) // <-- In there, signals from QWebPage are > // connected to the show() and > // setInspectedElement(QWE&) slots It is possible to use the same web inspector for more pages? I don't like the set page so much, as it is more a "watch page" or so. Would it be possible to do it the other way around? QWebPage::setInspectable(new QWebInspector) ? 159769 7 jturcotte 2009-11-02 03:14:22 -0800 (In reply to comment #6) > It is possible to use the same web inspector for more pages? > > I don't like the set page so much, as it is more a "watch page" or so. Would it > be possible to do it the other way around? > > QWebPage::setInspectable(new QWebInspector) ? The relation is 1<->1, so if you assign the same inspector to a different page, the previous page is unassociated (explained to the user in the documentation). A reason I like the "set" is because it suggests that the objects are bound together. We first called it setInspectedPage, but since the web inspector implicitly inspects pages, so we omitted the "inspected". Maybe we should re-add inspected or watched to the method just in case we want the QWebInspector to have different interractions with a QWebPage? (what would that be?) The method is in the inspector class to make it feel similar to QWebView::setPage, they are both UI layer objects that interact with the web page which feels more like a "content" layer object. 159943 8 benjamin 2009-11-02 16:35:16 -0800 (In reply to comment #6) > It is possible to use the same web inspector for more pages? That's a good point, but I think we can handle that later without braking the current behavior. Later, we can add the signal webInspectorTriggered() and have setInspectedElement() to change the element. In that case, you will be able to use the same inspector for different pages. We can also add webInspectorTriggered() without the argument in the future. 160010 9 jturcotte 2009-11-03 01:12:44 -0800 (In reply to comment #8) > (In reply to comment #6) > > It is possible to use the same web inspector for more pages? > > That's a good point, but I think we can handle that later without braking the > current behavior. > > Later, we can add the signal webInspectorTriggered() and have > setInspectedElement() to change the element. In that case, you will be able to > use the same inspector for different pages. > > We can also add webInspectorTriggered() without the argument in the future. The backend don't support this scenario. Moreover, the UI is not designed to display information from more than one page at the same time. The javascript console would also cause problems. Behind the scene, the web inspector is tightly bound to WebCore's page and is not an independent API. What we could do is to keep states like the current displayed tab within the QWebInspector and re-set this value when we do a setPage on a different QWebPage. 161563 10 hausmann 2009-11-09 06:59:29 -0800 Committed r50656: <http://trac.webkit.org/changeset/50656> 161569 11 kenneth 2009-11-09 07:46:38 -0800 Still shouldn't it be setWebInspector then and not setInspectable? > > > > QWebPage::setInspectable(new QWebInspector) ? 42119 2009-10-29 11:44:34 -0700 2009-11-09 06:57:58 -0800 Patch: Remove the signal qwebinspector_removeQwebpageSignal.patch text/plain 5020 jturcotte ZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcXdlYmluc3BlY3Rvci5jcHAgYi9XZWJLaXQvcXQv QXBpL3F3ZWJpbnNwZWN0b3IuY3BwCmluZGV4IDQ1NzhkYzkuLjdmYWU2ZjggMTAwNjQ0Ci0tLSBh L1dlYktpdC9xdC9BcGkvcXdlYmluc3BlY3Rvci5jcHAKKysrIGIvV2ViS2l0L3F0L0FwaS9xd2Vi aW5zcGVjdG9yLmNwcApAQCAtNTAsNiArNTAsNyBAQAogCiAgICAgXHNlY3Rpb24xIFJlc291cmNl cwogCisgICAgVGhpcyBjbGFzcyBhY3RzIG1vc3RseSBhcyBhIGNvbnRhaW5lciBhbmQgYSBjb250 cm9sbGVyIGZvciB0aGUgaW5zcGVjdG9yLgogICAgIE1vc3Qgb2YgdGhlIHJlc291cmNlcyBuZWVk ZWQgYnkgdGhlIGluc3BlY3RvciBhcmUgb3duZWQgYnkgdGhlIGFzc29jaWF0ZWQKICAgICBRV2Vi UGFnZSBhbmQgYXJlIGFsbG9jYXRlZCB0aGUgZmlyc3QgdGltZSB0aGF0OgogICAgIFxsaXN0CkBA IC01NywxMSArNTgsNiBAQAogICAgICAgICBcbyB0aGUgUVdlYkluc3BlY3RvciBpcyBzaG93bi4K ICAgICBcZW5kbGlzdAogCi0gICAgVGhpcyBjbGFzcyBhY3RzIG1vc3RseSBhcyBhIGNvbnRhaW5l ciBhbmQgYSBjb250cm9sbGVyIGZvciB0aGUgaW5zcGVjdG9yLgotICAgIFlvdSBjYW4gZGVmZXIg dGhlIGNyZWF0aW9uIGFuZCBhc3NvY2lhdGlvbiBvZiB0aGUgUVdlYkluc3BlY3RvciB1bnRpbAot ICAgIHRoZSBmaXJzdCBlbWlzc2lvbiBvZiBRV2ViUGFnZTo6d2ViSW5zcGVjdG9yVHJpZ2dlcmVk KCkgdG8gc2F2ZSBhZGRpdGlvbmFsCi0gICAgcmVzb3VyY2VzLgotCiAgICAgXHNlY3Rpb24xIElu c3BlY3RvciBjb25maWd1cmF0aW9uIHBlcnNpc3RlbmNlCiAKICAgICBUaGUgaW5zcGVjdG9yIGFs bG93cyB0aGUgdXNlciB0byBjb25maWd1cmUgc29tZSBvcHRpb25zIHRocm91Z2ggaXRzCmRpZmYg LS1naXQgYS9XZWJLaXQvcXQvQXBpL3F3ZWJwYWdlLmNwcCBiL1dlYktpdC9xdC9BcGkvcXdlYnBh Z2UuY3BwCmluZGV4IDBiMTIyYjQuLjYzY2U0ZWIgMTAwNjQ0Ci0tLSBhL1dlYktpdC9xdC9BcGkv cXdlYnBhZ2UuY3BwCisrKyBiL1dlYktpdC9xdC9BcGkvcXdlYnBhZ2UuY3BwCkBAIC0xMzc4LDgg KzEzNzgsNiBAQCBRV2ViSW5zcGVjdG9yKiBRV2ViUGFnZVByaXZhdGU6OmdldE9yQ3JlYXRlSW5z cGVjdG9yKCkKICAgICBpZiAoIWluc3BlY3RvcikgewogICAgICAgICBRV2ViSW5zcGVjdG9yKiBp bnNwID0gbmV3IFFXZWJJbnNwZWN0b3I7CiAgICAgICAgIGluc3AtPnNldFBhZ2UocSk7Ci0gICAg ICAgIGluc3AtPmNvbm5lY3QocSwgU0lHTkFMKHdlYkluc3BlY3RvclRyaWdnZXJlZChjb25zdCBR V2ViRWxlbWVudCYpKSwgU0xPVChzaG93KCkpKTsKLSAgICAgICAgaW5zcC0+c2hvdygpOyAvLyBU aGUgaW5zcGVjdG9yIGlzIGV4cGVjdGVkIHRvIGJlIHNob3duIG9uIGluc3BlY3Rpb24KICAgICAg ICAgaW5zcGVjdG9ySXNJbnRlcm5hbE9ubHkgPSB0cnVlOwogCiAgICAgICAgIFFfQVNTRVJUKGlu c3BlY3Rvcik7IC8vIEFzc29jaWF0ZWQgdGhyb3VnaCBRV2ViSW5zcGVjdG9yOjpzZXRQYWdlKHEp CkBAIC0xOTE2LDExICsxOTE0LDkgQEAgdm9pZCBRV2ViUGFnZTo6dHJpZ2dlckFjdGlvbihXZWJB Y3Rpb24gYWN0aW9uLCBib29sKQogICAgICAgICAgICAgZWRpdG9yLT5zZXRCYXNlV3JpdGluZ0Rp cmVjdGlvbihSaWdodFRvTGVmdFdyaXRpbmdEaXJlY3Rpb24pOwogICAgICAgICAgICAgYnJlYWs7 CiAgICAgICAgIGNhc2UgSW5zcGVjdEVsZW1lbnQ6IHsKLSAgICAgICAgICAgIFFXZWJFbGVtZW50 IGluc3BlY3RlZEVsZW1lbnQoUVdlYkVsZW1lbnQ6OmVuY2xvc2luZ0VsZW1lbnQoZC0+aGl0VGVz dFJlc3VsdC5kLT5pbm5lck5vblNoYXJlZE5vZGUuZ2V0KCkpKTsKLSAgICAgICAgICAgIGVtaXQg d2ViSW5zcGVjdG9yVHJpZ2dlcmVkKGluc3BlY3RlZEVsZW1lbnQpOwotCiAgICAgICAgICAgICBp ZiAoIWQtPmhpdFRlc3RSZXN1bHQuaXNOdWxsKCkpIHsKICAgICAgICAgICAgICAgICBkLT5nZXRP ckNyZWF0ZUluc3BlY3RvcigpOyAvLyBNYWtlIHN1cmUgdGhlIGluc3BlY3RvciBpcyBjcmVhdGVk CisgICAgICAgICAgICAgICAgZC0+aW5zcGVjdG9yLT5zaG93KCk7IC8vIFRoZSBpbnNwZWN0b3Ig aXMgZXhwZWN0ZWQgdG8gYmUgc2hvd24gb24gaW5zcGVjdGlvbgogICAgICAgICAgICAgICAgIGQt PnBhZ2UtPmluc3BlY3RvckNvbnRyb2xsZXIoKS0+aW5zcGVjdChkLT5oaXRUZXN0UmVzdWx0LmQt PmlubmVyTm9uU2hhcmVkTm9kZS5nZXQoKSk7CiAgICAgICAgICAgICB9CiAgICAgICAgICAgICBi cmVhazsKQEAgLTMzMjEsMjQgKzMzMTcsNiBAQCBxdWludDY0IFFXZWJQYWdlOjpieXRlc1JlY2Vp dmVkKCkgY29uc3QKICovCiAKIC8qIQotICAgIFxmbiB2b2lkIFFXZWJQYWdlOjp3ZWJJbnNwZWN0 b3JUcmlnZ2VyZWQoY29uc3QgUVdlYkVsZW1lbnQmIGluc3BlY3RlZEVsZW1lbnQpOwotICAgIFxz aW5jZSA0LjYKLQotICAgIFRoaXMgc2lnbmFsIGlzIGVtaXR0ZWQgd2hlbiB0aGUgdXNlciB0cmln Z2VyZWQgYW4gaW5zcGVjdGlvbiB0aHJvdWdoIHRoZQotICAgIGNvbnRleHQgbWVudS4gSWYgYSBR V2ViSW5zcGVjdG9yIGlzIGFzc29jaWF0ZWQgdG8gdGhpcyBwYWdlLCBpdCBzaG91bGQgYmUKLSAg ICB2aXNpYmxlIHRvIHRoZSB1c2VyIGFmdGVyIHRoaXMgc2lnbmFsIGhhcyBiZWVuIGVtaXR0ZWQu Ci0KLSAgICBJZiBzdGlsbCBubyBRV2ViSW5zcGVjdG9yIGlzIGFzc29jaWF0ZWQgdG8gdGhpcyBR V2ViUGFnZSBhZnRlciB0aGUgZW1pc3Npb24KLSAgICBvZiB0aGlzIHNpZ25hbCwgYSBwcml2YXRl bHkgb3duZWQgaW5zcGVjdG9yIHdpbGwgYmUgc2hvd24gdG8gdGhlIHVzZXIuCi0KLSAgICBcbm90 ZSBcYSBpbnNwZWN0ZWRFbGVtZW50IGNvbnRhaW5zIHRoZSBRV2ViRWxlbWVudCB1bmRlciB0aGUg Y29udGV4dCBtZW51LgotICAgIEl0IGlzIG5vdCBnYXJhbnRlZWQgdG8gYmUgdGhlIHNhbWUgYXMg dGhlIGZvY3VzZWQgZWxlbWVudCBpbiB0aGUgd2ViCi0gICAgaW5zcGVjdG9yLgotCi0gICAgXHNh IFFXZWJJbnNwZWN0b3IKLSovCi0KLS8qIQogICAgIFxmbiB2b2lkIFFXZWJQYWdlOjp0b29sQmFy VmlzaWJpbGl0eUNoYW5nZVJlcXVlc3RlZChib29sIHZpc2libGUpCiAKICAgICBUaGlzIHNpZ25h bCBpcyBlbWl0dGVkIHdoZW5ldmVyIHRoZSB2aXNpYmlsaXR5IG9mIHRoZSB0b29sYmFyIGluIGEg d2ViIGJyb3dzZXIKZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcXdlYnBhZ2UuaCBiL1dlYktp dC9xdC9BcGkvcXdlYnBhZ2UuaAppbmRleCBmMmJiZGUwLi5jYTczZGQ4IDEwMDY0NAotLS0gYS9X ZWJLaXQvcXQvQXBpL3F3ZWJwYWdlLmgKKysrIGIvV2ViS2l0L3F0L0FwaS9xd2VicGFnZS5oCkBA IC0zMjksNyArMzI5LDYgQEAgUV9TSUdOQUxTOgogICAgIHZvaWQgd2luZG93Q2xvc2VSZXF1ZXN0 ZWQoKTsKICAgICB2b2lkIHByaW50UmVxdWVzdGVkKFFXZWJGcmFtZSAqZnJhbWUpOwogICAgIHZv aWQgbGlua0NsaWNrZWQoY29uc3QgUVVybCAmdXJsKTsKLSAgICB2b2lkIHdlYkluc3BlY3RvclRy aWdnZXJlZChjb25zdCBRV2ViRWxlbWVudCYgaW5zcGVjdGVkRWxlbWVudCk7CiAKICAgICB2b2lk IHRvb2xCYXJWaXNpYmlsaXR5Q2hhbmdlUmVxdWVzdGVkKGJvb2wgdmlzaWJsZSk7CiAgICAgdm9p ZCBzdGF0dXNCYXJWaXNpYmlsaXR5Q2hhbmdlUmVxdWVzdGVkKGJvb2wgdmlzaWJsZSk7CmRpZmYg LS1naXQgYS9XZWJLaXQvcXQvQ2hhbmdlTG9nIGIvV2ViS2l0L3F0L0NoYW5nZUxvZwppbmRleCAz ZjkzZTZiLi5jNDAzYTAwIDEwMDY0NAotLS0gYS9XZWJLaXQvcXQvQ2hhbmdlTG9nCisrKyBiL1dl YktpdC9xdC9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAwOS0xMC0yOSAgSm9jZWx5biBU dXJjb3R0ZSAgPGpvY2VseW4udHVyY290dGVAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtRdF0gUmVtb3ZlIHRoZSBRV2ViUGFnZTp3 ZWJJbnNwZWN0b3JUcmlnZ2VyZWQgc2lnbmFsLgorICAgICAgICBVc2VyIGN1c3RvbWl6YXRpb24g b2YgdGhlIGNvbW11bmljYXRpb24gYmV0d2VlbiBRV2ViUGFnZQorICAgICAgICBhbmQgUVdlYklu c3BlY3RvciB3aWxsIGJlIHByb3Blcmx5IGRlc2lnbmVkIGluIHRoZSBuZXh0IHZlcnNpb24uCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMDc3MworCisg ICAgICAgICogQXBpL3F3ZWJpbnNwZWN0b3IuY3BwOgorICAgICAgICAqIEFwaS9xd2VicGFnZS5j cHA6CisgICAgICAgIChRV2ViUGFnZVByaXZhdGU6OmdldE9yQ3JlYXRlSW5zcGVjdG9yKToKKyAg ICAgICAgKFFXZWJQYWdlOjp0cmlnZ2VyQWN0aW9uKToKKyAgICAgICAgKiBBcGkvcXdlYnBhZ2Uu aDoKKyAgICAgICAgKiBRdExhdW5jaGVyL21haW4uY3BwOgorICAgICAgICAoTWFpbldpbmRvdzo6 TWFpbldpbmRvdyk6CisKIDIwMDktMTAtMjcgIFNoaW5pY2hpcm8gSGFtYWppICA8aGFtYWppQGNo cm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KZGlmZiAtLWdp dCBhL1dlYktpdC9xdC9RdExhdW5jaGVyL21haW4uY3BwIGIvV2ViS2l0L3F0L1F0TGF1bmNoZXIv bWFpbi5jcHAKaW5kZXggZTNjNjExNi4uYTM0ZjUyMiAxMDA2NDQKLS0tIGEvV2ViS2l0L3F0L1F0 TGF1bmNoZXIvbWFpbi5jcHAKKysrIGIvV2ViS2l0L3F0L1F0TGF1bmNoZXIvbWFpbi5jcHAKQEAg LTEwMSw3ICsxMDEsNiBAQCBwdWJsaWM6CiAgICAgICAgIGluc3BlY3Rvci0+c2V0UGFnZShwYWdl KTsKICAgICAgICAgaW5zcGVjdG9yLT5oaWRlKCk7CiAgICAgICAgIGNvbm5lY3QodGhpcywgU0lH TkFMKGRlc3Ryb3llZCgpKSwgaW5zcGVjdG9yLCBTTE9UKGRlbGV0ZUxhdGVyKCkpKTsKLSAgICAg ICAgY29ubmVjdChwYWdlLCBTSUdOQUwod2ViSW5zcGVjdG9yVHJpZ2dlcmVkKGNvbnN0IFFXZWJF bGVtZW50JikpLCBpbnNwZWN0b3IsIFNMT1Qoc2hvdygpKSk7CiAKICAgICAgICAgc2V0dXBVSSgp OwogCg==