Bug 122771 - Move TestRunner::setCacheModel to Window::Internals
Summary: Move TestRunner::setCacheModel to Window::Internals
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 87284
  Show dependency treegraph
 
Reported: 2013-10-14 12:06 PDT by Andre Loureiro (IRC: loureiro)
Modified: 2013-10-23 12:58 PDT (History)
7 users (show)

See Also:


Attachments
WIP Patch for EWS testing (22.70 KB, patch)
2013-10-21 06:12 PDT, Andre Loureiro (IRC: loureiro)
gtk-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Loureiro (IRC: loureiro) 2013-10-14 12:06:17 PDT
Since Mac, Efl, Win and Gtk ports implement this method we could move this to Internals.
Comment 1 Andre Loureiro (IRC: loureiro) 2013-10-21 06:12:05 PDT
Created attachment 214732 [details]
WIP Patch for EWS testing

Patch to check WIN build
Comment 2 WebKit Commit Bot 2013-10-21 06:13:32 PDT
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.
Comment 3 kov's GTK+ EWS bot 2013-10-21 07:10:15 PDT
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
Comment 4 Alexey Proskuryakov 2013-10-21 08:41:54 PDT
Cache model is a WebKit concept, not a WebCore one. I don't think that it makes sense to "test" it in WebCore.
Comment 5 Andre Loureiro (IRC: loureiro) 2013-10-21 09:14:15 PDT
(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?
Comment 6 Alexey Proskuryakov 2013-10-21 09:24:04 PDT
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.
Comment 7 Build Bot 2013-10-21 17:24:25 PDT
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
Comment 8 Andre Loureiro (IRC: loureiro) 2013-10-23 06:38:54 PDT
(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.
Comment 9 Alexey Proskuryakov 2013-10-23 10:44:05 PDT
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.
Comment 10 Andre Loureiro (IRC: loureiro) 2013-10-23 12:19:27 PDT
(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