ewebkit2 does not support extensions such as DOM and JS binding. This is to introduce EWebKit_Extension, basic structure for these functionalities.
Created attachment 234910 [details] Patch
Created attachment 235204 [details] Patch
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.
Created attachment 235207 [details] Patch
(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 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.
Created attachment 235212 [details] Patch
(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.
Created attachment 235217 [details] Patch
(In reply to comment #9) > Created an attachment (id=235217) [details] > Patch I added an option (-x) for MiniBrowser to test.
Created attachment 236643 [details] Patch
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 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.
Created attachment 237149 [details] Patch
Created attachment 237470 [details] rebased
Created attachment 238465 [details] Patch
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.
(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.
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 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.
Created attachment 238694 [details] Patch
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 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 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.
Created attachment 239092 [details] Patch
(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 on attachment 239092 [details] Patch LGTM. However, jinwoo might want to have final look.
Comment on attachment 239092 [details] Patch Clearing flags on attachment: 239092 Committed r174335: <http://trac.webkit.org/changeset/174335>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 137515
Created attachment 239603 [details] fixed layouttest crash
Comment on attachment 239603 [details] fixed layouttest crash r+ed again. Please check efl buildbot after landing.
Comment on attachment 239603 [details] fixed layouttest crash Clearing flags on attachment: 239603 Committed r174636: <http://trac.webkit.org/changeset/174636>