| Summary: | [chromium] fix the platform result fallback order on mac/win | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tony Chang <tony> | ||||
| Component: | New Bugs | Assignee: | Tony Chang <tony> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | dpranke, eric, victorw | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Other | ||||||
| OS: | OS X 10.5 | ||||||
| Attachments: |
|
||||||
|
Description
Tony Chang
2010-08-23 19:12:39 PDT
Created attachment 65206 [details]
Patch
The previous code happened to work on Linux because the fallback order on linux was hardcoded. On Mac and Win, it's based on the port name. To match existing behavior, we need to pass in null for port name. To clarify some more, the bug was that --platform 'google-chrome-win' wasn't including chromium-win-xp or chromium-win-vista. This fixes it so the port name will still be chromium-win-{xp,vista}, even though we insert the google-chrome-win path in the baseline search path.
I'm still confused why None is required here. (In reply to comment #4) > I'm still confused why None is required here. None causes us to assign the default port name: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py#L47 http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py#L44 For windows, this is important because that's where -xp or -vista comes from. Comment on attachment 65206 [details]
Patch
So these ports end up maskerading as win or mac or whatever? Or I guess they get the chromium through inheriting from the Chromium port?
(In reply to comment #6) > (From update of attachment 65206 [details]) > So these ports end up maskerading as win or mac or whatever? Or I guess they get the chromium through inheriting from the Chromium port? Yes, these ports are identical to the chromium-win or chromium-mac ports. We just want an extra layout test directory to check first for results. Comment on attachment 65206 [details]
Patch
I'm still a bit confused, but I trust you.
Comment on attachment 65206 [details]
Patch
Good catch. LGTM (although I'm not a reviewer).
Comment on attachment 65206 [details] Patch Clearing flags on attachment: 65206 Committed r65944: <http://trac.webkit.org/changeset/65944> All reviewed patches have been landed. Closing bug. |