[EFL] Add Web Inspector to WebKit-EFL

[Seokju Kwon]

Add EFL port implementation for Web Inspector

[Seokju Kwon]

Created attachment 137052 [details]
Patch

[Leandro Pereira]

Comment on attachment 137052 [details]
Patch

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

It's a good start, but there are some issues to take care first.

> Source/WebKit/ChangeLog:6
> +        [EFL] Add Web Inspector to WebKit-EFL
> +        https://bugs.webkit.org/show_bug.cgi?id=83865
> +
> +        Add EFL port implementation for Web Inspector

Please try to be more descriptive in the changelog entries.

> Source/WebKit/PlatformEfl.cmake:291
> +    FILE(COPY ${WEB_INSPECTOR_DATA} DESTINATION ${WEB_INSPECTOR_DIR})
> +    FILE(COPY ${WEB_INSPECTOR_UGLIFYJS_DATA} DESTINATION ${WEB_INSPECTOR_UGLIFYJS_DIR})
> +    FILE(COPY ${WEB_INSPECTOR_IMAGES_DATA} DESTINATION ${WEB_INSPECTOR_IMAGES_DIR})

This will be executed only during the build process; unless I'm mistaken, the inspector won't work properly unless these are actually installed.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:34
> +    InspectorFrontendClientEfl* inspectorFrontendClient = (InspectorFrontendClientEfl*)userData;

C++-style cast.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:36
> +        inspectorFrontendClient->destroyInspectorWindow(true);

I'm not sure what 'true' means here; consider using an enum.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:41
> +public:
> +    virtual ~InspectorFrontendSettingsEfl() { }

No need to implement this.

[Seokju Kwon]

(In reply to comment #2)
> (From update of attachment 137052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137052&action=review
> 
> It's a good start, but there are some issues to take care first.
> 
> > Source/WebKit/ChangeLog:6
> > +        [EFL] Add Web Inspector to WebKit-EFL
> > +        https://bugs.webkit.org/show_bug.cgi?id=83865
> > +
> > +        Add EFL port implementation for Web Inspector
> 
> Please try to be more descriptive in the changelog entries.
> 
I will add additional comments in changelog entries.

> > Source/WebKit/PlatformEfl.cmake:291
> > +    FILE(COPY ${WEB_INSPECTOR_DATA} DESTINATION ${WEB_INSPECTOR_DIR})
> > +    FILE(COPY ${WEB_INSPECTOR_UGLIFYJS_DATA} DESTINATION ${WEB_INSPECTOR_UGLIFYJS_DIR})
> > +    FILE(COPY ${WEB_INSPECTOR_IMAGES_DATA} DESTINATION ${WEB_INSPECTOR_IMAGES_DIR})
> 
> This will be executed only during the build process; unless I'm mistaken, the inspector won't work properly unless these are actually installed.
> 
You're right. I will chanage it for these are actually installed.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:34
> > +    InspectorFrontendClientEfl* inspectorFrontendClient = (InspectorFrontendClientEfl*)userData;
> 
> C++-style cast.
>
Ok, I will fix it.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:36
> > +        inspectorFrontendClient->destroyInspectorWindow(true);
> 
> I'm not sure what 'true' means here; consider using an enum.

The argument of destoryInspectorWindow(bool notifyInspectorController) decides whether to notify inspectorController when destorying window. Other ports(Qt, GTK) are also using it like this.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:41
> > +public:
> > +    virtual ~InspectorFrontendSettingsEfl() { }
> 
> No need to implement this.

Sure, I will remove it.

[Seokju Kwon]

Created attachment 138304 [details]
Patch

[Kangil Han]

acidx: Could you please review this patch?

[Gyuyoung Kim]

Comment on attachment 138304 [details]
Patch

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

I wonder how many test cases are covered by this patch.

> Source/WebKit/ChangeLog:7
> +        - Add a stub implementation of InspectorClientEfl

Generally, WebKit tends not to use - in ChangeLog.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:96
> +    InspectorController* controller = ewk_view_core_page_get(m_inspectorView)->inspectorController();

ewk_view_core_page_get() was removed. Use EWKPrivate::corePage()

> Source/WebKit/efl/ewk/ewk_view.h:2446
> +EAPI void ewk_view_web_inspector_show(Evas_Object *o);

It looks it is good to add const keyword as below,

const Evas_Object *o

> Source/WebKit/efl/ewk/ewk_view.h:2453
> +EAPI void ewk_view_web_inspector_close(Evas_Object *o);

ditto.

[Seokju Kwon]

Created attachment 143527 [details]
Patch

[Seokju Kwon]

(In reply to comment #6)
> (From update of attachment 138304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138304&action=review
> 
> I wonder how many test cases are covered by this patch.
> 
I have added web inspector to EWebLauncher with this patch.
And I am working on DRT related to Inspector in WebKit-EFL with this patch now.

This patch is too old. Others have been fixed.
> > Source/WebKit/ChangeLog:7
> > +        - Add a stub implementation of InspectorClientEfl
> 
> Generally, WebKit tends not to use - in ChangeLog.
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:96
> > +    InspectorController* controller = ewk_view_core_page_get(m_inspectorView)->inspectorController();
> 
> ewk_view_core_page_get() was removed. Use EWKPrivate::corePage()
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2446
> > +EAPI void ewk_view_web_inspector_show(Evas_Object *o);
> 
> It looks it is good to add const keyword as below,
> 
> const Evas_Object *o
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2453
> > +EAPI void ewk_view_web_inspector_close(Evas_Object *o);
> 
> ditto.

[Seokju Kwon]

(In reply to comment #6)
> (From update of attachment 138304 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138304&action=review
> 
> I wonder how many test cases are covered by this patch.

I have done below test cases with this patch. As you know, It is the first step.
LayoutTests/inspector : 261/280 tests passed (93.2%)

[Gyuyoung Kim]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 138304 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138304&action=review
> > 
> > I wonder how many test cases are covered by this patch.
> 
> I have done below test cases with this patch. As you know, It is the first step.
> LayoutTests/inspector : 261/280 tests passed (93.2%)

Wow, good news !!. Could you file a bug for this tests supported by this patch ? And also, link it to this bug. I will review it.

[Gyuyoung Kim]

Comment on attachment 143527 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:27
> +#include "ewk_private.h"

Remove unneeded include.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:56
> +InspectorClientEfl::InspectorClientEfl(Evas_Object* view)

I wonder whether view is webview. 157 line have used inspectorView for parameter name. Don't you need more clear name?

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:129
> +#if ENABLE(INSPECTOR)

By the way, should we use ENABLE(INSPECTOR) macro in each function? I think we can do InspectorClientEfl.cpp isn't included in CMake file When ENABLE_INSPECTOR feature is disabled. If so, we don't need to use ENABLE(INSPECTOR) macro per a function.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:146
> +    String inspectorFilesPath = "file://"WEB_INSPECTOR_INSTALL_DIR"/inspector.html";

Use string utility function to combine multiple strings. For example, makeString().

[Seokju Kwon]

(In reply to comment #11)
> (From update of attachment 143527 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143527&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:27
> > +#include "ewk_private.h"
> 
> Remove unneeded include.
I will remove it.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:56
> > +InspectorClientEfl::InspectorClientEfl(Evas_Object* view)
> 
> I wonder whether view is webview. 157 line have used inspectorView for parameter name. Don't you need more clear name?
You're right. webView is better than view. I will fix it.

> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:129
> > +#if ENABLE(INSPECTOR)
> 
> By the way, should we use ENABLE(INSPECTOR) macro in each function? I think we can do InspectorClientEfl.cpp isn't included in CMake file When ENABLE_INSPECTOR feature is disabled. If so, we don't need to use ENABLE(INSPECTOR) macro per a function.
InspectorClientEfl can be created regardless of ENABLE_INSPECTOR feature in _ewk_view_priv_new function. I can remove some macro only to prevent build break minimally, (for example WEB_INSPECTOR_DIR, InspectorController) if you want it. 
 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:146
> > +    String inspectorFilesPath = "file://"WEB_INSPECTOR_INSTALL_DIR"/inspector.html";
> 
> Use string utility function to combine multiple strings. For example, makeString().
Oh thanks, I will use it.

[Seokju Kwon]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > (From update of attachment 138304 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=138304&action=review
> > > 
> > > I wonder how many test cases are covered by this patch.
> > 
> > I have done below test cases with this patch. As you know, It is the first step.
> > LayoutTests/inspector : 261/280 tests passed (93.2%)
> 
> Wow, good news !!. Could you file a bug for this tests supported by this patch ? And also, link it to this bug. I will review it.


I have made a bug : [EFL][DRT] Add support for Web Inspector in WebKit-EFL DRT (https://bugs.webkit.org/show_bug.cgi?id=87935)

Test result on 2012/05/30
LayoutTests/inspector : 261/280 tests passed (93.2%) 
LayoutTests/http/test/inspector : 61/82 test passed (74.4%)
LayoutTests/http/test/inspector-enabled : 10/10 tests passed (100%)

[Seokju Kwon]

Created attachment 145050 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 145050 [details]
Patch

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

I think this patch is much better than before. Submit modified patch again.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:84
> +    evas_object_smart_callback_call(m_inspectedView, "inspector,view,create", &inspectorView);

It looks m_inspectedView is webview. If so, you have to mention description of this signal to ewk_view.h

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L35

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:167
> +    evas_object_smart_callback_del(m_inspectorView, "inspector,view,destroy", notifyWebInspectorDestroy);

ditto.

> Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:225
> +    evas_object_smart_callback_call(m_inspectedView, "inspector,view,close", m_inspectorView);

ditto.

[Seokju Kwon]

Created attachment 148490 [details]
Patch

[Seokju Kwon]

(In reply to comment #15)
> (From update of attachment 145050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145050&action=review
> 
> I think this patch is much better than before. Submit modified patch again.
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:84
> > +    evas_object_smart_callback_call(m_inspectedView, "inspector,view,create", &inspectorView);
> 
> It looks m_inspectedView is webview. If so, you have to mention description of this signal to ewk_view.h
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L35
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:167
> > +    evas_object_smart_callback_del(m_inspectorView, "inspector,view,destroy", notifyWebInspectorDestroy);
> 
> ditto.
> 
> > Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp:225
> > +    evas_object_smart_callback_call(m_inspectedView, "inspector,view,close", m_inspectorView);
> 
> ditto.

I have done what you said. Thank you for reviewing this patch.

[Gyuyoung Kim]

Comment on attachment 148490 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.h:2643
> + * "inspector,view,create" and "inspector,view,close"

No, you have to move this signal description to signal description list as I already said.

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L35

[Seokju Kwon]

Created attachment 148718 [details]
Patch

[Seokju Kwon]

(In reply to comment #18)
> (From update of attachment 148490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148490&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2643
> > + * "inspector,view,create" and "inspector,view,close"
> 
> No, you have to move this signal description to signal description list as I already said.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L35

Oops, I am sorry that I have missed it(In reply to comment #18)
> (From update of attachment 148490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148490&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2643
> > + * "inspector,view,create" and "inspector,view,close"
> 
> No, you have to move this signal description to signal description list as I already said.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_view.h#L35

Oops, I missed it. Another patch has been uploaded.

[Seokju Kwon]

Created attachment 150085 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150085 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:44
> +#include "InspectorController.h"

Hmm, I don't know why style checker didn't find this style error. Adhere alphabetic order.

> Source/WebKit/efl/ewk/ewk_view.h:2686
> +EAPI void ewk_view_web_inspector_view_set(Evas_Object* o, Evas_Object* obj);

If possible, I think we need to use more meaningful parameter name. Is it better to use inspector_view ?

In addition, move * to variable side.

[Gyuyoung Kim]

Comment on attachment 150085 [details]
Patch

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

>> Source/WebKit/efl/ewk/ewk_view.cpp:44
>> +#include "InspectorController.h"
> 
> Hmm, I don't know why style checker didn't find this style error. Adhere alphabetic order.

Oops. I'm sorry. this is correct now.

[Seokju Kwon]

Created attachment 150363 [details]
Patch

[Seokju Kwon]

Created attachment 150364 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150364 [details]
Patch

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

Need to rebase.

> Source/WebKit/efl/ewk/ewk_view.h:109
> + *  - "inspector,view,create", void: request to create the new view for web inspector.

Adhere alphabetical order.

[Seokju Kwon]

Created attachment 150913 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150913 [details]
Patch

LGTM.

[Pavel Feldman]

Comment on attachment 150913 [details]
Patch

This looks sane to me. There is a lot of detail in the plumbing implementation, but if you followed the Qt or Gtk code, you should be fine. Please test following scenarios of closing the inspector prior to landing:
1) via closing inspector view
2) via clicking inspector's close button
3) make sure that inspector closes upon inspected view close.

You should be able to re-open inspector for these cases. Also, make sure things are working for multiple tabs and inspected page reloads. I think it is safe to land this as a first cut and follow up with the bugfixes as problems arise.

[Seokju Kwon]

(In reply to comment #29)
> (From update of attachment 150913 [details])
> This looks sane to me. There is a lot of detail in the plumbing implementation, but if you followed the Qt or Gtk code, you should be fine. Please test following scenarios of closing the inspector prior to landing:
> 1) via closing inspector view
> 2) via clicking inspector's close button
> 3) make sure that inspector closes upon inspected view close.
> 
> You should be able to re-open inspector for these cases. Also, make sure things are working for multiple tabs and inspected page reloads. I think it is safe to land this as a first cut and follow up with the bugfixes as problems arise.

I have already tested your scenarios with EWebLauncher browser and Dumprendertree.
Also I will upload some patches for using web inspector.

[Seokju Kwon]

Created attachment 151021 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151021 [details]
Patch

Clearing flags on attachment: 151021

Committed r121947: <http://trac.webkit.org/changeset/121947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.