A function to obtain current WebCore::VisibleSelection text selection type.
Created attachment 98737 [details] proposed patch
Comment on attachment 98737 [details] proposed patch Is it up for review? I am wondering how would this API be used?
Comment on attachment 98737 [details] proposed patch It seems Michal miss to request review and cq. I request them instead of him.
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.
(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.
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'?
> 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.
Created attachment 98929 [details] second patch set
(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.
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;
Created attachment 99049 [details] another patch
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
LGTM.
Comment on attachment 99049 [details] another patch LGTM
Comment on attachment 99049 [details] another patch Clearing flags on attachment: 99049 Committed r90015: <http://trac.webkit.org/changeset/90015>
All reviewed patches have been landed. Closing bug.