[Webkit-unassigned] [Bug 135795] [EFL][WK2] Minibrowser : Enhance application to be able to support history list navigation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135795





--- Comment #10 from Tanay <tanay.c at samsung.com>  2014-08-20 00:05:46 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.