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

The embedder should be handling prompts and alerts #19992

Closed
paulrouget opened this issue Feb 8, 2018 · 7 comments
Closed

The embedder should be handling prompts and alerts #19992

paulrouget opened this issue Feb 8, 2018 · 7 comments
Labels

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Feb 8, 2018

As of now, alerts are handled by browserhtml via a mozbrowser event, or we use tinyfiledialogs in the script thread. mozbrowser is going away.

We should not use tinyfiledialogs in libservo, but in /ports/.

@fabricedesre
Copy link
Contributor

@fabricedesre fabricedesre commented Feb 17, 2018

How do you see that work? Using an additional method in the embedder and let the embedder send back a message to unblock the script thread execution?

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Feb 19, 2018

Yes. See how allow_navigation works for example.

@jdm jdm added the A-embedding label Feb 20, 2018
@jdm
Copy link
Member

@jdm jdm commented Mar 25, 2018

Relevant code:

  • // Allow the embedder to handle the url itself
    let (chan, port) = ipc::channel().expect("Failed to create IPC channel!");
    let msg = EmbedderMsg::AllowNavigation(top_level_browsing_context_id, load_data.url.clone(), chan);
    self.embedder_proxy.send(msg);
    if let Ok(false) = port.recv() {
    return None;
    }
    (example of communicating with the embedder that can be used as a model)
  • let (sender, receiver) = ProfiledIpc::channel(self.global().time_profiler_chan().clone()).unwrap();
    self.send_to_constellation(ScriptMsg::Alert(s.to_string(), sender));
    let should_display_alert_dialog = receiver.recv().unwrap();
    if should_display_alert_dialog {
    display_alert_dialog(&s);
    }
    (this already communicates with the constellation to decide whether to show a prompt, then shows the prompt)
  • #[cfg(target_os = "linux")]
    fn prompt_user(message: &str) -> PermissionState {
    if opts::get().headless {
    return PermissionState::Denied;
    }
    match tinyfiledialogs::message_box_yes_no(DIALOG_TITLE,
    message,
    MessageBoxIcon::Question,
    YesNo::No) {
    YesNo::Yes => PermissionState::Granted,
    YesNo::No => PermissionState::Denied,
    }
    }
    #[cfg(not(target_os = "linux"))]
    fn prompt_user(_message: &str) -> PermissionState {
    // TODO popup only supported on linux
    PermissionState::Denied
    }
    (this needs to communicate with the embedder via the constellation)

I'll file separate issues for the uses in components/bluetooth and components/net, since they need to be solved differently. I'm providing this information so some specific contributors can work on this issue; please check with me first before starting to work on it if I have not discussed it with you.

@jdm
Copy link
Member

@jdm jdm commented Mar 26, 2018

Filed #20428 and #20429 for the net and bluetooth bits.

@gterzian
Copy link
Member

@gterzian gterzian commented Apr 8, 2018

In the light of #20329, where Script would need to ask the embedder to allow unloading of a document:

  1. I starting looking into it and realized script doesn't have a channel to the embedder yet, and also that script doesn't seem to be aware of it's own top level browsing context.
  2. I assume constellation shouldn't block waiting for the response from the embedder.
  3. In the light of the above, it seems we will have to send a first message from script to constellation, containing a response sender, and then constellation could send a message to the embedder containing the top level browsing context id, and the response sender. The embedder could then directly respond to script via this response sender.

Does this architecture makes sense? Should we instead add a EmbedderProxy directly on script_thread and ask for the top level browsing context id from constellation?

I saw that currently the Alert message also isn't yet forwarded to the embedder, see

// FIXME: forward alert event to embedder

So I propose a separate PR to deal with all of this. Sounds good?

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Apr 10, 2018

A very similar problem is being addressed here: #20480 and see #20480 (comment)

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Apr 10, 2018

I had a quick chat with @cbrewster and we agree that it's fine if the script thread has access to the EmbedderProxy. We could short-circuit the constellation.

bors-servo added a commit that referenced this issue May 21, 2018
…ts_2, r=paulrouget

Embedder handling of prompts and alerts

<!-- Please describe your changes on the following line: -->

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19992  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20707)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 23, 2018
…ts_2, r=paulrouget

Embedder handling of prompts and alerts

<!-- Please describe your changes on the following line: -->

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19992  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20707)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 23, 2018
…ts_2, r=paul

Embedder handling of prompts and alerts

<!-- Please describe your changes on the following line: -->

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19992  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20707)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 24, 2018
…ts_2, r=paul

Embedder handling of prompts and alerts

<!-- Please describe your changes on the following line: -->

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19992  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20707)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 24, 2018
…ts_2, r=paul

Embedder handling of prompts and alerts

<!-- Please describe your changes on the following line: -->

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19992  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20707)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 24, 2018
…ts_2, r=payl

Embedder handling of prompts and alerts

<!-- Please describe your changes on the following line: -->

Allow embedder to allow alerts and prompts, but via constellation forwarding messages from script.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach build-geckolib` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #19992  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20707)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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