Bug 83121 – [EFL] Add setting API for allow universal/file access from file URLs.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kamil Blank]

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.

[Kamil Blank]

Created attachment 135526 [details]
patch

[Gyuyoung Kim]

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.

[Kamil Blank]

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

[Gyuyoung Kim]

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.

[Kamil Blank]

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

[Tomasz Morawski]

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

[Gyuyoung Kim]

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

[Kamil Blank]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Gyuyoung Kim]

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

[Kamil Blank]

Created attachment 135998 [details]
patch

Added description in:
Source/WebKit/efl/ChangeLog
Source/WebKit/efl/ewk/ewk_view.h

[Grzegorz Czajkowski]

LGTM.

[Kamil Blank]

Do you have any comments to current patch?

[Grzegorz Czajkowski]

(In reply to comment #13)
> Do you have any comments to current patch?

From my side LGTM.

[Kamil Blank]

To prevent similar situation to https://bugs.webkit.org/show_bug.cgi?id=82860 I'll add reset settings inside DRT

[Kamil Blank]

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)

[Gyuyoung Kim]

Comment on attachment 138090 [details]
patch

In my opinion, it is better to split this patch into two patches. But, looks fine.

[Kamil Blank]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Kamil Blank]

Created attachment 139145 [details]
patch

[Kamil Blank]

Created attachment 139160 [details]
patch

[Kamil Blank]

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

[Gyuyoung Kim]

Comment on attachment 139160 [details]
patch

LGTM.

[Thiago Marcos P. Santos]

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.

[Eric Seidel (no email)]

Comment on attachment 139160 [details]
patch

r+, based on above LGTMs.

[WebKit Review Bot]

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

[Kamil Blank]

Created attachment 161150 [details]
patch

Patch rebased.

[Chris Dumez]

Comment on attachment 161150 [details]
patch

No cq flag?

[WebKit Review Bot]

Comment on attachment 161150 [details]
patch

Clearing flags on attachment: 161150

Committed r127024: <http://trac.webkit.org/changeset/127024>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.