Bug 134486 - [EFL][WK2] Minibrowser: Enhance the application to show text search count
Summary: [EFL][WK2] Minibrowser: Enhance the application to show text search count
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-30 22:14 PDT by Shivakumar J M
Modified: 2014-07-03 22:38 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.02 KB, patch)
2014-07-01 03:21 PDT, Shivakumar J M
no flags Details | Formatted Diff | Diff
searchbar-new-ui (164.13 KB, image/png)
2014-07-01 03:50 PDT, Shivakumar J M
no flags Details
Patch-Updated-Review (6.78 KB, patch)
2014-07-02 22:38 PDT, Shivakumar J M
no flags Details | Formatted Diff | Diff
Patch for landing (6.78 KB, patch)
2014-07-02 22:48 PDT, Shivakumar J M
buildbot: commit-queue-
Details | Formatted Diff | Diff
searchbar-new-updated (905.69 KB, image/png)
2014-07-02 22:50 PDT, Shivakumar J M
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (522.48 KB, application/zip)
2014-07-02 23:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (539.12 KB, application/zip)
2014-07-03 01:01 PDT, Build Bot
no flags Details
Patch-Updated-Review2 (6.65 KB, patch)
2014-07-03 01:57 PDT, Shivakumar J M
no flags Details | Formatted Diff | Diff
Patch for landing (6.60 KB, patch)
2014-07-03 21:24 PDT, Shivakumar J M
no flags Details | Formatted Diff | Diff
Patch for landing (6.67 KB, patch)
2014-07-03 21:59 PDT, Shivakumar J M
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shivakumar J M 2014-06-30 22:14:08 PDT
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.
Comment 1 Shivakumar J M 2014-07-01 03:21:50 PDT
Created attachment 234153 [details]
Patch

Use 'text,found' callback to show search count.
Comment 2 Shivakumar J M 2014-07-01 03:50:25 PDT
Created attachment 234154 [details]
searchbar-new-ui

image showing the new search bar ui
Comment 3 Ryuan Choi 2014-07-02 01:42:40 PDT
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?
Comment 4 Gyuyoung Kim 2014-07-02 09:24:32 PDT
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); ?
Comment 5 Shivakumar J M 2014-07-02 22:38:13 PDT
Created attachment 234318 [details]
Patch-Updated-Review

Updated patch with review comments, used label to show search count text, moved close button to end.
Comment 6 Shivakumar J M 2014-07-02 22:48:17 PDT
Created attachment 234319 [details]
Patch for landing

Updated patch with comments fixed.
Comment 7 Shivakumar J M 2014-07-02 22:50:27 PDT
Created attachment 234320 [details]
searchbar-new-updated

image showing the new search bar ui
Comment 8 Build Bot 2014-07-02 23:54:21 PDT
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
Comment 9 Build Bot 2014-07-02 23:54:24 PDT
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
Comment 10 Ryuan Choi 2014-07-03 00:25:46 PDT
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?
Comment 11 Build Bot 2014-07-03 01:00:57 PDT
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
Comment 12 Build Bot 2014-07-03 01:01:02 PDT
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
Comment 13 Shivakumar J M 2014-07-03 01:57:35 PDT
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.
Comment 14 Gyuyoung Kim 2014-07-03 20:35:14 PDT
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.
Comment 15 Shivakumar J M 2014-07-03 21:24:38 PDT
Created attachment 234389 [details]
Patch for landing
Comment 16 Gyuyoung Kim 2014-07-03 21:31:43 PDT
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().
Comment 17 Shivakumar J M 2014-07-03 21:59:35 PDT
Created attachment 234390 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2014-07-03 22:38:45 PDT
Comment on attachment 234390 [details]
Patch for landing

Clearing flags on attachment: 234390

Committed r170786: <http://trac.webkit.org/changeset/170786>
Comment 19 WebKit Commit Bot 2014-07-03 22:38:51 PDT
All reviewed patches have been landed.  Closing bug.