Bug 86704 – Convert setDomainRelaxationForbiddenForURLScheme to use InternalSettings interface.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Gyuyoung Kim]

Adjust setDomainRelaxationForbiddenForURLScheme tests to use InternalSettings instead of LayoutTestController interface. In my humble opinion, setDomainRelaxationForbiddenForURLScheme() is able to be supported by InternalSettings because it looks this function is implemented by WebCore layer, not WebKit layer. If reviewers think this test function is able to be dealt with by WebKit layer, I will remove this patch.

[Gyuyoung Kim]

Created attachment 142416 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142416 [details]
Patch

Attachment 142416 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12716489

[Build Bot]

Comment on attachment 142416 [details]
Patch

Attachment 142416 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12723245

[Gyuyoung Kim]

Created attachment 142427 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 142427 [details]
Patch

Attachment 142427 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12719441

[Gyuyoung Kim]

Created attachment 142439 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 142439 [details]
Patch

Yay!!

[WebKit Review Bot]

Comment on attachment 142439 [details]
Patch

Clearing flags on attachment: 142439

Committed r117428: <http://trac.webkit.org/changeset/117428>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[John Sullivan]

Safari was using [WebView _setDomainRelaxationForbiddenForURLScheme:], so this change broke the Safari build.

[John Sullivan]

We need to roll this change out or restore [WebKit _setDomainRelaxationForbidden:forURLScheme:] ASAP.

[WebKit Review Bot]

Re-opened since this is blocked by 86748

[Brady Eidson]

I'm not sure if we have guidelines on rules for touching other platform's API layers, but this patch did so a bit recklessly..

In general you have to treat "ClassNamePrivate.h" headers the same as "ClassName.h" headers - the methods are APIs that embedding applications use.

[Brady Eidson]

This change was reverted in http://trac.webkit.org/changeset/117460

[Alexey Proskuryakov]

This has been discussed before in similar bugs. Patches that change *Private.h in WebKit/mac headers need to be reviewed by Apple engineers, who can do at least partial verification that this is OK.

Since routing through LayoutTestController is testing SPI that cannot be removed, this seems like a WONTFIX.

[Brady Eidson]

(In reply to comment #16)
> This has been discussed before in similar bugs. Patches that change *Private.h in WebKit/mac headers need to be reviewed by Apple engineers, who can do at least partial verification that this is OK.

In general it seems like this rule needs to extend to all API layers of all platforms.  This type of change could very easily have broken others as well.

[Eric Seidel (no email)]

My apologies for the build break.

I would strongly encourage folks who wish to monitor Apple's SPI/APIs to add themselves to the watchlist for the apprpriate regexps:
http://trac.webkit.org/wiki/WatchList

That may help catch this sort of thing earlier.

[Gyuyoung Kim]

First of all, I'm so sorry for build problem. However, in my humble opinion, it seems to me this patch is still valuable except for mac API. Because, setDomainRelaxationForbiddenForURLScheme() function just adds|remove a scheme to SchemeRegistry's hash table. So, I thought this patch doesn't need to do something in WebKit layer. 

If mac port is using _setDomainRelaxationForbidden:forURLScheme as public APIs and this APIs should be tested by LTC, I'm enough to understand this patch was wrong. However, this APIs was added to WebViewPrivate.h in mac port. So, it looks this is internal function for testing.

[Adam Barth]

Comment on attachment 142439 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:-145
> -    // Allows disabling domain relaxation.
> -    virtual void setDomainRelaxationForbidden(bool, const WebString& scheme) = 0;

For what it's worth, this part of the change is correct.  This API is not needed by Chromium, and if it were, this API should be on WebSecurityPolicy, not WebView.

[Gyuyoung Kim]

(In reply to comment #20)
> (From update of attachment 142439 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142439&action=review
> 
> > Source/WebKit/chromium/public/WebView.h:-145
> > -    // Allows disabling domain relaxation.
> > -    virtual void setDomainRelaxationForbidden(bool, const WebString& scheme) = 0;
> 
> For what it's worth, this part of the change is correct.  This API is not needed by Chromium, and if it were, this API should be on WebSecurityPolicy, not WebView.

If this patch can't be landed, I will file new bug for your comment.

[Brady Eidson]

(In reply to comment #19)
> First of all, I'm so sorry for build problem. However, in my humble opinion, it seems to me this patch is still valuable except for mac API. 

No one was commenting on the value of the patch.  However, I disagree that moving forward with it was okay "except for mac API."

The patch changed 3 platform's API layers.  Clearly the chromium platform change was reviewed and okay'ed by chromium folks.

But it changed both Mac and Windows API without getting an all clear from their ports.

Fortunately the gtk and qt changes were kept contained within DRT support specifically.
 
> However, this APIs was added to WebViewPrivate.h in mac port. So, it looks this is internal function for testing.

And IWebViewPrivate.dll in the windows port.  As I mentioned earlier:

> In general you have to treat "ClassNamePrivate.h" headers the same as "ClassName.h" headers - the methods are APIs that embedding applications use.

Mac and Windows have the following naming schemes for headers/idls:
"ClassName.h/idl" for published API
"ClassNamePrivate.h/idl" for unpublished API (but still a big binary compatibility risk)
"ClassNameInternal.h/idl" for functions 100% internal to WebKit.

Perhaps this needs to be better documented somewhere, but that is how it has always been since the start of the project.

[Gyuyoung Kim]

(In reply to comment #22)
I agree with your opinion. We can only remove | move APIs when there are no issues related to APIs in all ports.