Bug 63462 – [EFL] Add ewk_frame_text_selection_type_get function

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

[Michal Pakula vel Rutka]

A function to obtain current WebCore::VisibleSelection text selection type.

[Michal Pakula vel Rutka]

Created attachment 98737 [details]
proposed patch

[Antonio Gomes]

Comment on attachment 98737 [details]
proposed patch

Is it up for review? I am wondering how would this API be used?

[Gyuyoung Kim]

Comment on attachment 98737 [details]
proposed patch

It seems Michal miss to request review and cq. I request them instead of him.

[Gyuyoung Kim]

Comment on attachment 98737 [details]
proposed patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:1536
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, EWK_TEXT_SELECTION_NONE);

I'm not sure if we can use above usage. There is similar usage on Bug 63229. If there are no problems, I think this usage is good.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1537
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(sd->frame, EWK_TEXT_SELECTION_NONE);

ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1541
> +        return EWK_TEXT_SELECTION_NONE;

Adhere indentation. 

if (!controller)
     return XXX

> Source/WebKit/efl/ewk/ewk_frame.cpp:1547
> +        type = EWK_TEXT_SELECTION_CARET;

Adhere indentation. We need to use 4 spaces.

case WebCore::XXX
      type = EWK_XXX

> Source/WebKit/efl/ewk/ewk_frame.cpp:1550
> +        type = EWK_TEXT_SELECTION_RANGE;

ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1553
> +        type = EWK_TEXT_SELECTION_NONE;

ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1555
> +    default:

ditto.

> Source/WebKit/efl/ewk/ewk_frame.h:158
> +    EWK_TEXT_SELECTION_NONE,

ditto.

[Michal Pakula vel Rutka]

(In reply to comment #2)
> (From update of attachment 98737 [details])
> Is it up for review? I am wondering how would this API be used?

Sorry, forgot to set the flag as Gyuyoung said.
This API can be used by webview to determine text selection changes. Here: https://bugs.webkit.org/show_bug.cgi?id=63525 I proposed a method of informing webview of selection changes.

[Michal Pakula vel Rutka]

Comment on attachment 98737 [details]
proposed patch

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

>> Source/WebKit/efl/ewk/ewk_frame.cpp:1541
>> +        return EWK_TEXT_SELECTION_NONE;
> 
> Adhere indentation. 
> 
> if (!controller)
>      return XXX

I think that indentation here is OK, no tabs used, only spaces: 4 before if 8 before return

>> Source/WebKit/efl/ewk/ewk_frame.cpp:1547
>> +        type = EWK_TEXT_SELECTION_CARET;
> 
> Adhere indentation. We need to use 4 spaces.
> 
> case WebCore::XXX
>       type = EWK_XXX

So here do you want me to match 'type' indentation with 'case'?

[Raphael Kubo da Costa (:rakuco)]

> Source/WebKit/efl/ewk/ewk_frame.cpp:1543
> +    Ewk_Text_Selection_Type type;

You could return directly in the switch statement and get rid of this variable.

[Michal Pakula vel Rutka]

Created attachment 98929 [details]
second patch set

[Michal Pakula vel Rutka]

(In reply to comment #7)
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1543
> > +    Ewk_Text_Selection_Type type;
> 
> You could return directly in the switch statement and get rid of this variable.

Done.

I left case WebCore::VisibleSelection::NoSelection with the same return value as default one because of warnings when it is removed.

[Leandro Pereira]

Comment on attachment 98929 [details]
second patch set

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

> Source/WebKit/efl/ChangeLog:9
> +        This patch adds a method to obtain current WebCore::VisibleSelection
> +        text selection type.

No need to say 'This patch'; just begin the sentence with 'Add a method to...'.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1548
> +    case WebCore::VisibleSelection::NoSelection:

default:

> Source/WebKit/efl/ewk/ewk_frame.cpp:1552
> +    return EWK_TEXT_SELECTION_NONE;

Delete this line.

> Source/WebKit/efl/ewk/ewk_frame.h:162
> +enum _Ewk_Text_Selection_Type {
> +    EWK_TEXT_SELECTION_NONE,
> +    EWK_TEXT_SELECTION_CARET,
> +    EWK_TEXT_SELECTION_RANGE
> +};
> +typedef enum _Ewk_Text_Selection_Type Ewk_Text_Selection_Type;

typedef enum { ... } Ewk_Text_Selection_Type;

[Michal Pakula vel Rutka]

Created attachment 99049 [details]
another patch

[Michal Pakula vel Rutka]

Comment on attachment 98929 [details]
second patch set

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

>> Source/WebKit/efl/ChangeLog:9
>> +        text selection type.
> 
> No need to say 'This patch'; just begin the sentence with 'Add a method to...'.

fixed

>> Source/WebKit/efl/ewk/ewk_frame.cpp:1548
>> +    case WebCore::VisibleSelection::NoSelection:
> 
> default:

fixed

>> Source/WebKit/efl/ewk/ewk_frame.cpp:1552
>> +    return EWK_TEXT_SELECTION_NONE;
> 
> Delete this line.

fixed

>> Source/WebKit/efl/ewk/ewk_frame.h:162
>> +typedef enum _Ewk_Text_Selection_Type Ewk_Text_Selection_Type;
> 
> typedef enum { ... } Ewk_Text_Selection_Type;

fixed

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 99049 [details]
another patch

LGTM

[WebKit Review Bot]

Comment on attachment 99049 [details]
another patch

Clearing flags on attachment: 99049

Committed r90015: <http://trac.webkit.org/changeset/90015>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.