Bug 61850

Summary:

[EFL][WK2] Add MiniBrowserEfl.

Product:

WebKit

Reporter:

Ryuan Choi <ryuan.choi>

Component:

WebKit2

Assignee:

Ryuan Choi <ryuan.choi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cshu, d-r, gyuyoung.kim, gyuyoung.kim, kenneth, leandro, ljaehun.lim, lucas.de.marchi, rakuco, sangseok.lim, sh919.park, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

61903, 84124

Bug Blocks:

61838

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch_changed_copyright	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
rebased	none
Patch	none
rebased to catch r120024	none
Fix mistake	none

Ryuan Choi

Reported 2011-06-01 02:45:38 PDT

Patch coming. It will work when all of WebKit2/EFL land.

Attachments
Patch (6.74 KB, patch) 2011-06-01 03:02 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (6.77 KB, patch) 2011-06-01 06:39 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (6.79 KB, patch) 2011-06-01 19:17 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (6.79 KB, patch) 2011-06-02 16:53 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (6.87 KB, patch) 2011-06-13 23:53 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch_changed_copyright (6.81 KB, patch) 2011-06-22 17:10 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (10.69 KB, patch) 2011-06-27 04:45 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (10.70 KB, patch) 2011-06-27 04:58 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.06 KB, patch) 2011-12-29 22:46 PST, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (6.96 KB, patch) 2012-01-01 20:25 PST, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.06 KB, patch) 2012-01-01 20:40 PST, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.04 KB, patch) 2012-01-01 20:46 PST, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.05 KB, patch) 2012-01-02 18:03 PST, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.70 KB, patch) 2012-04-16 20:27 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.36 KB, patch) 2012-04-17 02:19 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
rebased (7.26 KB, patch) 2012-06-03 23:37 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Patch (7.28 KB, patch) 2012-06-10 08:03 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
rebased to catch r120024 (7.23 KB, patch) 2012-06-11 18:22 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Fix mistake (7.22 KB, patch) 2012-06-11 21:31 PDT, Ryuan Choi	no flags	Details Formatted Diff Diff
Show Obsolete (18) View All Add attachment proposed patch, testcase, etc.

Ryuan Choi

Comment 1 2011-06-01 03:02:11 PDT

Created attachment 95572 [details] Patch

Gyuyoung Kim

Comment 2 2011-06-01 03:14:31 PDT

I think it is better to insert CMakeListEfl.txt into efl directory. MiniBrowser of gtk, qt have build script in its directory.

Lucas De Marchi

Comment 3 2011-06-01 04:21:05 PDT

Comment on attachment 95572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95572&action=review Once WebKit2 is compiled in EFL port, I think this can enter. I agree with Gyuyoung that the cmake file should be in efl directory. But we can do a bit better and split it in a generic CMakeLists.txt file and another efl/CMakeListsEfl.txt. This way other ports that use cmake can profit from this work too. > Tools/MiniBrowser/CMakeListsEfl.txt:15 > + ${WEBKIT2_DIR}/UIProcess/API/efl > + ${WEBKIT2_DIR} > + ${DERIVED_SOURCES_WEBKIT2_DIR}/include Since we don't support webkit2, are you sure you need this? > Tools/MiniBrowser/efl/main.c:55 > +static int browserCreate(const char *url, const char *engine) > +{ > + MiniBrowser *app = (MiniBrowser*) malloc(sizeof(MiniBrowser)); > + > + app->ee = ecore_evas_software_x11_new(NULL, 0, 0, 0, DEFAULT_WIDTH, DEFAULT_HEIGHT); Could you use ecore_evas_new() here instead? > Tools/MiniBrowser/efl/main.c:56 > + ecore_evas_title_set(app->ee, "Samsung MiniBrowser"); I think it's not good to let Samsung here. WebKit would be a better name. > Tools/MiniBrowser/efl/main.c:71 > + /* Create webview */ > + app->browser = ewk_view_add(app->evas, WKContextGetSharedProcessContext(), 0); It seems you are indeed using webkit2. I'm wondering what's happening since we don't have support to webkit2 yet. > Tools/MiniBrowser/efl/main.c:97 > + browserCreate(url, engine); > + > + ecore_event_handler_add(ECORE_EVENT_SIGNAL_EXIT, mainSignalExit, NULL); Since in this file we are following EFL style, I think it's better not to use camelCase for function names.

Lucas De Marchi

Comment 4 2011-06-01 05:19:10 PDT

(In reply to comment #3) > (From update of attachment 95572 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95572&action=review > > Once WebKit2 is compiled in EFL port, I think this can enter. I agree with Gyuyoung that the cmake file should be in efl directory. But we can do a bit better and split it in a generic CMakeLists.txt file and another efl/CMakeListsEfl.txt. This way other ports that use cmake can profit from this work too. > > > Tools/MiniBrowser/CMakeListsEfl.txt:15 > > + ${WEBKIT2_DIR}/UIProcess/API/efl > > + ${WEBKIT2_DIR} > > + ${DERIVED_SOURCES_WEBKIT2_DIR}/include > > Since we don't support webkit2, are you sure you need this? I've just seen you already answered this on your first comment. Sorry for the noise.

Gyuyoung Kim

Comment 5 2011-06-01 05:22:31 PDT

(In reply to comment #3) > It seems you are indeed using webkit2. I'm wondering what's happening since we don't have support to webkit2 yet. We start to support WebKit2 for EFL port. We're going to notify this news to webkit-dev mailing list.

Ryuan Choi

Comment 6 2011-06-01 06:39:23 PDT

Created attachment 95591 [details] Patch

WebKit Review Bot

Comment 7 2011-06-01 06:42:35 PDT

Attachment 95591 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/efl/..." exit_code: 1 Tools/MiniBrowser/efl/main.c:45: main_signal_exit is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryuan Choi

Comment 8 2011-06-01 06:46:50 PDT

(In reply to comment #2) > I think it is better to insert CMakeListEfl.txt into efl directory. MiniBrowser of gtk, qt have build script in its directory. It's my mistake. I moved it into efl directory. (In reply to comment #3) > (From update of attachment 95572 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95572&action=review > > Once WebKit2 is compiled in EFL port, I think this can enter. I agree with Gyuyoung that the cmake file should be in efl directory. But we can do a bit better and split it in a generic CMakeLists.txt file and another efl/CMakeListsEfl.txt. This way other ports that use cmake can profit from this work too. > Done. It's my mistake. > > Tools/MiniBrowser/CMakeListsEfl.txt:15 > > + ${WEBKIT2_DIR}/UIProcess/API/efl > > + ${WEBKIT2_DIR} > > + ${DERIVED_SOURCES_WEBKIT2_DIR}/include > > Since we don't support webkit2, are you sure you need this? > > > Tools/MiniBrowser/efl/main.c:55 > > +static int browserCreate(const char *url, const char *engine) > > +{ > > + MiniBrowser *app = (MiniBrowser*) malloc(sizeof(MiniBrowser)); > > + > > + app->ee = ecore_evas_software_x11_new(NULL, 0, 0, 0, DEFAULT_WIDTH, DEFAULT_HEIGHT); > > Could you use ecore_evas_new() here instead? Done. > > > Tools/MiniBrowser/efl/main.c:56 > > + ecore_evas_title_set(app->ee, "Samsung MiniBrowser"); > > I think it's not good to let Samsung here. WebKit would be a better name. > > > Tools/MiniBrowser/efl/main.c:71 > > + /* Create webview */ > > + app->browser = ewk_view_add(app->evas, WKContextGetSharedProcessContext(), 0); > > It seems you are indeed using webkit2. I'm wondering what's happening since we don't have support to webkit2 yet. > > > Tools/MiniBrowser/efl/main.c:97 > > + browserCreate(url, engine); > > + > > + ecore_event_handler_add(ECORE_EVENT_SIGNAL_EXIT, mainSignalExit, NULL); > > Since in this file we are following EFL style, I think it's better not to use camelCase for function names. I changed it EFL style. but we already use browserCreate. Is it ok? (In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 95572 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=95572&action=review > > > > Once WebKit2 is compiled in EFL port, I think this can enter. I agree with Gyuyoung that the cmake file should be in efl directory. But we can do a bit better and split it in a generic CMakeLists.txt file and another efl/CMakeListsEfl.txt. This way other ports that use cmake can profit from this work too. > > > > > Tools/MiniBrowser/CMakeListsEfl.txt:15 > > > + ${WEBKIT2_DIR}/UIProcess/API/efl > > > + ${WEBKIT2_DIR} > > > + ${DERIVED_SOURCES_WEBKIT2_DIR}/include > > > > Since we don't support webkit2, are you sure you need this? > > I've just seen you already answered this on your first comment. Sorry for the noise. No problem.

Gyuyoung Kim

Comment 9 2011-06-01 18:08:24 PDT

Comment on attachment 95591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95591&action=review Why don't you add WebKit2 prefix to Bug summary ? For example, [EFL/WebKit2] and so on. > Tools/MiniBrowser/efl/main.c:98 > + browserCreate(url, engine) Missing semicolon.

Ryuan Choi

Comment 10 2011-06-01 19:17:11 PDT

Created attachment 95704 [details] Patch

WebKit Review Bot

Comment 11 2011-06-01 19:20:26 PDT

Attachment 95704 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/efl/..." exit_code: 1 Tools/MiniBrowser/efl/main.c:45: main_signal_exit is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Gyuyoung Kim

Comment 12 2011-06-01 21:27:52 PDT

Please upload this patch again after landing bug 61903's patch

Ryuan Choi

Comment 13 2011-06-02 16:53:14 PDT

Created attachment 95833 [details] Patch

Gyuyoung Kim

Comment 14 2011-06-13 20:02:11 PDT

LGTM.

Lucas De Marchi

Comment 15 2011-06-13 20:20:42 PDT

Comment on attachment 95833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95833&action=review > Tools/MiniBrowser/efl/CMakeListsEfl.txt:64 > +ADD_CUSTOM_TARGET(forwarding-headerMiniBrowserSoup > + COMMAND ${PERL_EXECUTABLE} ${WEBKIT2_DIR}/Scripts/generate-forwarding-headers.pl ${MiniBrowser_DIR} ${DERIVED_SOURCES_WEBKIT2_DIR}/include soup > +) Do you really need this if WTF_USE_CURL is defined? > Tools/MiniBrowser/efl/CMakeListsEfl.txt:71 > +ADD_DEPENDENCIES(Programs/MiniBrowser forwarding-headerMiniBrowserSoup) ditto.

Ryuan Choi

Comment 16 2011-06-13 23:53:02 PDT

Created attachment 97075 [details] Patch

Ryuan Choi

Comment 17 2011-06-13 23:53:42 PDT

(In reply to comment #15) > (From update of attachment 95833 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95833&action=review > > > Tools/MiniBrowser/efl/CMakeListsEfl.txt:64 > > +ADD_CUSTOM_TARGET(forwarding-headerMiniBrowserSoup > > + COMMAND ${PERL_EXECUTABLE} ${WEBKIT2_DIR}/Scripts/generate-forwarding-headers.pl ${MiniBrowser_DIR} ${DERIVED_SOURCES_WEBKIT2_DIR}/include soup > > +) > > Do you really need this if WTF_USE_CURL is defined? > > > Tools/MiniBrowser/efl/CMakeListsEfl.txt:71 > > +ADD_DEPENDENCIES(Programs/MiniBrowser forwarding-headerMiniBrowserSoup) > > ditto. Sorry, I fixed.

Leandro Pereira

Comment 18 2011-06-14 09:00:57 PDT

Comment on attachment 97075 [details] Patch Informal r+

Ryuan Choi

Comment 19 2011-06-22 17:10:01 PDT

Created attachment 98267 [details] Patch_changed_copyright

Ryuan Choi

Comment 20 2011-06-27 04:45:14 PDT

Created attachment 98700 [details] Patch

Gyuyoung Kim

Comment 21 2011-06-27 04:54:06 PDT

Comment on attachment 98700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98700&action=review > Tools/MiniBrowser/efl/main.c:45 > + return 1; Why do you use 1 instead of EINA_TRUE ?

Ryuan Choi

Comment 22 2011-06-27 04:58:53 PDT

Created attachment 98703 [details] Patch

Ryuan Choi

Comment 23 2011-06-27 04:59:22 PDT

(In reply to comment #21) > (From update of attachment 98700 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98700&action=review > > > Tools/MiniBrowser/efl/main.c:45 > > + return 1; > > Why do you use 1 instead of EINA_TRUE ? Sorry, It's mistake. Updated.

Ryuan Choi

Comment 24 2011-12-22 16:10:35 PST

Comment on attachment 98703 [details] Patch This patch is outdated. I'll upload newer after tested.

Ryuan Choi

Comment 25 2011-12-29 22:46:02 PST

Created attachment 120781 [details] Patch

Raphael Kubo da Costa (:rakuco)

Comment 26 2011-12-30 06:26:02 PST

Comment on attachment 120781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120781&action=review > Tools/MiniBrowser/efl/CMakeLists.txt:17 > + ${CMAKE_SOURCE_DIR} Is this one really needed? > Tools/MiniBrowser/efl/main.c:23 > +#include <Ecore_Getopt.h> > +#include <Ecore_X.h> These headers do not seem to be used, and Eina.h is missing. > Tools/MiniBrowser/efl/main.c:27 > +#define DEFAULT_WIDTH 800 > +#define DEFAULT_HEIGHT 600 static const int would be better than #define. > Tools/MiniBrowser/efl/main.c:42 > +static Eina_Bool browserCreate(const char *url, const char *engine) The return type is not being checked; why not make the function void or return a MiniBrowser* so you can free it later? > Tools/MiniBrowser/efl/main.c:44 > + MiniBrowser *app = (MiniBrowser*) malloc(sizeof(MiniBrowser)); No need for the cast, and the pointer is being leaked. > Tools/MiniBrowser/efl/main.c:46 > + app->ee = ecore_evas_new(engine, 0, 0, DEFAULT_WIDTH, DEFAULT_HEIGHT, NULL); s/NULL/0/? > Tools/MiniBrowser/efl/main.c:78 > + char *engine = NULL; This is not being used. > Tools/MiniBrowser/efl/main.c:87 > + url = (char*) default_url; Isn't it better to make url const instead of casting? > Tools/MiniBrowser/efl/main.c:91 > + ecore_event_handler_add(ECORE_EVENT_SIGNAL_EXIT, main_signal_exit, NULL); The handler is being leaked.

Ryuan Choi

Comment 27 2012-01-01 20:25:02 PST

Created attachment 120857 [details] Patch

Ryuan Choi

Comment 28 2012-01-01 20:40:59 PST

Created attachment 120860 [details] Patch

Ryuan Choi

Comment 29 2012-01-01 20:46:00 PST

Created attachment 120861 [details] Patch

Ryuan Choi

Comment 30 2012-01-01 20:48:40 PST

(In reply to comment #29) > Created an attachment (id=120861) [details] > Patch kubo, thank you for your comment. Fixed all you commented.

Gyuyoung Kim

Comment 31 2012-01-01 21:31:33 PST

Comment on attachment 120861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120861&action=review > Tools/MiniBrowser/efl/main.c:27 > +static const char *DEFAULT_URL = "http://www.google.com/"; As you may know, this violate *whitespace/declaration* rule. Bug 75424 will remove the rule's exception from EFL port. But, I am not sure if we should implement Mini Browser according to EFL coding style. GTK port implemented it according to GTK coding style.(So, there are style errors gtk MiniBrowser.) But, it seems other ports don't violate webkit coding rule. IMO, why don't we adhere *whitespace/declaration* rule in here ?

Ryuan Choi

Comment 32 2012-01-01 21:40:51 PST

(In reply to comment #31) > (From update of attachment 120861 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120861&action=review > > > Tools/MiniBrowser/efl/main.c:27 > > +static const char *DEFAULT_URL = "http://www.google.com/"; > > As you may know, this violate *whitespace/declaration* rule. Bug 75424 will remove the rule's exception from EFL port. But, I am not sure if we should implement Mini Browser according to EFL coding style. GTK port implemented it according to GTK coding style.(So, there are style errors gtk MiniBrowser.) But, it seems other ports don't violate webkit coding rule. IMO, why don't we adhere *whitespace/declaration* rule in here ? Right, I know that MiniBrowser and EWebLauncher doesn't keep the webkit coding style and it's sad for me. IMO, We can fix them after getting approval from other webkit/efl guys.

Gyuyoung Kim

Comment 33 2012-01-01 22:10:39 PST

(In reply to comment #32) > (In reply to comment #31) > > (From update of attachment 120861 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=120861&action=review > > > > > Tools/MiniBrowser/efl/main.c:27 > > > +static const char *DEFAULT_URL = "http://www.google.com/"; > > > > As you may know, this violate *whitespace/declaration* rule. Bug 75424 will remove the rule's exception from EFL port. But, I am not sure if we should implement Mini Browser according to EFL coding style. GTK port implemented it according to GTK coding style.(So, there are style errors gtk MiniBrowser.) But, it seems other ports don't violate webkit coding rule. IMO, why don't we adhere *whitespace/declaration* rule in here ? > > Right, I know that MiniBrowser and EWebLauncher doesn't keep the webkit coding style and it's sad for me. > > IMO, We can fix them after getting approval from other webkit/efl guys. My though is changed. EWebLauncher and MiniBrowser are EFL application. So, I think it is better to keep to implement with EFL coding style. In Bug 75424, I keep *whitespace/declaration* exception for EWebLauncher and MiniBrowser.

Ryuan Choi

Comment 34 2012-01-01 22:21:30 PST

(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #31) > > IMO, We can fix them after getting approval from other webkit/efl guys. > > My though is changed. EWebLauncher and MiniBrowser are EFL application. So, I think it is better to keep to implement with EFL coding style. In Bug 75424, I keep *whitespace/declaration* exception for EWebLauncher and MiniBrowser. OK, I love webkit coding style, but I'm not sure which one we should keep it. I'll follow the rule which the most people like. Thanks for your changes.

Raphael Kubo da Costa (:rakuco)

Comment 35 2012-01-02 04:41:43 PST

Comment on attachment 120861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120861&action=review Looks mostly OK even though I'm unable to test it. >>>> Tools/MiniBrowser/efl/main.c:27 >>>> +static const char *DEFAULT_URL = "http://www.google.com/"; >>> >>> As you may know, this violate *whitespace/declaration* rule. Bug 75424 will remove the rule's exception from EFL port. But, I am not sure if we should implement Mini Browser according to EFL coding style. GTK port implemented it according to GTK coding style.(So, there are style errors gtk MiniBrowser.) But, it seems other ports don't violate webkit coding rule. IMO, why don't we adhere *whitespace/declaration* rule in here ? >> >> Right, I know that MiniBrowser and EWebLauncher doesn't keep the webkit coding style and it's sad for me. >> >> IMO, We can fix them after getting approval from other webkit/efl guys. > > My though is changed. EWebLauncher and MiniBrowser are EFL application. So, I think it is better to keep to implement with EFL coding style. In Bug 75424, I keep *whitespace/declaration* exception for EWebLauncher and MiniBrowser. Another (minor) issue here is that this does not go to the .rodata section in the binary; use static const char foo[] instead.

Ryuan Choi

Comment 36 2012-01-02 18:03:18 PST

Created attachment 120903 [details] Patch

Ryuan Choi

Comment 37 2012-01-02 18:07:07 PST

(In reply to comment #35) > (From update of attachment 120861 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120861&action=review > > Looks mostly OK even though I'm unable to test it. > Ah, I shared minimum patch to run webkit2/efl. you can run minibrowser/efl using it. > >>>> Tools/MiniBrowser/efl/main.c:27 > >>>> +static const char *DEFAULT_URL = "http://www.google.com/"; > >>> > >>> As you may know, this violate *whitespace/declaration* rule. Bug 75424 will remove the rule's exception from EFL port. But, I am not sure if we should implement Mini Browser according to EFL coding style. GTK port implemented it according to GTK coding style.(So, there are style errors gtk MiniBrowser.) But, it seems other ports don't violate webkit coding rule. IMO, why don't we adhere *whitespace/declaration* rule in here ? > >> > >> Right, I know that MiniBrowser and EWebLauncher doesn't keep the webkit coding style and it's sad for me. > >> > >> IMO, We can fix them after getting approval from other webkit/efl guys. > > > > My though is changed. EWebLauncher and MiniBrowser are EFL application. So, I think it is better to keep to implement with EFL coding style. In Bug 75424, I keep *whitespace/declaration* exception for EWebLauncher and MiniBrowser. > > Another (minor) issue here is that this does not go to the .rodata section in the binary; use static const char foo[] instead. Wow, Thanks. Fixed like you mentioned.

Raphael Kubo da Costa (:rakuco)

Comment 38 2012-02-07 04:23:50 PST

As briefly discussed on IRC a few minutes ago, the patch isn't quite ready because of the WK* calls it makes. MiniBrowser should be fairly self-contained in the sense that users should base their code on it and build it outside the tree. That's not possible with the current WK* stuff.

Ryuan Choi

Comment 39 2012-02-20 00:24:33 PST

Comment on attachment 120903 [details] Patch clear flags until efl styled API were prepared.

Ryuan Choi

Comment 40 2012-04-16 20:27:56 PDT

Created attachment 137470 [details] Patch

Ryuan Choi

Comment 41 2012-04-17 02:19:15 PDT

Created attachment 137497 [details] Patch

Dominik Röttsches (drott)

Comment 42 2012-05-28 07:23:59 PDT

(In reply to comment #38) > As briefly discussed on IRC a few minutes ago, the patch isn't quite ready because of the WK* calls it makes. MiniBrowser should be fairly self-contained in the sense that users should base their code on it and build it outside the tree. That's not possible with the current WK* stuff. Raphael, do you see all your comments addressed in the most recent version of Ryuan's patch? Let's get this in.

Raphael Kubo da Costa (:rakuco)

Comment 43 2012-05-28 07:32:57 PDT

I thought the required WK2 APIs had not been added yet? The patches are passing the EWS bot because we don't enable WebKit2 by default, IIRC (or at least didn't when the last patch was uploaded).

Gyuyoung Kim

Comment 44 2012-05-29 18:08:03 PDT

(In reply to comment #43) > I thought the required WK2 APIs had not been added yet? The patches are passing the EWS bot because we don't enable WebKit2 by default, IIRC (or at least didn't when the last patch was uploaded). As far as I know, this patch is not built on EWS yet. Ryuan is submitting patches to support this patch. It looks this patch is able to be reviewed after landing some WK2 patches.

Ryuan Choi

Comment 45 2012-06-03 23:37:00 PDT

Created attachment 145521 [details] rebased

Ryuan Choi

Comment 46 2012-06-03 23:39:04 PDT

rebased , but Bug 84124 should be landed before this.

Gyuyoung Kim

Comment 47 2012-06-03 23:41:55 PDT

Comment on attachment 145521 [details] rebased It looks there are no critical issues as initial implementation. Looks fine.

Chris Dumez

Comment 48 2012-06-06 05:42:16 PDT

Comment on attachment 145521 [details] rebased View in context: https://bugs.webkit.org/attachment.cgi?id=145521&action=review > Tools/MiniBrowser/efl/main.c:116 > + free(browser); You never free browser->ee. You probably need a "ecore_evas_free(browser->ee);" before the "free(browser);", right?

Ryuan Choi

Comment 49 2012-06-06 20:13:28 PDT

(In reply to comment #48) > (From update of attachment 145521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145521&action=review > > > Tools/MiniBrowser/efl/main.c:116 > > + free(browser); > > You never free browser->ee. You probably need a "ecore_evas_free(browser->ee);" before the "free(browser);", right? ecore_evas_shutdown will release all allocated ecore_evas although we don't call ecore_evas_free explicitly. If you want to call it explicitly, I will add it.

Ryuan Choi

Comment 50 2012-06-10 08:03:33 PDT

Created attachment 146737 [details] Patch

Ryuan Choi

Comment 51 2012-06-11 18:22:54 PDT

Created attachment 146986 [details] rebased to catch r120024

Gyuyoung Kim

Comment 52 2012-06-11 20:30:51 PDT

Comment on attachment 146986 [details] rebased to catch r120024 View in context: https://bugs.webkit.org/attachment.cgi?id=146986&action=review > Tools/MiniBrowser/efl/CMakeLists.txt:62 > +SET_TARGET_PROPERTIES(DumpRenderTree PROPERTIES FOLDER "Tools") By the way, why do you set target folder for DumpRenderTree? Is this correct ?

Ryuan Choi

Comment 53 2012-06-11 21:31:32 PDT

Created attachment 147008 [details] Fix mistake

Ryuan Choi

Comment 54 2012-06-11 21:32:24 PDT

(In reply to comment #52) > (From update of attachment 146986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146986&action=review > > > Tools/MiniBrowser/efl/CMakeLists.txt:62 > > +SET_TARGET_PROPERTIES(DumpRenderTree PROPERTIES FOLDER "Tools") > > By the way, why do you set target folder for DumpRenderTree? Is this correct ? Oops, sorry I mistake it while catching up. Fixed.

Chang Shu

Comment 55 2012-06-12 11:44:02 PDT

Comment on attachment 147008 [details] Fix mistake Nice!

WebKit Review Bot

Comment 56 2012-06-12 15:20:11 PDT

Comment on attachment 147008 [details] Fix mistake Clearing flags on attachment: 147008 Committed r120130: <http://trac.webkit.org/changeset/120130>

WebKit Review Bot

Comment 57 2012-06-12 15:20:20 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.