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

Figure out if Servo can just communicate iframe resizes to itself instead of relying on WebRender #14682

Closed
jrmuizel opened this issue Dec 22, 2016 · 21 comments

Comments

@jrmuizel
Copy link

@jrmuizel jrmuizel commented Dec 22, 2016

Currently WebRender sends a FrameSize message to the constellation when it detects an Iframe resize. It seems like any consumers of this should be getting the information directly from layout instead of indirectly via WebRender.

@jdm

@jdm
Copy link
Member

@jdm jdm commented Dec 23, 2016

I do not see any reason it needs to be structured this way. The layout thread creates display items for each iframe that get passed directly to WR, and WR stores the LayerSize values in a hashmap unmodified. These values are passed unmodified by WR to the pipeline_size_changed notification in the compositor, which sends them unmodified to the constellation. Finally in the constellation they are sent to the script thread, which uses them to resize things.

Since the constellation already has a hashmap of pipelines that contains a size, we could theoretically send them from layout to the constellation and do the checking there. This should let us remove all the related code from WR.

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 10, 2017

Right, I'm taking this one!

@jdm jdm added the C-assigned label Jan 10, 2017
@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 12, 2017

Since this will introduce changes in both servo's and webrender's repos, how will the bot build/test these changes?

@jdm
Copy link
Member

@jdm jdm commented Jan 12, 2017

The bot can't test it unless you publish a fork of webrender with your changes (or they get merged into the original) and the PR includes an update to the WR revision.

@glennw
Copy link
Member

@glennw glennw commented Jan 12, 2017

My suggestion would be to change Servo to work without using the WR APIs at all first. Then we could actually land this without any changes to WR. Once that work is done, we could remove the deprecated WR APIs as a follow up, which will then have no effect on Servo since they are unused.

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 14, 2017

I'm sending a resize message from layout to constellation in compute_abs_pos_and_build_display_list here, where the layout thread also sends the display list to WR.

Later, I compare the size I sent in the message to constellation to the old size here. But when I check the debug logs, I notice that the two sizes are always equal. Am I correct in assuming that pipeline.size contains the old size?

@jdm
Copy link
Member

@jdm jdm commented Jan 14, 2017

I believe the problem is that sending the viewport size from layout to the constellation is self-fulfilling - the viewport is provided to layout by the script thread, and was provided to the script thread by the constellation. Instead, when layout encounters an iframe element and determines its size, it should inform the constellation of the iframe's dimensions for the pipeline contained inside of the iframe.

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 16, 2017

@jdm What do you mean by pipelines contained inside the iframe?

@jdm
Copy link
Member

@jdm jdm commented Jan 16, 2017

An iframe embeds one document inside of another. Each document has a corresponding pipeline, therefore there is a pipeline associated with the child document embedded in the iframe.

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 17, 2017

I don't know if I understood this right, but we were checking whether there was a mismatch in the old and new sizes and updating the frame size itself. Why do we want to update the children's sizes in this refactor?

@jdm
Copy link
Member

@jdm jdm commented Jan 17, 2017

For a document A that contains an iframe embedding document B, there are two layout threads. The viewport size of document B depends on the size of the iframe calculated by the layout thread for document A.

In Servo master, the layout thread for document A sends a display list to the compositor that includes the iframe size, and at some point WebRender sees this and checks whether that iframe changed sizes. It then notifies the compositor, which notifies the constellation, which sends a resize message to the script thread for document B. Next time layout happens for document B, the layout thread is informed of the updated viewport size.

In the new model that excludes WebRender, the layout thread for document A still has the information about the iframe size. The layout thread for document B will not receive an updated viewport size unless the constellation finds out about document A's iframe size, so it can propagate that information to document B's script thread. Does that make sense?

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 17, 2017

Doesn't layout thread send the display list to WR instead of the compositor?

@jdm
Copy link
Member

@jdm jdm commented Jan 17, 2017

That may be the case, yes.

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 17, 2017

Ah, it makes sense now. Thanks!

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 18, 2017

@jdm handle_frame_size_msg now sends messages to children iframes here.

I tested tests/wpt/mozilla/tests/mozilla/window_resize_not_triggered_on_load.html and pasted the debug output here. I'm having trouble reading the logs, I don't know what I should be looking for. Help?

@jdm
Copy link
Member

@jdm jdm commented Jan 18, 2017

If you want debug logs, it's better to select the modules that will actually be useful (eg. RUST_LOG=constellation::constellation,layout_thread,script::script_thread). If you'd like assistance, please explain what you're having difficulty figuring out based on the logging output and what you expect to be seeing?

@cynicaldevil
Copy link
Contributor

@cynicaldevil cynicaldevil commented Jan 20, 2017

I've changed the code so that the display list is checked in handle_reflow and a resize message is sent if the display item is an iframe. But when I run the tests, the display list isn't updated at all. Because of this, the child iframe doesn't receive the constrained viewport message at all.
I performed a diff after adding a few debug messages of my own to print the display list: https://www.diffchecker.com/ePu6XPte

@jdm
Copy link
Member

@jdm jdm commented Jan 20, 2017

The debug messages show that the existing pipeline size in the constellation is always None. This means that cynicaldevil@fd427ea#diff-55c92a6a5ba7654ce45fe6fc6c63740fR1336 skips the code that updates the current size and the resize message is never sent.

bors-servo added a commit that referenced this issue Jan 22, 2017
Refactor to send iframe resize messages directly from layout thread to constellation

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

---
<!-- 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 #14682.
<!-- Either: -->

r? @jdm
passing tests:
tests/wpt/mozilla/tests/css/matchMedia.html, tests/wpt/mozilla/tests/mozilla/window_resize_not_triggered_on_load.html, tests/wpt/mozilla/tests/mozilla/iframe/resize_after_load.html, tests/wpt/mozilla/tests/css/meta_viewport_resize.html

<!-- 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/15129)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 23, 2017
Refactor to send iframe resize messages directly from layout thread to constellation

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

---
<!-- 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 #14682.
<!-- Either: -->

r? @jdm
passing tests:
tests/wpt/mozilla/tests/css/matchMedia.html, tests/wpt/mozilla/tests/mozilla/window_resize_not_triggered_on_load.html, tests/wpt/mozilla/tests/mozilla/iframe/resize_after_load.html, tests/wpt/mozilla/tests/css/meta_viewport_resize.html

<!-- 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/15129)
<!-- Reviewable:end -->
@jdm
Copy link
Member

@jdm jdm commented Jan 24, 2017

#15129 was reverted as part of #15164.

@jdm jdm reopened this Jan 24, 2017
@emilio
Copy link
Member

@emilio emilio commented Jan 24, 2017

Trying to reland them in #15186

@emilio
Copy link
Member

@emilio emilio commented Jan 25, 2017

Relanded

@emilio emilio closed this Jan 25, 2017
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.

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