Summary: | [EFL] Add Web Inspector to EWebLauncher | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Seokju Kwon <seokju.kwon> | ||||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Seokju Kwon <seokju.kwon> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, dglazkov, gyuyoung.kim, gyuyoung.kim, joone, kenneth, lucas.de.marchi, rakuco, sw0524.lee, tmpsantos, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||
Bug Depends on: | 96499 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Seokju Kwon
2012-07-19 00:18:32 PDT
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. |