Bug 63966 – [EFL] Add url bar to EWebLauncher and MiniBrowser/Efl.

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

[Hyerim Bae]

Add url bar to EWebLauncher.

[Hyerim Bae]

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.

[Hyerim Bae]

Created attachment 99778 [details]
Snapshot of this patch.

[Gyuyoung Kim]

LGTM. Kubo and Lucas, how do you think about it ?

[Raphael Kubo da Costa (:rakuco)]

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).

[Ryuan Choi]

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 :(

[Hyerim Bae]

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).

[Hyerim Bae]

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).

[Raphael Kubo da Costa (:rakuco)]

(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?

[Hyerim Bae]

(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.

[Hyerim Bae]

Created attachment 100443 [details]
Another patch using edje.

Another patch using edje which Kubo recommanded.

[Ryuan Choi]

Comment on attachment 100443 [details]
Another patch using edje.

Next time, please give r? and c?.

[Lucas De Marchi]

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?

[Hyerim Bae]

Created attachment 100600 [details]
patch for Lucas's comment.

[Raphael Kubo da Costa (:rakuco)]

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.

[Hyerim Bae]

(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.

[Ryuan Choi]

(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}

[Hyerim Bae]

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.

[Gyuyoung Kim]

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 ?

[Hyerim Bae]

Created attachment 100766 [details]
Make a font size to 12, modify the copy right.

[Raphael Kubo da Costa (:rakuco)]

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.

[Hyerim Bae]

(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 ?

[Raphael Kubo da Costa (:rakuco)]

(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.

[Hyerim Bae]

(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
)

[Raphael Kubo da Costa (:rakuco)]

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

[Hyerim Bae]

(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}
)

[Raphael Kubo da Costa (:rakuco)]

(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.

[Hyerim Bae]

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.

[Raphael Kubo da Costa (:rakuco)]

> 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.

[Hyerim Bae]

\> > 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.

[Hyerim Bae]

Created attachment 102086 [details]
patch for kubo's comment.

Remove unnecessary exception code.
Modify the change log.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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.

[Hyerim Bae]

Created attachment 102089 [details]
patch for kubo's & kuyung's comments.

[Hyerim Bae]

(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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 102089 [details]
patch for kubo's & kuyung's comments.

Clearing flags.

[Ryuan Choi]

Created attachment 153752 [details]
Patch

[Gyuyoung Kim]

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.

[Ryuan Choi]

Created attachment 153963 [details]
Patch

[WebKit Review Bot]

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.

[Ryuan Choi]

(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.

[Ryuan Choi]

Created attachment 154007 [details]
rebased for style bot

[WebKit Review Bot]

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.

[Ryuan Choi]

Created attachment 154008 [details]
rebased for style bot2

[Ryuan Choi]

Created attachment 156613 [details]
Patch

[Gyuyoung Kim]

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.

[Chris Dumez]

Comment on attachment 156613 [details]
Patch

LGTM.

[Kenneth Rohde Christiansen]

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

[Ryuan Choi]

Created attachment 160135 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160135 [details]
Patch

Clearing flags on attachment: 160135

Committed r126419: <http://trac.webkit.org/changeset/126419>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.