Bug 84577 - [EFL] [DRT] Implement setDomainRelaxationForbiddenForURLScheme in EFL DRT
: [EFL] [DRT] Implement setDomainRelaxationForbiddenForURLScheme in EFL DRT
Status: RESOLVED FIXED
: WebKit
WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-23 02:04 PST by
Modified: 2012-06-14 01:01 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-23 02:04:23 PST
Implement setDomainRelaxationForbiddenForURLScheme method in EFL's LayoutTestController.
------- Comment #1 From 2012-04-23 07:01:31 PST -------
Created an attachment (id=138340) [details]
Patch

Implement setDomainRelaxationForbiddenForURLScheme
------- Comment #2 From 2012-04-23 19:01:56 PST -------
(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()).
------- Comment #3 From 2012-04-24 05:33:50 PST -------
(In reply to comment #2)
> (From update of attachment 138340 [details] [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 From 2012-04-24 05:35:14 PST -------
Created an attachment (id=138542) [details]
patch

fixed comment #2
------- Comment #5 From 2012-04-24 13:43:18 PST -------
(From update of attachment 138542 [details])
Looks good.
------- Comment #6 From 2012-04-27 09:35:45 PST -------
(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.
------- Comment #7 From 2012-04-27 09:35:54 PST -------
(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.
------- Comment #8 From 2012-04-27 14:39:00 PST -------
(In reply to comment #7)
> (From update of attachment 138542 [details] [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 From 2012-05-09 03:54:07 PST -------
Created an attachment (id=140907) [details]
Patch

rebased
------- Comment #10 From 2012-05-10 06:36:58 PST -------
Martin, could you please have a look at this bug again?
------- Comment #11 From 2012-05-16 07:48:47 PST -------
(From update of attachment 140907 [details])
Needs rebasing.
------- Comment #12 From 2012-05-16 17:53:00 PST -------
(From update of attachment 140907 [details])
Looks fine.
------- Comment #13 From 2012-05-17 04:16:26 PST -------
(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 ?
------- Comment #14 From 2012-05-17 05:14:04 PST -------
(In reply to comment #13)
> (From update of attachment 140907 [details] [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 From 2012-05-17 05:24:51 PST -------
Created an attachment (id=142457) [details]
Patch

Unskip passing test case.
------- Comment #16 From 2012-05-17 06:12:05 PST -------
(From update of attachment 142457 [details])
It is possible that r117428 will be reverted because it may cause build break on Safari. Let's wait for final decision
------- Comment #17 From 2012-05-21 02:37:20 PST -------
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 From 2012-05-29 02:05:27 PST -------
(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 From 2012-05-29 06:15:57 PST -------
Created an attachment (id=144537) [details]
patch

r117428 from 86704 was rolled out, so restoring the original patch.
------- Comment #20 From 2012-05-29 09:37:21 PST -------
(From update of attachment 144537 [details])
Looks good to me.
------- Comment #21 From 2012-06-14 01:01:07 PST -------
(From update of attachment 144537 [details])
Clearing flags on attachment: 144537

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