Bug 63966 - [EFL] Add url bar to EWebLauncher and MiniBrowser/Efl.
Summary: [EFL] Add url bar to EWebLauncher and MiniBrowser/Efl.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on: 92070 95670
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-05 18:59 PDT by Hyerim Bae
Modified: 2012-09-03 00:32 PDT (History)
10 users (show)

See Also:


Attachments
Add url bar to EWebLauncher. (14.87 KB, patch)
2011-07-05 19:05 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
Snapshot of this patch. (46.13 KB, image/x-png)
2011-07-05 19:06 PDT, Hyerim Bae
no flags Details
Another patch using edje. (19.43 KB, patch)
2011-07-12 00:01 PDT, Hyerim Bae
lucas.de.marchi: review-
Details | Formatted Diff | Diff
patch for Lucas's comment. (18.95 KB, patch)
2011-07-12 18:05 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
patch for kubo's comment. (18.00 KB, patch)
2011-07-13 18:37 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
Make a font size to 12, modify the copy right. (17.95 KB, patch)
2011-07-13 21:54 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
patch for kubo's comment. (16.79 KB, patch)
2011-07-25 17:47 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
patch for kubo's comment. (16.75 KB, patch)
2011-07-26 18:38 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
patch for kubo's & kuyung's comments. (16.83 KB, patch)
2011-07-26 18:50 PDT, Hyerim Bae
no flags Details | Formatted Diff | Diff
Patch (21.95 KB, patch)
2012-07-23 01:18 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (21.96 KB, patch)
2012-07-23 22:40 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
rebased for style bot (22.05 KB, patch)
2012-07-24 03:29 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
rebased for style bot2 (22.05 KB, patch)
2012-07-24 03:38 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (21.68 KB, patch)
2012-08-06 00:28 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (21.73 KB, patch)
2012-08-23 05:20 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 Hyerim Bae 2011-07-05 18:59:35 PDT
Add url bar to EWebLauncher.
Comment 1 Hyerim Bae 2011-07-05 19:05:41 PDT
Created attachment 99777 [details]
Add url bar to EWebLauncher.

This patch is for adding URL bar to EWebLauncher(EFL) sample program.
When a user clicks a URL bar, the whole text in the bar will be selected.
Then the user can navigate to the site where he or she intends to do.
Comment 2 Hyerim Bae 2011-07-05 19:06:33 PDT
Created attachment 99778 [details]
Snapshot of this patch.
Comment 3 Gyuyoung Kim 2011-07-05 20:59:57 PDT
LGTM. Kubo and Lucas, how do you think about it ?
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-07-06 05:59:50 PDT
Apart from the comments below, I would much rather if the patch was changed to use Edje instead of using textblock directly: most of the code in this patch simply does a lot of manual work which should not be required in an application in the 21st century :)

> Tools/ChangeLog:10
> +        Then the user can navigate to the site where he or she intends to do.

The "do" is not needed here.

> Tools/EWebLauncher/main.c:762
> +    Evas_Event_Mouse_Down *ev = (Evas_Event_Mouse_Down*) event_info;

This kind of cast is not needed.

> Tools/EWebLauncher/main.c:952
> +            snprintf(full_url, 4096, "http://%s", url);

As this code is used in two different places, it would be good to factor it out into a separate function (or even provide it as a utility function in ewk later. eve needs to do the same thing in its code).
Comment 5 Ryuan Choi 2011-07-06 17:32:43 PDT
Comment on attachment 99777 [details]
Add url bar to EWebLauncher.

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

> Tools/EWebLauncher/main.c:1110
> +    createUrlBar();

How do you think about adding new function to initialize common code like ecore_evas between createUrlBar and browserCreate.
And then you can check before calling createUrlBar whether it returns EXIT_SUCCESS.

BTW, we use two different naming rules, createUrlBar and browserCreate :(
Comment 6 Hyerim Bae 2011-07-06 18:49:32 PDT
I'll modify what you mentioned below.
Also, regarding using textblock, you are absolutely right.
However, the Edje can't receive the user input key value directly as far as I know.
That's why I used the textblock and handle it manually.


(In reply to comment #4)
> Apart from the comments below, I would much rather if the patch was changed to use Edje instead of using textblock directly: most of the code in this patch simply does a lot of manual work which should not be required in an application in the 21st century :)
> > Tools/ChangeLog:10
> > +        Then the user can navigate to the site where he or she intends to do.
> The "do" is not needed here.
> > Tools/EWebLauncher/main.c:762
> > +    Evas_Event_Mouse_Down *ev = (Evas_Event_Mouse_Down*) event_info;
> This kind of cast is not needed.
> > Tools/EWebLauncher/main.c:952
> > +            snprintf(full_url, 4096, "http://%s", url);
> As this code is used in two different places, it would be good to factor it out into a separate function (or even provide it as a utility function in ewk later. eve needs to do the same thing in its code).
Comment 7 Hyerim Bae 2011-07-07 03:30:51 PDT
Also, if I use 'edje entry' without the 'elementary', It's more complicated than using the 'textblock' directly.
That means I should implement the 'elementary entry widget' with using the 'edje entry'.
It's much more easy to use the 'elementary entry' directly instead, but as you know, the webkit can't has a elementary dependency.

(In reply to comment #6)
> I'll modify what you mentioned below.
> Also, regarding using textblock, you are absolutely right.
> However, the Edje can't receive the user input key value directly as far as I know.
> That's why I used the textblock and handle it manually.
> (In reply to comment #4)
> > Apart from the comments below, I would much rather if the patch was changed to use Edje instead of using textblock directly: most of the code in this patch simply does a lot of manual work which should not be required in an application in the 21st century :)
> > > Tools/ChangeLog:10
> > > +        Then the user can navigate to the site where he or she intends to do.
> > The "do" is not needed here.
> > > Tools/EWebLauncher/main.c:762
> > > +    Evas_Event_Mouse_Down *ev = (Evas_Event_Mouse_Down*) event_info;
> > This kind of cast is not needed.
> > > Tools/EWebLauncher/main.c:952
> > > +            snprintf(full_url, 4096, "http://%s", url);
> > As this code is used in two different places, it would be good to factor it out into a separate function (or even provide it as a utility function in ewk later. eve needs to do the same thing in its code).
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-07-07 07:34:48 PDT
(In reply to comment #6)
> I'll modify what you mentioned below.
> Also, regarding using textblock, you are absolutely right.
> However, the Edje can't receive the user input key value directly as far as I know.
> That's why I used the textblock and handle it manually.

Are you really sure using Edje is more troublesome than doing all the work done in this patch? Using an entry and then using edje_object_signal_callback_add to check if Enter was pressed should require less code than using textblock directly, shouldn't it?
Comment 9 Hyerim Bae 2011-07-07 21:14:39 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > I'll modify what you mentioned below.
> > Also, regarding using textblock, you are absolutely right.
> > However, the Edje can't receive the user input key value directly as far as I know.
> > That's why I used the textblock and handle it manually.
> Are you really sure using Edje is more troublesome than doing all the work done in this patch? Using an entry and then using edje_object_signal_callback_add to check if Enter was pressed should require less code than using textblock directly, shouldn't it?

Okay, I'll try to do that.
Comment 10 Hyerim Bae 2011-07-12 00:01:46 PDT
Created attachment 100443 [details]
Another patch using edje.

Another patch using edje which Kubo recommanded.
Comment 11 Ryuan Choi 2011-07-12 00:04:32 PDT
Comment on attachment 100443 [details]
Another patch using edje.

Next time, please give r? and c?.
Comment 12 Lucas De Marchi 2011-07-12 04:42:39 PDT
Comment on attachment 100443 [details]
Another patch using edje.

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

Are those "if (!app)", "if (!app || !ev | ...)" really necessary? Seems like over protection. Just guarantee that the function cannot be called with NULL pointers. Since most of them come from evas_object_event_*, you can be sure they are not NULL.

> Tools/EWebLauncher/main.c:507
> +    if (!app || !ev)
> +        return;

why?

> Tools/EWebLauncher/main.c:536
> +    if (!app || !ev)
> +        return;

why?

> Tools/EWebLauncher/main.c:667
> +    if (!app)
> +        return;

why?

> Tools/EWebLauncher/main.c:717
> +                memset(full_url, 0x00, len);

You don't need this. Set only the last byte as '\0'

> Tools/EWebLauncher/main.c:747
> +            ewk_view_uri_set(app->browser, full_url);
> +            evas_object_focus_set(app->browser, EINA_TRUE);
> +
> +            if (full_url)
> +                free(full_url);

Why this is not after char *full_url?
Comment 13 Hyerim Bae 2011-07-12 18:05:44 PDT
Created attachment 100600 [details]
patch for Lucas's comment.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-07-13 10:25:08 PDT
Informal r-.

I'm still not convinced that all this is needed. EWebLauncher currently has 911 LOC. With this patch (which is supposed to simply add an input box) it goes up to 1260 LOC. I know this is mostly not your fault, as we simply do not have a ready-made input box without using Elementary. But even so, the EDC could still be trimmed down a bit more -- most of the focus and cursor stuff can be just removed, as EWebLauncher is supposed to be kept as simple as possible. IMO the EDC should have the absolute bare minimum to just let a user edit text and press Enter.

> Tools/CMakeListsEfl.txt:52
> +

Extra newline.

> Tools/CMakeListsEfl.txt:68
> +                ${TOOLS_DIR}/EWebLauncher/entry.edc ENTRY_EDJ_PATH

This is wrong, you need ${ENTRY_EDJ_PATH}.

> Tools/CMakeListsEfl.txt:71
> +ADD_DEPENDENCIES(${WebKit_LIBRARY_NAME} entry.edj)

This feels hackish, is it really needed?

> Tools/CMakeListsEfl.txt:72
> +

Extra newline.

> Tools/ChangeLog:8
> +        This patch is for adding URL bar to EWebLauncher(EFL) sample program.

The reviewers usually prefer that ChangeLog entries do not begin with "This patch does ...". As you already said that this patch adds a URL bar to EWebLauncher above, you can start this longer description with the following line.

> Tools/EWebLauncher/entry.edc:1
> +

The EDC files in ewk usually have copyright headers, so this one should have one too.

> Tools/EWebLauncher/main.c:703
> +            full_url = (char *)malloc(len);

The cast is not needed.

> Tools/EWebLauncher/main.c:705
> +                full_url[len]='\0';

Coding style: spaces before and after the '='.

> Tools/EWebLauncher/main.c:732
> +            if (full_url)

ewk_view_uri_set will return EINA_FALSE if full_uri is NULL to, so the check could be moved up:

if (full_url) {
  ewk_view_uri_set(...);
  free(full_url);
}

> Tools/EWebLauncher/main.c:733
> +                free(full_url);

This call crashes EWebLauncher if I run it with MALLOC_CHECK_=3:

*** glibc detected *** /home/rakuco/dev/webkit/drt/WebKitBuild/Debug-efl/Programs/EWebLauncher: free(): invalid pointer: 0x081a0d70 ***

> Tools/EWebLauncher/main.c:882
> +        if (full_url)

Ditto about moving the check up.
Comment 15 Hyerim Bae 2011-07-13 17:57:42 PDT
(In reply to comment #14)

> > Tools/CMakeListsEfl.txt:71
> > +ADD_DEPENDENCIES(${WebKit_LIBRARY_NAME} entry.edj)
> This feels hackish, is it really needed?

I found that without above line, the entry.edc can't be compiled with edje cc.
The edje compiler ignore the entry.edc without above line.
Comment 16 Ryuan Choi 2011-07-13 18:01:29 PDT
(In reply to comment #15)
> (In reply to comment #14)
> 
> > > Tools/CMakeListsEfl.txt:71
> > > +ADD_DEPENDENCIES(${WebKit_LIBRARY_NAME} entry.edj)
> > This feels hackish, is it really needed?
> 
> I found that without above line, the entry.edc can't be compiled with edje cc.
> The edje compiler ignore the entry.edc without above line.

No, kubo is riggt.

please try to test using Programs/EWebLauncher instead of ${WebKit_LIBRARY_NAME}
Comment 17 Hyerim Bae 2011-07-13 18:37:06 PDT
Created attachment 100751 [details]
patch for kubo's comment.

Modify according to kubo's comment, and trim down the entry.edc as short as possible.
Comment 18 Gyuyoung Kim 2011-07-13 21:37:16 PDT
Comment on attachment 100751 [details]
patch for kubo's comment.

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

> Tools/EWebLauncher/entry.edc:4
> +    Copyright (C) 2009,2010 Samsung Electronics

Change 2009,2010 with 2011.

> Tools/EWebLauncher/entry.edc:28
> +      data.item: "default_font_size" "16";

I think font size is a little big. Could you cut it down a little ?
Comment 19 Hyerim Bae 2011-07-13 21:54:50 PDT
Created attachment 100766 [details]
Make a font size to 12, modify the copy right.
Comment 20 Raphael Kubo da Costa (:rakuco) 2011-07-14 07:24:04 PDT
Still r-, as some concerns have not been addressed yet. The EDC still looks too big for my taste 

> Tools/CMakeListsEfl.txt:65
> +ADD_CUSTOM_TARGET(entry.edj

This target name still rubs me in a wrong way. What if you do what Source/WebKit/efl/CMakeListsEfl.txt does and use ADD_CUSTOM_COMMAND with the proper dependencies instead of using this? If it doesn't work, please use a more specific name, such as "Programs/EWebLauncher_theme".

> Tools/EWebLauncher/entry.edc:32
> +            base: "font=Roman font_size=16 color="ENTRY_TEXT_COLOR_INC" wrap=mixed";

The font size here is still 16.

> Tools/EWebLauncher/main.c:705
> +                full_url[len] = '\0';

I don't understand why you are doing this, as this is the faulty call which causes some bad memory management issues that I described in my previous review.

If you set the MALLOC_CHECK_ environment variable to 3, run EWebLauncher and go to "webkit.org" (without the "http://" before), the program will crash.
Comment 21 Hyerim Bae 2011-07-14 21:00:28 PDT
(In reply to comment #20)
> > Tools/CMakeListsEfl.txt:65
> > +ADD_CUSTOM_TARGET(entry.edj
> This target name still rubs me in a wrong way. What if you do what Source/WebKit/efl/CMakeListsEfl.txt does and use ADD_CUSTOM_COMMAND with the proper dependencies instead of using this? If it doesn't work, please use a more specific name, such as "Programs/EWebLauncher_theme".

I failed to use ADD_CUSTOM_COMMAND, so I may use the ADD_CUSTOM_TARGET. The target name can't have slash(/), how about "EWEbLauncher_entry_theme" instead ?
Comment 22 Raphael Kubo da Costa (:rakuco) 2011-07-15 09:52:18 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > > Tools/CMakeListsEfl.txt:65
> > > +ADD_CUSTOM_TARGET(entry.edj
> > This target name still rubs me in a wrong way. What if you do what Source/WebKit/efl/CMakeListsEfl.txt does and use ADD_CUSTOM_COMMAND with the proper dependencies instead of using this? If it doesn't work, please use a more specific name, such as "Programs/EWebLauncher_theme".
> 
> I failed to use ADD_CUSTOM_COMMAND, so I may use the ADD_CUSTOM_TARGET. The target name can't have slash(/), how about "EWEbLauncher_entry_theme" instead ?

In the worst case, yes, but I'd appreciate if you could describe what problems you had with ADD_CUSTOM_COMMAND first.
Comment 23 Hyerim Bae 2011-07-18 05:29:22 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > > Tools/CMakeListsEfl.txt:65
> > > > +ADD_CUSTOM_TARGET(entry.edj
> > > This target name still rubs me in a wrong way. What if you do what Source/WebKit/efl/CMakeListsEfl.txt does and use ADD_CUSTOM_COMMAND with the proper dependencies instead of using this? If it doesn't work, please use a more specific name, such as "Programs/EWebLauncher_theme".
> > 
> > I failed to use ADD_CUSTOM_COMMAND, so I may use the ADD_CUSTOM_TARGET. The target name can't have slash(/), how about "EWEbLauncher_entry_theme" instead ?
> In the worst case, yes, but I'd appreciate if you could describe what problems you had with ADD_CUSTOM_COMMAND first.

I tried to use ADD_CUSTOM_COMMAND like below instead of ADD_CUSTOM_TARGET.
However, the entry.edc can't be compiled at all. Do you know what's the reason?
It's a troublesome problem that I have.

ADD_CUSTOM_COMMAND(
  OUTPUT ${ENTRY_EDJ_PATH}
  COMMAND ${EDJE_CC_EXECUTABLE} -v -id ${TOOLS_DIR}/EWebLauncher/entry.edc ${ENTRY_EDJ_PATH}
  DEPENDS
     ${TOOLS_DIR}/EWebLauncher/entry.edc
  VERBATIM
)
Comment 24 Raphael Kubo da Costa (:rakuco) 2011-07-18 06:42:31 PDT
Please take a look at CMake's explanation for the add_custom_command option [1]. If you use this first form of the command:

  "A target created in the same directory (CMakeLists.txt file) that specifies any output of the custom command as a source file is given a rule to generate the file using the command at build time"

If you choose the other form:

  "The second signature adds a custom command to a target such as a library or executable. This is useful for performing an operation before or after building the target. The command becomes part of the target and will only execute when the target itself is built. If the target is already built, the command will not execute."

Notice how Source/WebKit/efl/CMakeListsEfl.txt does

  LIST(APPEND WebKit_SOURCES
       ${WebKit_THEME}
  )

according to what is described in the documentation of the first form of the command.

[1] http://www.cmake.org/cmake/help/cmake-2-8-docs.html#command:add_custom_command
Comment 25 Hyerim Bae 2011-07-25 01:53:32 PDT
(In reply to comment #24)
> Please take a look at CMake's explanation for the add_custom_command option [1]. If you use this first form of the command:
>   "A target created in the same directory (CMakeLists.txt file) that specifies any output of the custom command as a source file is given a rule to generate the file using the command at build time"
> If you choose the other form:
>   "The second signature adds a custom command to a target such as a library or executable. This is useful for performing an operation before or after building the target. The command becomes part of the target and will only execute when the target itself is built. If the target is already built, the command will not execute."
> Notice how Source/WebKit/efl/CMakeListsEfl.txt does
>   LIST(APPEND WebKit_SOURCES
>        ${WebKit_THEME}
>   )
> according to what is described in the documentation of the first form of the command.
> [1] http://www.cmake.org/cmake/help/cmake-2-8-docs.html#command:add_custom_command

How about this one?

ADD_CUSTOM_COMMAND(TARGET Programs/EWebLauncher
                   COMMAND ${EDJE_CC_EXECUTABLE} -v ${TOOLS_DIR}/EWebLauncher/entry.edc ${ENTRY_EDJ_PATH}
                   DEPENDS
                      ${TOOLS_DIR}/EWebLauncher/entry.edc
                   VERBATIM
)

LIST(APPEND EWebLauncher_SOURCES
     ${ENTRY_EDJ_PATH}
)
Comment 26 Raphael Kubo da Costa (:rakuco) 2011-07-25 09:18:56 PDT
(In reply to comment #25)
> How about this one?
> 
> ADD_CUSTOM_COMMAND(TARGET Programs/EWebLauncher
>                    COMMAND ${EDJE_CC_EXECUTABLE} -v ${TOOLS_DIR}/EWebLauncher/entry.edc ${ENTRY_EDJ_PATH}
>                    DEPENDS
>                       ${TOOLS_DIR}/EWebLauncher/entry.edc
>                    VERBATIM
> )
> 
> LIST(APPEND EWebLauncher_SOURCES
>      ${ENTRY_EDJ_PATH}
> )

If you have verified it works, it's OK to me.
Comment 27 Hyerim Bae 2011-07-25 17:47:56 PDT
Created attachment 101954 [details]
patch for kubo's comment.

Minimize the edc file.
Make the font size to 14, it's more proper I think.
Comment 28 Raphael Kubo da Costa (:rakuco) 2011-07-26 05:35:24 PDT
> Tools/ChangeLog:3
> +        Reviewed by NOBODY (OOPS!).

The ChangeLog format has changed recently, and the "Reviewed by" line should come after the bug URL.

> Tools/EWebLauncher/entry.edc:29
> +            base: "font=Roman font_size=14 color=#000000 wrap=mixed";

A few patches ago you said you were going to make the font size 12. Why have you chosen 14 now?

> Tools/EWebLauncher/main.c:710
> +                full_url[len-1] = '\0';

This is not needed. sprintf(3) already adds the trailing null byte, as it comes automatically from the string you added.
Comment 29 Hyerim Bae 2011-07-26 18:31:17 PDT
\> > Tools/EWebLauncher/entry.edc:29
> > +            base: "font=Roman font_size=14 color=#000000 wrap=mixed";
> A few patches ago you said you were going to make the font size 12. Why have you chosen 14 now?

font size 12 is too small, 14 is more proper I think.
Comment 30 Hyerim Bae 2011-07-26 18:38:17 PDT
Created attachment 102086 [details]
patch for kubo's comment.

Remove unnecessary exception code.
Modify the change log.
Comment 31 Gyuyoung Kim 2011-07-26 18:42:03 PDT
Comment on attachment 102086 [details]
patch for kubo's comment.

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

> Tools/EWebLauncher/entry.edc:86
> +            description { state: "default" 0.0;

Why do you put "state: "default" 0.0" on new line?

> Tools/EWebLauncher/entry.edc:129
> +            description { state: "default" 0.0;

ditto.
Comment 32 Gyuyoung Kim 2011-07-26 18:44:06 PDT
One more thing. Please do not modify patch directly when you update patch. If If you modify patch directly, style and build bot fail to merge.
Comment 33 Hyerim Bae 2011-07-26 18:50:42 PDT
Created attachment 102089 [details]
patch for kubo's & kuyung's comments.
Comment 34 Hyerim Bae 2011-07-26 18:52:34 PDT
(In reply to comment #33)
> Created an attachment (id=102089) [details]
> patch for kubo's & kuyung's comments.

Okay, I'll upload the patch later. Plz ignore the patches.
Comment 35 Raphael Kubo da Costa (:rakuco) 2011-09-20 10:11:04 PDT
Comment on attachment 102089 [details]
patch for kubo's & kuyung's comments.

Clearing flags.
Comment 36 Ryuan Choi 2012-07-23 01:18:35 PDT
Created attachment 153752 [details]
Patch
Comment 37 Gyuyoung Kim 2012-07-23 18:33:51 PDT
Comment on attachment 153752 [details]
Patch

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

There were sometimes difficulties whenever I test patches via EWebLauncher. So, I hope that this patch is landed successfully.

> Tools/EWebLauncher/ControlTheme/entry.edc:44
> +            source4: "control/entry/cursor/default"; // cursorover

What is cursorover ? Do you mean "cursor over" ?

In addition, I wonder what does *source4* mean.

> Tools/EWebLauncher/main.c:726
> +

It looks this is unneeded line.

> Tools/EWebLauncher/main.c:755
> +    urlBarDestroy(((ELauncher*)app)->urlBar);

Nit : g/(ELauncher*)/(ELauncher *)/g

> Tools/EWebLauncher/main.c:777
> +    const char *defaultTheme = THEME_DIR"/default.edj";

NIT : Use _ for defaultTheme.

> Tools/EWebLauncher/url_bar.c:34
> +getUriWithProtocol(const char *url)

In my opinion, EWebLauncher is EFL simple application. So, it is good to adhere EFL coding style.

> Tools/EWebLauncher/url_bar.c:38
> +        char *token = strstr(url, "://");

Because this is EFL application, If possible, I would like to recommend to use *eina_stringshare_strlen* instead of strlen.

> Tools/EWebLauncher/url_bar.c:57
> +    UrlBar *urlBar = (UrlBar*)data;

s/(UrlBar*)/(UrlBar *)/g

> Tools/EWebLauncher/url_bar.c:79
> +    UrlBar *urlBar = (UrlBar*)data;

ditto.

> Tools/EWebLauncher/url_bar.c:90
> +    UrlBar *urlBar = (UrlBar*)data;

ditto.

> Tools/EWebLauncher/url_bar.c:96
> +urlBarCreate(Evas_Object* webView, int width)

ditto.

> Tools/EWebLauncher/url_bar.c:99
> +    UrlBar *urlBar;

ditto.

> Tools/EWebLauncher/url_bar.c:135
> +urlBarDestroy(UrlBar *urlBar)

ditto.

> Tools/EWebLauncher/url_bar.c:150
> +urlBarSetURL(UrlBar *urlBar, const char* url)

Nit : Move '*' to variable side. In addition, s/urlBar/url_bar/g

> Tools/EWebLauncher/url_bar.h:37
> +typedef struct _UrlBar {

Good to use _ as below

typedef struct _Url_Bar {

> Tools/EWebLauncher/url_bar.h:45
> +void urlBarSetURL(UrlBar *urlBar, const char* url);

Nit : Move '*' to variable side.
Comment 38 Ryuan Choi 2012-07-23 22:40:38 PDT
Created attachment 153963 [details]
Patch
Comment 39 WebKit Review Bot 2012-07-23 22:44:19 PDT
Attachment 153963 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/CMakeLists.txt', u'Tools/ChangeLog',..." exit_code: 1
Tools/EWebLauncher/url_bar.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Ryuan Choi 2012-07-23 22:57:18 PDT
(In reply to comment #37)
> (From update of attachment 153752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153752&action=review
> 
> There were sometimes difficulties whenever I test patches via EWebLauncher. So, I hope that this patch is landed successfully.
> 
> > Tools/EWebLauncher/ControlTheme/entry.edc:44
> > +            source4: "control/entry/cursor/default"; // cursorover
> 
> What is cursorover ? Do you mean "cursor over" ?

Yes, I did.

> 
> In addition, I wonder what does *source4* mean.
> 

source4 is used for group to be loaded and used for cursor display OVER the cursor position.
Related url is http://docs.enlightenment.org/auto/edje/edcref.html

(In reply to comment #39)
> Attachment 153963 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/CMakeLists.txt', u'Tools/ChangeLog',..." exit_code: 1
> Tools/EWebLauncher/url_bar.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Strange, EWebLauncher did not use config.h before.

To fix this style issue,
 1) add config.h in source and include several path to EWebLauncher build script such WEBCORE_DIR, WTF_DIR, WEBCORE_DIR/platform and so on.
 2) skip this rule for EWebLauncher.

I am not sure what is better.
Comment 41 Ryuan Choi 2012-07-24 03:29:32 PDT
Created attachment 154007 [details]
rebased for style bot
Comment 42 WebKit Review Bot 2012-07-24 03:31:15 PDT
Attachment 154007 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/CMakeLists.txt', u'Tools/ChangeLog',..." exit_code: 1
Tools/EWebLauncher/url_bar.c:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Ryuan Choi 2012-07-24 03:38:16 PDT
Created attachment 154008 [details]
rebased for style bot2
Comment 44 Ryuan Choi 2012-08-06 00:28:13 PDT
Created attachment 156613 [details]
Patch
Comment 45 Gyuyoung Kim 2012-08-06 19:43:32 PDT
Comment on attachment 156613 [details]
Patch

It seems to me there is no critical problems in latest patch. If there is no objection to land this patch anymore, I agree to land this patch. Thanks.
Comment 46 Chris Dumez 2012-08-23 00:09:02 PDT
Comment on attachment 156613 [details]
Patch

LGTM.
Comment 47 Kenneth Rohde Christiansen 2012-08-23 00:34:08 PDT
Comment on attachment 156613 [details]
Patch

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

> Tools/EWebLauncher/url_bar.c:42
> +{
> +    const char *url = edje_object_part_text_get(urlBar->entry, "url.text");
> +    if (!url || !strlen(url))
> +        return NULL;
> +
> +    if (!strstr(url, "://"))
> +        return eina_stringshare_printf("http://%s", url);
> +    return eina_stringshare_add(url);
> +}

Qt has some method urlFromUserInput, you should have a look at that
Comment 48 Ryuan Choi 2012-08-23 05:20:16 PDT
Created attachment 160135 [details]
Patch
Comment 49 WebKit Review Bot 2012-08-23 06:04:25 PDT
Comment on attachment 160135 [details]
Patch

Clearing flags on attachment: 160135

Committed r126419: <http://trac.webkit.org/changeset/126419>
Comment 50 WebKit Review Bot 2012-08-23 06:04:32 PDT
All reviewed patches have been landed.  Closing bug.