Bug 84577 – [EFL] [DRT] Implement setDomainRelaxationForbiddenForURLScheme in EFL DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

Implement setDomainRelaxationForbiddenForURLScheme method in EFL's LayoutTestController.

[Sudarsana Nagineni (babu)]

Created attachment 138340 [details]
Patch

Implement setDomainRelaxationForbiddenForURLScheme

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138340&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:611
> +    DumpRenderTreeSupportEfl::setDomainRelaxationForbiddenForURLScheme(forbidden, scheme->ustring().utf8().data());

Please pass a WTF::String directly in the second argument to avoid unneeded const char* conversions. You can create a WTF::String very easily via String(scheme->ustring().impl()).

[Sudarsana Nagineni (babu)]

(In reply to comment #2)
> (From update of attachment 138340 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138340&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:611
> > +    DumpRenderTreeSupportEfl::setDomainRelaxationForbiddenForURLScheme(forbidden, scheme->ustring().utf8().data());
> 
> Please pass a WTF::String directly in the second argument to avoid unneeded const char* conversions. You can create a WTF::String very easily via String(scheme->ustring().impl()).

Thanks for the review Raphael.

[Sudarsana Nagineni (babu)]

Created attachment 138542 [details]
patch

fixed comment #2

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 138542 [details]
patch

Looks good.

[Martin Robinson]

Comment on attachment 138542 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138542&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:612
> +    DumpRenderTreeSupportEfl::setDomainRelaxationForbiddenForURLScheme(forbidden, String(scheme->ustring().impl()));

Is it common to reach into the guts of a JSStringRef. From what I've seen of the DRT, typically you use the JavaScriptCore API to convert into a char* or a String.

[Martin Robinson]

Comment on attachment 138542 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138542&action=review

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:612
> +    DumpRenderTreeSupportEfl::setDomainRelaxationForbiddenForURLScheme(forbidden, String(scheme->ustring().impl()));

Is it common to reach into the guts of a JSStringRef? From what I've seen of the DRT, typically you use the JavaScriptCore API to convert into a char* or a String.

[Sudarsana Nagineni (babu)]

(In reply to comment #7)
> (From update of attachment 138542 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138542&action=review
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:612
> > +    DumpRenderTreeSupportEfl::setDomainRelaxationForbiddenForURLScheme(forbidden, String(scheme->ustring().impl()));
> 
> Is it common to reach into the guts of a JSStringRef? From what I've seen of the DRT, typically you use the JavaScriptCore API to convert into a char* or a String.

Thanks for the review. Looked around the EFL's DRT code and this is how we have been doing the convertion everywhere. For example we are converting JSStringRef into a char* like 'jsstringref'->ustring().utf8().data(). So, I thought it's Ok to construct a String directly without using JavaScriptCore API.

[Sudarsana Nagineni (babu)]

Created attachment 140907 [details]
Patch

rebased

[Sudarsana Nagineni (babu)]

Martin, could you please have a look at this bug again?

[Sudarsana Nagineni (babu)]

Comment on attachment 140907 [details]
Patch

Needs rebasing.

[Gyuyoung Kim]

Comment on attachment 140907 [details]
Patch

Looks fine.

[Gyuyoung Kim]

Comment on attachment 140907 [details]
Patch

R117428 moves setDomainRelaxationForbiddenForURLScheme to InternalSettings.
http://trac.webkit.org/changeset/117428

So, this patch just is enough to touches LayoutTests/platform/efl/Skipped file. Could you please modify this patch ?

[Sudarsana Nagineni (babu)]

(In reply to comment #13)
> (From update of attachment 140907 [details])
> R117428 moves setDomainRelaxationForbiddenForURLScheme to InternalSettings.
> http://trac.webkit.org/changeset/117428
> 
> So, this patch just is enough to touches LayoutTests/platform/efl/Skipped file. Could you please modify this patch ?

Thanks for notifying me. I will update the patch now.

[Sudarsana Nagineni (babu)]

Created attachment 142457 [details]
Patch

Unskip passing test case.

[Grzegorz Czajkowski]

Comment on attachment 142457 [details]
Patch

It is possible that r117428 will be reverted because it may cause build break on Safari. Let's wait for final decision

[Grzegorz Czajkowski]

Hi Gyuyoung,
Do you think that the previous Sudarsana Nagineni's patch with setDomainRelaxationForbiddenForURLScheme implementation should be restored? It looks like there are some discussion about 86704 so we cannot just unskip http/tests/security/setDomainRelaxationForbiddenForURLScheme.html.

[Gyuyoung Kim]

(In reply to comment #17)
> Hi Gyuyoung,
> Do you think that the previous Sudarsana Nagineni's patch with setDomainRelaxationForbiddenForURLScheme implementation should be restored? It looks like there are some discussion about 86704 so we cannot just unskip http/tests/security/setDomainRelaxationForbiddenForURLScheme.html.

Yes, please restore this patch. My original patch was reverted because of Safari build.

[Sudarsana Nagineni (babu)]

Created attachment 144537 [details]
patch

r117428 from 86704 was rolled out, so restoring the original patch.

[Gyuyoung Kim]

Comment on attachment 144537 [details]
patch

Looks good to me.

[WebKit Review Bot]

Comment on attachment 144537 [details]
patch

Clearing flags on attachment: 144537

Committed r120294: <http://trac.webkit.org/changeset/120294>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.