Bug 98639

Summary: [EFL][WK2] Add support for Inspector
Product: WebKit Reporter: Seokju Kwon <seokju.kwon>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on: 98748    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Seokju Kwon 2012-10-08 03:33:48 PDT
Implementation of Web Inspector in MiniBrowser.
Comment 1 Chris Dumez 2012-10-08 05:24:05 PDT
Any update on this? I'm willing to work on this otherwise.
Comment 2 Seokju Kwon 2012-10-08 16:16:26 PDT
(In reply to comment #1)
> Any update on this? I'm willing to work on this otherwise.

I am working on it.
Comment 3 Seokju Kwon 2012-10-10 00:08:07 PDT
Created attachment 167940 [details]
Patch
Comment 4 Chris Dumez 2012-10-10 00:29:40 PDT
I think it is better to wait for Bug 98748 to be closed since this is going to impact this patch a bit.
Comment 5 Seokju Kwon 2012-10-10 00:38:32 PDT
(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
Comment 6 Raphael Kubo da Costa (:rakuco) 2012-10-10 05:16:30 PDT
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.
Comment 7 Seokju Kwon 2012-10-10 17:03:59 PDT
(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.
Comment 8 Seokju Kwon 2012-10-10 17:04:08 PDT
Created attachment 168100 [details]
Patch
Comment 9 Gyuyoung Kim 2012-10-10 18:53:52 PDT
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.
Comment 10 Chris Dumez 2012-10-10 22:29:40 PDT
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"
Comment 11 Seokju Kwon 2012-10-10 23:55:38 PDT
Created attachment 168149 [details]
Patch
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-10-11 05:50:37 PDT
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.
Comment 13 Seokju Kwon 2012-10-11 15:48:08 PDT
(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.
Comment 14 Seokju Kwon 2012-10-11 15:49:37 PDT
(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.
Comment 15 Seokju Kwon 2012-10-11 15:50:13 PDT
(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.
Comment 16 Seokju Kwon 2012-10-11 15:53:16 PDT
Created attachment 168298 [details]
Patch
Comment 17 Kenneth Rohde Christiansen 2012-10-11 16:11:16 PDT
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!
Comment 18 Seokju Kwon 2012-10-11 16:50:38 PDT
(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.
Comment 19 Seokju Kwon 2012-10-11 16:52:18 PDT
Created attachment 168318 [details]
Patch
Comment 20 WebKit Review Bot 2012-10-11 19:09:24 PDT
Comment on attachment 168318 [details]
Patch

Clearing flags on attachment: 168318

Committed r131123: <http://trac.webkit.org/changeset/131123>
Comment 21 WebKit Review Bot 2012-10-11 19:09:31 PDT
All reviewed patches have been landed.  Closing bug.