WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
140585
iOS EWS queue name should be consistent
https://bugs.webkit.org/show_bug.cgi?id=140585
Summary
iOS EWS queue name should be consistent
David Kilzer (:ddkilzer)
Reported
2015-01-17 16:43:00 PST
Bug 140476
adds support to create an iOS EWS queue, but it wasn't consistent about using a name of 'ios-device-ews'. This patch corrects that.
Attachments
Patch v1
(5.15 KB, patch)
2015-01-17 16:52 PST
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v2
(2.81 KB, patch)
2015-01-17 18:53 PST
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v3
(4.33 KB, patch)
2015-01-18 21:06 PST
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1
2015-01-17 16:52:18 PST
Created
attachment 244845
[details]
Patch v1
Alexey Proskuryakov
Comment 2
2015-01-17 16:54:22 PST
Comment on
attachment 244845
[details]
Patch v1 View in context:
https://bugs.webkit.org/attachment.cgi?id=244845&action=review
> Tools/ChangeLog:8 > + * EWSTools/start-queue-mac.sh: Update for rename of 'ios-ews' to
Ugh, this will make the bubble a lot wider. Not a fan.
David Kilzer (:ddkilzer)
Comment 3
2015-01-17 18:38:08 PST
Alexey expressed concern about making the queue name "ios-device-ews", so changing the patch to be consistent the other way so the queue name is "ios-ews".
David Kilzer (:ddkilzer)
Comment 4
2015-01-17 18:53:05 PST
Created
attachment 244848
[details]
Patch v2
Alexey Proskuryakov
Comment 5
2015-01-17 19:00:12 PST
Comment on
attachment 244848
[details]
Patch v2 Looks worth trying, hopefully this won't break ios-simulator.
David Kilzer (:ddkilzer)
Comment 6
2015-01-18 11:11:46 PST
(In reply to
comment #5
)
> Comment on
attachment 244848
[details]
> Patch v2 > > Looks worth trying, hopefully this won't break ios-simulator.
Did a previous version of the patch for
Bug 140476
break iso-simulator?
Alexey Proskuryakov
Comment 7
2015-01-18 11:54:11 PST
Not sure how (or what) to check. Dan Bates and Simon Fraser would probably be the ones to advise.
David Kilzer (:ddkilzer)
Comment 8
2015-01-18 16:17:35 PST
(In reply to
comment #7
)
> Not sure how (or what) to check. Dan Bates and Simon Fraser would probably > be the ones to advise.
I'm pretty sure it's just `run-webkit-tests --ios-simulator` that needs to be tested with this change.
David Kilzer (:ddkilzer)
Comment 9
2015-01-18 18:52:28 PST
(In reply to
comment #8
)
> (In reply to
comment #7
) > > Not sure how (or what) to check. Dan Bates and Simon Fraser would probably > > be the ones to advise. > > I'm pretty sure it's just `run-webkit-tests --ios-simulator` that needs to > be tested with this change.
Well, after applying the patch, this happens: $ ./Tools/Scripts/run-webkit-tests --ios-simulator --debug LayoutTests/fast/url Traceback (most recent call last): File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 425, in <module> sys.exit(main(sys.argv[1:], sys.stdout, sys.stderr)) File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 73, in main port = host.port_factory.get(options.platform, options) File "Tools/Scripts/webkitpy/port/factory.py", line 119, in get return cls(self._host, port_name, options=options, **kwargs) File "Tools/Scripts/webkitpy/port/ios.py", line 66, in __init__ super(IOSPort, self).__init__(*args, **kwargs) File "Tools/Scripts/webkitpy/port/apple.py", line 88, in __init__ assert port_name in allowed_port_names, "%s is not in %s" % (port_name, allowed_port_names) AssertionError: ios-simulator is not in ['ios-7', 'ios-8', 'ios-future'] (Without the patch applied, everything works as expected.) Need to fix that issue first.
David Kilzer (:ddkilzer)
Comment 10
2015-01-18 20:53:37 PST
(In reply to
comment #9
)
> Well, after applying the patch, this happens: > > $ ./Tools/Scripts/run-webkit-tests --ios-simulator --debug > LayoutTests/fast/url > Traceback (most recent call last): > File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 425, > in <module> > sys.exit(main(sys.argv[1:], sys.stdout, sys.stderr)) > File "Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 73, > in main > port = host.port_factory.get(options.platform, options) > File "Tools/Scripts/webkitpy/port/factory.py", line 119, in get > return cls(self._host, port_name, options=options, **kwargs) > File "Tools/Scripts/webkitpy/port/ios.py", line 66, in __init__ > super(IOSPort, self).__init__(*args, **kwargs) > File "Tools/Scripts/webkitpy/port/apple.py", line 88, in __init__ > assert port_name in allowed_port_names, "%s is not in %s" % (port_name, > allowed_port_names) > AssertionError: ios-simulator is not in ['ios-7', 'ios-8', 'ios-future'] > > (Without the patch applied, everything works as expected.) > > Need to fix that issue first.
Sigh. In webkitpy/port/factory.py, PortFactory.get() does prefix matching on the PORT_CLASSES list. Because "IOSPort" appears in the list before "IOSSimulatorPort", the IOSPort.port_name ("ios") is a prefix of "ios-simulator" (the port_name derived from the --ios-simulator command line switch), so IOSPort is chosen instead of IOSSimulatorPort. The reason using "ios-device" for the port name worked before is that "ios-device" is not a prefix of "ios-simulator", so IOSPort was not used (and IOSSimulatorPort was chosen instead). The easiest solution is to make sure PortFactory.PORT_CLASSES always lists "more specific" ports first when port_name values have a common prefix.
David Kilzer (:ddkilzer)
Comment 11
2015-01-18 21:06:22 PST
Created
attachment 244876
[details]
Patch v3
Daniel Bates
Comment 12
2015-01-18 21:28:05 PST
Comment on
attachment 244876
[details]
Patch v3 View in context:
https://bugs.webkit.org/attachment.cgi?id=244876&action=review
> Tools/ChangeLog:22 > + (PortFactory): Re-order PORT_CLASSES so ios.IOSSimulatorPort > + appears before ios.IOSPort. If this is not done, > + `run-webkit-tests --ios-simulator` will get the wrong Port class > + (IOSPort instead of IOSSimulatorPort) due to the way that > + PortFactory.get() uses prefix matching of <Port>.port_name to > + find the correct class to use.
The approach taken in this patch to re-order the array values in PortFactory.PORT_CLASSES is OK. We may want to investigate using strict equality instead of prefix matching in PortFactory.get().
> Tools/Scripts/webkitpy/port/ios.py:49 > + VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8']
I take it we plan to build for iOS 7?
David Kilzer (:ddkilzer)
Comment 13
2015-01-18 22:31:46 PST
(In reply to
comment #12
)
> Comment on
attachment 244876
[details]
> Patch v3 > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=244876&action=review
> > > Tools/ChangeLog:22 > > + (PortFactory): Re-order PORT_CLASSES so ios.IOSSimulatorPort > > + appears before ios.IOSPort. If this is not done, > > + `run-webkit-tests --ios-simulator` will get the wrong Port class > > + (IOSPort instead of IOSSimulatorPort) due to the way that > > + PortFactory.get() uses prefix matching of <Port>.port_name to > > + find the correct class to use. > > The approach taken in this patch to re-order the array values in > PortFactory.PORT_CLASSES is OK. We may want to investigate using strict > equality instead of prefix matching in PortFactory.get().
Yes, that would be the preferred, long-term fix.
> > Tools/Scripts/webkitpy/port/ios.py:49 > > + VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8'] > > I take it we plan to build for iOS 7?
No, adding 'ios-7' was a workaround to make test-webkitpy pass on systems with 10.8 and 10.9 SDKs installed. See comments in
Bug 140476 Comment #16
for more details.
WebKit Commit Bot
Comment 14
2015-01-18 23:15:36 PST
Comment on
attachment 244876
[details]
Patch v3 Clearing flags on attachment: 244876 Committed
r178641
: <
http://trac.webkit.org/changeset/178641
>
WebKit Commit Bot
Comment 15
2015-01-18 23:15:42 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug