Add url bar to EWebLauncher.
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.
Created attachment 99778 [details] Snapshot of this patch.
LGTM. Kubo and Lucas, how do you think about it ?
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 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 :(
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).
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).
(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?
(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.
Created attachment 100443 [details] Another patch using edje. Another patch using edje which Kubo recommanded.
Comment on attachment 100443 [details] Another patch using edje. Next time, please give r? and c?.
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?
Created attachment 100600 [details] patch for Lucas's comment.
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.
(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.
(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}
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 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 ?
Created attachment 100766 [details] Make a font size to 12, modify the copy right.
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.
(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 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.
(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 )
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
(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} )
(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.
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.
> 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.
\> > 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.
Created attachment 102086 [details] patch for kubo's comment. Remove unnecessary exception code. Modify the change log.
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.
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.
Created attachment 102089 [details] patch for kubo's & kuyung's comments.
(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 on attachment 102089 [details] patch for kubo's & kuyung's comments. Clearing flags.
Created attachment 153752 [details] Patch
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.
Created attachment 153963 [details] Patch
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.
(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.
Created attachment 154007 [details] rebased for style 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.
Created attachment 154008 [details] rebased for style bot2
Created attachment 156613 [details] Patch
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 on attachment 156613 [details] Patch LGTM.
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
Created attachment 160135 [details] Patch
Comment on attachment 160135 [details] Patch Clearing flags on attachment: 160135 Committed r126419: <http://trac.webkit.org/changeset/126419>
All reviewed patches have been landed. Closing bug.