Bug 34474

Summary: Share the DRT values maxViewWidth/Height among ports
Product: WebKit Reporter: Shinichiro Hamaji <hamaji>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch v3
none
Patch v4 eric: review+

Description Shinichiro Hamaji 2010-02-02 05:13:57 PST 
I put this FIXME in http://trac.webkit.org/changeset/54205
Comment 1 Shinichiro Hamaji 2010-02-02 05:15:22 PST 
Created attachment 47925 [details]
Patch v1
Comment 2 Shinichiro Hamaji 2010-02-02 05:38:21 PST 
I'm not sure if config.h is the best place to put the constant values, but I chose this header because Qt port doesn't include LayoutTestController.h nor DumpRenderTree.h
Comment 3 Darin Adler 2010-02-02 10:03:03 PST 
Comment on attachment 47925 [details]
Patch v1

Good goal. But why not share these in a header as plain old C++ constants? It seems wrong to put these into config.h as macros. Macros are deprecated and should only be used when there is no better solution. And config.h should be kept as simple as possible. We're working longer term to simplify further.

I suggest we just move maxViewWidth and maxViewHeight from DumpRenderTree.mm into LayoutTestController.h and use those in LayoutTestController.cpp.
Comment 4 Shinichiro Hamaji 2010-02-04 03:45:04 PST 
Created attachment 48129 [details]
Patch v2
Comment 5 Shinichiro Hamaji 2010-02-04 03:51:07 PST 
Created attachment 48130 [details]
Patch v3
Comment 6 Shinichiro Hamaji 2010-02-04 03:53:37 PST 
The changelog of Patch v2 was wrong, sorry.

> I suggest we just move maxViewWidth and maxViewHeight from DumpRenderTree.mm
> into LayoutTestController.h and use those in LayoutTestController.cpp.

Thanks for your review! OK, I moved them into LayoutTestController.h. I'm not sure if we can share the value with Qt, but sharing the value among mac, win, and gtk is still nice.
Comment 7 Shinichiro Hamaji 2010-02-08 00:34:52 PST 
Created attachment 48324 [details]
Patch v4
Comment 8 Shinichiro Hamaji 2010-02-08 00:44:08 PST 
In Patch v4, maxViewWidth/Height are static member variables of LayoutTestController (in Patch v3 I used static member *function*). I'm not sure which is the preferable way. Also, I'm not sure if we want m_ prefixes for static member variables. Any suggestions will be appreciated, thanks.
Comment 9 Eric Seidel (no email) 2010-02-17 15:46:49 PST 
Comment on attachment 48324 [details]
Patch v4

Woohoo!  Yay for more shared code!
Comment 10 Shinichiro Hamaji 2010-02-18 00:17:58 PST 
Committed r54942: <http://trac.webkit.org/changeset/54942>