Bug 91718 – [EFL] Add Web Inspector to EWebLauncher

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Seokju Kwon]

Implemenation of Web Inspector on EWebLauncher.
The Web Inspector can be opened or closed by pressing ctrl+i on a web page.

[Seokju Kwon]

Created attachment 153193 [details]
Patch

[Gyuyoung Kim]

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.

[Seokju Kwon]

Created attachment 153209 [details]
Patch

[Seokju Kwon]

(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.

[Chris Dumez]

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.

[Seokju Kwon]

(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 :(

[Seokju Kwon]

Created attachment 154218 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Seokju Kwon]

Created attachment 154491 [details]
Patch

[Chris Dumez]

Comment on attachment 154491 [details]
Patch

LGTM. Thanks.

[Thiago Marcos P. Santos]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Gyuyoung Kim]

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.

[Chris Dumez]

Any update on this?

[Seokju Kwon]

Created attachment 163536 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 164677 [details]
Patch

[Chris Dumez]

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) ?

[Mikhail Pozdnyakov]

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?

[Seokju Kwon]

(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.

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 167471 [details]
Patch

[Gyuyoung Kim]

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.

[Seokju Kwon]

Created attachment 167500 [details]
Patch

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 167501 [details]
Patch

[Ryuan Choi]

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?

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 167504 [details]
Patch

[Ryuan Choi]

Comment on attachment 167504 [details]
Patch

Looks fine to me.

[WebKit Review Bot]

Comment on attachment 167504 [details]
Patch

Clearing flags on attachment: 167504

Committed r130617: <http://trac.webkit.org/changeset/130617>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.