[EFL][WK2] Port MiniBrowser to Elementary

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

We should port MiniBrowser to Elementary so that we can simplify the code and more easily add functionality.

[Chris Dumez]

Created attachment 167747 [details]
Patch

[Kenneth Rohde Christiansen]

Ok, if it is not too much of a dependency. Maybe we could only build the minibrowser if elementary is actually present to not make it a hard dependency

[Kenneth Rohde Christiansen]

Comment on attachment 167747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167747&action=review

> Tools/MiniBrowser/efl/main.c:29
> -static const int DEFAULT_WIDTH = 800;
> -static const int DEFAULT_HEIGHT = 600;
> +static const int DEFAULT_WIDTH = 1024;
> +static const int DEFAULT_HEIGHT = 768;

I think the old size is better, as it is also used by tests (and suggested by W3C as a good testing size)

[Chris Dumez]

Created attachment 167749 [details]
Patch for landing

Take Kenneth's feedback into consideration.

[Ryuan Choi]

Comment on attachment 167747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167747&action=review

I am not sure whether it is acceptable for efl side.

elementary has dependency of webkit/efl although it is optional.

At least, I think that we need to get concensus with some efl folks.

> Tools/MiniBrowser/efl/main.c:371
> +    evas_object_focus_set(app_data->webview, EINA_TRUE);

Did you check any regression which is related to focus behavior ?
IIRC, focus should be controlled carefully when we change focus between elm_entry and evas_object.

[Chris Dumez]

Comment on attachment 167749 [details]
Patch for landing

I think there is a focus issue I need to investigate.

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 167747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167747&action=review
> 
> I am not sure whether it is acceptable for efl side.
> 
> elementary has dependency of webkit/efl although it is optional.
> 
> At least, I think that we need to get concensus with some efl folks.

I'm not proposing to use elementary for WebKit EFL itself.

However, MiniBrowser is an application (distinct from WebKit EFL) and it makes a lot of sense to use Elementary for it. Elementary brings widgets, layouts, dialogs that will help us a lot make MiniBrowser better. This current patch is merely for porting but the idea is to bring a lot more functionality to MiniBrowser in the near future.

I'll ask rakuco to help me with CMake so that Elementary becomes an optional dependency and so that MiniBrowser is not built if Elementary is not installed.

> 
> > Tools/MiniBrowser/efl/main.c:371
> > +    evas_object_focus_set(app_data->webview, EINA_TRUE);
> 
> Did you check any regression which is related to focus behavior ?
> IIRC, focus should be controlled carefully when we change focus between elm_entry and evas_object.

Yes. There is indeed an issue with focus. I noticed it afterwards. I have a patch for it locally so I will reupload soon.

[Chris Dumez]

Created attachment 167775 [details]
Patch

- Fix focus issue between view and URL bar
- Make Elementary dependency optional and build MiniBrowser only if Elementary is found (Thanks rakuco for the help!)

I also sent an email on the mailing list to explain the change.

[Chris Dumez]

Created attachment 167785 [details]
Patch

- Hide the end widget of the URL bar entry

[Kenneth Rohde Christiansen]

Comment on attachment 167785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167785&action=review

Forward/back button coming up? :)

> Tools/MiniBrowser/efl/main.c:323
> +win_del_cb(void *user_data, Evas_Object *window, void *event_info)

why is this called win_del_cb and other callbacks called like on_url_bar_clicked, why not on_win_del?

[Ryuan Choi]

(In reply to comment #8)
> Created an attachment (id=167775) [details]
> Patch
> 
> - Fix focus issue between view and URL bar
> - Make Elementary dependency optional and build MiniBrowser only if Elementary is found (Thanks rakuco for the help!)
> 
> I also sent an email on the mailing list to explain the change.

Now, it looks reasonable to me.

[Ryuan Choi]

Comment on attachment 167785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167785&action=review

> Tools/MiniBrowser/efl/CMakeLists.txt:-4
> -    ${TOOLS_DIR}/EWebLauncher/url_bar.c

I think that you should remove ControlTheme dependency also.

> Tools/MiniBrowser/efl/main.c:101
> +    evas_object_del(window->webview);
> +    evas_object_del(window->url_bar);
> +    evas_object_del(window->bg);
> +    evas_object_del(window->window);
> +    evas_object_del(window->vertical_layout);

IIRC, when you remove elm_window, all child objects will be removed.

> Tools/MiniBrowser/efl/main.c:367
> +    ewk_view_theme_set(app_data->webview, THEME_DIR"/default.edj");

Nit: How about adding space THEME_DIR "/default.edj" ?

[Chris Dumez]

Created attachment 167941 [details]
Patch

Took into consideration feedback from Kenneth and Ryuan.

[Chris Dumez]

Created attachment 167945 [details]
Patch

- Remove weird focus highlight (only useful for debugging)

[WebKit Review Bot]

Comment on attachment 167945 [details]
Patch

Clearing flags on attachment: 167945

Committed r130887: <http://trac.webkit.org/changeset/130887>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

After this change, we cannot change the evas engine in use.
Filed https://bugs.webkit.org/show_bug.cgi?id=99286 for that.