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
Overwrite default RunInfo values with the first real ones encountered. #16258
Conversation
Heads up! This PR modifies the following files:
|
@stshine Is this something you could review? |
@bors-servo: try |
Overwrite default RunInfo values with the first real ones encountered. TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14783 - [X] There are tests for these changes <!-- 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/16258) <!-- Reviewable:end -->
💔 Test failed - linux-rel-wpt |
Sigh. The new test passes on my macbook and fails on both the linux and mac builders. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. However, to make text-overflow
actually working we need #16265 get merged first.
components/layout/text.rs
Outdated
@@ -230,6 +231,14 @@ impl TextRunScanner { | |||
None => false | |||
}; | |||
|
|||
if !initialized_run_info { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the iteration every time we start to deal with a fragment if the font changed we may create an empty mapping, and when we are done with the fragment we flush all the remaining text to mapping, so instead using a temp variable for the whole iteration we should check if end_position
is zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@bors-servo try |
@bors-servo retry |
Overwrite default RunInfo values with the first real ones encountered. TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14783 - [X] There are tests for these changes <!-- 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/16258) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev |
60f09b7
to
a2e8ab4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@bors-servo: r=stshine |
📌 Commit a2e8ab4 has been approved by |
Overwrite default RunInfo values with the first real ones encountered. TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14783 - [X] There are tests for these changes <!-- 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/16258) <!-- Reviewable:end -->
💔 Test failed - mac-rel-css |
@bors-servo: try |
Overwrite default RunInfo values with the first real ones encountered. TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14783 - [X] There are tests for these changes <!-- 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/16258) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev |
@pcwalton Could you verify that this makes sense? |
Looks good overall. Only one nit: maybe rename |
7c9efa6
to
a20d0bc
Compare
@bors-servo: r=pcwalton,stshine |
📌 Commit a20d0bc has been approved by |
Overwrite default RunInfo values with the first real ones encountered. TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14783 - [X] There are tests for these changes <!-- 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/16258) <!-- Reviewable:end -->
💔 Test failed - android |
@bors-servo: retry |
Overwrite default RunInfo values with the first real ones encountered. TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14783 - [X] There are tests for these changes <!-- 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/16258) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev |
@jdm Thank you for letting me review. |
TextRunScanner::flush_clump_to_list compares the values in the RunInfo object against the ones determined from the current fragment, but these values can be arbitrary defaults that don't mean anything useful.
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsThis change is