| Summary: | [EFL][WK2] Minibrowser : Enhance application to be able to support history list navigation | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tanay <tanay.c> | ||||||||||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, gyuyoung.kim, ryuan.choi | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Tanay
2014-08-11 03:02:41 PDT
Created attachment 236363 [details]
Patch
Comment on attachment 236363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236363&action=review > Tools/MiniBrowser/efl/main.c:62 > +static Eina_Bool forward_navigation_enabled = EINA_FALSE; Patch looks good. However, these two boolean flags may disturb upcoming patch for MiniBrowser. Can't we call callbacks when long press is detected directly ? (In reply to comment #2) > (From update of attachment 236363 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236363&action=review > > > Tools/MiniBrowser/efl/main.c:62 > > +static Eina_Bool forward_navigation_enabled = EINA_FALSE; > > Patch looks good. However, these two boolean flags may disturb upcoming patch for MiniBrowser. Can't we call callbacks when long press is detected directly ? The flags are required due to following reasons: 1) evas button registers both 'clicked' + 'repeated' event when the button is released after longpress. This results in callback for 'clicked' which should be avoided. longpress_enabled is required to differentiate between 'repeated' and 'clicked' events. 2)Also multiple longpress callbacks are recieved when the user longpress the button. We require to process only once and ignore the rest of the calls. 3) forward_navigation flag is required to differentiate the navigation direction. Without this we would not be able to know the direction of navigation. If there are alternatives to avoid the flags please do suggest. Due to above limitations I am using this approach. Comment on attachment 236363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236363&action=review > Tools/MiniBrowser/efl/main.c:1138 > +on_navigation_button_longpress(void *user_data) This is not callback function which is registered by evas_object_event_callback_add. So, how do you think just to use navigaton_button_longpress_process() ? > Tools/MiniBrowser/efl/main.c:1149 > + int item_count, x = 0, y = 0, width = 0, height = 0; Looks C coding style. MiniBrowser is compiled by C++ compiler. > Tools/MiniBrowser/efl/main.c:1163 > + for (index = 0; index < item_count; index++) { index -> int index ? > Tools/MiniBrowser/efl/main.c:2154 > + Eina_Bool auto_repeat = elm_button_autorepeat_get(window->back_button); Could you explain why we need to check if the autorepeat is enabled ? It looks back_button's auto repeat setting is always enabled or disabled. Created attachment 236752 [details]
Patch
(In reply to comment #4) > (From update of attachment 236363 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236363&action=review > > > Tools/MiniBrowser/efl/main.c:1138 > > +on_navigation_button_longpress(void *user_data) > > This is not callback function which is registered by evas_object_event_callback_add. So, how do you think just to use navigaton_button_longpress_process() ? > Yes, you are right. Changed the function name. > > Tools/MiniBrowser/efl/main.c:1149 > > + int item_count, x = 0, y = 0, width = 0, height = 0; > > Looks C coding style. MiniBrowser is compiled by C++ compiler. > Fixed. > > Tools/MiniBrowser/efl/main.c:1163 > > + for (index = 0; index < item_count; index++) { > > index -> int index ? > index a size_t type variable defined with other local variables at ln 1164. > > Tools/MiniBrowser/efl/main.c:2154 > > + Eina_Bool auto_repeat = elm_button_autorepeat_get(window->back_button); > > Could you explain why we need to check if the autorepeat is enabled ? It looks back_button's auto repeat setting is always enabled or disabled. This was part of the code to check if "repeats" is enabled for the buttons. Refactored to always enable the longpress for back and forward evas buttons Comment on attachment 236752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236752&action=review LGTM. However, ryuan might wanna take a final look. > Tools/MiniBrowser/efl/main.c:1204 > + nit: Unneeded line. Comment on attachment 236752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236752&action=review > Tools/MiniBrowser/efl/main.c:1062 > + if (!longpress_enabled) { Nit, why don't just escape using below? if (longpress_enabled) return; > Tools/MiniBrowser/efl/main.c:1143 > + ewk_view_navigate_to(window->ewk_view, eina_list_nth(history_list_get(user_data), item_index-1)); space between - ? > Tools/MiniBrowser/efl/main.c:1149 > +on_navigation_button_longpress_process(void *user_data) on_ looks not required > Tools/MiniBrowser/efl/main.c:1151 > + if (!longpress_enabled) { Isn't early return better? > Tools/MiniBrowser/efl/main.c:1182 > + for (index = 0; index < item_count; index++) { > + title = ewk_back_forward_list_item_title_get(eina_list_nth(list, index)); > + info(" title = %s", title); > + elm_genlist_item_append(window->history.history_list, list_item, (void*)(title), NULL, ELM_GENLIST_ITEM_NONE, on_list_item_select, user_data); > + } Why don't you use EINA_LIST_FOREACH ? It's cheaper than the loop with eina_list_nth and usual way which EFL does. > Tools/MiniBrowser/efl/main.c:1195 > + if (forward_navigation_enabled) { > + evas_object_move(window->history.history_box , x+TOOL_BAR_BUTTON_SIZE+1, y+TOOL_BAR_BUTTON_SIZE); > + evas_object_move(window->history.history_list , x+TOOL_BAR_BUTTON_SIZE+1, y+TOOL_BAR_BUTTON_SIZE); > + } else { > + evas_object_move(window->history.history_box , x, y+TOOL_BAR_BUTTON_SIZE); > + evas_object_move(window->history.history_list , x, y+TOOL_BAR_BUTTON_SIZE); > + } space between + ? > Tools/MiniBrowser/efl/main.c:1213 > + forward_navigation_enabled = EINA_TRUE; Should we keep this as static variable? Can we pass this as the argument of on_navigation_button_longpress_process ? Created attachment 236858 [details]
Patch
(In reply to comment #8) > (From update of attachment 236752 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236752&action=review > > > Tools/MiniBrowser/efl/main.c:1062 > > + if (!longpress_enabled) { > > Nit, why don't just escape using below? > > if (longpress_enabled) return; > Corrected. > > Tools/MiniBrowser/efl/main.c:1143 > > + ewk_view_navigate_to(window->ewk_view, eina_list_nth(history_list_get(user_data), item_index-1)); > > space between - ? > Fixed. > > Tools/MiniBrowser/efl/main.c:1149 > > +on_navigation_button_longpress_process(void *user_data) > > on_ looks not required > Changed the function name to navigation_button_longpress_process. > > Tools/MiniBrowser/efl/main.c:1151 > > + if (!longpress_enabled) { > > Isn't early return better? > Yes, modified the conditions. > > Tools/MiniBrowser/efl/main.c:1182 > > + for (index = 0; index < item_count; index++) { > > + title = ewk_back_forward_list_item_title_get(eina_list_nth(list, index)); > > + info(" title = %s", title); > > + elm_genlist_item_append(window->history.history_list, list_item, (void*)(title), NULL, ELM_GENLIST_ITEM_NONE, on_list_item_select, user_data); > > + } > > Why don't you use EINA_LIST_FOREACH ? > It's cheaper than the loop with eina_list_nth and usual way which EFL does. > Thanks for the suggestion, incorporated the changes. > > Tools/MiniBrowser/efl/main.c:1195 > > + if (forward_navigation_enabled) { > > + evas_object_move(window->history.history_box , x+TOOL_BAR_BUTTON_SIZE+1, y+TOOL_BAR_BUTTON_SIZE); > > + evas_object_move(window->history.history_list , x+TOOL_BAR_BUTTON_SIZE+1, y+TOOL_BAR_BUTTON_SIZE); > > + } else { > > + evas_object_move(window->history.history_box , x, y+TOOL_BAR_BUTTON_SIZE); > > + evas_object_move(window->history.history_list , x, y+TOOL_BAR_BUTTON_SIZE); > > + } > > space between + ? > Fixed. > > Tools/MiniBrowser/efl/main.c:1213 > > + forward_navigation_enabled = EINA_TRUE; > > Should we keep this as static variable? > > Can we pass this as the argument of on_navigation_button_longpress_process ? We need to keep this as a static variable as it is being reset in history_list_hide() function which is called from key and mouse event callbacks.So, I would like to retain the same. Comment on attachment 236858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236858&action=review > Tools/MiniBrowser/efl/main.c:992 > + if (forward_navigation_enabled) > + history_items = ewk_back_forward_list_n_forward_items_copy(list, count); > + else > + history_items = ewk_back_forward_list_n_back_items_copy(list, count); ewk_back_forward_list_{back|forward}_items_copy looks better. (without _n_ and count) > Tools/MiniBrowser/efl/main.c:1133 > +list_item_label_get(void *data, Evas_Object *obj, const char* part) > +{ > + int len = strlen((char*)data); > + char* buf = (char*)malloc(sizeof(char)*(len+1)); The position of * is different. > Tools/MiniBrowser/efl/main.c:1134 > + snprintf(buf, len+1, "%s", (char*)data); space between + > Tools/MiniBrowser/efl/main.c:1145 > + ewk_view_navigate_to(window->ewk_view, eina_list_nth(history_list_get(user_data), item_index - 1)); With you comment, I understood that forward_navigation_enabled looks required because of this line. I think that we should not do this. While showing history box, ewk_view can navigate back or forward so history can be changed. So, item_index might not be same with newly allocated history_items. Instead, I think that we should pass the Ewk_Back_Forward_List_Item as user_data. I'll leave the comment more about it below. > Tools/MiniBrowser/efl/main.c:1159 > + Eina_List* list = history_list_get(user_data); Leak. ( and check the position of *) Below is doxygen of ewk_back_forward_list_n_back_items_copy. * @return @c Eina_List containing @c Ewk_Back_Forward_List_Item elements or @c NULL in case of error, * the Eina_List and its items should be freed after use. Use ewk_object_unref() * to free the items > Tools/MiniBrowser/efl/main.c:1161 > + const Eina_List* l; > + void* data; The position of * > Tools/MiniBrowser/efl/main.c:1186 > + elm_genlist_item_append(window->history.history_list, list_item, (void*)(title), NULL, ELM_GENLIST_ITEM_NONE, on_list_item_select, user_data); Instead of passing user_data(which is Browser_Window), we should pass Ewk_Back_Forward_List_Item after called evas_object_ref(). I think that user_data can be passed to the callbacks using evas_object_data_set() Created attachment 236867 [details]
Patch
Created attachment 236868 [details]
Patch
(In reply to comment #11) > (From update of attachment 236858 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236858&action=review > > > Tools/MiniBrowser/efl/main.c:992 > > + if (forward_navigation_enabled) > > + history_items = ewk_back_forward_list_n_forward_items_copy(list, count); > > + else > > + history_items = ewk_back_forward_list_n_back_items_copy(list, count); > > ewk_back_forward_list_{back|forward}_items_copy looks better. (without _n_ and count) > Done. > > Tools/MiniBrowser/efl/main.c:1133 > > +list_item_label_get(void *data, Evas_Object *obj, const char* part) > > +{ > > + int len = strlen((char*)data); > > + char* buf = (char*)malloc(sizeof(char)*(len+1)); > > The position of * is different. > Done. > > Tools/MiniBrowser/efl/main.c:1134 > > + snprintf(buf, len+1, "%s", (char*)data); > > space between + > Done. > > Tools/MiniBrowser/efl/main.c:1145 > > + ewk_view_navigate_to(window->ewk_view, eina_list_nth(history_list_get(user_data), item_index - 1)); > > With you comment, I understood that forward_navigation_enabled looks required because of this line. > > I think that we should not do this. > While showing history box, ewk_view can navigate back or forward so history can be changed. > > So, item_index might not be same with newly allocated history_items. > > Instead, I think that we should pass the Ewk_Back_Forward_List_Item as user_data. > > I'll leave the comment more about it below. > Modified. More details below. > > Tools/MiniBrowser/efl/main.c:1159 > > + Eina_List* list = history_list_get(user_data); > > Leak. ( and check the position of *) > > Below is doxygen of ewk_back_forward_list_n_back_items_copy. > > * @return @c Eina_List containing @c Ewk_Back_Forward_List_Item elements or @c NULL in case of error, > * the Eina_List and its items should be freed after use. Use ewk_object_unref() > * to free the items > Using EINA_LIST_FREE to free the list and data. > > Tools/MiniBrowser/efl/main.c:1161 > > + const Eina_List* l; > > + void* data; > > The position of * > Done. > > Tools/MiniBrowser/efl/main.c:1186 > > + elm_genlist_item_append(window->history.history_list, list_item, (void*)(title), NULL, ELM_GENLIST_ITEM_NONE, on_list_item_select, user_data); > > Instead of passing user_data(which is Browser_Window), we should pass Ewk_Back_Forward_List_Item after called evas_object_ref(). > > I think that user_data can be passed to the callbacks using evas_object_data_set() Appending the Ewk_Back_Forward_List_Item instead of user_data. Passing the window pointer using evas_object_data_set. Now ewk_view_navigate_to() in on_list_item_select uses these values instead of index. Comment on attachment 236868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236868&action=review > Tools/MiniBrowser/efl/main.c:62 > +static Eina_Bool forward_navigation_enabled = EINA_FALSE; I still think that we don't need to keep this. > Tools/MiniBrowser/efl/main.c:122 > + Evas_Object *history_list; Keep the last allocated list. > Tools/MiniBrowser/efl/main.c:410 > +static void > +history_list_hide(Browser_Window *window) How about moving this function to near 1127 line ? > Tools/MiniBrowser/efl/main.c:416 > + release last allocated list here like below. EINA_LIST_FREE(list, data) { ewk_object_unref(data); } > Tools/MiniBrowser/efl/main.c:981 > +history_list_get(void *user_data) history_list_get(void *user_data, Eina_Bool forward) > Tools/MiniBrowser/efl/main.c:1134 > + char *buf = (char *)malloc(sizeof(char) * (len + 1)); > + snprintf(buf, len + 1, "%s", (char *)data); > + return strdup(buf); Who release the buf? > Tools/MiniBrowser/efl/main.c:1147 > +navigation_button_longpress_process(void *user_data) (void *user_data, Eina_Bool forward) > Tools/MiniBrowser/efl/main.c:1168 > + evas_object_ref(window->history.history_list); It's not what I wanted. history_list is always valid and no need to increase reference. > Tools/MiniBrowser/efl/main.c:1188 > + EINA_LIST_FREE(list, data); Like I memtioned above, I'd like to move this to history_list_hide() > Tools/MiniBrowser/efl/main.c:1211 > + evas_object_unref(window->history.history_list); Ditto. Comment on attachment 236868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236868&action=review >> Tools/MiniBrowser/efl/main.c:122 >> + Evas_Object *history_list; > > Keep the last allocated list. ewk_back_forward_list_XXX returns Eina_List and the items of that list are reference counted. It's because we should keep those items which webkit allocates internally although back forward list are changed. So, I think that we'd better to keep the list (with ref counted items). It will be released in history_list_hide(). (In reply to comment #16) > (From update of attachment 236868 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236868&action=review > > >> Tools/MiniBrowser/efl/main.c:122 > >> + Evas_Object *history_list; > > > > Keep the last allocated list. > > ewk_back_forward_list_XXX returns Eina_List and the items of that list are reference counted. > > It's because we should keep those items which webkit allocates internally although back forward list are changed. > > So, I think that we'd better to keep the list (with ref counted items). > It will be released in history_list_hide(). Thanks, from your explanation it looks like a valid concern. Since the list is reference counted we will maintain it in the window->history struct and dereference the items it in the history_list_hide() call. I will make the changes and update the patch. Created attachment 236979 [details]
Patch
(In reply to comment #18) > Created an attachment (id=236979) [details] > Patch Incorporated the comments. > Tools/MiniBrowser/efl/main.c:1134 > + char *buf = (char *)malloc(sizeof(char) * (len + 1)); > + snprintf(buf, len + 1, "%s", (char *)data); > + return strdup(buf); Who release the buf? From the EFL Genlist documentation the callback function is to return the text for the list item: "text_get - This function must return a strdup'()ed string, as the caller will free() it when done." This will be freed with the call to elm_genlist_clear(): "The application can clear the list with elm_genlist_clear() which deletes all the items in the list." I have removed the strdup since we are allocating buf anyway and strdup does the same. Comment on attachment 236979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236979&action=review Good, looks good to me (with nit) > Tools/MiniBrowser/efl/main.c:1188 > + elm_genlist_item_append(window->history.history_list, list_item, (void*)(title), NULL, ELM_GENLIST_ITEM_NONE, on_list_item_select, data); (void *) Created attachment 237078 [details]
Patch
(In reply to comment #21) > Created an attachment (id=237078) [details] > Patch looks good to me Comment on attachment 237078 [details]
Patch
r+ed based on me and Ryuan's review.
Comment on attachment 237078 [details] Patch Clearing flags on attachment: 237078 Committed r172923: <http://trac.webkit.org/changeset/172923> All reviewed patches have been landed. Closing bug. |