Bug 100513 - [EFL][WK2] Get rid of C'ism in text checker API
Summary: [EFL][WK2] Get rid of C'ism in text checker API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-26 06:18 PDT by Chris Dumez
Modified: 2012-10-26 07:58 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.94 KB, patch)
2012-10-26 06:40 PDT, Chris Dumez
kenneth: review+
Details | Formatted Diff | Diff
Patch (21.90 KB, patch)
2012-10-26 07:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-10-26 06:18:53 PDT
The current text checker code (ewk_text_checker and WebKitTextChecker) is very C like although the rest of our Ewk code has been ported to C++ style already. We need to port the text checker code as well for consistency.
Comment 1 Chris Dumez 2012-10-26 06:40:23 PDT
Created attachment 170911 [details]
Patch
Comment 2 Mikhail Pozdnyakov 2012-10-26 06:55:32 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=170911&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:93
> +    Ewk_Text_Checker::initialize();

do not see any instance of Ewk_Text_Checker, maybe it could be just set of functions in a namespace?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:241
> +    Vector<String> languages = Ewk_Text_Checker::availableSpellCheckingLanguages();

const ref here?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:116
> +            WKRetainPtr<WKStringRef> suggestion(AdoptWK, WKStringCreateWithUTF8CString(static_cast<char*>(item)));

static_cast<const char*> ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:118
> +            free(item);

could we use OwnPtr<const char> as scope pointer here instead?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:121
> +        Vector<String> guesses = textCheckerEnchant()->getGuessesForWord(toImpl(word)->string());

const ref?
Comment 3 Chris Dumez 2012-10-26 07:07:51 PDT
Created attachment 170916 [details]
Patch

Take Mikhail's feedback into consideration. I'm using a namespace now.
Asking r? again since this is not a minor revision.
Comment 4 Kenneth Rohde Christiansen 2012-10-26 07:09:26 PDT
Comment on attachment 170916 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:211
> +
> +#define EWK_TEXT_CHECKER_CALLBACK_SET(TYPE_NAME, NAME)  \
> +void ewk_text_checker_##NAME##_cb_set(TYPE_NAME cb)     \
> +{                                                       \
> +    clientCallbacks().NAME = cb;      \
>  }

so these are all internal?
Comment 5 Chris Dumez 2012-10-26 07:49:29 PDT
Comment on attachment 170916 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:211
>>  }
> 
> so these are all internal?

I'm not sure I understand the question. This preexisting macro is used to generate the implementation for the public C API.
Comment 6 Kenneth Rohde Christiansen 2012-10-26 07:50:48 PDT
Comment on attachment 170916 [details]
Patch

Yeah sure. my mistake
Comment 7 WebKit Review Bot 2012-10-26 07:58:49 PDT
Comment on attachment 170916 [details]
Patch

Clearing flags on attachment: 170916

Committed r132657: <http://trac.webkit.org/changeset/132657>
Comment 8 WebKit Review Bot 2012-10-26 07:58:54 PDT
All reviewed patches have been landed.  Closing bug.