Bug 34474 - Share the DRT values maxViewWidth/Height among ports
Summary: Share the DRT values maxViewWidth/Height among ports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-02 05:13 PST by Shinichiro Hamaji
Modified: 2010-02-18 00:17 PST (History)
0 users

See Also:


Attachments
Patch v1 (2.61 KB, patch)
2010-02-02 05:15 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (7.37 KB, patch)
2010-02-04 03:45 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v3 (6.47 KB, patch)
2010-02-04 03:51 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v4 (6.70 KB, patch)
2010-02-08 00:34 PST, Shinichiro Hamaji
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>