Bug 112122 - [EFL][MiniBrowser] Add a search field to the MiniBrowser
Summary: [EFL][MiniBrowser] Add a search field to the MiniBrowser
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: Jinwoo Song
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-12 02:49 PDT by Jinwoo Song
Modified: 2013-03-19 06:15 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.98 KB, patch)
2013-03-12 02:53 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.07 KB, patch)
2013-03-12 18:28 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2013-03-14 02:39 PDT, Jinwoo Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jinwoo Song 2013-03-12 02:49:00 PDT
Add a search field to test API ewk_view_text_find().
Comment 1 Jinwoo Song 2013-03-12 02:50:29 PDT
change the component to WebKit EFL.
Comment 2 Jinwoo Song 2013-03-12 02:53:41 PDT
Created attachment 192686 [details]
Patch
Comment 3 Build Bot 2013-03-12 10:20:46 PDT
Comment on attachment 192686 [details]
Patch

Attachment 192686 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17155178

New failing tests:
editing/selection/selection-modify-crash.html
Comment 4 Jinwoo Song 2013-03-12 18:28:26 PDT
Created attachment 192851 [details]
Patch

Add EWK_FIND_OPTIONS_WRAP_AROUND option in finding text.
Comment 5 Chris Dumez 2013-03-13 00:13:59 PDT
Comment on attachment 192851 [details]
Patch

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

Patch needs to be rebased.

> Tools/MiniBrowser/efl/main.c:96
> +    Evas_Object *search_bar;

Could we group all those 4 members in a "search" sub struct?

> Tools/MiniBrowser/efl/main.c:98
> +    Evas_Object *search_up_button;

search_backward_button?

> Tools/MiniBrowser/efl/main.c:99
> +    Evas_Object *search_down_button;

search_forward_button?

> Tools/MiniBrowser/efl/main.c:280
> +    elm_object_focus_set(elm_object_top_widget_get(window->elm_window), EINA_FALSE);

Is this line really needed?

> Tools/MiniBrowser/efl/main.c:356
> +        search_box_hide(window);

I would exit fullscreen OR hide search but not both at the same time. How about the following:
1. If search box is displayed, hide it and abort.
2. If search box is hidden and window is fullscreen, exit fullscreen.

> Tools/MiniBrowser/efl/main.c:671
> +    ewk_view_text_find(window->ewk_view, text, EWK_FIND_OPTIONS_SHOW_HIGHLIGHT | EWK_FIND_OPTIONS_CASE_INSENSITIVE | EWK_FIND_OPTIONS_WRAP_AROUND, 100);

Maybe we could add a global constant for this 100 magic value.
Also maybe we could keep "EWK_FIND_OPTIONS_SHOW_HIGHLIGHT | EWK_FIND_OPTIONS_CASE_INSENSITIVE | EWK_FIND_OPTIONS_WRAP_AROUND" in a global "DEFAULT_SEARCH_FLAGS" constant to simplify the code a bit.

> Tools/MiniBrowser/efl/main.c:710
> +on_search_up_button_clicked(void *user_data, Evas_Object *search_up_button, void *event_info)

on_search_backward_button_clicked?

> Tools/MiniBrowser/efl/main.c:720
> +on_search_down_button_clicked(void *user_data, Evas_Object *search_down_button, void *event_info)

on_search_forward_button_clicked?

> Tools/MiniBrowser/efl/main.c:1160
> +on_window_resize(void *user_data, Evas *e, Evas_Object *elm_window, void *event_info)

This should really not be needed if we do the layout properly.

> Tools/MiniBrowser/efl/main.c:1203
> +    evas_object_event_callback_add(window->elm_window, EVAS_CALLBACK_RESIZE, on_window_resize, window);

Ditto.
Comment 6 Jinwoo Song 2013-03-14 02:39:39 PDT
Created attachment 193095 [details]
Patch

Rebased the patch and applied Christophe's comments. Thanks for review. :)
Comment 7 Kenneth Rohde Christiansen 2013-03-14 14:06:06 PDT
Comment on attachment 193095 [details]
Patch

r=me but check with chris before landing
Comment 8 Chris Dumez 2013-03-14 14:13:46 PDT
Comment on attachment 193095 [details]
Patch

Looks fine, thanks.
Comment 9 WebKit Review Bot 2013-03-14 17:55:02 PDT
Comment on attachment 193095 [details]
Patch

Clearing flags on attachment: 193095

Committed r145863: <http://trac.webkit.org/changeset/145863>
Comment 10 WebKit Review Bot 2013-03-14 17:55:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Mikhail Pozdnyakov 2013-03-19 05:54:22 PDT
Looks like it brings regression: a Gray stripe at the bottom of the window when launch mini browser. Will look at this to find more details..
Comment 12 Sudarsana Nagineni (babu) 2013-03-19 06:15:23 PDT
(In reply to comment #11)
> Looks like it brings regression: a Gray stripe at the bottom of the window when launch mini browser. Will look at this to find more details..

I also noticed the same issue. Created a bug about it bug# 112687.