Bug 134921 - [EFL] Introduce EWebKit_Extension
Summary: [EFL] Introduce EWebKit_Extension
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: Ryuan Choi
URL:
Keywords:
Depends on: 137515
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-15 00:26 PDT by Ryuan Choi
Modified: 2014-10-12 00:33 PDT (History)
9 users (show)

See Also:


Attachments
Patch (41.14 KB, patch)
2014-07-15 01:51 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (39.77 KB, patch)
2014-07-20 22:51 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (39.69 KB, patch)
2014-07-20 23:44 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (39.72 KB, patch)
2014-07-21 01:50 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (42.08 KB, patch)
2014-07-21 06:53 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (43.23 KB, patch)
2014-08-15 00:02 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (43.25 KB, patch)
2014-08-26 07:34 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
rebased (43.23 KB, patch)
2014-09-01 17:24 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (45.23 KB, patch)
2014-09-22 01:27 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (45.41 KB, patch)
2014-09-25 21:19 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (45.60 KB, patch)
2014-10-02 01:55 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
fixed layouttest crash (45.61 KB, patch)
2014-10-10 00:12 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2014-07-15 00:26:19 PDT
ewebkit2 does not support extensions such as DOM and JS binding.

This is to introduce EWebKit_Extension, basic structure for these functionalities.
Comment 1 Ryuan Choi 2014-07-15 01:51:15 PDT
Created attachment 234910 [details]
Patch
Comment 2 Ryuan Choi 2014-07-20 22:51:36 PDT
Created attachment 235204 [details]
Patch
Comment 3 Gyuyoung Kim 2014-07-20 23:04:26 PDT
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.
Comment 4 Ryuan Choi 2014-07-20 23:44:27 PDT
Created attachment 235207 [details]
Patch
Comment 5 Ryuan Choi 2014-07-20 23:48:52 PDT
(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.
Comment 6 Gyuyoung Kim 2014-07-21 00:12:14 PDT
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.
Comment 7 Ryuan Choi 2014-07-21 01:50:06 PDT
Created attachment 235212 [details]
Patch
Comment 8 Ryuan Choi 2014-07-21 01:51:32 PDT
(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.
Comment 9 Ryuan Choi 2014-07-21 06:53:06 PDT
Created attachment 235217 [details]
Patch
Comment 10 Ryuan Choi 2014-07-21 06:54:09 PDT
(In reply to comment #9)
> Created an attachment (id=235217) [details]
> Patch

I added an option (-x) for MiniBrowser to test.
Comment 11 Ryuan Choi 2014-08-15 00:02:02 PDT
Created attachment 236643 [details]
Patch
Comment 12 Grzegorz Czajkowski 2014-08-26 02:46:40 PDT
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.
Comment 13 Ryuan Choi 2014-08-26 05:08:23 PDT
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.
Comment 14 Ryuan Choi 2014-08-26 07:34:53 PDT
Created attachment 237149 [details]
Patch
Comment 15 Ryuan Choi 2014-09-01 17:24:27 PDT
Created attachment 237470 [details]
rebased
Comment 16 Ryuan Choi 2014-09-22 01:27:14 PDT
Created attachment 238465 [details]
Patch
Comment 17 WebKit Commit Bot 2014-09-22 01:28:55 PDT
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.
Comment 18 Ryuan Choi 2014-09-22 01:38:13 PDT
(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.
Comment 19 Ryuan Choi 2014-09-22 03:44:19 PDT
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.
Comment 20 Gyuyoung Kim 2014-09-25 21:01:41 PDT
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.
Comment 21 Ryuan Choi 2014-09-25 21:19:44 PDT
Created attachment 238694 [details]
Patch
Comment 22 Ryuan Choi 2014-09-25 21:26:14 PDT
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.
Comment 23 Gyuyoung Kim 2014-10-01 22:18:03 PDT
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.
Comment 24 Ryuan Choi 2014-10-02 01:32:34 PDT
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.
Comment 25 Ryuan Choi 2014-10-02 01:55:40 PDT
Created attachment 239092 [details]
Patch
Comment 26 Gyuyoung Kim 2014-10-02 02:24:17 PDT
(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}\"

)
Comment 27 Gyuyoung Kim 2014-10-02 02:25:36 PDT
Comment on attachment 239092 [details]
Patch

LGTM. However, jinwoo might want to have final look.
Comment 28 Ryuan Choi 2014-10-05 17:47:38 PDT
Comment on attachment 239092 [details]
Patch

Clearing flags on attachment: 239092

Committed r174335: <http://trac.webkit.org/changeset/174335>
Comment 29 Ryuan Choi 2014-10-05 17:47:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 WebKit Commit Bot 2014-10-08 01:24:33 PDT
Re-opened since this is blocked by bug 137515
Comment 31 Ryuan Choi 2014-10-10 00:12:03 PDT
Created attachment 239603 [details]
fixed layouttest crash
Comment 32 Gyuyoung Kim 2014-10-10 22:42:41 PDT
Comment on attachment 239603 [details]
fixed layouttest crash

r+ed again. Please check efl buildbot after landing.
Comment 33 Ryuan Choi 2014-10-12 00:32:48 PDT
Comment on attachment 239603 [details]
fixed layouttest crash

Clearing flags on attachment: 239603

Committed r174636: <http://trac.webkit.org/changeset/174636>
Comment 34 Ryuan Choi 2014-10-12 00:33:02 PDT
All reviewed patches have been landed.  Closing bug.