Skip to content

Let the embedder decide if servo should follow a link or not#15795

Merged
bors-servo merged 1 commit intoservo:masterfrom
paulrouget:follow-link
Mar 14, 2017
Merged

Let the embedder decide if servo should follow a link or not#15795
bors-servo merged 1 commit intoservo:masterfrom
paulrouget:follow-link

Conversation

@paulrouget
Copy link
Copy Markdown
Contributor

@paulrouget paulrouget commented Mar 2, 2017 •
edited by larsbergstrom
Loading

We want to give a chance to the embedder to handle a link itself.
Is it a problem that this will add a round trip to the main thread every time load_url is called?

  • There are tests for these changes OR
  • These changes do not require tests because I'm not sure how to test that

This change is Reviewable

@highfive
Copy link
Copy Markdown

highfive commented Mar 2, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 2, 2017
@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Mar 8, 2017

@glennw review ping

@asajeffrey
Copy link
Copy Markdown
Contributor

asajeffrey commented Mar 8, 2017

Sorry, been OOO for a few days.

I'm a bit confused about why the compositor is involved with making navigation decisions. Is this because all communication between the constellation and the embedding application goes via the compositor?

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Mar 8, 2017

Is this because all communication between the constellation and the embedding application goes via the compositor?

Yes. If I'm not mistaken, only the compositor has access to the embedder window and can wake up the main thread.

@asajeffrey
Copy link
Copy Markdown
Contributor

asajeffrey commented Mar 8, 2017

Hmm, this seems to be conflating two (at least conceptually) different threads: the compositor, and the embedding application. Are these separate entities? Certainly the naming implies that the compositor ix part of servo, whereas the embedding application is a third party.

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Mar 9, 2017

Can you recommend a different approach? How can the constellation make as synchronous request to the embedder?

@asajeffrey
Copy link
Copy Markdown
Contributor

asajeffrey commented Mar 9, 2017

I think the basic approach of using an rpc-like call using channels is right, it's just that it's odd conflating the compositor and the embedder. They're running in the same thread, but the compositor is written by us, and the embedder is a third party. It might be worth using different channels for these?

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Mar 9, 2017 •
edited
Loading

It seems to me that it's a more general problem. The compositor make some other calls like set_page_title(), supports_clipboard() or status() that are not directly related to compositing.

Is it ok if we land this as it is, and have a specific issue for creating a dedicated channel to communicate with the embedder?

@paulrouget paulrouget mentioned this pull request Mar 14, 2017
Closed
@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Mar 14, 2017 •
edited
Loading

I filed #15934

@asajeffrey
Copy link
Copy Markdown
Contributor

asajeffrey commented Mar 14, 2017

Back from being ill :/ Thanks for filing the issue, it sounds like we're in agreement about where we'd like to go, so I'm happy to land this as-is and postpone thinking about the embedder/compositor split.

@bors-servo r+

@highfive highfive assigned asajeffrey and unassigned glennw Mar 14, 2017
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 14, 2017

📌 Commit 79a5bf0 has been approved by asajeffrey

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 14, 2017
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 14, 2017

⌛ Testing commit 79a5bf0 with merge 808ffff...

bors-servo pushed a commit that referenced this pull request Mar 14, 2017
Auto merge of #15795 - paulrouget:follow-link, r=asajeffrey
808ffff 
Let the embedder decide if servo should follow a link or not

We want to give a chance to the embedder to handle a link itself.
Is it a problem that this will add a round trip to the main thread every time `load_url` is called?

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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because I'm not sure how to test that

<!-- 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/15795)
<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Mar 14, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: asajeffrey
Pushing 808ffff to master...

@bors-servo bors-servo merged commit 79a5bf0 into servo:master Mar 14, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@asajeffrey asajeffrey

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ask the embedder the permission to navigate

5 participants

@paulrouget @highfive @asajeffrey @bors-servo @glennw