The EWebLauncher should only create one directory to settings databases while startup. Previous it creates two directories in /tmp and user's home dir which only one was used. Added settings databases path parameter to ewk_init function.
Created attachment 92568 [details] Patch
Attachment 92568 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 92569 [details] Patch 2
Comment on attachment 92569 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=92569&action=review > Source/WebKit/efl/ewk/ewk_main.cpp:66 > +static Eina_Bool _ewk_init_body(const char *settings_db_path); codding style: (const char* settings_db_path) > Source/WebKit/efl/ewk/ewk_main.cpp:80 > +int ewk_init(const char *settings_db_path) The same here. > Source/WebKit/efl/ewk/ewk_main.cpp:166 > +Eina_Bool _ewk_init_body(const char *settings_db_path) And here. > Source/WebKit/efl/ewk/ewk_main.cpp:197 > + if (!settings_db_path) { Since you are creating the temp directory and returning from main() if temp directory creation fails, then it seems that all the code inside this if is dead code. Is it? Would not be better to make the decision about which directory to use in main and make parameter settings_db_path obligatory? > Source/WebKit/efl/ewk/ewk_main.cpp:201 > + // check home directory first codding style: // Check home directory first. > Source/WebKit/efl/ewk/ewk_main.cpp:203 > + // Exit now - otherwise you may have some crash later codding style: missing period at the end of comment. > Source/WebKit/efl/ewk/ewk_main.cpp:205 > + CRITICAL("Can't access HOME dir (or /tmp) %s - no place to save databases: %s", path.utf8().data(), strerror(errnowas)); Why are you mentioning /temp if you made path = WebCore::homeDirectoryPath()?
Thank you for your comments. > codding style: (const char *settings_db_path) Could you tell me what is wrong? I don't see what I have done wrong. Thare are many parameters name in this style in our headers. > > Source/WebKit/efl/ewk/ewk_main.cpp:197 > > + if (!settings_db_path) { > > Since you are creating the temp directory and returning from main() if temp directory creation fails, then it seems that all the code inside this if is dead code. Is it? > Would not be better to make the decision about which directory to use in main and make parameter settings_db_path obligatory? This is good idea but I have decided to not to make revolution in the code. But with pleasure I will do it. :) > > Source/WebKit/efl/ewk/ewk_main.cpp:201 > > + // check home directory first > > codding style: // Check home directory first. > > > Source/WebKit/efl/ewk/ewk_main.cpp:203 > > + // Exit now - otherwise you may have some crash later > > codding style: missing period at the end of comment. This is an old code... anyways what is wrong? Only missing dot at the end of comment?
(In reply to comment #5) > Thank you for your comments. > > > codding style: (const char *settings_db_path) > Could you tell me what is wrong? I don't see what I have done wrong. Thare are many parameters name in this style in our headers. It is just about the * that should be at the type side. it should be char* instead of char *. And you should avoid underlines as well. > > > > Source/WebKit/efl/ewk/ewk_main.cpp:197 > > > + if (!settings_db_path) { > > > > Since you are creating the temp directory and returning from main() if temp directory creation fails, then it seems that all the code inside this if is dead code. Is it? > > Would not be better to make the decision about which directory to use in main and make parameter settings_db_path obligatory? > This is good idea but I have decided to not to make revolution in the code. But with pleasure I will do it. :) If it is about leaving dead code then I think that you should. > > > > Source/WebKit/efl/ewk/ewk_main.cpp:201 > > > + // check home directory first > > > > codding style: // Check home directory first. > > > > > Source/WebKit/efl/ewk/ewk_main.cpp:203 > > > + // Exit now - otherwise you may have some crash later > > > > codding style: missing period at the end of comment. > This is an old code... anyways what is wrong? Only missing dot at the end of comment? Comments should start with a capital letter and end with a period.
Created attachment 93081 [details] Patch 3
(In reply to comment #6) Thank you for your review. Could you lookat my new patch and check if it meets your needs?
Could you explain why you added the stat()/S_ISDIR()/access() calls? WebCore::makeAllDirectories() already performs some access() calls besides calling mkdir() itself, so if you really want to ensure more information about the created path, IMO the changes should go there, not here.
(In reply to comment #9) > Could you explain why you added the stat()/S_ISDIR()/access() calls? WebCore::makeAllDirectories() already performs some access() calls besides calling mkdir() itself, so if you really want to ensure more information about the created path, IMO the changes should go there, not here. If you give a path that point to a directory that already exist (even if the proces doesn't have rights to read or write to it) or to a file. The makeAllDirectory will return true. What is good because the path exist. No directory is needed to create. So, additional checking is needed in our code to test if given path is a directory and if the proces have rights to write and read.
IMO it makes sense to at least check if the path is already a directory in WebCore itself. As for the permission part, if you decide to keep it in WebKit, you could use ecore_file_can_{read,exec,write}.
(In reply to comment #11) > IMO it makes sense to at least check if the path is already a directory in WebCore itself. Lets lookat the makeAllDirectories function: if (!access(fullPath.data(), F_OK)) return true; The access(fullPath.data(), F_OK) function, returns 0 if path already exist. So, I can check there if this is a directory path and if user have rights to it... if not return false insted of true. I may try to make a patch for makeAllDirectories that will do it. But I am not sure if it will be accepted. > As for the permission part, if you decide to keep it in WebKit, you could use ecore_file_can_{read,exec,write}. OK, this is a good idea. Thank you for that. I suggest to keep it if patch for makeAllDirectories will be not accepted.
*** Bug 59238 has been marked as a duplicate of this bug. ***
Just based on the name makeAllDirectories it is not possible to conclude what should it return in case the path already existed and if it is or not a directory, but I would say that permissions are not related to makeAllDirectories. There is no documentation and it seems that different platform implementations differ a bit from each other, but none of the ones I've seen are checking permissions. This patch just considers that makeAllDirectories(path) will not return false if path already is a directory. I like the patch as it is.
(In reply to comment #14) > Just based on the name makeAllDirectories it is not possible to conclude what should it return in case the path already existed and if it is or not a directory, but I would say that permissions are not related to makeAllDirectories. There is no documentation and it seems that different platform implementations differ a bit from each other, but none of the ones I've seen are checking permissions. > > This patch just considers that makeAllDirectories(path) will not return false if path already is a directory. > > I like the patch as it is. I looked at the patch and it is definitely an improvement. I think the above should be considered and can easily be fixed/improved in another patch. Thanks Luiz
Comment on attachment 93081 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=93081&action=review > Tools/EWebLauncher/main.c:880 > - ecore_file_mkpath(path); > - ewk_settings_icon_database_path_set(path); > - ewk_settings_web_database_path_set(path); > + > + if (!ewk_init(path)) I think this is really API related so I would like Lucas input on it
Comment on attachment 93081 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=93081&action=review For the reason pointed above, my informal r- >> Tools/EWebLauncher/main.c:880 >> + if (!ewk_init(path)) > > I think this is really API related so I would like Lucas input on it This is not how EFL works. There's no EFL lib with arguments in *_init() functions because you have to do only the bare minimum to initialize the lib. If you feel like you need an argument in init, you better split that functionality to another function that you call later. In this case, IMO an acceptable patch would be not to set the cache/database/icon directory until the application (EWebLauncher) explicitly tells webkit to do so.
Comment on attachment 93081 [details] Patch 3 r- for above reasons
Close this bug. I agree with Lucas's opinion and there are no comment for a long time.