[Webkit-unassigned] [Bug 63966] [EFL] Add url bar to EWebLauncher.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=63966





--- Comment #14 from Raphael Kubo da Costa <kubo at profusion.mobi>  2011-07-13 10:25:08 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.