Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upTake origin from current window instead of creating a new one in event of reflow #25777
Conversation
Everytime a new LayoutContext was created, it created a new origin which caused endless stream of image loads to occur in case of reflow. The reason for this was that the existing image, although cached successfully, was not used because the entry in hashmap did not match because of different(new) origin. This is solved by storing the origin of a window in enum ScriptReflow and used in creating new LayoutContext in case of reflow.
highfive
commented
Feb 15, 2020
|
Heads up! This PR modifies the following files:
|
highfive
commented
Feb 15, 2020
|
|
|
@bors-servo try=wpt |
|
@kunalmohan: |
|
Worth a try |
kunalmohan
changed the title
|
These are the new logs: |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Feb 17, 2020
Take origin from current window instead of creating a new one in event of reflow Everytime a new `LayoutContext` was created, it created a new origin which caused endless stream of image loads to occur in case of reflow. The reason for this was that the existing image, although cached successfully, was not used because the entry in hashmap did not match because of different(new) origin. This is solved by storing the origin of a window in enum `ScriptReflow` and used in creating new `LayoutContext` in case of reflow. <!-- Please describe your changes on the following line: --> r?@jdm --- <!-- 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 #24720 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
|
|
Those failures appear like intermittents from #24726 |
components/layout_thread/lib.rs
Outdated
| @@ -1738,7 +1746,7 @@ impl LayoutThread { | |||
| ua_or_user: &ua_or_user_guard, | |||
| }; | |||
| let snapshots = SnapshotMap::new(); | |||
| let mut layout_context = self.build_layout_context(guards, false, &snapshots); | |||
| let mut layout_context = self.build_layout_context(guards, false, &snapshots, None); | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Feb 17, 2020
Member
Here you might be able to receive the origin as part of the LayoutControlMsg::TickAnimations.
It is sent from
servo/components/constellation/constellation.rs
Line 3231 in a5b43b7
So there you have the Pipeline available, so you could to change the below:
servo/components/constellation/constellation.rs
Line 3233 in a5b43b7
into something like
Some(pipeline) => {
let msg = LayoutControlMsg::TickAnimations(pipeline.load_data.url.origin());
pipeline.layout_chan.send(msg);
}
This comment has been minimized.
This comment has been minimized.
kunalmohan
Feb 17, 2020
Author
Collaborator
Oh...I was misunderstanding the purpose and use of LayoutContext. And it was a really stupid one None was a part of it. I'll correct that now.
Just out of curiosity what is the difference between layout_thread and layout_thread_2020
This comment has been minimized.
This comment has been minimized.
gterzian
Feb 17, 2020
•
Member
Note that I'm not quite sure about the purpose of LayoutContext, I just saw the None there and was thinking that you might not need the option.
layout_thread_2020 is the "new version" of layout, so your fix might require to be implemented in both, or maybe on 2020? I don't know, @SimonSapin can probably tell you more.
This comment has been minimized.
This comment has been minimized.
SimonSapin
Feb 17, 2020
Member
Yes, there’s also components/layout_thread_2020/lib.rs which is very similar to components/layout_thread/lib.rs. See https://github.com/servo/servo/wiki/Layout-2020#using for build instructions.
This comment has been minimized.
This comment has been minimized.
kunalmohan
Feb 17, 2020
Author
Collaborator
Okay. Earlier, I was under the impression that there might be cases where LayoutContext had to be assigned a new origin and was considering the above code to be a part of that.
|
The changes to the messaging from the constellation looks good to me. I'm not familiar enough with layout to review the rest. @SimonSapin r? |
|
I forgot about how many places this would require touching to fix. Great work! |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Feb 19, 2020
Take origin from current window instead of creating a new one in event of reflow Everytime a new `LayoutContext` was created, it created a new origin which caused endless stream of image loads to occur in case of reflow. The reason for this was that the existing image, although cached successfully, was not used because the entry in hashmap did not match because of different(new) origin. This is solved by storing the origin of a window in enum `ScriptReflow` and used in creating new `LayoutContext` in case of reflow. <!-- Please describe your changes on the following line: --> r?@jdm --- <!-- 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 #24720 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
|
|
@bors-servo retry |
bors-servo
added a commit
that referenced
this pull request
Feb 19, 2020
Take origin from current window instead of creating a new one in event of reflow Everytime a new `LayoutContext` was created, it created a new origin which caused endless stream of image loads to occur in case of reflow. The reason for this was that the existing image, although cached successfully, was not used because the entry in hashmap did not match because of different(new) origin. This is solved by storing the origin of a window in enum `ScriptReflow` and used in creating new `LayoutContext` in case of reflow. <!-- Please describe your changes on the following line: --> r?@jdm --- <!-- 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 #24720 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
|
|
@bors-servo retry |
bors-servo
added a commit
that referenced
this pull request
Feb 19, 2020
Take origin from current window instead of creating a new one in event of reflow Everytime a new `LayoutContext` was created, it created a new origin which caused endless stream of image loads to occur in case of reflow. The reason for this was that the existing image, although cached successfully, was not used because the entry in hashmap did not match because of different(new) origin. This is solved by storing the origin of a window in enum `ScriptReflow` and used in creating new `LayoutContext` in case of reflow. <!-- Please describe your changes on the following line: --> r?@jdm --- <!-- 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 #24720 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
|
This was referenced Feb 19, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
kunalmohan commentedFeb 15, 2020
Everytime a new
LayoutContextwas created, it created a new origin whichcaused endless stream of image loads to occur in case of reflow. The reason
for this was that the existing image, although cached successfully, was not
used because the entry in hashmap did not match because of different(new)
origin.
This is solved by storing the origin of a window in enum
ScriptReflowandused in creating new
LayoutContextin case of reflow.r?@jdm
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors