Summary: | [EFL][WK2] Minibrowser: Enhance the application to show text search count | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shivakumar J M <shiva.jm> | ||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rniwa, sergio | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Shivakumar J M
2014-06-30 22:14:08 PDT
Created attachment 234153 [details]
Patch
Use 'text,found' callback to show search count.
Created attachment 234154 [details]
searchbar-new-ui
image showing the new search bar ui
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 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); ? Created attachment 234318 [details]
Patch-Updated-Review
Updated patch with review comments, used label to show search count text, moved close button to end.
Created attachment 234319 [details]
Patch for landing
Updated patch with comments fixed.
Created attachment 234320 [details]
searchbar-new-updated
image showing the new search bar ui
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 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 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 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 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
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 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. Created attachment 234389 [details]
Patch for landing
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(). Created attachment 234390 [details]
Patch for landing
Comment on attachment 234390 [details] Patch for landing Clearing flags on attachment: 234390 Committed r170786: <http://trac.webkit.org/changeset/170786> All reviewed patches have been landed. Closing bug. |