Summary: | [EFL][WK2] Add support for Inspector | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Seokju Kwon <seokju.kwon> | ||||||||||||
Component: | WebKit EFL | Assignee: | 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
Seokju Kwon
2012-10-08 03:33:48 PDT
Any update on this? I'm willing to work on this otherwise. (In reply to comment #1) > Any update on this? I'm willing to work on this otherwise. I am working on it. Created attachment 167940 [details]
Patch
I think it is better to wait for Bug 98748 to be closed since this is going to impact this patch a bit. (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 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. (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. Created attachment 168100 [details]
Patch
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 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" Created attachment 168149 [details]
Patch
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. (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. (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. (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. Created attachment 168298 [details]
Patch
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! (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. Created attachment 168318 [details]
Patch
Comment on attachment 168318 [details] Patch Clearing flags on attachment: 168318 Committed r131123: <http://trac.webkit.org/changeset/131123> All reviewed patches have been landed. Closing bug. |