Bug 84577 - [EFL] [DRT] Implement setDomainRelaxationForbiddenForURLScheme in EFL DRT
: [EFL] [DRT] Implement setDomainRelaxationForbiddenForURLScheme in EFL DRT
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Sudarsana Nagineni (babu)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 02:04 PDT by Sudarsana Nagineni (babu)
Modified: 2012-06-14 01:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.23 KB, patch)
2012-04-23 07:01 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
patch (5.24 KB, patch)
2012-04-24 05:35 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2012-05-09 03:54 PDT, Sudarsana Nagineni (babu)
gyuyoung.kim: review-
Details | Formatted Diff | Diff
Patch (1.27 KB, patch)
2012-05-17 05:24 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
patch (5.17 KB, patch)
2012-05-29 06:15 PDT, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2012-04-23 02:04:23 PDT
Implement setDomainRelaxationForbiddenForURLScheme method in EFL's LayoutTestController.
Comment 1 Sudarsana Nagineni (babu) 2012-04-23 07:01:31 PDT
Created attachment 138340 [details]
Patch

Implement setDomainRelaxationForbiddenForURLScheme
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-04-23 19:01:56 PDT
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()).
Comment 3 Sudarsana Nagineni (babu) 2012-04-24 05:33:50 PDT
(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.
Comment 4 Sudarsana Nagineni (babu) 2012-04-24 05:35:14 PDT
Created attachment 138542 [details]
patch

fixed comment #2
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-04-24 13:43:18 PDT
Comment on attachment 138542 [details]
patch

Looks good.
Comment 6 Martin Robinson 2012-04-27 09:35:45 PDT
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.
Comment 7 Martin Robinson 2012-04-27 09:35:54 PDT
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.
Comment 8 Sudarsana Nagineni (babu) 2012-04-27 14:39:00 PDT
(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.
Comment 9 Sudarsana Nagineni (babu) 2012-05-09 03:54:07 PDT
Created attachment 140907 [details]
Patch

rebased
Comment 10 Sudarsana Nagineni (babu) 2012-05-10 06:36:58 PDT
Martin, could you please have a look at this bug again?
Comment 11 Sudarsana Nagineni (babu) 2012-05-16 07:48:47 PDT
Comment on attachment 140907 [details]
Patch

Needs rebasing.
Comment 12 Gyuyoung Kim 2012-05-16 17:53:00 PDT
Comment on attachment 140907 [details]
Patch

Looks fine.
Comment 13 Gyuyoung Kim 2012-05-17 04:16:26 PDT
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 ?
Comment 14 Sudarsana Nagineni (babu) 2012-05-17 05:14:04 PDT
(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.
Comment 15 Sudarsana Nagineni (babu) 2012-05-17 05:24:51 PDT
Created attachment 142457 [details]
Patch

Unskip passing test case.
Comment 16 Grzegorz Czajkowski 2012-05-17 06:12:05 PDT
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
Comment 17 Grzegorz Czajkowski 2012-05-21 02:37:20 PDT
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.
Comment 18 Gyuyoung Kim 2012-05-29 02:05:27 PDT
(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.
Comment 19 Sudarsana Nagineni (babu) 2012-05-29 06:15:57 PDT
Created attachment 144537 [details]
patch

r117428 from 86704 was rolled out, so restoring the original patch.
Comment 20 Gyuyoung Kim 2012-05-29 09:37:21 PDT
Comment on attachment 144537 [details]
patch

Looks good to me.
Comment 21 WebKit Review Bot 2012-06-14 01:01:07 PDT
Comment on attachment 144537 [details]
patch

Clearing flags on attachment: 144537

Committed r120294: <http://trac.webkit.org/changeset/120294>
Comment 22 WebKit Review Bot 2012-06-14 01:01:13 PDT
All reviewed patches have been landed.  Closing bug.