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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44258


Lucas De Marchi <demarchi at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #65095|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #14 from Lucas De Marchi <demarchi at webkit.org>  2010-08-23 07:40:42 PST ---
(From update of attachment 65095)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.