We should port MiniBrowser to Elementary so that we can simplify the code and more easily add functionality.
Created attachment 167747 [details] Patch
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
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)
Created attachment 167749 [details] Patch for landing Take Kenneth's feedback into consideration.
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.
Comment on attachment 167749 [details] Patch for landing I think there is a focus issue I need to investigate.
(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.
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.
Created attachment 167785 [details] Patch - Hide the end widget of the URL bar entry
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?
(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.
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" ?
Created attachment 167941 [details] Patch Took into consideration feedback from Kenneth and Ryuan.
Created attachment 167945 [details] Patch - Remove weird focus highlight (only useful for debugging)
Comment on attachment 167945 [details] Patch Clearing flags on attachment: 167945 Committed r130887: <http://trac.webkit.org/changeset/130887>
All reviewed patches have been landed. Closing bug.
After this change, we cannot change the evas engine in use. Filed https://bugs.webkit.org/show_bug.cgi?id=99286 for that.