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+

Shinichiro Hamaji
Reported 2010-02-02 05:13:57 PST
I put this FIXME in http://trac.webkit.org/changeset/54205
Attachments
Patch v1 (2.61 KB, patch)
2010-02-02 05:15 PST, Shinichiro Hamaji 		no flags
DetailsFormatted DiffDiff
Patch v2 (7.37 KB, patch)
2010-02-04 03:45 PST, Shinichiro Hamaji 		no flags
DetailsFormatted DiffDiff
Patch v3 (6.47 KB, patch)
2010-02-04 03:51 PST, Shinichiro Hamaji 		no flags
DetailsFormatted DiffDiff
Patch v4 (6.70 KB, patch)
2010-02-08 00:34 PST, Shinichiro Hamaji 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Shinichiro Hamaji
Comment 1 2010-02-02 05:15:22 PST
Created attachment 47925 [details] Patch v1
Shinichiro Hamaji
Comment 2 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
Darin Adler
Comment 3 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.
Shinichiro Hamaji
Comment 4 2010-02-04 03:45:04 PST
Created attachment 48129 [details] Patch v2
Shinichiro Hamaji
Comment 5 2010-02-04 03:51:07 PST
Created attachment 48130 [details] Patch v3
Shinichiro Hamaji
Comment 6 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.
Shinichiro Hamaji
Comment 7 2010-02-08 00:34:52 PST
Created attachment 48324 [details] Patch v4
Shinichiro Hamaji
Comment 8 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.
Eric Seidel (no email)
Comment 9 2010-02-17 15:46:49 PST
Comment on attachment 48324 [details] Patch v4 Woohoo! Yay for more shared code!
Shinichiro Hamaji
Comment 10 2010-02-18 00:17:58 PST
Committed r54942: <http://trac.webkit.org/changeset/54942>
Note You need to log in before you can comment on or make changes to this bug.