Bug 44258 – [EFL] Webkit-EFL API which returns position of n-th text matches mark.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kamil Blank]

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.

[Kamil Blank]

Created attachment 64848 [details]
patch

[Gyuyoung Kim]

(In reply to comment #1)
> Created an attachment (id=64848) [details]
> patch

Hello Kamil ,

Please request review and cq for this patch.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

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

[Kamil Blank]

Created attachment 64929 [details]
patch2

Removed style errors from ewk_frame.cpp

[Rafael Antognolli]

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

[Kamil Blank]

Created attachment 65079 [details]
WebCore::Document patch

[Kamil Blank]

Created attachment 65080 [details]
WebKit-EFL patch

[Kamil Blank]

Created attachment 65094 [details]
WebCore:DocumentMarkerController patch

Due to last changes in WebCore::Document I had to move my changes from Document to DocumentMarkerController.

[Kamil Blank]

Created attachment 65095 [details]
WebKit-EFL patch

[Gyuyoung Kim]

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.

[Kamil Blank]

I've created new bug containing changes in WebCore::DocumentMarkerController
https://bugs.webkit.org/show_bug.cgi?id=44417

[Kamil Blank]

Comment on attachment 65094 [details]
WebCore:DocumentMarkerController patch

Patch moved to new bug:
https://bugs.webkit.org/show_bug.cgi?id=44417

[Lucas De Marchi]

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.

[Rafael Antognolli]

(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.

[Kamil Blank]

Created attachment 65228 [details]
patch

Modified regarding to Lucas and Rafael comments.

[Lucas De Marchi]

(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?

[Adam Barth]

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.

[WebKit Commit Bot]

Comment on attachment 65228 [details]
patch

Clearing flags on attachment: 65228

Committed r66454: <http://trac.webkit.org/changeset/66454>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

(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?

[Rafael Antognolli]

(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.