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.

Sign up for GitHub

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

Jump to bottom

Create own file for background calculations in layout #19686

Merged
merged 1 commit into from Jan 7, 2018
Merged

Create own file for background calculations in layout #19686

merged 1 commit into from Jan 7, 2018

Conversation

pyfisch
Copy link
Contributor

@pyfisch pyfisch commented Jan 4, 2018 •
edited by SimonSapin

See #19676

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Jan 4, 2018

Heads up! This PR modifies the following files:

  • @emilio: components/layout/display_list_builder.rs, components/layout/background.rs, components/layout/lib.rs

@highfive
Copy link

highfive commented Jan 4, 2018

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 4, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 4, 2018

Maybe compute_background_placement should also be moved but while it only is a method of Fragment to access self.border_padding it depends on a lot of inputs.

emilio
emilio reviewed Jan 4, 2018
View reviewed changes
Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should consider moving this to gfx? Or to a dedicated layout directory like layout/display_list/ (moving display_list_builder to layout/display_list/builder.rs)?

@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 4, 2018

Maybe we should consider moving this to gfx? Or to a dedicated layout directory like layout/display_list/ (moving display_list_builder to layout/display_list/builder.rs)?

I am not sure how layout and gfx should be separated. From what I can see gfx mainly handles fonts. It also has the display lists code but it would be no longer need if only servos display lists are used. (Some code would probably remain.)

I am fine with both suggestions.

@emilio
Copy link
Member

emilio commented Jan 4, 2018

@mbrubeck or @glennw, any opinion on the discussion above?

@emilio
Copy link
Member

emilio commented Jan 6, 2018

Given radio silence, I'd go with layout/display_list for now, does that sound fine @pyfisch? :)

@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 6, 2018

Updated. I have also moved the webrender_helpers. See the commit message.

@pyfisch
Create own file for background calculations in layout
ea062e6 
Move display_list_builder.rs and webrender_helpers.rs
along with the new file to components/layout/display_list/

Remove apparently unused IdType enum.
Only variant used was OverflowClip.

See servo#19676
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 7, 2018

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit ea062e6 with merge 307fe60...

bors-servo pushed a commit that referenced this pull request Jan 7, 2018
Auto merge of #19686 - pyfisch:layout-background, r=<try>
307fe60 
Create own file for background calculations in layout

See #19676

<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] 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/19686)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt2

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 7, 2018
@pyfisch
Copy link
Contributor Author

pyfisch commented Jan 7, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Trying commit ea062e6 with merge c21323c...

bors-servo pushed a commit that referenced this pull request Jan 7, 2018
Auto merge of #19686 - pyfisch:layout-background, r=<try>
c21323c 
Create own file for background calculations in layout

See #19676

<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] 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/19686)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

emilio
emilio approved these changes Jan 7, 2018
View reviewed changes
Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot!

@emilio
Copy link
Member

emilio commented Jan 7, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit ea062e6 has been approved by emilio

@highfive highfive assigned emilio and unassigned Manishearth Jan 7, 2018
@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. S-tests-failed The changes caused existing tests to fail. labels Jan 7, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit ea062e6 with merge 8e226fe...

bors-servo pushed a commit that referenced this pull request Jan 7, 2018
Auto merge of #19686 - pyfisch:layout-background, r=emilio
8e226fe 
Create own file for background calculations in layout

See #19676

<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] 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/19686)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 8e226fe to master...

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

@emilio emilio emilio approved these changes

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pyfisch @highfive @emilio @bors-servo @Manishearth