Implemenation of Web Inspector on EWebLauncher. The Web Inspector can be opened or closed by pressing ctrl+i on a web page.
Created attachment 153193 [details] Patch
Comment on attachment 153193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153193&action=review > Tools/EWebLauncher/main.c:651 > + { I think we don't need to use {..} here. > Tools/EWebLauncher/main.c:667 > + { ditto.
Created attachment 153209 [details] Patch
(In reply to comment #2) > (From update of attachment 153193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153193&action=review > > > Tools/EWebLauncher/main.c:651 > > + { > > I think we don't need to use {..} here. > > > Tools/EWebLauncher/main.c:667 > > + { > > ditto. I have removed them.
Comment on attachment 153209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153209&action=review > Tools/ChangeLog:8 > + Implemenation of Web Inspector on Browser called EWebLauncher. "implementation" "on Browser called EWebLauncher" -> "in EWebLauncher" > Tools/EWebLauncher/main.c:660 > +on_inspector_view_destoryed(Ecore_Evas *ee) destoryed -> destroyed > Tools/EWebLauncher/main.c:807 > + ecore_evas_callback_delete_request_set(inspector->ee, on_inspector_view_destoryed); Typo.
(In reply to comment #5) > (From update of attachment 153209 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153209&action=review > > > Tools/ChangeLog:8 > > + Implemenation of Web Inspector on Browser called EWebLauncher. > > "implementation" > "on Browser called EWebLauncher" -> "in EWebLauncher" > > > Tools/EWebLauncher/main.c:660 > > +on_inspector_view_destoryed(Ecore_Evas *ee) > > destoryed -> destroyed > > > Tools/EWebLauncher/main.c:807 > > + ecore_evas_callback_delete_request_set(inspector->ee, on_inspector_view_destoryed); > > Typo. Oops! my mistake :(
Created attachment 154218 [details] Patch
Comment on attachment 154218 [details] Patch Attachment 154218 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13348079 New failing tests: fast/forms/range/slider-onchange-event.html fast/forms/range/slider-mouse-events.html fast/forms/range/slider-delete-while-dragging-thumb.html
Created attachment 154233 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 154491 [details] Patch
Comment on attachment 154491 [details] Patch LGTM. Thanks.
Comment on attachment 154491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154491&action=review > Tools/EWebLauncher/main.c:233 > +on_inspector_ecore_evas_resize(Ecore_Evas *ee) We should rename the browser resize method to on_browser_ecore_evas_resize for the sake of consistence. > Tools/EWebLauncher/main.c:789 > +static int > +webInspectorCreate(ELauncher *app) This function doesn't need to return anything.
Comment on attachment 154491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154491&action=review >> Tools/EWebLauncher/main.c:789 >> +webInspectorCreate(ELauncher *app) > > This function doesn't need to return anything. Well, returning an int is consistent with browserCreate() in the same file.
Comment on attachment 154491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154491&action=review >> Tools/EWebLauncher/main.c:233 >> +on_inspector_ecore_evas_resize(Ecore_Evas *ee) > > We should rename the browser resize method to on_browser_ecore_evas_resize for the sake of consistence. Hmm, I'm all for consistency but the issue is that the function code seems inspector-specific so using a more generic name may be confusing here.
Comment on attachment 154491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154491&action=review This patch makes me kind of sad because it makes the code even uglier, partly due to the way the inspector was added to ewk_view, with all these signals which have to be passed from one side to the other. I wonder if at least part of the webInspectorCreate() code couldn't be factored out and shared with browserCreate(). > Tools/EWebLauncher/main.c:863 > + Evas_Object *inspectorView = ewk_view_web_inspector_view_get(((ELauncher*)app)->browser); > + if (inspectorView) > + evas_object_smart_callback_call(inspectorView, "inspector,view,destroy", NULL); Doesn't it make more sense to call ewk_view_web_inspector_close() here? > Tools/EWebLauncher/main.c:879 > + Evas_Object *inspectorView = ewk_view_web_inspector_view_get(app->browser); > + if (inspectorView) > + evas_object_smart_callback_call(inspectorView, "inspector,view,destroy", NULL); Ditto.
Comment on attachment 154491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154491&action=review > Tools/EWebLauncher/main.c:632 > + ELauncher *appBrowser = (ELauncher*) user_data; Nit: s/(ELauncher*) user_data/(ELauncher *)user_data/g. Fix remained stuff. > Tools/EWebLauncher/main.c:639 > + ELauncher *appBrowser = (ELauncher*) user_data; Nit: s/appBrowser/app_browser/g ? > Tools/EWebLauncher/main.c:640 > + Evas_Object *inspectorView = (Evas_Object*) event_info; ditto.
Any update on this?
Created attachment 163536 [details] Patch
Comment on attachment 163536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163536&action=review > Tools/EWebLauncher/main.c:782 > +browserCreate(const char *url, const char *theme, const char *userAgent, Eina_Rectangle geometry, const char *engine, const char *backingStore, unsigned char isFlattening, unsigned char isFullscreen, const char *databasePath) That is one crazy list of arguments... seems pretty much out of hand. There must be a better way. Lots of it seems more like settings.
(In reply to comment #19) > (From update of attachment 163536 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163536&action=review > > > Tools/EWebLauncher/main.c:782 > > +browserCreate(const char *url, const char *theme, const char *userAgent, Eina_Rectangle geometry, const char *engine, const char *backingStore, unsigned char isFlattening, unsigned char isFullscreen, const char *databasePath) > > That is one crazy list of arguments... seems pretty much out of hand. There must be a better way. Lots of it seems more like settings. I agree with you. I will make a bug for it.
Created attachment 164677 [details] Patch
Comment on attachment 164677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164677&action=review No major issue. > Tools/EWebLauncher/main.c:166 > +static int webInspectorCreate(ELauncher *app_browser); Shouldn't it be 'appBrowser' ? It seems the other arguments are using camel case. > Tools/EWebLauncher/main.c:645 > + ewk_view_setting_enable_developer_extras_set(obj, EINA_TRUE); Why do we not enable this setting once when the view is created? What's the point of enabling/disabling it every time? > Tools/EWebLauncher/main.c:764 > + if (url && (url[0] != '\0')) if (url && *url) ?
Comment on attachment 164677 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164677&action=review > Tools/EWebLauncher/main.c:944 > + if ((userArgs->geometry.w <= 0) && (userArgs->geometry.h <= 0)) { What if only the width (or only the height) is negative?
(In reply to comment #22) > (From update of attachment 164677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164677&action=review > > No major issue. > > > Tools/EWebLauncher/main.c:166 > > +static int webInspectorCreate(ELauncher *app_browser); > > Shouldn't it be 'appBrowser' ? It seems the other arguments are using camel case. I will rename it. > > Tools/EWebLauncher/main.c:645 > > + ewk_view_setting_enable_developer_extras_set(obj, EINA_TRUE); > > Why do we not enable this setting once when the view is created? What's the point of enabling/disabling it every time? It makes sense. I will fix it. > > Tools/EWebLauncher/main.c:764 > > + if (url && (url[0] != '\0')) > > if (url && *url) ? It was an existing code. I think it is unnecessary. I will remove it.
(In reply to comment #23) > (From update of attachment 164677 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164677&action=review > > > Tools/EWebLauncher/main.c:944 > > + if ((userArgs->geometry.w <= 0) && (userArgs->geometry.h <= 0)) { > > What if only the width (or only the height) is negative? I had to use the existing code. I will fix it.
Created attachment 167471 [details] Patch
Comment on attachment 167471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167471&action=review > Tools/EWebLauncher/main.c:731 > + evas_object_smart_callback_add(appBrowser->browser, "title,changed", on_title_changed, appBrowser); IMO, it would be good if you sort these by alphabetical order.
Created attachment 167500 [details] Patch
(In reply to comment #27) > (From update of attachment 167471 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167471&action=review > > > Tools/EWebLauncher/main.c:731 > > + evas_object_smart_callback_add(appBrowser->browser, "title,changed", on_title_changed, appBrowser); > > IMO, it would be good if you sort these by alphabetical order. Done.
Created attachment 167501 [details] Patch
Comment on attachment 167501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167501&action=review LGTM. below is style nit which you can choose. > Tools/EWebLauncher/main.c:678 > + Eina_List *l; > + void *app; Because this is c file, I prefered to define variables in top of function. > Tools/EWebLauncher/main.c:684 > + browserDestroy(((ELauncher *)app)->ee); You looks change browserCreate to windowCreate. Can we change browserDestroy to windowDestroy?
(In reply to comment #31) > (From update of attachment 167501 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167501&action=review > > LGTM. below is style nit which you can choose. > > > Tools/EWebLauncher/main.c:678 > > + Eina_List *l; > > + void *app; > > Because this is c file, I prefered to define variables in top of function. > > > Tools/EWebLauncher/main.c:684 > > + browserDestroy(((ELauncher *)app)->ee); > > You looks change browserCreate to windowCreate. > Can we change browserDestroy to windowDestroy? All done.
Created attachment 167504 [details] Patch
Comment on attachment 167504 [details] Patch Looks fine to me.
Comment on attachment 167504 [details] Patch Clearing flags on attachment: 167504 Committed r130617: <http://trac.webkit.org/changeset/130617>
All reviewed patches have been landed. Closing bug.