Bug 122771 – Move TestRunner::setCacheModel to Window::Internals

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Andre Loureiro (IRC: loureiro)]

Since Mac, Efl, Win and Gtk ports implement this method we could move this to Internals.

[Andre Loureiro (IRC: loureiro)]

Created attachment 214732 [details]
WIP Patch for EWS testing

Patch to check WIN build

[WebKit Commit Bot]

Attachment 214732 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/HTMLScriptElement/nested-execution.html', u'LayoutTests/storage/domstorage/localstorage/close-idle-localstorage-databases-immediately.html', u'LayoutTests/storage/domstorage/storage-close-database-on-idle.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/webkit/webkitglobals.cpp', u'Source/WebKit/gtk/webkit/webkitglobals.h', u'Tools/ChangeLog', u'Tools/DumpRenderTree/TestRunner.cpp', u'Tools/DumpRenderTree/TestRunner.h', u'Tools/DumpRenderTree/blackberry/TestRunnerBlackBerry.cpp', u'Tools/DumpRenderTree/efl/TestRunnerEfl.cpp', u'Tools/DumpRenderTree/gtk/TestRunnerGtk.cpp', u'Tools/DumpRenderTree/mac/TestRunnerMac.mm', u'Tools/DumpRenderTree/win/TestRunnerWin.cpp']" exit_code: 1
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/gtk/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/testing/Internals.cpp:2095:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebCore/testing/Internals.cpp:2096:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebCore/testing/Internals.cpp:2097:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebCore/testing/Internals.cpp:2099:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 8 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 214732 [details]
WIP Patch for EWS testing

Attachment 214732 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/8858037

[Alexey Proskuryakov]

Cache model is a WebKit concept, not a WebCore one. I don't think that it makes sense to "test" it in WebCore.

[Andre Loureiro (IRC: loureiro)]

(In reply to comment #4)
> Cache model is a WebKit concept, not a WebCore one. I don't think that it makes sense to "test" it in WebCore.

Hi, I see, but don't you think that it could be be considered to move to internals since only gtk port implements this feature? if not, could you explain better this issue?

[Alexey Proskuryakov]

Why are you saying that only Gtk implements this? -[WebView setCacheModel:] is part of Mac WebKit1 API, and this is what's exposed through TestRunner. Looking at the patch, it changes Efl too.

[Build Bot]

Comment on attachment 214732 [details]
WIP Patch for EWS testing

Attachment 214732 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/8878166

[Andre Loureiro (IRC: loureiro)]

(In reply to comment #6)
> Why are you saying that only Gtk implements this? -[WebView setCacheModel:] is part of Mac WebKit1 API, and this is what's exposed through TestRunner. Looking at the patch, it changes Efl too.

Hi Alexey,
Yes you are right, but what I'd like to say is, besides Mac port others ports has implementation to this method, for example, Gtk and EFL as well, so according with http://trac.webkit.org/wiki/Internals_Hackathon I thought that this method could be considered to move to Internals (just to reduce duplicated code), as I did in this other issue here https://bugs.webkit.org/show_bug.cgi?id=84398. Although, as you said before, cache model is a webkit concept and not a webcore concept, then my question is, which methods could be considered to move to Internals according the Internals Hackathon?

Thanks in advance.

[Alexey Proskuryakov]

One way to look at this is that we considered doing so in this bug, and decided against doing so. If you agree, you can just remove the suggestion from the wiki page, referencing this bug in a history comment.

There is no hard rule - as the obvious things were already moved to internals, moving anything else probably reduces test coverage on platforms that expose the functionality as WebKit API, but at the same time it also reduced code duplication between ports. In a case like this, where it's conceptually a WebKit feature anyway, I think that it's better to keep going through API where one exists.

[Andre Loureiro (IRC: loureiro)]

(In reply to comment #9)
> One way to look at this is that we considered doing so in this bug, and decided against doing so. If you agree, you can just remove the suggestion from the wiki page, referencing this bug in a history comment.

Agreed, I can update the wiki and refer this bug there, by the way, could you help me in update the wiki with bugs related to Internals, for example this bug https://bugs.webkit.org/show_bug.cgi?id=84395 do you think it is valid to move to internals?
 
> There is no hard rule - as the obvious things were already moved to internals, moving anything else probably reduces test coverage on platforms that expose the functionality as WebKit API, but at the same time it also reduced code duplication between ports. In a case like this, where it's conceptually a WebKit feature anyway, I think that it's better to keep going through API where one exists.

Ok, I'll mark this bug as won't fix.

Thanks in advance