Bug 44483 - [chromium] fix the platform result fallback order on mac/win
Summary: [chromium] fix the platform result fallback order on mac/win
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-23 19:12 PDT by Tony Chang
Modified: 2010-08-24 15:37 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.91 KB, patch)
2010-08-23 19:13 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-08-23 19:12:39 PDT
[chromium] fix the platform result fallback order on mac/win
Comment 1 Tony Chang 2010-08-23 19:13:09 PDT
Created attachment 65206 [details]
Patch
Comment 2 Tony Chang 2010-08-23 19:14:13 PDT
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.
Comment 3 Tony Chang 2010-08-23 19:16:39 PDT
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.
Comment 4 Eric Seidel (no email) 2010-08-24 14:36:36 PDT
I'm still confused why None is required here.
Comment 5 Tony Chang 2010-08-24 14:42:58 PDT
(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 6 Eric Seidel (no email) 2010-08-24 14:49:02 PDT
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?
Comment 7 Tony Chang 2010-08-24 14:52:10 PDT
(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 8 Eric Seidel (no email) 2010-08-24 15:19:36 PDT
Comment on attachment 65206 [details]
Patch

I'm still a bit confused, but I trust you.
Comment 9 Dirk Pranke 2010-08-24 15:29:07 PDT
Comment on attachment 65206 [details]
Patch

Good catch. LGTM (although I'm not a reviewer).
Comment 10 Tony Chang 2010-08-24 15:37:45 PDT
Comment on attachment 65206 [details]
Patch

Clearing flags on attachment: 65206

Committed r65944: <http://trac.webkit.org/changeset/65944>
Comment 11 Tony Chang 2010-08-24 15:37:50 PDT
All reviewed patches have been landed.  Closing bug.