Move tinyfiledialog call from script to embedder #25869

iulianR · 2020-02-28T20:09:09Z

PR is based on work started in #23651. I rebased on top of master and addressed review comments. Handling the PromptPermission message in libsimpleservo is probably not ideal. Looking forward to make more changes, just let me know how I should proceed.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #23057 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___


        Factored out permission_state_invocation_results API to GlobalScope

Instead of Window


        Move tinyfiledialog out of script into embedder.


        Delegate permission prompt dialog formatting to embedders


        Handle PromptPermission in libsimpleservo

highfive · 2020-02-28T20:09:20Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

highfive · 2020-02-28T20:09:21Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/permissions.rs, components/script/dom/globalscope.rs, components/script/Cargo.toml, components/script/dom/window.rs
@KiChjang: components/script/dom/permissions.rs, components/script/dom/globalscope.rs, components/script/Cargo.toml, components/script/dom/window.rs

highfive · 2020-02-28T20:09:23Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2020-02-29T14:54:14Z

While we could certainly add a member to HostTrait like fn permission_prompt(prompt: PermissionPrompt) -> PermissionRequest, and then add implementations in:

servo/ports/libmlservo/src/lib.rs

Line 358 in 5044379

impl HostTrait for HostCallbacks {
servo/ports/libsimpleservo/jniapi/src/lib.rs

Line 383 in 5044379

impl HostTrait for HostCallbacks {
servo/ports/libsimpleservo/capi/src/lib.rs

Line 722 in b5b9732

impl HostTrait for HostCallbacks {

I don't actually see a reason to do that work until we have a need to have a different per-embedding behaviour for the permission prompt. I'm happy with the current implementation for now!

@bors-servo r+

bors-servo · 2020-02-29T14:54:15Z

📌 Commit ffba9d0 has been approved by jdm

bors-servo · 2020-02-29T14:54:21Z

⌛ Testing commit ffba9d0 with merge f03f351...


        Auto merge of #25869 - iulianR:issue-23057-tinifiledialogs, r=jdm

Move tinyfiledialog call from script to embedder  PR is based on work started in #23651. I rebased on top of master and addressed review comments. Handling the `PromptPermission` message in `libsimpleservo` is probably not ideal. Looking forward to make more changes, just let me know how I should proceed. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23057 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-02-29T15:40:02Z

💔 Test failed - status-taskcluster

jdm · 2020-02-29T15:53:25Z

These changes now cause us to run more of the permissions API tests to completion!

  ▶ OK [expected CRASH] /permissions/idlharness.any.worker.html
  ▶ Unexpected subtest result in /permissions/idlharness.any.worker.html:
  │ FAIL [expected PASS] Permissions interface: calling query(object) on navigator.permissions with too few arguments must throw TypeError
  │   → assert_unreached: Throws "TypeError: Not enough arguments to "Permissions.query"." instead of rejecting promise Reached unreachable code
  │ 
  │ throwOrReject/<@http://web-platform.test:8000/resources/idlharness.js:116:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1926:25
  │ throwOrReject@http://web-platform.test:8000/resources/idlharness.js:115:20
  │ IdlInterface.prototype.test_interface_of/<@http://web-platform.test:8000/resources/idlharness.js:2958:21
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1926:25
  │ IdlInterface.prototype.test_interface_of@http://web-platform.test:8000/resources/idlharness.js:2933:20
  │ IdlInterface.prototype.test_object@http://web-platform.test:8000/resources/idlharness.js:2767:27
  │ IdlArray.prototype.test/<@http://web-platform.test:8000/resources/idlharness.js:917:36
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:912:21
  └ idl_test/</<@http://web-platform.test:8000/resources/idlharness.js:3440:31
  ▶ Unexpected subtest result in /permissions/idlharness.any.worker.html:
  │ FAIL [expected PASS] Permissions interface: operation query(object)
  │   → assert_unreached: Throws "TypeError: "this" object does not implement interface Permissions." instead of rejecting promise Reached unreachable code
  │ 
  │ throwOrReject/<@http://web-platform.test:8000/resources/idlharness.js:116:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1926:25
  │ throwOrReject@http://web-platform.test:8000/resources/idlharness.js:115:20
  │ IdlInterface.prototype.do_member_operation_asserts@http://web-platform.test:8000/resources/idlharness.js:2542:13
  │ IdlInterface.prototype.test_member_operation/<@http://web-platform.test:8000/resources/idlharness.js:2462:14
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1926:25
  │ IdlInterface.prototype.test_member_operation@http://web-platform.test:8000/resources/idlharness.js:2410:12
  │ IdlInterface.prototype.test_members@http://web-platform.test:8000/resources/idlharness.js:2720:26
  │ IdlInterface.prototype.test@http://web-platform.test:8000/resources/idlharness.js:1580:10
  │ IdlArray.prototype.test@http://web-platform.test:8000/resources/idlharness.js:905:28
  └ idl_test/</<@http://web-platform.test:8000/resources/idlharness.js:3440:31

You will need to update https://github.com/servo/servo/blob/master/tests/wpt/metadata/permissions/idlharness.any.js.ini to remove the CRASH line, and duplicate these lines but with the header idlharness.any.worker.html instead. Running ./mach test-wpt tests/wpt/web-platform-tests/permissions will verify that it's working as expected.


        Update expectations for permissions tests

iulianR · 2020-02-29T18:36:04Z

With these changes, ./mach test-wpt tests/wpt/web-platform-tests/permissions reports no errors. I can squash my commits if you'd like.

jdm · 2020-02-29T18:39:45Z

I'm fine with the current set of commits!
@bors-servo r+

bors-servo · 2020-02-29T18:39:46Z

📌 Commit 3b85c6a has been approved by jdm

bors-servo · 2020-02-29T18:39:52Z

⌛ Testing commit 3b85c6a with merge 9cab4aa...


        Auto merge of #25869 - iulianR:issue-23057-tinifiledialogs, r=jdm

Move tinyfiledialog call from script to embedder  PR is based on work started in #23651. I rebased on top of master and addressed review comments. Handling the `PromptPermission` message in `libsimpleservo` is probably not ideal. Looking forward to make more changes, just let me know how I should proceed. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23057 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-02-29T19:29:04Z

💔 Test failed - status-taskcluster

jdm · 2020-02-29T19:52:12Z

@bors-servo retry

#25875

bors-servo · 2020-02-29T19:52:18Z

⌛ Testing commit 3b85c6a with merge d42835b...

bors-servo · 2020-02-29T20:12:05Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing d42835b to master...

gatoWololo and others added 4 commits Jun 27, 2019

Factored out permission_state_invocation_results API to GlobalScope

24c14ac

Instead of Window

Move tinyfiledialog out of script into embedder.

675b36d

Delegate permission prompt dialog formatting to embedders

f75d547

Handle PromptPermission in libsimpleservo

Loading status checks…

ffba9d0

highfive assigned nox Feb 28, 2020

highfive added the S-awaiting-review label Feb 28, 2020

iulianR changed the title ~~Issue 23057 tinifiledialogs~~ Move tinyfiledialog call from script to embedder Feb 28, 2020

highfive assigned jdm and unassigned nox Feb 29, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 29, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 29, 2020

jdm added S-needs-code-changes and removed S-tests-failed labels Feb 29, 2020

Update expectations for permissions tests

Loading status checks…

3b85c6a

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 29, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 29, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 29, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 29, 2020

bors-servo merged commit d42835b into servo:master Feb 29, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Feb 29, 2020

servo / servo

Move tinyfiledialog call from script to embedder #25869

Move tinyfiledialog call from script to embedder #25869

iulianR commented Feb 28, 2020

highfive commented Feb 28, 2020

highfive commented Feb 28, 2020

highfive commented Feb 28, 2020

jdm commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

jdm commented Feb 29, 2020

iulianR commented Feb 29, 2020

jdm commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

jdm commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

servo / servo

Join GitHub today

Move tinyfiledialog call from script to embedder #25869

Move tinyfiledialog call from script to embedder #25869

Conversation

iulianR commented Feb 28, 2020

highfive commented Feb 28, 2020

highfive commented Feb 28, 2020

highfive commented Feb 28, 2020

jdm commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

jdm commented Feb 29, 2020

iulianR commented Feb 29, 2020

jdm commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020

jdm commented Feb 29, 2020

bors-servo commented Feb 29, 2020

bors-servo commented Feb 29, 2020