Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Add ThreadConfigSetter for Embedder api that can set thread priority #31626

Merged
dnfield merged 1 commit into from
Mar 2, 2022
Merged

Add ThreadConfigSetter for Embedder api that can set thread priority #31626

dnfield merged 1 commit into from
Mar 2, 2022

Conversation

@JsouLiang
Copy link
Contributor

@JsouLiang JsouLiang commented Feb 23, 2022

Change the embedder.cc create thread host api for that we can specified the thread priority

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Feb 23, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API needs tests labels Feb 23, 2022
@JsouLiang JsouLiang force-pushed the Add-ThreadConfig-API-For-Embedder branch from 9fb764c to c29939d Compare February 24, 2022 03:10
@JsouLiang JsouLiang changed the title [WIP] Add ThreadConfigSetter for Embedder api that can set thread priority Add ThreadConfigSetter for Embedder api that can set thread priority Feb 24, 2022
@JsouLiang
Copy link
Contributor Author

JsouLiang commented Feb 24, 2022 •
edited
Loading

@dnfield hello, please give me a review?

dnfield
dnfield reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems right to me - please add a test to embedder_unittests.cc for this

@JsouLiang JsouLiang force-pushed the Add-ThreadConfig-API-For-Embedder branch from c29939d to 129bc31 Compare February 25, 2022 03:38
dnfield
dnfield approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dnfield
Copy link
Contributor

dnfield commented Feb 25, 2022

LGTM once tests are passing anyway :)

@JsouLiang JsouLiang force-pushed the Add-ThreadConfig-API-For-Embedder branch 4 times, most recently from e100894 to 51cdb9b Compare February 26, 2022 16:36
@JsouLiang JsouLiang force-pushed the Add-ThreadConfig-API-For-Embedder branch from 51cdb9b to af2dd3e Compare February 27, 2022 02:07
@JsouLiang
Copy link
Contributor Author

JsouLiang commented Feb 27, 2022 •
edited
Loading

LGTM once tests are passing anyway :)

testes have done

@dnfield
Copy link
Contributor

dnfield commented Feb 28, 2022

This needs a second review, probably from @chinmaygarde or @gaaclarke

@dnfield
Copy link
Contributor

dnfield commented Mar 2, 2022

@cbracken maybe?

@dnfield dnfield mentioned this pull request Mar 2, 2022
Open
@gaaclarke
Copy link
Member

gaaclarke commented Mar 2, 2022

I reviewed the changes for iOS and Android, this LGTM.

@dnfield dnfield merged commit 2c396fe into flutter:main Mar 2, 2022
cbracken
cbracken reviewed Mar 2, 2022
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! lgtm

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 2, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2022
@engine-flutter-autoroll
2c396fe Add ThreadConfigSetter for Embedder api that can set thread p…
da3c90b 
…riority (flutter/engine#31626)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@cbracken cbracken cbracken left review comments

@dnfield dnfield dnfield approved these changes

@gaaclarke gaaclarke gaaclarke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

embedder Related to the embedder API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JsouLiang @dnfield @gaaclarke @cbracken