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

Make font template data load fallible #12076

Merged
merged 1 commit into from Sep 19, 2016
Merged

Make font template data load fallible #12076

merged 1 commit into from Sep 19, 2016

Conversation

@jdm
Copy link
Member

jdm commented Jul 1, 2016

Remove a TODO around dealing with a failed file operation.

Can we write an automated test for this? I don't really know what font template data is, but this failure seems to be fontconfig-specific...


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12037
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Jul 1, 2016

warning Warning warning

  • These commits modify gfx code, but no tests are modified. Please consider adding a test!
@cbrewster
Copy link
Member

cbrewster commented Jul 5, 2016

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/gfx/font_template.rs, line 157 [r1] (raw file):

                                       .expect("Instantiation successful but no descriptor?")
                                       .distance_from(requested_descriptor);
                    (self.data().ok().map(|data| (data, distance)))

Remove extra parenthesis.


components/gfx/platform/freetype/font_template.rs, line 6 [r1] (raw file):

use std::fs::File;
use std::io::{Read, Error};

Any reason why Error isn't imported as IoError here?


Comments from Reviewable

@jdm
Copy link
Member Author

jdm commented Jul 5, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/gfx/platform/freetype/font_template.rs, line 6 [r1] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Any reason why Error isn't imported as IoError here?

There's no conflicting Error already imported.

Comments from Reviewable

@jdm jdm force-pushed the jdm:font-load branch from 8efac94 to fb943ab Jul 5, 2016
@jdm
Copy link
Member Author

jdm commented Jul 5, 2016

@pcwalton Any idea what would be required in order to test this?

@jdm
Copy link
Member Author

jdm commented Jul 14, 2016

No idea about test stuff, but this is gathering dust. Let's review!

@pcwalton
Copy link
Contributor

pcwalton commented Jul 25, 2016

I don't know how testable this is. I think the error path only happens when the OS reports a font as installed but we couldn't open it for whatever reason, right?

@jdm
Copy link
Member Author

jdm commented Jul 26, 2016

Sounds plausible.

@pcwalton
Copy link
Contributor

pcwalton commented Jul 26, 2016

The only way I could think of to test this would be to either artificially simulate I/O errors or to do something silly like have the test harness set permissions on fonts to make them temporarily unreadable.

@jdm
Copy link
Member Author

jdm commented Sep 17, 2016

@bors-servo: r=pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Sep 17, 2016

📌 Commit fb943ab has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Sep 17, 2016

Testing commit fb943ab with merge df09bb9...

bors-servo added a commit that referenced this pull request Sep 17, 2016
Make font template data load fallible

Remove a TODO around dealing with a failed file operation.

Can we write an automated test for this? I don't really know what font template data is, but this failure seems to be fontconfig-specific...

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12037
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Sep 17, 2016

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member Author

jdm commented Sep 19, 2016

bors-servo added a commit that referenced this pull request Sep 19, 2016
Make font template data load fallible

Remove a TODO around dealing with a failed file operation.

Can we write an automated test for this? I don't really know what font template data is, but this failure seems to be fontconfig-specific...

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12037
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Sep 19, 2016

Testing commit fb943ab with merge a82d510...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2016

@bors-servo bors-servo merged commit fb943ab into servo:master Sep 19, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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.

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