Bug 134486 – [EFL][WK2] Minibrowser: Enhance the application to show text search count

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Shivakumar J M]

currently in many browsers the search count will be shown in browser search bar, so Enhance the application to show text search count. These will show case use on on_text_fond() call back as well. also add search close button as well.

[Shivakumar J M]

Created attachment 234153 [details]
Patch

Use 'text,found' callback to show search count.

[Shivakumar J M]

Created attachment 234154 [details]
searchbar-new-ui

image showing the new search bar ui

[Ryuan Choi]

Comment on attachment 234153 [details]
Patch

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

> Tools/ChangeLog:3
> +        [EFL][WK2] Minibrowser: Enhance the application to show text search count

Enhance the Minibrowser to show text search count and close search bar

> Tools/MiniBrowser/efl/main.c:1871
> +    window->search.search_field_count = elm_entry_add(window->elm_window);

Why do you use entry?

IMO, elm_label looks enough.
At least, it should be readonly, right?

> Tools/MiniBrowser/efl/main.c:1882
> +    window->search.close_button = create_toolbar_button(window->elm_window, "close");

Can we locate this button at right most position?

[Gyuyoung Kim]

Comment on attachment 234153 [details]
Patch

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

> Tools/MiniBrowser/efl/main.c:1700
> +    int *matchCount = (int*)(event_info);

s/matchCount/match_count/g

(int*)(event_info); -> (const int *)(event_info); ?

[Shivakumar J M]

Created attachment 234318 [details]
Patch-Updated-Review

Updated patch with review comments, used label to show search count text, moved close button to end.

[Shivakumar J M]

Created attachment 234319 [details]
Patch for landing

Updated patch with comments fixed.

[Shivakumar J M]

Created attachment 234320 [details]
searchbar-new-updated

image showing the new search bar ui

[Build Bot]

Comment on attachment 234319 [details]
Patch for landing

Attachment 234319 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5541842615533568

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 234323 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Ryuan Choi]

Comment on attachment 234319 [details]
Patch for landing

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

> Tools/MiniBrowser/efl/main.c:1703
> +    evas_object_focus_set(window->ewk_view, EINA_FALSE);
> +    elm_object_focus_set(window->search.search_field_count, EINA_TRUE);

Can I know why you touch the focus (and repeatedly)?

> Tools/MiniBrowser/efl/main.c:1715
> +    evas_object_focus_set(window->ewk_view, EINA_FALSE);
> +    elm_object_focus_set(window->search.search_field_count, EINA_FALSE);
> +    elm_object_focus_set(window->search.search_field, EINA_TRUE);

Ditto.

> Tools/MiniBrowser/efl/main.c:1881
> +    elm_object_text_set(window->search.search_field_count, " ");

Is space needed?

[Build Bot]

Comment on attachment 234319 [details]
Patch for landing

Attachment 234319 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4876152749424640

New failing tests:
media/media-fragments/TC0001.html

[Build Bot]

Created attachment 234324 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Shivakumar J M]

Created attachment 234326 [details]
Patch-Updated-Review2

Removed the space at line 1881, as we do not need it.
Removed the focus set at 1703, which is not needed.
some time while testing i observed, search field was not getting proper focus, so i added those lines at 1715. i.e after updating search text found count on label, set focus on search field.

[Gyuyoung Kim]

Comment on attachment 234326 [details]
Patch-Updated-Review2

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

LGTM except for nits.

> Tools/MiniBrowser/efl/main.c:1700
> +    const int *matchCount = (const int *)(event_info);

matchCount is not efl style. match_count ?

> Tools/MiniBrowser/efl/main.c:1963
> +    evas_object_smart_callback_add(window->ewk_view, "text,found", on_search_text_found, window);

Personally I prefer to place it alphabetically.

[Shivakumar J M]

Created attachment 234389 [details]
Patch for landing

[Gyuyoung Kim]

Comment on attachment 234389 [details]
Patch for landing

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

> Tools/ChangeLog:8
> +        Use 'text,found' callback to show search count.

I think you need to mention on_search_close_button_clicked() callback and on_search_text_found().

[Shivakumar J M]

Created attachment 234390 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 234390 [details]
Patch for landing

Clearing flags on attachment: 234390

Committed r170786: <http://trac.webkit.org/changeset/170786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.