Bug 40134 - Web Inspector: sendMessageToFrontend implementation required.
: Web Inspector: sendMessageToFrontend implementation required.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on: 40365
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-03 11:39 PDT by Ilya Tikhonovsky
Modified: 2010-06-14 09:04 PDT (History)
15 users (show)

See Also:


Attachments
[patch] initial version. (29.48 KB, patch)
2010-06-03 12:07 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] second iteration. With fixed gtk build and other minor changes. (30.10 KB, patch)
2010-06-03 12:51 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] third iteration (41.18 KB, patch)
2010-06-04 14:30 PDT, Ilya Tikhonovsky
yurys: review-
Details | Formatted Diff | Diff
[patch] fourth iteration. (38.38 KB, patch)
2010-06-07 09:40 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] Fifth version. Just try to get qt build happy. (38.50 KB, patch)
2010-06-08 00:00 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] sixth iteration. (43.12 KB, patch)
2010-06-08 08:06 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] seventh iteration. (39.64 KB, patch)
2010-06-09 06:00 PDT, Ilya Tikhonovsky
pfeldman: review+
Details | Formatted Diff | Diff
[patch] next iteration with fixes for gtk (41.42 KB, patch)
2010-06-10 10:56 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] next iteration. (41.36 KB, patch)
2010-06-11 01:35 PDT, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2010-06-03 11:39:17 PDT
It should be possible to transfer timeline/dom/etc data from inspected page side to the front-end side in serialized format.

Right now when we want to show something at front-end side we are using ScriptFunctionCall in front-end context and ScriptObject for passing the data.
That is not suitable for remote debugging use case.
Comment 1 Ilya Tikhonovsky 2010-06-03 12:07:21 PDT
Created attachment 57799 [details]
[patch] initial version.

This is the second step on the way to the remote debugging.
The first one was InspectorValues classes for native serialization to JSON.

This step is a transport for WebInspector data in JSON format between inspected page and a front-end.
Comment 2 WebKit Review Bot 2010-06-03 12:21:44 PDT
Attachment 57799 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2995042
Comment 3 Ilya Tikhonovsky 2010-06-03 12:51:47 PDT
Created attachment 57805 [details]
[patch] second iteration. With fixed gtk build and other minor changes.
Comment 4 Joseph Pecoraro 2010-06-03 13:35:04 PDT
Comment on attachment 57799 [details]
[patch] initial version.

> +++ b/WebCore/inspector/InspectorFrontend.cpp
> +InspectorFrontend::InspectorFrontend(ScriptObject webInspector, InspectorClient* inspectorClient)
> +    : m_webInspector(webInspector), m_inspectorClient(inspectorClient)

I think the usual style for this is:

    : m_a(a)
    , m_b(b)


> -  ASSERT(frontend);

Should we change this to ASSERT(webInspector)? Or will this be
expected to handle a case of a null ScriptCobject.


> +++ b/WebCore/inspector/InspectorFrontend.h
> +#include <wtf/RefPtr.h>
> ...
> +        void addRecordToTimeline(const RefPtr<InspectorObject>&);

Is this used in this patch? If not, I would say leave this out
for the next patch.


> +        InspectorClient* m_inspectorClient;

Likewise this is added and included in the constructor above
but it isn't used yet. I have an idea how it will be used, but
I think before this patch lands there should be another patch
showing an example usage, even if its trivial.


> +++ b/WebCore/inspector/InspectorClient.h
> + virtual bool sendMessageToFrontend(const String& message) = 0;

It might be worth mentioning what the bool return value here
means. If not a comment then certainly something in the ChangeLog.

Great work on this so far!
Comment 5 Ilya Tikhonovsky 2010-06-03 14:09:48 PDT
(In reply to comment #4)
> (From update of attachment 57799 [details])
> > +++ b/WebCore/inspector/InspectorFrontend.cpp
> > +InspectorFrontend::InspectorFrontend(ScriptObject webInspector, InspectorClient* inspectorClient)
> > +    : m_webInspector(webInspector), m_inspectorClient(inspectorClient)
> 
> I think the usual style for this is:
> 
>     : m_a(a)
>     , m_b(b)

ok

> > -  ASSERT(frontend);
> 
> Should we change this to ASSERT(webInspector)? Or will this be
> expected to handle a case of a null ScriptCobject.

as far as it is transfered by ref we don't need to check that.


> > +++ b/WebCore/inspector/InspectorFrontend.h
> > +#include <wtf/RefPtr.h>
> > ...
> > +        void addRecordToTimeline(const RefPtr<InspectorObject>&);
> 
> Is this used in this patch? If not, I would say leave this out
> for the next patch.

Already fixed this in the second iteration.


> > +        InspectorClient* m_inspectorClient;
> 
> Likewise this is added and included in the constructor above
> but it isn't used yet. I have an idea how it will be used, but
> I think before this patch lands there should be another patch
> showing an example usage, even if its trivial.

I'm trying to keep the patches small and  the next patches will 
use m_inspectorClient for the real transfer data.
As result I think we don't need a special example of usage.

> > +++ b/WebCore/inspector/InspectorClient.h
> > + virtual bool sendMessageToFrontend(const String& message) = 0;
> 
> It might be worth mentioning what the bool return value here
> means. If not a comment then certainly something in the ChangeLog.

ok.

> Great work on this so far!

thanks.
Comment 6 Pavel Feldman 2010-06-03 15:04:49 PDT
Comment on attachment 57805 [details]
[patch] second iteration. With fixed gtk build and other minor changes.

WebCore/ChangeLog:5
 +          WebInspector: It should be possible to transfer timeline/dom/etc data from inspected page side to the front-end side in serialized format.
Please wrap it at 80 chars or so.

WebCore/inspector/InspectorController.cpp:429
 +  void InspectorController::connectFrontend(const ScriptObject& webInspector)
I think "attach" would be a better name. Also I would leave a comment saying that webInspector parameter is going to go away once we migrate to the new schema.

WebKit/mac/WebCoreSupport/WebInspectorClient.mm:107
 +      return m_webInspectorFrontendClient->dispatchMessageToFrontend(message);
This one is I think wrong. You are telling client of the inspected page (InspectorClient) to dispatch message on the frontend via the InspectorFrontend's client. But methods on client are to be called by WebCore, not WebKit. You should simply get a pointer to the Page and dispatch the JS call there.

WebCore/WebCore.Inspector.exp:24
 +  __ZN7WebCore28InspectorFrontendClientLocal25dispatchMessageToFrontendERKNS_6StringE
This one should not be visible to WebKit. In fact, this entire InspectorFrontendClientLocal class should go away since it exposes a way too rich WebCore API to the WebKit.

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:165
 +      return false;
You plan on implementing these prior to landing, right?
Comment 7 Ilya Tikhonovsky 2010-06-04 14:30:36 PDT
Created attachment 57915 [details]
[patch] third iteration

Safari specific code for sendMessage was moved to new class InspectorClientLocal.
We have attachFrontend/disconnectFrontend pair and that was inconsistent.
Now it is consistent and I think there is no principal difference between connect/disconnect and attach/detach pairs.
I can use any.
Qt implementation was added.
Comment 8 Early Warning System Bot 2010-06-04 14:47:10 PDT
Attachment 57915 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3104031
Comment 9 Yury Semikhatsky 2010-06-06 23:43:39 PDT
Comment on attachment 57915 [details]
[patch] third iteration

WebKit/mac/WebCoreSupport/WebInspectorClient.mm:99
 +      m_frontendPage = core([windowController.get() webView]);
Here and in other clients m_frontendPage should be cleared when the frontend is destroyed.
Comment 10 Yury Semikhatsky 2010-06-06 23:50:24 PDT
Comment on attachment 57915 [details]
[patch] third iteration

WebKit/chromium/src/InspectorClientImpl.cpp:110
 +      if (!devToolsAgent)
is this code reachable? if not, put  ASSERT_NOT_REACHED(); here
Comment 11 Ilya Tikhonovsky 2010-06-07 09:40:42 PDT
Created attachment 58040 [details]
[patch] fourth iteration.
Comment 12 Early Warning System Bot 2010-06-07 09:47:24 PDT
Attachment 58040 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3133197
Comment 13 Pavel Feldman 2010-06-07 10:20:07 PDT
Comment on attachment 58040 [details]
[patch] fourth iteration.

WebCore/inspector/InspectorClient.h:53
 +      virtual bool sendMessageToFrontend(const String& message) = 0;
const String& (no 'message')



WebCore/inspector/InspectorController.h:139
 +      // transport via InspectorClient. After finish webInspector parameter should
... After migration, WebInspector parameter should ...


WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp:116
 +      String dispatchToFrontend("WebInspector.dispatchMessageToFrontend(");
String::format


WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:91
 +      m_frontendPage = 0;
What happens when page->inspectorController()->close() is called?


WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:127
 +      String dispatchToFrontend("WebInspector.dispatchMessageToFrontend(");
Should we format this message using utility method in WebCore?

WebKit/mac/WebCoreSupport/WebInspectorClient.h:46
 +  
no need in blank lines here and below.

WebKit/mac/WebCoreSupport/WebInspectorClient.h:66
 +      virtual bool sendMessageToFrontend(const WebCore::String& key);
key -> message (or even better, remove it!)

WebCore/inspector/InspectorController.cpp:535
 +      m_client->closeInspectorFrontend();
disconnectFrontend is being called by the WebKit layer. After this call, InspectorController should not do any attempts to talk to its frontend. You don't need to call WebCore's disconnectFrontend from WebKit in order to get immediately back to WebKit and execute closeInspectorFrontend...
Comment 14 Ilya Tikhonovsky 2010-06-08 00:00:25 PDT
Created attachment 58119 [details]
[patch] Fifth version. Just try to get qt build happy.
Comment 15 WebKit Review Bot 2010-06-08 00:01:48 PDT
Attachment 58119 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Ilya Tikhonovsky 2010-06-08 08:06:31 PDT
Created attachment 58139 [details]
[patch] sixth iteration.
Comment 17 WebKit Review Bot 2010-06-08 08:52:38 PDT
Attachment 58139 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3181043
Comment 18 Joseph Pecoraro 2010-06-08 12:15:07 PDT
Comment on attachment 58139 [details]
[patch] sixth iteration. 

> WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp
> +bool InspectorClientEfl::sendMessageToFrontend(const String&)
> +{
> +    notImpelemented();
> +    return false;
> +}

Eek, typo. s/notImpelemented/notImplemented/


> WebKit/mac/WebCoreSupport/WebInspectorClient.h
> +namespace WebCore {
> +
> +class Page;
> +
> +}

All other forwarding headers like this in WebKit/mac
are formatted like so:

    namespace WebCore {
      class Page;
    }


> WebKit/win/WebCoreSupport/WebInspectorClient.h
> +namespace WebCore {
> +
> +class Page;
> +
> +}

Ditto.


As you know Pavel's comments are still to be addressed
as well. But its looking good.
Comment 19 Pavel Feldman 2010-06-08 12:51:21 PDT
Comment on attachment 58139 [details]
[patch] sixth iteration. 

WebCore/inspector/InspectorClient.h:45
 +      virtual void closeInspectorFrontend() = 0;
This one is no longer used and can go away, right?


WebCore/inspector/front-end/inspector.js:575
 +  WebInspector.dispatchMessageToFrontend = function(arguments)
Nit: I'd call it dispatchMessageFromBackend. Also, we might want to have try / catch here for the transition phase that would log into WebInspector.log.

WebCore/loader/EmptyClients.h:483
 +      virtual void closeInspectorFrontend() { }
Remove this?

WebKit/chromium/src/InspectorClientImpl.cpp:70
 +  void InspectorClientImpl::closeInspectorFrontend()
Unused?

WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:41
 +  void InspectorClientEfl::closeInspectorFrontend()
Remove?

WebKit/efl/WebCoreSupport/InspectorClientEfl.h:47
 +      virtual void closeInspectorFrontend();
ditto
Comment 20 Pavel Feldman 2010-06-08 12:51:51 PDT
Pushed wrong button half-review. To be continued.
Comment 21 Pavel Feldman 2010-06-08 12:56:34 PDT
Comment on attachment 58139 [details]
[patch] sixth iteration. 

One minor cleanup and we are good to land!


WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:168
 +      m_inspectorClient->closeInspectorClient();
closeInspectorFrontend() ? Also, I'd name it "releaseFrontendPage" since it better reflects what it does.




WebKit/haiku/WebCoreSupport/InspectorClientHaiku.cpp:50
 +  void InspectorClientHaiku::closeInspectorFrontend()
nuke it

WebKit/haiku/WebCoreSupport/InspectorClientHaiku.h:46
 +          virtual void closeInspectorFrontend();
ditto

WebKit/mac/WebCoreSupport/WebInspectorClient.h:58
 +      virtual void closeInspectorFrontend();
same recommendation on the name...

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:113
 +  void InspectorClientQt::closeInspectorFrontend()
ditto

WebKit/wx/WebKitSupport/InspectorClientWx.cpp:55
 +  void InspectorClientWx::closeInspectorFrontend()
nuke
Comment 22 Ilya Tikhonovsky 2010-06-09 06:00:14 PDT
Created attachment 58238 [details]
[patch] seventh iteration.
Comment 23 Pavel Feldman 2010-06-09 06:16:29 PDT
Comment on attachment 58238 [details]
[patch] seventh iteration.

Nit: could you please work on the changelog entry to make our plans more clear?
Comment 24 WebKit Review Bot 2010-06-09 08:36:09 PDT
http://trac.webkit.org/changeset/60891 might have broken GTK Linux 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/60890
http://trac.webkit.org/changeset/60891
http://trac.webkit.org/changeset/60892
Comment 25 Ilya Tikhonovsky 2010-06-10 10:56:50 PDT
Created attachment 58390 [details]
[patch] next iteration with fixes for gtk
Comment 26 Yury Semikhatsky 2010-06-10 11:05:58 PDT
Comment on attachment 58390 [details]
[patch] next iteration with fixes for gtk



WebKit/gtk/WebCoreSupport/InspectorClientGtk.h:50
 +          ~InspectorFrontendClient()
Does it compile? Should be ~InspectorClient() I suppose
Comment 27 Yury Semikhatsky 2010-06-10 11:12:21 PDT
Comment on attachment 58390 [details]
[patch] next iteration with fixes for gtk

WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:174
 +      if (m_inspectorClient)
I see m_inspectorClient cleared only in the destructor, do you really need this check?
Comment 28 Ilya Tikhonovsky 2010-06-10 11:14:56 PDT
(In reply to comment #27)
> (From update of attachment 58390 [details])
> WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:174
>  +      if (m_inspectorClient)
> I see m_inspectorClient cleared only in the destructor, do you really need this check?

Really we have no guaranties about the order of deleting the InspectorClient and InspectorFrontend.
Comment 29 WebKit Review Bot 2010-06-10 11:24:59 PDT
Attachment 58390 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3177153
Comment 30 Ilya Tikhonovsky 2010-06-11 01:35:18 PDT
Created attachment 58454 [details]
[patch] next iteration.
Comment 31 Pavel Feldman 2010-06-11 05:02:17 PDT
Comment on attachment 58454 [details]
[patch] next iteration.

Best to land manually.
Comment 32 Ilya Tikhonovsky 2010-06-14 09:02:42 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorClient.h
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/InspectorFrontendClientLocal.cpp
	M	WebCore/inspector/InspectorValues.cpp
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/loader/EmptyClients.h
	M	WebKit/ChangeLog
	M	WebKit/cf/ChangeLog
	M	WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/InspectorClientImpl.cpp
	M	WebKit/chromium/src/InspectorClientImpl.h
	M	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
	M	WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp
	M	WebKit/efl/WebCoreSupport/InspectorClientEfl.h
	M	WebKit/gtk/ChangeLog
	M	WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
	M	WebKit/gtk/WebCoreSupport/InspectorClientGtk.h
	M	WebKit/haiku/ChangeLog
	M	WebKit/haiku/WebCoreSupport/InspectorClientHaiku.cpp
	M	WebKit/haiku/WebCoreSupport/InspectorClientHaiku.h
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/WebCoreSupport/WebInspectorClient.h
	M	WebKit/mac/WebCoreSupport/WebInspectorClient.mm
	M	WebKit/qt/ChangeLog
	M	WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
	M	WebKit/qt/WebCoreSupport/InspectorClientQt.h
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebCoreSupport/WebInspectorClient.cpp
	M	WebKit/win/WebCoreSupport/WebInspectorClient.h
	M	WebKit/wx/ChangeLog
	M	WebKit/wx/WebKitSupport/InspectorClientWx.cpp
	M	WebKit/wx/WebKitSupport/InspectorClientWx.h
Committed r61113
Comment 33 Ilya Tikhonovsky 2010-06-14 09:04:29 PDT
Flaky inspector tests fix for gtk.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/gtk/ChangeLog
	M	WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp
Committed r61124