Bug 134921 – [EFL] Introduce EWebKit_Extension

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

[Ryuan Choi]

ewebkit2 does not support extensions such as DOM and JS binding.

This is to introduce EWebKit_Extension, basic structure for these functionalities.

[Ryuan Choi]

Created attachment 234910 [details]
Patch

[Ryuan Choi]

Created attachment 235204 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 235204 [details]
Patch

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

> Source/WebKit2/PlatformEfl.cmake:405
> +    "${CMAKE_CURRENT_SOURCE_DIR}/WebProcess/InjectedBundle/API/efl/ewk_extension.h"

Why ewk_extension.h should be opened ? EWebKit_Extension.h includes it.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:161
> +    if (!bundlePath)

Is it possible bundlePath can be null ? injectedBundlePath() returns String() at least.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:61
> +#include "ewk_extension.h"

Do you have a plan to add new header file to this file ?

> Source/WebKit2/WebProcess/efl/WebEflInjectedBundleMain.cpp:33
> +#if defined(WIN32) || defined(_WIN32)

Should we support win 32 arch ?

> Tools/Scripts/webkitpy/common/config/watchlist:111
> +                        r"|Source/WebKit2/UIProcess/API/efl/"

Missing a comma(,) at the end of line.

[Ryuan Choi]

Created attachment 235207 [details]
Patch

[Ryuan Choi]

(In reply to comment #3)
> (From update of attachment 235204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235204&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:405
> > +    "${CMAKE_CURRENT_SOURCE_DIR}/WebProcess/InjectedBundle/API/efl/ewk_extension.h"
> 
> Why ewk_extension.h should be opened ? EWebKit_Extension.h includes it.
> 

Like EwebKit2.h, EWebKit_Extension.h will be used master header file.
ewk_extension.h is one of files for extensions.
I will add others in different bug.

> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:161
> > +    if (!bundlePath)
> 
> Is it possible bundlePath can be null ? injectedBundlePath() returns String() at least.
> 
right, I fixed.

> > Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:61
> > +#include "ewk_extension.h"
> 
> Do you have a plan to add new header file to this file ?
> 
At least, ewk_page.h will be added.

> > Source/WebKit2/WebProcess/efl/WebEflInjectedBundleMain.cpp:33
> > +#if defined(WIN32) || defined(_WIN32)
> 
> Should we support win 32 arch ?
> 
OK, I will remove it.

[Gyuyoung Kim]

Comment on attachment 235207 [details]
Patch

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

Basically I can understand what do you want. However, I need to take a look this patch further. Let me comment soon again.

> Source/WebKit2/PlatformEfl.cmake:418
> +set(INJECTED_BUNDLE_INSTALL_DIR "${LIB_INSTALL_DIR}/ewebkit2-0/" CACHE PATH "Whether to install injected bundle")

Any reason you hardcoded ewebkit2-0 instead of ${WebKit2_OUTPUT_NAME}-${PROJECT_VERSION_MAJOR} ?

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:127
> +    String m_extensionsPath;

Need to add a new line.

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:2
> + * Copyright (C) 2012 Samsung Electronics

2012 -> 2012-2014 ?

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:52
> +    String extensionsDirectory = toImpl(static_cast<WKStringRef>(userData))->string();

I'm not sure if "toImpl(static_cast<WKStringRef>(userData))->string()" can be null. In gtk, it doesn't check it.

    CString userDataString = toImpl(static_cast<WKStringRef>(userData))->string().utf8();
    GRefPtr<GVariant> variant = g_variant_parse(nullptr, userDataString.data(),
        userDataString.data() + userDataString.length(), nullptr, nullptr);

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:72
> +

Unneeded line.

[Ryuan Choi]

Created attachment 235212 [details]
Patch

[Ryuan Choi]

(In reply to comment #6)
> (From update of attachment 235207 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235207&action=review
> 
> Basically I can understand what do you want. However, I need to take a look this patch further. Let me comment soon again.
> 

Sure, feel free to review.
I fixed all you mentioned.

[Ryuan Choi]

Created attachment 235217 [details]
Patch

[Ryuan Choi]

(In reply to comment #9)
> Created an attachment (id=235217) [details]
> Patch

I added an option (-x) for MiniBrowser to test.

[Ryuan Choi]

Created attachment 236643 [details]
Patch

[Grzegorz Czajkowski]

Comment on attachment 236643 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Although there are ewk_context_new_with_injected_bundle_path() in ewebkit2 APIs,

nit: are -> is

> Source/WebKit2/PlatformEfl.cmake:419
> +set(INJECTED_BUNDLE_INSTALL_DIR "${LIB_INSTALL_DIR}/${WebKit2_OUTPUT_NAME}-${PROJECT_VERSION_MAJOR}/" CACHE PATH "Whether to install injected bundle")

Whether -> Where ? (Absolute path to the extension library installation directory)

> Source/WebKit2/PlatformEfl.cmake:421
> +add_library(ewkInjectedBundle SHARED "${WEBKIT2_DIR}/WebProcess/efl/WebEflInjectedBundleMain.cpp")

WebEflInjectedBundleMain.cpp -> WebInjectedBundleMainEfl.cpp ? (similarly to WebProcessMainEfl.cpp)

> Source/WebKit2/PlatformEfl.cmake:465
> +set(TEST_INJECTED_BUNDLE_DIR ${WEBKIT2_EFL_TEST_DIR}/extensions)

I think we should decide the proper name for this feature as InjectedBundle and extension(s) are mixed in this patch? How about WebExtensions ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:138
> +    String bundlePath;
> +    static const char* bundleDirectory = getenv("WEBKIT_INJECTED_BUNDLE_PATH");

IMHO, bundlePath and bundleDirectory resemble each other. How about clientWebExtensionsPath ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:143
> +        if (WebCore::fileExists(bundlePath))
> +            return bundlePath;

I think it's worth notifying the user that his extension path is invalid, for example using WARN(..) macro.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:146
> +    bundlePath = WebCore::pathByAppendingComponent(String::fromUTF8(TEST_LIB_DIR), INJECTEDBUNDLENAME);

Why are we looking for the extension library in TEST_LIB_DIR (OUTPUT_DIR)? Seems odd to me.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:154
> +    return String();

nit: emptyString()

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:160
> +    String bundlePath = injectedBundlePath();
> +    if (bundlePath.isEmpty())

Why do we force own library path regardless of the client's extensionsPath parameter?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:163
> +    WKRetainPtr<WKStringRef> path = adoptWK(toCopiedAPI(bundlePath));

path -> webExtensionsPath ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:165
> +    return adoptRef(new EwkContext(adoptWK(WKContextCreateWithInjectedBundlePath(path.get())).get(), extensionsPath));

Please correct me If I am wrong, shouldn't we pass (the client) extensionsPath if it's valid? Otherwise injectedBundlePath() should determine the proper path if any.


> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:89
> +    String extensionsPath() { return m_extensionsPath; }

nit: const String&

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:2
> + * Copyright (C) 2012-2014 Samsung Electronics

Nit: 2014

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Please use BSD license.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:35
> +EwkExtension::EwkExtension(InjectedBundle* bundle)
> +{
> +}

Nit: Could has been defined in the header as it's empty.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:37
> +EwkExtension::~EwkExtension()

Ditto,

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:54
> +    EINA_SAFETY_ON_NULL_RETURN(extension);
> +    EINA_SAFETY_ON_NULL_RETURN(client);

Similarly to existing ewk APIs, can we change this API to notify the user about wrongly API usage by returning false?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:61
> +    EINA_SAFETY_ON_NULL_RETURN(extension);

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Please use BSD license.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:34
> +#include <Eina.h>

Nit: is eina really required here?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:50
> + * @brief  Type definition for the entry of the extension.

Can we add bundle and reserved parameters description as well.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:58
> +struct EwkExtensionClient {
> +    int version;
> +    void *data;
> +};

Is it possible to hide it in cpp unless the client needs it?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:67
> + * This function add a client, which contains several callbacks receiving events

Typo: add -> adds

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:68
> + * in WebProcess side, to a @p extension.

Typo: in -> from

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:77
> + * @param extension extension to delete client to

Typo: without "to"

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension_private.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Please use BSD license.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Please use BSD license.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:46
> +ExtensionManagerEfl::ExtensionManagerEfl()
> +{
> +}

How about moving it to the header?

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:50
> +    m_extension = std::make_unique<EwkExtension>(toImpl(bundle));

how about preventing it from accidental second initialization:

static bool initialized = false;
if (initialized)
    return;
.
.
.
initalized = true;

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Please use BSD license.

[Ryuan Choi]

Comment on attachment 236643 [details]
Patch

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

First, thanks for your comment.
I will uplodate new patch after followed the most of your comments.
(Leave some inline comments)

And, I agree that we are confusing about the extension and injected bundle.
Now, I am considering like below.

extension : the shared object library which user creates, it should contain the implementation of ewk_extension_init() for the entry point.
ewebkit_extension (webkit_extension) : Injected bundle library which ewebkit installs and load and calls ewk_extension_init() for the `extension` which user creates.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:146
>> +    bundlePath = WebCore::pathByAppendingComponent(String::fromUTF8(TEST_LIB_DIR), INJECTEDBUNDLENAME);
> 
> Why are we looking for the extension library in TEST_LIB_DIR (OUTPUT_DIR)? Seems odd to me.

It's for the webkit developers who does not install ewebkit and extensions.

For example, build-webkit generate INJECTEDBUNDLENAME into WebKitBuild/{Release|Debug}/lib.

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:34
>> +#include <Eina.h>
> 
> Nit: is eina really required here?

At least we should include it somewhere to use EAPI or other eina structure/macros.

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:58
>> +};
> 
> Is it possible to hide it in cpp unless the client needs it?

This structure is just skeleton now, but will expose several callbacks in near future.

[Ryuan Choi]

Created attachment 237149 [details]
Patch

[Ryuan Choi]

Created attachment 237470 [details]
rebased

[Ryuan Choi]

Created attachment 238465 [details]
Patch

[WebKit Commit Bot]

Attachment 238465 [details] did not pass style-queue:


Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/patchreader.py", line 83, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/filereader.py", line 124, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/checker.py", line 905, in process
    checker.check(lines)
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/checkers/watchlist.py", line 51, in check
    WatchListParser(log_error=log_to_style_error).parse('\n'.join(lines))
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py", line 70, in parse
    dictionary = self._eval_watch_list(watch_list_contents)
  File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py", line 86, in _eval_watch_list
    return eval(watch_list_contents, {'__builtins__': None}, None)
  File "<string>", line 210
    r"|Tools/Scripts/webkitpy/replay",
                                     ^
SyntaxError: invalid syntax


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryuan Choi]

(In reply to comment #17)
> Attachment 238465 [details] did not pass style-queue:
> 
> 
> Traceback (most recent call last):
>   File "Tools/Scripts/check-webkit-style", line 48, in <module>
>     sys.exit(CheckWebKitStyle().main())
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
>     patch_checker.check(patch)
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/patchreader.py", line 83, in check
>     self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/filereader.py", line 124, in process_file
>     self._processor.process(lines, file_path, **kwargs)
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/checker.py", line 905, in process
>     checker.check(lines)
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/style/checkers/watchlist.py", line 51, in check
>     WatchListParser(log_error=log_to_style_error).parse('\n'.join(lines))
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py", line 70, in parse
>     dictionary = self._eval_watch_list(watch_list_contents)
>   File "/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py", line 86, in _eval_watch_list
>     return eval(watch_list_contents, {'__builtins__': None}, None)
>   File "<string>", line 210
>     r"|Tools/Scripts/webkitpy/replay",
>                                      ^
> SyntaxError: invalid syntax
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

It is not related to this bug.

I uploaded 136996 to fix it.

[Ryuan Choi]

I updated the name of InjectedBundle to extension manager.

So,
extension manager means injected bundle which controls user's extensions.
extension is user library to extend webcore's functionality.

[Gyuyoung Kim]

Comment on attachment 238465 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        EwebKit2 does not provide the functionality to extend WebProcess.

EwebKit2 -> EWebKit2.

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:191
> +EAPI Ewk_Context *ewk_context_new_with_extensions_path(const char *path);

Wrong * place in (const char *path);

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:2
> + * Copyright (C) 2014 Samsung Electronics

Please add "All right reserved."

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.cpp:2
> + * Copyright (C) 2014 Samsung Electronics

ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:2
> + * Copyright (C) 2014 Samsung Electronics

ditto.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:40
> +typedef struct EwkPage Ewk_Page;

I wonder where Ewk_Page is used.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension_private.h:2
> + * Copyright (C) 2014 Samsung Electronics

ditto.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:2
> + * Copyright (C) 2014 Samsung Electronics

ditto.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:55
> +    ASSERT(!extensionsDirectory.isEmpty());

ASSERT() should be used when condition should not be something. It looks extensionsDirectory can be empty because the string is passed by WKBundleInitialize(). Thus it seems to me that it would be good if you check it with "if ~ return" statement.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.h:2
> + * Copyright (C) 2014 Samsung Electronics

ditto.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.h:42
> +

No necessary line.

> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.h:49
> +    Vector<WebKit::Module*> m_extensionModules;

Looks unnecessary namespace use. Vector<Module*> is enough to here.

> Source/WebKit2/WebProcess/efl/WebInjectedBundleMainEfl.cpp:2
> + * Copyright (C) 2014 Samsung Electronics

ditto.

> Source/WebKit2/efl/ewebkit2-extension.pc.in:4
> +Name: ewebkit2-extension

What is benefit to add additional .pc.in file ? It looks application developer needs to include this pc file in order to use this extension feature additionally.

> Tools/Scripts/webkitpy/style/checker.py:193
> +      "Source/WebKit2/WebProcess/InjectedBundle/API/efl/",

Wrong alphabet order.

[Ryuan Choi]

Created attachment 238694 [details]
Patch

[Ryuan Choi]

Comment on attachment 238465 [details]
Patch

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

>> Source/WebKit2/ChangeLog:8
>> +        EwebKit2 does not provide the functionality to extend WebProcess.
> 
> EwebKit2 -> EWebKit2.

Done.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.h:191
>> +EAPI Ewk_Context *ewk_context_new_with_extensions_path(const char *path);
> 
> Wrong * place in (const char *path);

It's right because this is public header.

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:2
>> + * Copyright (C) 2014 Samsung Electronics
> 
> Please add "All right reserved."

Done, and I changed copyright by my name because I am not in samsung now.

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:40
>> +typedef struct EwkPage Ewk_Page;
> 
> I wonder where Ewk_Page is used.

Removed, it will be introduced in next patch for javascript binding and so on.

>> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.cpp:55
>> +    ASSERT(!extensionsDirectory.isEmpty());
> 
> ASSERT() should be used when condition should not be something. It looks extensionsDirectory can be empty because the string is passed by WKBundleInitialize(). Thus it seems to me that it would be good if you check it with "if ~ return" statement.

OK. although it should not be empty when we uses extension manager, it looks better not to make crash in debug build.

>> Source/WebKit2/WebProcess/efl/ExtensionManagerEfl.h:49
>> +    Vector<WebKit::Module*> m_extensionModules;
> 
> Looks unnecessary namespace use. Vector<Module*> is enough to here.

Done.

>> Source/WebKit2/efl/ewebkit2-extension.pc.in:4
>> +Name: ewebkit2-extension
> 
> What is benefit to add additional .pc.in file ? It looks application developer needs to include this pc file in order to use this extension feature additionally.

Application developer should distinguish ewebkit2.pc and ewebkit2-extension.pc
The former is to make application which uses ewebkit2 and the latter is to make shared object, extension.

Applications should not include the both because the former is valid only in UIProcess while the latter is valid in the WebProcess.

>> Tools/Scripts/webkitpy/style/checker.py:193
>> +      "Source/WebKit2/WebProcess/InjectedBundle/API/efl/",
> 
> Wrong alphabet order.

Done.

[Gyuyoung Kim]

Comment on attachment 238694 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:137
> +    String bundlePathForExtension = WebCore::pathByAppendingComponent(String::fromUTF8(TEST_LIB_DIR), EXTENSIONMANAGERNAME);

TEST_LIB_DIR looks like we only use it for EFL port test. Can't we use better name for it ?

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:33
> +{

notImplemented() ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:2
> + * Copyright (C) 2014 Samsung Electronics

Samsung copyright ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:50
> + * @param extension the extension instance helps the communication between WebProcess and your extension library.

Remove a period at the end of line.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:51
> + * @param reserved extra parameter for the future. NULL is fine, now.

If *reserved* is not used now, isn't it keep it as "void*" ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:63
> + * Add (register) a client which contains a several callbacks to the extension.

I don't know what "(register)" means in this sentence.

a several callbacks -> several callbacks.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:68
> + * This function add a client, which contains several callbacks receiving events

add -> adds.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_extension.h:79
> + * @param client client which contains version, data and callbacks.

ditto.

> Source/WebKit2/WebProcess/efl/WebInjectedBundleMainEfl.cpp:2
> + * Copyright (C) 2014 Ryuan Choi.  All rights reserved.

Missing your email address.

[Ryuan Choi]

Comment on attachment 238694 [details]
Patch

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

I will update patch with you comments. thanks.

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:137
>> +    String bundlePathForExtension = WebCore::pathByAppendingComponent(String::fromUTF8(TEST_LIB_DIR), EXTENSIONMANAGERNAME);
> 
> TEST_LIB_DIR looks like we only use it for EFL port test. Can't we use better name for it ?

Any good name suggestion?

I will consider it.

Anyway, this code is only for webkit developers who does not install ewebkit but want to test.

>> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:33
>> +{
> 
> notImplemented() ?

Because this is in C area, we can't use notImplemented().

>> Source/WebKit2/WebProcess/InjectedBundle/API/efl/EWebKit_Extension.h:2
>> + * Copyright (C) 2014 Samsung Electronics
> 
> Samsung copyright ?

Sorry, I missed.

[Ryuan Choi]

Created attachment 239092 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #24)
> (From update of attachment 238694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238694&action=review
> 
> I will update patch with you comments. thanks.
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:137
> >> +    String bundlePathForExtension = WebCore::pathByAppendingComponent(String::fromUTF8(TEST_LIB_DIR), EXTENSIONMANAGERNAME);
> > 
> > TEST_LIB_DIR looks like we only use it for EFL port test. Can't we use better name for it ?
> 
> Any good name suggestion?
> 
> I will consider it.


TEST_LIB_DIR is defined in Source/WebKit2/PlatformEfl.cmake. To use it for WebKit2, I think we need to define new variable, for example, LIB_OUTPUT_DIR, EWEBKIT_LIB_DIR and so on. Then, TEST_LIB_DIR can use it as well.


add_definitions(-DTEST_RESOURCES_DIR=\"${TEST_RESOURCES_DIR}\"
    -DTEST_LIB_DIR=\"${CMAKE_LIBRARY_OUTPUT_DIRECTORY}\" =>    -DTEST_LIB_DIR=\"${EWEBKIT_LIB_DIR}\"

)

[Gyuyoung Kim]

Comment on attachment 239092 [details]
Patch

LGTM. However, jinwoo might want to have final look.

[Ryuan Choi]

Comment on attachment 239092 [details]
Patch

Clearing flags on attachment: 239092

Committed r174335: <http://trac.webkit.org/changeset/174335>

[Ryuan Choi]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 137515

[Ryuan Choi]

Created attachment 239603 [details]
fixed layouttest crash

[Gyuyoung Kim]

Comment on attachment 239603 [details]
fixed layouttest crash

r+ed again. Please check efl buildbot after landing.

[Ryuan Choi]

Comment on attachment 239603 [details]
fixed layouttest crash

Clearing flags on attachment: 239603

Committed r174636: <http://trac.webkit.org/changeset/174636>

[Ryuan Choi]

All reviewed patches have been landed.  Closing bug.