Bug 91718 - [EFL] Add Web Inspector to EWebLauncher
Summary: [EFL] Add Web Inspector to EWebLauncher
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Seokju Kwon
URL:
Keywords:
Depends on: 96499
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-19 00:18 PDT by Seokju Kwon
Modified: 2012-10-07 23:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.15 KB, patch)
2012-07-19 00:21 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (8.12 KB, patch)
2012-07-19 01:46 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2012-07-24 19:00 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (1019.38 KB, application/zip)
2012-07-24 20:32 PDT, WebKit Review Bot
no flags Details
Patch (7.97 KB, patch)
2012-07-25 17:01 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (10.35 KB, patch)
2012-09-12 00:35 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (14.37 KB, patch)
2012-09-19 00:24 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (14.28 KB, patch)
2012-10-06 23:52 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (14.28 KB, patch)
2012-10-07 21:15 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (14.28 KB, patch)
2012-10-07 21:30 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (14.54 KB, patch)
2012-10-07 22:25 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Seokju Kwon 2012-07-19 00:18:32 PDT
Implemenation of Web Inspector on EWebLauncher.
The Web Inspector can be opened or closed by pressing ctrl+i on a web page.
Comment 1 Seokju Kwon 2012-07-19 00:21:15 PDT
Created attachment 153193 [details]
Patch
Comment 2 Gyuyoung Kim 2012-07-19 00:44:32 PDT
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.
Comment 3 Seokju Kwon 2012-07-19 01:46:41 PDT
Created attachment 153209 [details]
Patch
Comment 4 Seokju Kwon 2012-07-19 21:52:25 PDT
(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 5 Chris Dumez 2012-07-24 04:56:30 PDT
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.
Comment 6 Seokju Kwon 2012-07-24 16:29:30 PDT
(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 :(
Comment 7 Seokju Kwon 2012-07-24 19:00:26 PDT
Created attachment 154218 [details]
Patch
Comment 8 WebKit Review Bot 2012-07-24 20:32:01 PDT
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
Comment 9 WebKit Review Bot 2012-07-24 20:32:06 PDT
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
Comment 10 Seokju Kwon 2012-07-25 17:01:26 PDT
Created attachment 154491 [details]
Patch
Comment 11 Chris Dumez 2012-08-01 05:46:05 PDT
Comment on attachment 154491 [details]
Patch

LGTM. Thanks.
Comment 12 Thiago Marcos P. Santos 2012-08-01 06:07:14 PDT
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 13 Chris Dumez 2012-08-01 06:10:48 PDT
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 14 Chris Dumez 2012-08-01 06:17:52 PDT
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 15 Raphael Kubo da Costa (:rakuco) 2012-08-02 11:15:02 PDT
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 16 Gyuyoung Kim 2012-08-27 18:57:42 PDT
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.
Comment 17 Chris Dumez 2012-09-10 06:16:28 PDT
Any update on this?
Comment 18 Seokju Kwon 2012-09-12 00:35:45 PDT
Created attachment 163536 [details]
Patch
Comment 19 Kenneth Rohde Christiansen 2012-09-12 00:42:31 PDT
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.
Comment 20 Seokju Kwon 2012-09-12 02:14:21 PDT
(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.
Comment 21 Seokju Kwon 2012-09-19 00:24:35 PDT
Created attachment 164677 [details]
Patch
Comment 22 Chris Dumez 2012-09-20 03:50:12 PDT
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 23 Mikhail Pozdnyakov 2012-09-20 03:55:50 PDT
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?
Comment 24 Seokju Kwon 2012-10-06 22:59:50 PDT
(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.
Comment 25 Seokju Kwon 2012-10-06 23:02:27 PDT
(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.
Comment 26 Seokju Kwon 2012-10-06 23:52:56 PDT
Created attachment 167471 [details]
Patch
Comment 27 Gyuyoung Kim 2012-10-07 19:16:05 PDT
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.
Comment 28 Seokju Kwon 2012-10-07 21:15:20 PDT
Created attachment 167500 [details]
Patch
Comment 29 Seokju Kwon 2012-10-07 21:15:51 PDT
(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.
Comment 30 Seokju Kwon 2012-10-07 21:30:19 PDT
Created attachment 167501 [details]
Patch
Comment 31 Ryuan Choi 2012-10-07 22:11:24 PDT
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?
Comment 32 Seokju Kwon 2012-10-07 22:24:28 PDT
(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.
Comment 33 Seokju Kwon 2012-10-07 22:25:52 PDT
Created attachment 167504 [details]
Patch
Comment 34 Ryuan Choi 2012-10-07 22:40:08 PDT
Comment on attachment 167504 [details]
Patch

Looks fine to me.
Comment 35 WebKit Review Bot 2012-10-07 23:28:01 PDT
Comment on attachment 167504 [details]
Patch

Clearing flags on attachment: 167504

Committed r130617: <http://trac.webkit.org/changeset/130617>
Comment 36 WebKit Review Bot 2012-10-07 23:28:07 PDT
All reviewed patches have been landed.  Closing bug.