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

fix(capture_webrender): try fallback capture dir #20750

Merged
merged 1 commit into from May 16, 2018

Conversation

@kwonoj
Copy link
Contributor

kwonoj commented May 4, 2018

This PR try to update behavior of webrender capture to have fallback dir - first it try to create under current working dir, if fails go back to $TMPDIR. I was debating between TMP vs $home, bit hesitate to create some folder under user's home dir directly so choose TMP instead. Can be easily changed, or add few more if needed.


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

This change is Reviewable

@highfive
Copy link

highfive commented May 4, 2018

Heads up! This PR modifies the following files:

@kwonoj kwonoj force-pushed the kwonoj:fix-wr-path branch from 6b8e43b to 1ec078a May 5, 2018
@paulrouget
Copy link
Contributor

paulrouget commented May 16, 2018

Any reason to not always save under a temp directory?

Otherwise, lgtm.

@kwonoj
Copy link
Contributor Author

kwonoj commented May 16, 2018

Any reason to not always save under a temp directory?

Mostly I felt it's somewhat easier to explain to ppl - such as run servo you just downloaded by click in explorer (finder, or else), capture, you'll see new dir generated vs. please try to traverse down TMPDIR based on your platform. Other than those, no strong reason.

@paulrouget
Copy link
Contributor

paulrouget commented May 16, 2018

@bors-servo
Copy link
Contributor

bors-servo commented May 16, 2018

📌 Commit 1ec078a has been approved by paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented May 16, 2018

Testing commit 1ec078a with merge d1f4fcf...

bors-servo added a commit that referenced this pull request May 16, 2018
fix(capture_webrender): try fallback capture dir

<!-- Please describe your changes on the following line: -->

This PR try to update behavior of webrender capture to have fallback dir - first it try to create under current working dir, if fails go back to $TMPDIR. I was debating between TMP vs $home, bit hesitate to create some folder under user's home dir directly so choose TMP instead. Can be easily changed, or add few more if needed.

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

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

<!-- 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/20750)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented May 16, 2018

@bors-servo treeclosed=8000

@bors-servo
Copy link
Contributor

bors-servo commented May 16, 2018

@bors-servo bors-servo merged commit 1ec078a into servo:master May 16, 2018
2 of 4 checks passed
2 of 4 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@nox
Copy link
Member

nox commented May 17, 2018

@bors-servo treeclosed-

@jdm
Copy link
Member

jdm commented May 17, 2018

@bors-servo treeclosed=8000

@nox
Copy link
Member

nox commented May 17, 2018

Can't do that in an already-merged PR.

@kwonoj kwonoj deleted the kwonoj:fix-wr-path branch Aug 30, 2018
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.

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