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

layout: Unify size calculation of replaced elements #14490

Merged
merged 7 commits into from Dec 12, 2016

Conversation

@stshine
Copy link
Contributor

stshine commented Dec 8, 2016


  • ./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 Dec 8, 2016

Heads up! This PR modifies the following files:

  • @emilio: components/layout/construct.rs, components/layout/fragment.rs, components/layout/display_list_builder.rs, components/layout/block.rs, components/layout/flex.rs, components/layout/model.rs
@highfive
Copy link

highfive commented Dec 8, 2016

warning Warning warning

  • These commits modify layout code, but no tests are modified. Please consider adding a test!
@stshine
Copy link
Contributor Author

stshine commented Dec 8, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Dec 8, 2016

Trying commit 1c6b563 with merge 3c3bf82...

bors-servo added a commit that referenced this pull request Dec 8, 2016
[wip] layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 8, 2016

💔 Test failed - linux-rel-css

@stshine stshine force-pushed the stshine:replaced-size branch from 1c6b563 to 8d47987 Dec 8, 2016
@highfive highfive removed the S-tests-failed label Dec 8, 2016
@stshine
Copy link
Contributor Author

stshine commented Dec 8, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Dec 8, 2016

Trying commit 8d47987 with merge a81a1bb...

bors-servo added a commit that referenced this pull request Dec 8, 2016
[wip] layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 8, 2016

💔 Test failed - linux-rel-css

@stshine stshine force-pushed the stshine:replaced-size branch from 8d47987 to 95a8e07 Dec 8, 2016
@highfive highfive removed the S-tests-failed label Dec 8, 2016
@stshine
Copy link
Contributor Author

stshine commented Dec 8, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Dec 8, 2016

Trying commit 95a8e07 with merge da30026...

bors-servo added a commit that referenced this pull request Dec 8, 2016
[wip] layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 8, 2016

💔 Test failed - windows-dev

@stshine
Copy link
Contributor Author

stshine commented Dec 8, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Dec 8, 2016

Trying commit 95a8e07 with merge 864fafe...

bors-servo added a commit that referenced this pull request Dec 8, 2016
[wip] layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 8, 2016

💔 Test failed - mac-rel-wpt2

Copy link
Member

asajeffrey left a comment

You probably need a reviewer who knows the layout system better than me!

SpecificFragmentInfo::Svg(_) |
SpecificFragmentInfo::Image(_) |
SpecificFragmentInfo::Canvas(_) |
SpecificFragmentInfo::InlineBlock(_) => true,
SpecificFragmentInfo::Iframe(_) => true,
SpecificFragmentInfo::ScannedText(_) => panic!("A block that is also a text fragment!"),

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Dec 8, 2016

Member

Why are we introducing a panic here? isn't is_replaced_content just a side-effect-free test?

This comment has been minimized.

Copy link
@emilio

emilio Dec 8, 2016

Member

@asajeffrey This is never supposed to happen, but I agree this panic is out of place.

This comment has been minimized.

Copy link
@stshine

stshine Dec 9, 2016

Author Contributor

I thought I could complete this PR last night, but unfortunately ran into another bug: on replaced elements margins in block direction do not take effect. While this bug is really weird, I will try to fix it this morning.

This comment has been minimized.

Copy link
@stshine

stshine Dec 10, 2016

Author Contributor

I will try to move this to fragment.rs.

@stshine
Copy link
Contributor Author

stshine commented Dec 12, 2016

Oh no.
@bors-servo r-

@stshine
Copy link
Contributor Author

stshine commented Dec 12, 2016

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2016

📌 Commit 7bca303 has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2016

Testing commit 7bca303 with merge c7ecbfc...

bors-servo added a commit that referenced this pull request Dec 12, 2016
layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 12, 2016

💔 Test failed - mac-dev-unit

@KiChjang
Copy link
Member

KiChjang commented Dec 12, 2016

@bors-servo retry infra

@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2016

Testing commit 7bca303 with merge 9775d97...

bors-servo added a commit that referenced this pull request Dec 12, 2016
layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 12, 2016

💔 Test failed - mac-rel-wpt2

@stshine
Copy link
Contributor Author

stshine commented Dec 12, 2016

@bors-servo retry

infra

@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2016

Testing commit 7bca303 with merge 1993b6e...

bors-servo added a commit that referenced this pull request Dec 12, 2016
layout: Unify size calculation of replaced elements

<!-- 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: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Dec 12, 2016

@bors-servo bors-servo merged commit 7bca303 into servo:master Dec 12, 2016
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
@stshine stshine deleted the stshine:replaced-size branch Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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