New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing UA matching for Safari #4378
Conversation
@@ -106,6 +106,18 @@ class TestConfig { | |||
return fn.apply(this, arguments); | |||
}); | |||
} | |||
|
|||
/** @private */ | |||
isAgentMatched_(agent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse platform.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh didn't know about platform.js
. Yeah, should be able too. Wait for the next patch before reviewing..
PTAL |
* List of predicate functions that are called before running each test | ||
* suite to check whether the suite should be skipped or not. | ||
* If any of the functions return 'true', the suite will be skipped. | ||
* @type {!Array<function>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add ()
should be type {!Array<function()>}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function():boolean
LGTM |
@@ -55,25 +55,26 @@ class TestConfig { | |||
* @type {!Array<function(!TestSuite)>} | |||
*/ | |||
this.configTasks = []; | |||
this.platform_ = platformFor(window); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/$/s/
. 😜
Closes #4374