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.

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

Mechanism to allow Servo to prompt the user #25242

Merged
merged 3 commits into from Feb 10, 2020
Merged

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Dec 11, 2019

This blocks the embedder thread (compositor thread). Not ideal. I don't think it's too much work to only block script, I'll do that in a follow up bug.

Fix #23376

@Manishearth we have a few new APIs. Hopefully this will cover your needs. A thing I haven't implemented yet is a way to ask the user to pick from a list. Let me know if it's something you'll need.

@highfive
Copy link

highfive commented Dec 11, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/window.rs, components/script/dom/webidls/Window.webidl
  • @KiChjang: components/script/dom/window.rs, components/script/dom/webidls/Window.webidl
@highfive
Copy link

highfive commented Dec 11, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm jdm added the S-fails-tidy label Dec 11, 2019
Copy link
Member

asajeffrey left a comment

Small nit, otherwise the changes to the embedder LGTM. I'll leave the HL code up to @Manishearth.


#[derive(Deserialize, Serialize, PartialEq)]
pub enum PromptOrigin {
Untrusted,

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Dec 11, 2019

Member

A comment or spec link for what is meant by (un)trusted?

#[derive(Deserialize, Serialize)]
pub enum PromptDefinition {
Alert(String, IpcSender<()>),
OkCancel(String, IpcSender<bool>),

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Dec 11, 2019

Member

An enum rather than a bool? Or at least a comment saying that true is Ok?

@paulrouget paulrouget force-pushed the paulrouget:prompt branch from c7ca82a to b33b855 Dec 12, 2019
@paulrouget
Copy link
Contributor Author

paulrouget commented Dec 12, 2019

Added comments and enum instead of bool.

@paulrouget paulrouget force-pushed the paulrouget:prompt branch from b33b855 to e24059b Dec 13, 2019
@paulrouget
Copy link
Contributor Author

paulrouget commented Dec 13, 2019

@highfive highfive assigned Manishearth and unassigned asajeffrey Dec 13, 2019
@jdm
Copy link
Member

jdm commented Dec 17, 2019

@Manishearth Review ping.

Copy link
Member

Manishearth left a comment

looks good to me!

(sorry for the delay, was waiting on some XR permissions discussions to happen)

@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2020

The latest upstream changes (presumably #25525) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Jan 15, 2020

Shoot, we should have merged this :(

@pshaughn
Copy link
Member

pshaughn commented Jan 15, 2020

It looks like this is adding more tinyfiledialogs calls; if any of them can come from strings written by page authors, send them through the escaper.

@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 4, 2020

I’ll get back to that once I figured #25683

@paulrouget paulrouget force-pushed the paulrouget:prompt branch from e24059b to 79d5ffd Feb 6, 2020
@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 6, 2020

I’ve put together the 3 prompt/devtools related PR (this one + #25266 + #25307).

@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 7, 2020

@bors-servo r=manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2020

📌 Commit cf09d72 has been approved by manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2020

Testing commit cf09d72 with merge ada40b2...

bors-servo added a commit that referenced this pull request Feb 7, 2020
Mechanism to allow Servo to prompt the user

This blocks the embedder thread (compositor thread). Not ideal. I don't think it's too much work to only block script, I'll do that in a follow up bug.

Fix #23376

@Manishearth we have a few new APIs. Hopefully this will cover your needs. A thing I haven't implemented yet is a way to ask the user to pick from a list. Let me know if it's something you'll need.
@paulrouget paulrouget force-pushed the paulrouget:prompt branch from 3244d0d to d838200 Feb 10, 2020
@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 10, 2020

@bors-servo try=linux,windows

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

🙁 There is no try chooser linux,windows for this repo, try one of: linux, mac, windows, wpt, wpt-2020, wpt-mac, wpt-android, android, magicleap, arm

@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 10, 2020

Didn’t try to build locally:

@bors-servo try=magicleap

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

Trying commit d838200 with merge 9271532...

bors-servo added a commit that referenced this pull request Feb 10, 2020
Mechanism to allow Servo to prompt the user

This blocks the embedder thread (compositor thread). Not ideal. I don't think it's too much work to only block script, I'll do that in a follow up bug.

Fix #23376

@Manishearth we have a few new APIs. Hopefully this will cover your needs. A thing I haven't implemented yet is a way to ask the user to pick from a list. Let me know if it's something you'll need.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True

@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 10, 2020

@bors-servo r=manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

📌 Commit d838200 has been approved by manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

Testing commit d838200 with merge b2f40eb...

bors-servo added a commit that referenced this pull request Feb 10, 2020
Mechanism to allow Servo to prompt the user

This blocks the embedder thread (compositor thread). Not ideal. I don't think it's too much work to only block script, I'll do that in a follow up bug.

Fix #23376

@Manishearth we have a few new APIs. Hopefully this will cover your needs. A thing I haven't implemented yet is a way to ask the user to pick from a list. Let me know if it's something you'll need.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

💔 Test failed - status-taskcluster

@paulrouget
Copy link
Contributor Author

paulrouget commented Feb 10, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

Testing commit d838200 with merge 5044379...

bors-servo added a commit that referenced this pull request Feb 10, 2020
Mechanism to allow Servo to prompt the user

This blocks the embedder thread (compositor thread). Not ideal. I don't think it's too much work to only block script, I'll do that in a follow up bug.

Fix #23376

@Manishearth we have a few new APIs. Hopefully this will cover your needs. A thing I haven't implemented yet is a way to ask the user to pick from a list. Let me know if it's something you'll need.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

☀️ Test successful - status-taskcluster
Approved by: manishearth
Pushing 5044379 to master...

@bors-servo bors-servo merged commit d838200 into servo:master Feb 10, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.