Skip to content

/ servo

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

Burn SelectedFileId in fire #12898

Merged
merged 1 commit into from Aug 22, 2016
Merged

Burn SelectedFileId in fire #12898

merged 1 commit into from Aug 22, 2016

Conversation

@izgzhen
Copy link
Contributor

izgzhen commented Aug 16, 2016 •
edited by larsbergstrom

r? @Manishearth

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

This change is Reviewable
@highfive
Copy link

highfive commented Aug 16, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/filemanager_thread.rs, components/script/dom/url.rs, components/net/blob_loader.rs, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/script/dom/bindings/trace.rs, components/script/dom/blob.rs
@highfive
Copy link

highfive commented Aug 16, 2016

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 16, 2016

It's a bad title now I found ... I didn't burn it, I just aliased it. Should I burn it?
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 16, 2016

In case it is not clear, I invent the wrapper before since at that time Uuid doesn't implement Serialize/Deserialize.
components/net_traits/filemanager_thread.rs Outdated
/// FileID used in inter-process message
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct SelectedFileId(pub String);
pub type SelectedFileId = Uuid;

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Aug 17, 2016

Member

Why keep the typedef at all?
@Manishearth
Copy link
Member

Manishearth commented Aug 17, 2016

Yes, I feel that it can be burned. Otherwise lgtm
@Manishearth
Copy link
Member

Manishearth commented Aug 17, 2016

Unit tests need updating
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 17, 2016

:/ thread 'filemanager_thread::test_filemanager' panicked at 'Broken channel: Serde(Custom("bincode does not support Deserializer::deserialize"))', ../src/libcore/result.rs:788

Thing is worse than I originally thought.
@bors-servo
Copy link
Contributor

bors-servo commented Aug 17, 2016

☔ The latest upstream changes (presumably #12897) made this pull request unmergeable. Please resolve the merge conflicts.
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 17, 2016

I am digging in uuid implementation. I will park this PR for a few days until I fixed this
@izgzhen izgzhen mentioned this pull request Aug 17, 2016
Merged
@izgzhen izgzhen force-pushed the izgzhen:burn-selected-file-id branch 2 times, most recently from a8f0561 to 8f81e48 Aug 18, 2016
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 18, 2016

bincode problem fixed. crates updated.
@Manishearth
Copy link
Member

Manishearth commented Aug 18, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2016

📌 Commit 8f81e48 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2016

⌛ Testing commit 8f81e48 with merge e28753d...
bors-servo added a commit that referenced this pull request Aug 18, 2016
@bors-servo
Auto merge of #12898 - izgzhen:burn-selected-file-id, r=Manishearth
Loading status checks…
e28753d 
Burn SelectedFileId in fire

r? @Manishearth

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

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

bors-servo commented Aug 18, 2016

💔 Test failed - linux-dev
@izgzhen izgzhen force-pushed the izgzhen:burn-selected-file-id branch from 8f81e48 to 0e55b1d Aug 18, 2016
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 19, 2016

resolved
@Manishearth
Copy link
Member

Manishearth commented Aug 19, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

📌 Commit e7d8230 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Aug 19, 2016

⌛ Testing commit e7d8230 with merge e94f905...
bors-servo added a commit that referenced this pull request Aug 19, 2016
@bors-servo
Auto merge of #12898 - izgzhen:burn-selected-file-id, r=Manishearth
Loading status checks…
e94f905 
Burn SelectedFileId in fire

r? @Manishearth

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- 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/12898)
<!-- Reviewable:end -->
@Ms2ger Ms2ger closed this Aug 19, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 19, 2016

Will reopen in a bit, homu got confused again
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 22, 2016 •
edited

good to reopen?
@Manishearth Manishearth reopened this Aug 22, 2016
@Manishearth
Copy link
Member

Manishearth commented Aug 22, 2016

needs merge conflict fixes
@Manishearth
Copy link
Member

Manishearth commented Aug 22, 2016

@bors-servo clean r-
@KiChjang
Copy link
Member

KiChjang commented Aug 22, 2016

Needs a new commit hash. @izgzhen can you do git commit --amend -C HEAD followed by git push -f?
@izgzhen
Burn SelectedFileId in fire
Loading status checks…
2527dc0
@izgzhen izgzhen force-pushed the izgzhen:burn-selected-file-id branch from e7d8230 to 2527dc0 Aug 22, 2016
@izgzhen
Copy link
Contributor Author

izgzhen commented Aug 22, 2016

I resolved the conflict and it looks like a new hash now to me
@Manishearth
Copy link
Member

Manishearth commented Aug 22, 2016

@bors-servo r+ clean
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2016

📌 Commit 2527dc0 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2016

⌛ Testing commit 2527dc0 with merge 43d4a45...
bors-servo added a commit that referenced this pull request Aug 22, 2016
@bors-servo
Auto merge of #12898 - izgzhen:burn-selected-file-id, r=Manishearth
Loading status checks…
43d4a45 
Burn SelectedFileId in fire

r? @Manishearth

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- 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/12898)
<!-- Reviewable:end -->
@bors-servo bors-servo mentioned this pull request Aug 22, 2016
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit 2527dc0 into servo:master Aug 22, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
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
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@izgzhen @highfive @Manishearth @bors-servo @Ms2ger @KiChjang
You can’t perform that action at this time.