Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix partial issues from #10198 #10962

Merged
merged 2 commits into from
Jun 14, 2018
Merged

Fix partial issues from #10198 #10962

merged 2 commits into from
Jun 14, 2018

Conversation

Honry
Copy link
Contributor

@Honry Honry commented May 11, 2018 •
edited by wpt-reviewable-bot
Loading

  • if (testRunner) should be if (window.testRunner)
  • Use String to pass sensor object param to avoid undefined error

This change is Reviewable

@Honry
Fix partial issues from web-platform-tests#10198
f6f180c 
 - `if (testRunner)` should be `if (window.testRunner)`
 - Use String to pass sensor object param to avoid undefined error
@Honry Honry mentioned this pull request May 11, 2018
Closed
@Honry Honry requested a review from alexshalamov May 16, 2018 01:52
@Honry
Copy link
Contributor Author

Honry commented May 16, 2018

@pozdnyakov, @alexshalamov, PTAL.

@Honry
Copy link
Contributor Author

Honry commented Jun 6, 2018

@foolip @rakuco, could you help review this PR? I am going to bring more methods from Chromium sensor mojo interfaces to WPT. e.g. setUpdateSensorReadingFunction, setStartShouldFail, etc., which is somehow based on this PR.

@Honry Honry requested review from rakuco and foolip and removed request for gsnedders, dontcallmedom, jgraham, anssiko, riju and zqzhang June 6, 2018 06:11
rakuco
rakuco reviewed Jun 8, 2018
View reviewed changes
resources/chromium/generic_sensor_mocks.js Outdated
['accelerometer', 'gyroscope',
'magnetometer', 'ambient-light-sensor'].forEach((entry) => {
testRunner.setPermission(entry, 'granted',
window.testRunner.setPermission(entry, 'granted',
location.origin, location.origin);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to reindent this line as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! Fixed.

@wpt-pr-bot wpt-pr-bot requested review from riju and zqzhang June 8, 2018 15:12
rakuco
rakuco reviewed Jun 8, 2018
View reviewed changes
resources/chromium/generic_sensor_mocks.js
@@ -201,10 +201,10 @@ var GenericSensorTest = (() => {
if (testInternal.initialized)
throw new Error('Call reset() before initialize().');

if (testRunner) { // Grant sensor permissions for Chromium testrunner.
if (window.testRunner) { // Grant sensor permissions for Chromium testrunner.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if this sounds dumb, but isn't testRunner the same as window.testRunner here? Or do the Mojo JS files behave differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This testRunner is specific for Chromium testrunner, not sure if it is still useful in LayoutTests/external/wpt, but for sure, it is undefined in w-p-t runnner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. tests will fail with Uncaught ReferenceError: testRunner is not defined

foolip
foolip requested changes Jun 11, 2018
View reviewed changes
Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't all the tests failing now? run_fp_tests_* and runGenericSensor* need to updated, right?

@Honry
Copy link
Contributor Author

Honry commented Jun 11, 2018

@foolip, most tests are still failing, for the reason, please see my comment at #10906 (comment)

rakuco
rakuco approved these changes Jun 12, 2018
View reviewed changes
Copy link
Member

@rakuco rakuco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look fine to me, even if there might be additional work left. @foolip?

@foolip
Copy link
Member

foolip commented Jun 14, 2018

@Honry, even if the tests are already failing, won't these changes make them further from passing? If they're already passing in Chromium, could you try making the changes in a Chromium CL and see if they stay passing?

@rakuco
Copy link
Member

rakuco commented Jun 14, 2018

@foolip AFAICS, this PR should get these tests closer, not further, to passing. ambient-light/ and proximity are currently not imported into Chromium, and geolocation-sensor/ is failing there due to GeolocationSensor not being implemented at all in Blink.

Other directories which call those functions (e.g. magnetometer/) are already using these function call forms.

@foolip
Copy link
Member

foolip commented Jun 14, 2018

Hmm, something is very strange, run_fp_tests_disabled and runGenericSensorTests already assumes a string argument, but some tests still pass interface objects. So yes, these tests are moving things in the right direction. (I assumed that the helpers weren't updated, in which case this made no sense.)

@foolip foolip merged commit 7cbefa7 into web-platform-tests:master Jun 14, 2018
@Honry Honry deleted the sensor-fix-partial-10198 branch June 15, 2018 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foolip foolip foolip approved these changes

@rakuco rakuco rakuco approved these changes

@alexshalamov alexshalamov Awaiting requested review from alexshalamov

@anssiko anssiko Awaiting requested review from anssiko

@dontcallmedom dontcallmedom Awaiting requested review from dontcallmedom

@gsnedders gsnedders Awaiting requested review from gsnedders

@jgraham jgraham Awaiting requested review from jgraham

@riju riju Awaiting requested review from riju

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees
No one assigned
Labels
ambient-light geolocation-sensor infra proximity wg-dap wg-das
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Honry @foolip @rakuco @wpt-pr-bot