Bug 85034 – Convert setPrivateBrowsingEnabled to use InternalSettings interface

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

[Gyuyoung Kim]

Adjust setPrivateBrowsingEnabled tests to use Internals instead of LayoutTestController interface. In my opinion, setPrivateBrowsingEnabled() is able to be supported by Internals because this feature just enables/disables private browsing feature in page settings. I found this function in our contributor meeting's hackaton. http://trac.webkit.org/wiki/Internals_Hackathon.

[Gyuyoung Kim]

Created attachment 139158 [details]
Patch for EWS

[Build Bot]

Comment on attachment 139158 [details]
Patch for EWS

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

[Philippe Normand]

Comment on attachment 139158 [details]
Patch for EWS

Attachment 139158 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12556160

[Alexey Proskuryakov]

I'm not sure that I'm a fan of this change. WebKit part of this is non-trivial, with preferences being involved. It's quite easy to break WebKit part, and hard to discover that when using nightly builds.

Every mature port is going to have an API for private browsing, so it's not like we're dating any WebKit effort by not using the API.

[Gyuyoung Kim]

Created attachment 139420 [details]
Patch for EWS-2

[Gyuyoung Kim]

(In reply to comment #4)
> I'm not sure that I'm a fan of this change. WebKit part of this is non-trivial, with preferences being involved. It's quite easy to break WebKit part, and hard to discover that when using nightly builds.
> 
> Every mature port is going to have an API for private browsing, so it's not like we're dating any WebKit effort by not using the API.

In my humble opinion, this patch is valuable to reduce duplicated LTC functions. As you know, LTC in each port have implemented "setPrivateBrowsingEnabled()" in order to just enable/disable private browsing. It seems to me that this APIs are able to be replaced with InternalSettings APIs. 

InternalSettings already have supported js test APIs for setting.
http://trac.webkit.org/browser/trunk/Source/WebCore/testing/InternalSettings.h

In my opinion, we don't need to keep duplicating functions in LTC by this patch.

CC'ing Ryosuke,

[Alexey Proskuryakov]

duplicating code in LayoutTestController is an obvious cost, but let's not forget that the reason why we have regression testing is to catch regressions. Where there is an API, testing it is way more important than saving the effort of writing a few lines of code in LayoutTestController.

[Gyuyoung Kim]

(In reply to comment #7)
> duplicating code in LayoutTestController is an obvious cost, but let's not forget that the reason why we have regression testing is to catch regressions. Where there is an API, testing it is way more important than saving the effort of writing a few lines of code in LayoutTestController.

I thought this patch is accorded with moving from LTC to Internals. If other reviewers don't like this patch as well, I give up this patch. But, I'm still thinking this patch is not bad to land.

[Alexey Proskuryakov]

As mentioned at contributor meeting hackathon, there are several things to consider when making the decision whether to move a LayoutTestController method:

- Everything that makes many ports skip tests should be considered.
- Functions that use internal WebKit methods whose only purpose is to support DRT (as opposed as providing public API) should be strongly considered.
- Functions that rely upon a significant amount of WebKit code may be best to remain in LayoutTestController, so that this code would be tested. 

In this case, the stated reasons for proposed change are (1) because WebKit layer is not very complicated, and (2) to reduce code duplication in LTC. I don't think that these are necessarily sufficient - it's best to look at changes that offer practical benefits along aforementioned lines first, and only then tackle ones where cost to benefit is questionable.

[Ryosuke Niwa]

Comment on attachment 139420 [details]
Patch for EWS-2

It seems like this patch is missing the changes to Source/WebCore/testing/InternalSettings.idl

[Gyuyoung Kim]

Created attachment 139575 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 139420 [details])
> It seems like this patch is missing the changes to Source/WebCore/testing/InternalSettings.idl

Oops, I'm sorry. patch in WebCore was missing.

[Alexey Proskuryakov]

Comment on attachment 139575 [details]
Patch

r-, because as discussed above, we shouldn't be doing this, or at least shouldn't start with this. Making less contentious moves would give us more experience as to whether to want this, too.

[Gyuyoung Kim]

(In reply to comment #13)
> (From update of attachment 139575 [details])
> r-, because as discussed above, we shouldn't be doing this, or at least shouldn't start with this. Making less contentious moves would give us more experience as to whether to want this, too.

Now I'm understanding what your point as below,

Every ports don't support this feature yet. But, this feature will be supported by mature ports soon. So, it is better to test this feature by API, right?

If not so, please let me know your point.

Anyway, in this case, LTC implementation is not complicate as well as aim of test is clear. In addition, all ports supports private browsing feature, can we move this to internals further? Because, this is common feature supported by all ports.

I don't still understand why this moving can break WebKit part easily. If you don't mind, could you explain your concern again ?

[Ryosuke Niwa]

(In reply to comment #14)
> Anyway, in this case, LTC implementation is not complicate as well as aim of test is clear. In addition, all ports supports private browsing feature, can we move this to internals further? Because, this is common feature supported by all ports.

Does this setting need to be plumbed through WebKit2 layer?

> I don't still understand why this moving can break WebKit part easily. If you don't mind, could you explain your concern again ?

I suspect that things like private browsing would depend on WebKit code, not just WebCore code.

[Alexey Proskuryakov]

> I don't still understand why this moving can break WebKit part easily. If you don't mind, could you explain your concern again ?

This change removes testing of WebKit layer, so future regressions would go undetected.

[Gyuyoung Kim]

(In reply to comment #16)
> > I don't still understand why this moving can break WebKit part easily. If you don't mind, could you explain your concern again ?
> 
> This change removes testing of WebKit layer, so future regressions would go undetected.

If there are something to do for private browsing, I don't tried to move to internals. However, this is just to enable / disable private browsing feature. So, I thought there is no possibility to be plumbed by WebKit1/WebKit2 layer. If you guys think port maintainers will modify these functions to do something further or add something into this LTC implementation in future, I also agree with your opinion.

[Alexey Proskuryakov]

*** Bug 87278 has been marked as a duplicate of this bug. ***