Bug 44258

Summary: [EFL] Webkit-EFL API which returns position of n-th text matches mark.
Product: WebKit Reporter: Kamil Blank <k.blank>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, commit-queue, gyuyoung.kim, leandro, lucas.de.marchi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 44417    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch2
none
WebCore::Document patch
none
WebKit-EFL patch
none
WebCore:DocumentMarkerController patch
none
WebKit-EFL patch
lucas.de.marchi: commit-queue-
patch none

Description Kamil Blank 2010-08-19 07:02:42 PDT
I need functionality which returns the position of n-th marker so I could be able to show it in browser.

First of all I needed WebCore::Document functionality which returns a vector of IntRects describing text matches markers.
Unfortunately Webcore::Document::renderedRectsForMarkers() returns only rects in the visible part of document.
I added new functionality WebCore::Document::firstRectsForMarkers() which returns needed vector. 
Prefix "first" means that function only return first IntRect for every marker.

I also added functionality to WebKit-EFL port using above functionality which returns the position of n-th text matches marker.
Comment 1 Kamil Blank 2010-08-19 07:34:01 PDT
Created attachment 64848 [details]
patch
Comment 2 Gyuyoung Kim 2010-08-19 16:51:01 PDT
(In reply to comment #1)
> Created an attachment (id=64848) [details]
> patch

Hello Kamil ,

Please request review and cq for this patch.
Comment 3 WebKit Review Bot 2010-08-19 22:56:13 PDT
Attachment 64848 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_frame.cpp:56:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Gyuyoung Kim 2010-08-19 23:06:23 PDT
Hello Kamil,

You can check if your patch has any style errors by "WebKitTools/Scripts/check-webkit-style" or "WebKitTools/Scripts/webkit-patch" scripts.

Current ewk_frame.cpp file has style errors as below,

gyuyoung@gyuyoung-desktop:~/webkit/WebKit-EFL$ WebKitTools/Scripts/check-webkit-style WebKit/efl/ewk/ewk_frame.cpp 
WebKit/efl/ewk/ewk_frame.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/efl/ewk/ewk_frame.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 1 files

If you can solve this errors, I think it is better to fix the style errors together.

Thanks,
Gyuyoung Kim
Comment 5 Kamil Blank 2010-08-19 23:42:26 PDT
Created attachment 64929 [details]
patch2

Removed style errors from ewk_frame.cpp
Comment 6 Rafael Antognolli 2010-08-20 15:11:19 PDT
Hi Kamil. I just did a quick look over the WebCore::Document part, and didn't see any problem, but I think some reviewer that understands deeply that part of code should take a look at it. So maybe you should find who recently contributed to it and add to cc too.

I'm not sure, but I also think that this should be split into two patches. One adding the Document::firstRectsForMarkers, and another one using this functionality inside WebKit-EFL.

And please, pay attention to the coding style in ewk_frame.cpp. It does use WebKit coding style (as all the c++ files inside ewk/). So the pointers should be declared as:

Evas_Object* o;

instead of

Evas_Object *o;

Regards,
Rafael
Comment 7 Kamil Blank 2010-08-22 23:41:48 PDT
Created attachment 65079 [details]
WebCore::Document patch
Comment 8 Kamil Blank 2010-08-22 23:43:12 PDT
Created attachment 65080 [details]
WebKit-EFL patch
Comment 9 Kamil Blank 2010-08-23 03:12:17 PDT
Created attachment 65094 [details]
WebCore:DocumentMarkerController patch

Due to last changes in WebCore::Document I had to move my changes from Document to DocumentMarkerController.
Comment 10 Kamil Blank 2010-08-23 03:13:46 PDT
Created attachment 65095 [details]
WebKit-EFL patch
Comment 11 Gyuyoung Kim 2010-08-23 03:46:26 PDT
I am not sure if two patches can be reviewed in a bug. In my opinion, it seems that DOM patch needs to be reviewed by other bug.
Comment 12 Kamil Blank 2010-08-23 05:34:56 PDT
I've created new bug containing changes in WebCore::DocumentMarkerController
https://bugs.webkit.org/show_bug.cgi?id=44417
Comment 13 Kamil Blank 2010-08-23 05:36:23 PDT
Comment on attachment 65094 [details]
WebCore:DocumentMarkerController patch

Patch moved to new bug:
https://bugs.webkit.org/show_bug.cgi?id=44417
Comment 14 Lucas De Marchi 2010-08-23 07:40:42 PDT
Comment on attachment 65095 [details]
WebKit-EFL patch

I'm not a reviewer, just an informal review:

> diff --git a/WebKit/efl/ewk/ewk_frame.cpp b/WebKit/efl/ewk/ewk_frame.cpp
> index 7a2af5a..a52ed19 100644
> --- a/WebKit/efl/ewk/ewk_frame.cpp
> +++ b/WebKit/efl/ewk/ewk_frame.cpp
[..]
> +/** 
> + * Comparison function used by ewk_frame_text_matches_nth_pos_get
> + */
> +static bool compIntRect(WebCore::IntRect i, WebCore::IntRect j)

This name is not good for 2 reasons:

1) It's meaningless. You are comparing rects for what? Give a name that tells what this function does. Since it's used by std::sort
2) Naming convention in this file is a bit different. We do not use CamelCase. My suggestion is: _ewk_frame_rect_cmp_less_than

About the arguments, you are missing the consts and you are passing them in stack when you could just use references. So, this is the signature of this function I'd like to see:

static bool _ewk_frame_rect_cmp_less_than(const WebCore::IntRect& i, const WebCore::IntRect& j)

> +{
> +    return (i.y() < j.y() || (i.y() == j.y() && i.x() < j.x()));
> +}   
> +    
> +/**
> + * Predicate used by ewk_frame_text_matches_nth_pos_get
> + */
> +static bool isNegativeValue(WebCore::IntRect i)

Same comment above for this one.

> +{
[...]

> +/**
> + * Get x, y position of n-th text match in frame
> + *
> + * @param o frame object where matches are highlighted.
> + * @param n index of element 
> + * @param x where to return x position 
> + * @param y where to return y position 
> + *
> + * @return @c EINA_TRUE on success, @c EINA_FALSE for failure - when no matches found or 
> + *         n bigger than search results.
> + */
> +Eina_Bool ewk_frame_text_matches_nth_pos_get(Evas_Object* o, int n, int* x, int* y)
> +{
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(sd->frame, EINA_FALSE);

In exported function you must check all incoming pointers if you are going to modify their content. So:
    EINA_SAFETY_ON_NULL_RETURN_VAL(x, EINA_FALSE);
    EINA_SAFETY_ON_NULL_RETURN_VAL(y, EINA_FALSE);

> +
> +    Vector<WebCore::IntRect> intRects = sd->frame->document()->markers()->firstRectsForMarkers(WebCore::DocumentMarker::TextMatch);
> +
> +    /* remove useless values */
> +    std::remove_if(intRects.begin(), intRects.end(), isNegativeValue);
> +
> +    if (intRects.isEmpty() || n > intRects.size())
> +      return EINA_FALSE;
> +
> +    std::sort(intRects.begin(), intRects.end(), compIntRect);
> +
> +    *x = intRects[n - 1].x();
Otherwise, you could receive a segv here.

> +    *y = intRects[n - 1].y();
and here.
Comment 15 Rafael Antognolli 2010-08-23 09:28:06 PDT
(In reply to comment #14)
> In exported function you must check all incoming pointers if you are going to modify their content. So:
>     EINA_SAFETY_ON_NULL_RETURN_VAL(x, EINA_FALSE);
>     EINA_SAFETY_ON_NULL_RETURN_VAL(y, EINA_FALSE);
> 
> > +
> > +    Vector<WebCore::IntRect> intRects = sd->frame->document()->markers()->firstRectsForMarkers(WebCore::DocumentMarker::TextMatch);
> > +
> > +    /* remove useless values */
> > +    std::remove_if(intRects.begin(), intRects.end(), isNegativeValue);
> > +
> > +    if (intRects.isEmpty() || n > intRects.size())
> > +      return EINA_FALSE;
> > +
> > +    std::sort(intRects.begin(), intRects.end(), compIntRect);
> > +
> > +    *x = intRects[n - 1].x();
> Otherwise, you could receive a segv here.
> 
> > +    *y = intRects[n - 1].y();
> and here.

Alternatively, you can check if the pointer is not NULL before assigning to it. This would make the parameters not obligatory:

if (x)
    *x = intRects[n - 1].x();

Same for y.
Comment 16 Kamil Blank 2010-08-24 01:20:35 PDT
Created attachment 65228 [details]
patch

Modified regarding to Lucas and Rafael comments.
Comment 17 Lucas De Marchi 2010-08-24 04:20:34 PDT
(In reply to comment #16)
> Created an attachment (id=65228) [details]
> patch
> 
> Modified regarding to Lucas and Rafael comments.

lgtm, although I'm wondering if this shouldn't go to WebCore so other platforms benefit from it. 

Antonio, what do you think?
Comment 18 Adam Barth 2010-08-30 16:35:17 PDT
Comment on attachment 65228 [details]
patch

> WebKit/efl/ewk/ewk_frame.cpp:830
> +static bool _ewk_frame_rect_cmp_less_than(const WebCore::IntRect& i, const WebCore::IntRect& j)
> +{
> +    return (i.y() < j.y() || (i.y() == j.y() && i.x() < j.x()));
> +}   
We generally frown upon one letter variable names.
Comment 19 WebKit Commit Bot 2010-08-30 22:06:49 PDT
Comment on attachment 65228 [details]
patch

Clearing flags on attachment: 65228

Committed r66454: <http://trac.webkit.org/changeset/66454>
Comment 20 WebKit Commit Bot 2010-08-30 22:06:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Antonio Gomes 2010-08-31 09:15:36 PDT
(In reply to comment #20)
> All reviewed patches have been landed.  Closing bug.

Does not it broke the EFL build  since the WebCore part of the patch/feature did not land yet?
Comment 22 Rafael Antognolli 2010-08-31 09:19:03 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > All reviewed patches have been landed.  Closing bug.
> 
> Does not it broke the EFL build  since the WebCore part of the patch/feature did not land yet?

Yes, it was broken, but the fix in http://trac.webkit.org/changeset/66488 fixes the build. I'm looking at the implementation right now, but it seems that the functionality in both functions is almost the same.