Bug 103841 – [chromium] move WebPreferences and methods for modifying them to the TestRunner library

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] move WebPreferences and methods for modifying them to the TestRunner library

Created attachment 177150 [details]
Patch

[Adam Barth]

Comment on attachment 177150 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/public/WebPreferences.h:45
> +struct WebPreferences {

I'm not sure I follow why WebPreferences is exposed as part of the API.  It seems like it should be an implementation detail of the TestRunner library.

(In reply to comment #2)
> (From update of attachment 177150 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177150&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebPreferences.h:45
> > +struct WebPreferences {
> 
> I'm not sure I follow why WebPreferences is exposed as part of the API.  It seems like it should be an implementation detail of the TestRunner library.

The embedder needs the same preferences Tod create new wwindow (which happens out of process in chrome)

[Tony Chang]

Comment on attachment 177150 [details]
Patch

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

>>> Tools/DumpRenderTree/chromium/TestRunner/public/WebPreferences.h:45
>>> +struct WebPreferences {
>> 
>> I'm not sure I follow why WebPreferences is exposed as part of the API.  It seems like it should be an implementation detail of the TestRunner library.
> 
> The embedder needs the same preferences Tod create new wwindow (which happens out of process in chrome)

How is this WebPreferences different from the webkit_glue::WebPreferences?

Would it also work to have the embedder use a WebSettings directly and pass that to WebTestRunner?

Comment on attachment 177150 [details]
Patch

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

>>>> Tools/DumpRenderTree/chromium/TestRunner/public/WebPreferences.h:45
>>>> +struct WebPreferences {
>>> 
>>> I'm not sure I follow why WebPreferences is exposed as part of the API.  It seems like it should be an implementation detail of the TestRunner library.
>> 
>> The embedder needs the same preferences Tod create new wwindow (which happens out of process in chrome)
> 
> How is this WebPreferences different from the webkit_glue::WebPreferences?
> 
> Would it also work to have the embedder use a WebSettings directly and pass that to WebTestRunner?

They differ by the defaults they set.

The WebSettings have no nice way to set the script font family map. Also, WebSettings is just an interface, so the embedder would need to implement it.

[Adam Barth]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 177150 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177150&action=review
> > 
> > > Tools/DumpRenderTree/chromium/TestRunner/public/WebPreferences.h:45
> > > +struct WebPreferences {
> > 
> > I'm not sure I follow why WebPreferences is exposed as part of the API.  It seems like it should be an implementation detail of the TestRunner library.
> 
> The embedder needs the same preferences Tod create new wwindow (which happens out of process in chrome)

Ah, that makes sense.

I wonder if we should have a way to serialize and deserialize the (opaque) state from the TestRunner library without exposing all the implementation details.  The goal would be to avoid having to write a Chromium patch whenever we add more things to this list.

Another option would be to force window.open to always be in-process for LayoutTests.  I imagine that's what LayoutTests expect today given that DumpRenderTree and WebKitTestRunner both only have one web process.

Created attachment 177437 [details]
Patch

Rebased on ToT.

Darin, can you take a look please?

[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.

[WebKit Review Bot]

Comment on attachment 177437 [details]
Patch

Rejecting attachment 177437 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   9cffc45..e9d9059  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at e9d90592d3ba8ebe3e8a5ae811eea04f2940e996 but expected 9cffc451c10239895188285fa8cdb2dbd36f385d
 ! 9cffc45..e9d9059  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15148154

Comment on attachment 177437 [details]
Patch

Clearing flags on attachment: 177437

Committed r136552: <http://trac.webkit.org/changeset/136552>

All reviewed patches have been landed.  Closing bug.