Bug 98639 – [EFL][WK2] Add support for Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Seokju Kwon]

Implementation of Web Inspector in MiniBrowser.

[Chris Dumez]

Any update on this? I'm willing to work on this otherwise.

[Seokju Kwon]

(In reply to comment #1)
> Any update on this? I'm willing to work on this otherwise.

I am working on it.

[Seokju Kwon]

Created attachment 167940 [details]
Patch

[Chris Dumez]

I think it is better to wait for Bug 98748 to be closed since this is going to impact this patch a bit.

[Seokju Kwon]

(In reply to comment #4)
> I think it is better to wait for Bug 98748 to be closed since this is going to impact this patch a bit.

Thanks, I have added Bug 98748 into "Depends on" list

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 167940 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:767
> +EAPI void ewk_view_inspector_show(const Evas_Object *o);

I think it makes sense to remove the const specifier here, since this function changes the view.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:776
> +EAPI void ewk_view_inspector_close(const Evas_Object *o);

Ditto.

[Seokju Kwon]

(In reply to comment #6)
> (From update of attachment 167940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167940&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:767
> > +EAPI void ewk_view_inspector_show(const Evas_Object *o);
> 
> I think it makes sense to remove the const specifier here, since this function changes the view.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:776
> > +EAPI void ewk_view_inspector_close(const Evas_Object *o);
> 
> Ditto.

All Done.

[Seokju Kwon]

Created attachment 168100 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 168100 [details]
Patch

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

This patch needs to be rebased.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:767
> +EAPI void ewk_view_inspector_show(Evas_Object *o);

Missing unit test for this API

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:776
> +EAPI void ewk_view_inspector_close(Evas_Object *o);

ditto.

[Chris Dumez]

Comment on attachment 168100 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:45
> +    ecore_evas_geometry_get(inspectorWindow, 0, 0, &width, &height);

These 2 lines should be moved after the inspectorView NULL-check.

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:48
> +    if (inspectorView) {

if (!inspectorView)
    return;

Returning early is more commonly used pattern in WebKit.

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:54
> +static void destoryInspectorWindow(Ecore_Evas* inspectorWindow)

"destroy"

[Seokju Kwon]

Created attachment 168149 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 168149 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:114
> +        evas_object_smart_callback_del(m_inspectorView, "inspector,view,close", closeInspectorWindow);

If you are deleting the object in the next line I don't think you need to delete the callback manually before that.

[Seokju Kwon]

(In reply to comment #10)
> (From update of attachment 168100 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168100&action=review
> 
> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:45
> > +    ecore_evas_geometry_get(inspectorWindow, 0, 0, &width, &height);
> 
> These 2 lines should be moved after the inspectorView NULL-check.
> 
> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:48
> > +    if (inspectorView) {
> 
> if (!inspectorView)
>     return;
> 
> Returning early is more commonly used pattern in WebKit.
> 
> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:54
> > +static void destoryInspectorWindow(Ecore_Evas* inspectorWindow)
> 
> "destroy"

All done.

[Seokju Kwon]

(In reply to comment #12)
> (From update of attachment 168149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168149&action=review
> 
> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:114
> > +        evas_object_smart_callback_del(m_inspectorView, "inspector,view,close", closeInspectorWindow);
> 
> If you are deleting the object in the next line I don't think you need to delete the callback manually before that.

Ok. I will remove it in the next patch.

[Seokju Kwon]

(In reply to comment #9)
> (From update of attachment 168100 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168100&action=review
> 
> This patch needs to be rebased.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:767
> > +EAPI void ewk_view_inspector_show(Evas_Object *o);
> 
> Missing unit test for this API
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:776
> > +EAPI void ewk_view_inspector_close(Evas_Object *o);
> 
> ditto.

All done.

[Seokju Kwon]

Created attachment 168298 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 168298 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1975
> +Eina_Bool ewk_view_inspector_show(Evas_Object* ewkView)

Why can't I show it for an element? Most browsers support this. Did you check the api of other ports? like the Qt webkit1 api, that one at least suports this

Being able to inspect an element directly when showing the inspector is a really nice feature

> Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:96
> +    Ewk_Settings* settings = ewk_view_settings_get(m_inspectorView);
> +    ewk_settings_file_access_from_file_urls_allowed_set(settings, true);

can't this be dangerous? comment?

> Tools/MiniBrowser/efl/main.c:150
> +    } else if (!strcmp(ev->key, "i") && ctrlPressed) {
> +        info("Show Inspector (Ctrl+i) was pressed.\n");
> +        ewk_view_inspector_show(obj);

Can't we add a context menu instead? the launcher could really need this!

[Seokju Kwon]

(In reply to comment #17)
> (From update of attachment 168298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168298&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1975
> > +Eina_Bool ewk_view_inspector_show(Evas_Object* ewkView)
> 
> Why can't I show it for an element? Most browsers support this. Did you check the api of other ports? like the Qt webkit1 api, that one at least suports this
> 
> Being able to inspect an element directly when showing the inspector is a really nice feature
> 

What kind of api on other ports? I think we can inspect an element when showing the inspector. But attaching the window does not support yet.

> > Source/WebKit2/UIProcess/efl/WebInspectorProxyEfl.cpp:96
> > +    Ewk_Settings* settings = ewk_view_settings_get(m_inspectorView);
> > +    ewk_settings_file_access_from_file_urls_allowed_set(settings, true);
> 
> can't this be dangerous? comment?
> 

We should access the inspector.html on inspector view. 

> > Tools/MiniBrowser/efl/main.c:150
> > +    } else if (!strcmp(ev->key, "i") && ctrlPressed) {
> > +        info("Show Inspector (Ctrl+i) was pressed.\n");
> > +        ewk_view_inspector_show(obj);
> 
> Can't we add a context menu instead? the launcher could really need this!

Right. But we will use it until adding a context menu temporarily.

[Seokju Kwon]

Created attachment 168318 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168318 [details]
Patch

Clearing flags on attachment: 168318

Committed r131123: <http://trac.webkit.org/changeset/131123>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.