Add setting API for allow universal/file access from file URLs. Patch contains: + API for EWK + Enable layout test connected with setAllowUniversalAccessFromFileURLs and setAllowFileAccessFromFileURLs. + DRT: Implementation of setAllowUniversalAccessFromFileURLs and setAllowFileAccessFromFileURLs.
Created attachment 135526 [details] patch
Comment on attachment 135526 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=135526&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:2445 > +Eina_Bool ewk_view_setting_allow_universal_access_from_file_urls_set(Evas_Object* ewkView, Eina_Bool enable) I'm not sure whether we add new APIs for this setting obstinately. According to I take a look other ports, almost ports don't support this feature as APIs. If you don't think that EFL port should support this setting feature, I think it is better to move from ewk_view to DumpRenderTreeSupportEfl for just testing.
(In reply to comment #2) > (From update of attachment 135526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135526&action=review > > > Source/WebKit/efl/ewk/ewk_view.cpp:2445 > > +Eina_Bool ewk_view_setting_allow_universal_access_from_file_urls_set(Evas_Object* ewkView, Eina_Bool enable) > > I'm not sure whether we add new APIs for this setting obstinately. According to I take a look other ports, almost ports don't support this feature as APIs. If you don't think that EFL port should support this setting feature, I think it is better to move from ewk_view to DumpRenderTreeSupportEfl for just testing. As I checked both settings are supported by API in QT, GTK and Chromium ports. Isn't it a good reason to add them also in EFL port?
Comment on attachment 135526 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=135526&action=review >>> Source/WebKit/efl/ewk/ewk_view.cpp:2445 >>> +Eina_Bool ewk_view_setting_allow_universal_access_from_file_urls_set(Evas_Object* ewkView, Eina_Bool enable) >> >> I'm not sure whether we add new APIs for this setting obstinately. According to I take a look other ports, almost ports don't support this feature as APIs. If you don't think that EFL port should support this setting feature, I think it is better to move from ewk_view to DumpRenderTreeSupportEfl for just testing. > > As I checked both settings are supported by API in QT, GTK and Chromium ports. Isn't it a good reason to add them also in EFL port? I don't think we can add new APIs without obvious reason. I think you can add this API to DumpRenderTreeSupportEfl first for testing, then we can move this function to API layer when we need to support this functionality for application. If you have obvious reason, please let me know.
(In reply to comment #4) > (From update of attachment 135526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135526&action=review > > >>> Source/WebKit/efl/ewk/ewk_view.cpp:2445 > >>> +Eina_Bool ewk_view_setting_allow_universal_access_from_file_urls_set(Evas_Object* ewkView, Eina_Bool enable) > >> > >> I'm not sure whether we add new APIs for this setting obstinately. According to I take a look other ports, almost ports don't support this feature as APIs. If you don't think that EFL port should support this setting feature, I think it is better to move from ewk_view to DumpRenderTreeSupportEfl for just testing. > > > > As I checked both settings are supported by API in QT, GTK and Chromium ports. Isn't it a good reason to add them also in EFL port? > > I don't think we can add new APIs without obvious reason. What defines obvious reason?
(In reply to comment #2) > (From update of attachment 135526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135526&action=review > > > Source/WebKit/efl/ewk/ewk_view.cpp:2445 > > +Eina_Bool ewk_view_setting_allow_universal_access_from_file_urls_set(Evas_Object* ewkView, Eina_Bool enable) > > I'm not sure whether we add new APIs for this setting obstinately. According to I take a look other ports, almost ports don't support this feature as APIs. If you don't think that EFL port should support this setting feature, I think it is better to move from ewk_view to DumpRenderTreeSupportEfl for just testing. I take a look other ports too it seems that Chromium port and win port use this setting as API. Please see: Source/WebKit/chromium/public/WebSettings.h Source/WebKit/win/WebPreferences.h After adding this settings, the DRT code will look more clear and objective. Anyway, after removing all setting functions by replace it by single get/set setting functions will resolve your concerns related to API changes.
(In reply to comment #5) > > > As I checked both settings are supported by API in QT, GTK and Chromium ports. Isn't it a good reason to add them also in EFL port? > > > > I don't think we can add new APIs without obvious reason. > > What defines obvious reason? I mean obvious reason is that application needs to set this setting for specific functionality. For example, web app needs to access file access by file name. If we wanna add new API, in my humble opinion, we need to show this API's role or functionality. I think it is not enough to add new APIs because other ports has similar APIs. You didn't explain when this API will be used or when application needs this functionality. By the way, If bug 83007 supports universal setting set/get APIs, it looks argument like this will be reduced.
(In reply to comment #7) > (In reply to comment #5) > > > > > As I checked both settings are supported by API in QT, GTK and Chromium ports. Isn't it a good reason to add them also in EFL port? > > > > > > I don't think we can add new APIs without obvious reason. > > > > What defines obvious reason? > > I mean obvious reason is that application needs to set this setting for specific functionality. For example, web app needs to access file access by file name. If we wanna add new API, in my humble opinion, we need to show this API's role or functionality. I think it is not enough to add new APIs because other ports has similar APIs. You didn't explain when this API will be used or when application needs this functionality. > Following Qt documentation: 1) setAllowUniversalAccessFromFileURLs - Specifies whether locally loaded documents are allowed to access remote urls 2) setAllowFileAccessFromFileURLs - Specifies whether locally loaded documents are allowed to access other local urls. Do you want me to add it in bug description (Changelog) ?
Comment on attachment 135526 [details] patch IMO it makes sense to keep this as publicly exposed API: being able to control feels useful enough to me (I admit this is subjective, though). The documentation should be improved with the details from the previous comment; the more detailed the documentation, the better.
(In reply to comment #8) > Following Qt documentation: > 1) setAllowUniversalAccessFromFileURLs - Specifies whether locally loaded documents are allowed to access remote urls > > 2) setAllowFileAccessFromFileURLs - Specifies whether locally loaded documents are allowed to access other local urls. > > Do you want me to add it in bug description (Changelog) ? Ok, I think it is better to add above description to ChangeLog.
Created attachment 135998 [details] patch Added description in: Source/WebKit/efl/ChangeLog Source/WebKit/efl/ewk/ewk_view.h
LGTM.
Do you have any comments to current patch?
(In reply to comment #13) > Do you have any comments to current patch? From my side LGTM.
To prevent similar situation to https://bugs.webkit.org/show_bug.cgi?id=82860 I'll add reset settings inside DRT
Created attachment 138090 [details] patch Changes: + Added reset for settings insideDumpRenderTree/efl/DumpRenderTreeChrome.cpp + 1 test more is removed from Skipped list - missing function were implemented in the meantime (fast/xmlhttprequest/xmlhttprequest-no-file-access.html)
Comment on attachment 138090 [details] patch In my opinion, it is better to split this patch into two patches. But, looks fine.
(In reply to comment #17) > (From update of attachment 138090 [details]) > In my opinion, it is better to split this patch into two patches. But, looks fine. Do you think about 1 patch containing only EWK API and 2nd one containing DRT things? Is it really needed? It's not that big patch ;)
Comment on attachment 138090 [details] patch The patch looks mostly OK to me, but please document what the default for each of the newly introduced settings is.
Created attachment 139145 [details] patch
Created attachment 139160 [details] patch
(In reply to comment #19) > (From update of attachment 138090 [details]) > The patch looks mostly OK to me, but please document what the default for each of the newly introduced settings is. Done.
Comment on attachment 139160 [details] patch LGTM.
Is there anyone working on this for WK2? We have a test suite here that really needs this feature to be configurable via command line option at the MiniBrowser.
Comment on attachment 139160 [details] patch r+, based on above LGTMs.
Comment on attachment 139160 [details] patch Rejecting attachment 139160 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: eChrome.cpp Hunk #1 FAILED at 221. 1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp.rej patching file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp Hunk #1 FAILED at 352. 1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13565452
Created attachment 161150 [details] patch Patch rebased.
Comment on attachment 161150 [details] patch No cq flag?
Comment on attachment 161150 [details] patch Clearing flags on attachment: 161150 Committed r127024: <http://trac.webkit.org/changeset/127024>
All reviewed patches have been landed. Closing bug.