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

Warning patrol #17872

Merged
merged 4 commits into from Jul 27, 2017

Conversation

@jdm
Copy link
Member

commented Jul 26, 2017

This greens up every build except the android one.


This change is Reviewable

jdm added 3 commits Jul 26, 2017
@canaltinova

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

📌 Commit b8d8e15 has been approved by canaltinova

@highfive highfive assigned canaltinova and unassigned nox Jul 26, 2017

@KiChjang

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

@bors-servo r-

I'd like @mbrubeck or someone else familiar to the CEF architecture to review the 2nd commit.

@jdm

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2017

@KiChjang Why? It's an unused variable since 9591d1e.

@KiChjang

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

@jdm Thanks for linking that up! However, there may be other intended usages of rendering_thread that I am not aware of, and I'm being conservative when removing code that may potentially be used somewhere.

@jdm jdm assigned mbrubeck and unassigned canaltinova Jul 26, 2017

@emilio

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

That code is used nowhere. There's no point of keeping it in tree. If someone ever wants to add an option for that back it's pretty easy to do so.

r=me, we can remove the extra struct field in cef_options_t as a followup.

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

📌 Commit b8d8e15 has been approved by emilio

@highfive highfive assigned emilio and unassigned mbrubeck Jul 26, 2017

@emilio

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

Err, cef_settings_t.

@emilio

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

If you're interested, the option was used introduced in #4193

@emilio

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

Seems like it could be used to set the layout.threads pref. However, that option was introduced in the time when rust had green threads, so using 128 by default would be insane.

Anyway, I'm fine with removing dead code. If someone ever tries to maintain the CEF port again and needs it it's super-easy to add back.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

⌛️ Testing commit b8d8e15 with merge fa9a85a...

bors-servo added a commit that referenced this pull request Jul 27, 2017
Auto merge of #17872 - servo:jdm-patch-1, r=emilio
Warning patrol

This greens up every build except the android one.

<!-- 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/17872)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

⛄️ The build was interrupted to prioritize another pull request.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

⌛️ Testing commit b8d8e15 with merge 6d364151845c350c5a02a4248826c85674987db5...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

💔 Test failed - mac-dev-unit

@KiChjang

This comment has been minimized.

Copy link
Member

commented Jul 27, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

📌 Commit 7359b76 has been approved by KiChjang

@highfive highfive assigned KiChjang and unassigned emilio Jul 27, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

⌛️ Testing commit 7359b76 with merge ad38516...

bors-servo added a commit that referenced this pull request Jul 27, 2017
Auto merge of #17872 - servo:jdm-patch-1, r=KiChjang
Warning patrol

This greens up every build except the android one.

<!-- 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/17872)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

@bors-servo bors-servo merged commit 7359b76 into master Jul 27, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details

@SimonSapin SimonSapin deleted the jdm-patch-1 branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.