Skip to content

/ webrender

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

Make ClipScrollGroup per ClipScrollInfo #1500

Merged
merged 1 commit into from Jul 24, 2017
Merged

Make ClipScrollGroup per ClipScrollInfo #1500

merged 1 commit into from Jul 24, 2017
+89 −104

Conversation

@mrobinson
Copy link
Member

mrobinson commented Jul 19, 2017 •
edited by larsbergstrom

Instead of making each ClipScrollGroup per stacking context and
ClipScrollInfo combination, make them per ClipScrollInfo. This should
reduce the amount of work done per stacking context and is the first
step toward accepting all coordinates relative to reference frames.

This change is Reviewable
@mrobinson mrobinson requested review from glennw and kvark Jul 19, 2017
@kvark
kvark approved these changes Jul 19, 2017
View changes
Copy link
Member

kvark left a comment

looks solid to me!
@glennw
glennw approved these changes Jul 19, 2017
View changes
Copy link
Member

glennw left a comment

Couple of minor comments.
webrender/src/frame_builder.rs Outdated
@@ -294,6 +295,7 @@ pub struct FrameBuilder {

stacking_context_store: Vec<StackingContext>,
clip_scroll_group_store: Vec<ClipScrollGroup>,
clip_scroll_group_indices: HashMap<ClipAndScrollInfo, ClipScrollGroupIndex>,

This comment has been minimized.

Sign in to view
@glennw

glennw Jul 19, 2017

Member

We should make this use Fnv as the hasher.

This comment has been minimized.

Sign in to view
@mrobinson

mrobinson Jul 20, 2017

Author Member

Okay. I'll replace this with FnvHasher.
webrender/src/frame_builder.rs Outdated
@@ -1050,6 +1046,7 @@ impl FrameBuilder {
run: TextRun {
font_key,
logical_font_size: size,
run_offset,

This comment has been minimized.

Sign in to view
@glennw

glennw Jul 19, 2017

Member

There is an offset field in text runs with this #1498 - perhaps we can make use of that here, which applies the offset in the vertex shader? That could be done as a follow up though.

This comment has been minimized.

Sign in to view
@mrobinson

mrobinson Jul 20, 2017

Author Member

The newest version just reuses this offset. Thanks for the suggestion!
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2017

☔ The latest upstream changes (presumably #1498) made this pull request unmergeable. Please resolve the merge conflicts.
@mrobinson mrobinson force-pushed the mrobinson:per-clip-scroll-info-clip-scroll-groups branch from 6c5fa65 to 3fd614c Jul 20, 2017
@glennw
Copy link
Member

glennw commented Jul 21, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

📌 Commit 3fd614c has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

🔒 Merge conflict
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

☔ The latest upstream changes (presumably #1504) made this pull request unmergeable. Please resolve the merge conflicts.
@glennw
Copy link
Member

glennw commented Jul 23, 2017

Needs a rebase.
@mrobinson
Make ClipScrollGroup per ClipScrollInfo
Loading status checks…
028f2bf 
Instead of making each ClipScrollGroup per stacking context and
ClipScrollInfo combination, make them per ClipScrollInfo. This should
reduce the amount of work done per stacking context and is the first
step toward accepting all coordinates relative to reference frames.
@mrobinson mrobinson force-pushed the mrobinson:per-clip-scroll-info-clip-scroll-groups branch from 3fd614c to 028f2bf Jul 24, 2017
@mrobinson
Copy link
Member Author

mrobinson commented Jul 24, 2017

Rebased. Going to land the new version. Thank you both for the reviews!

@bors-servo r=glennw
@bors-servo
Copy link
Contributor

bors-servo commented Jul 24, 2017

📌 Commit 028f2bf has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented Jul 24, 2017

⌛ Testing commit 028f2bf with merge 9ebb50b...
bors-servo added a commit that referenced this pull request Jul 24, 2017
@bors-servo
Auto merge of #1500 - mrobinson:per-clip-scroll-info-clip-scroll-grou…
Loading status checks…
9ebb50b 
…ps, r=glennw

Make ClipScrollGroup per ClipScrollInfo

Instead of making each ClipScrollGroup per stacking context and
ClipScrollInfo combination, make them per ClipScrollInfo. This should
reduce the amount of work done per stacking context and is the first
step toward accepting all coordinates relative to reference frames.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1500)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 24, 2017

☀️ Test successful - status-travis
Approved by: glennw
Pushing 9ebb50b to master...
@bors-servo bors-servo merged commit 028f2bf into servo:master Jul 24, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mrobinson mrobinson deleted the mrobinson:per-clip-scroll-info-clip-scroll-groups branch Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark

@glennw glennw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@mrobinson @bors-servo @glennw @kvark
You can’t perform that action at this time.