This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Fix lint errors in lib/ui#19988
Merged
Merged
Conversation
gaaclarke
approved these changes
Jul 24, 2020
| if (!image) { | ||
| Dart_ThrowException( | ||
| ToDart("Canvas.drawImageRect called with non-genuine Image.")); | ||
| return; |
Contributor
There was a problem hiding this comment.
These seem like a pretty big oversight if they are necessary. I guess people assummed Dart_ThrowException would roll back the stack?
Contributor
Author
There was a problem hiding this comment.
Yeah, I think so. Basically, I think whoever wrote it assumed they were "real" exceptions.
| auto ui_task = fml::MakeCopyable([raw_image_callback, dart_state, | ||
| unref_queue]( | ||
| sk_sp<SkImage> raster_image) mutable { | ||
| auto image_callback = std::make_unique<tonic::DartPersistentValue>( |
Contributor
There was a problem hiding this comment.
This would be a safer change if we created the image_callback in the same scope it used to, then used std::move to let the lambda own it.
My concern is that raw_image_callback might be deleted in this case before image_callback is created. DartPersistentValue might be doing some retain count on that value.
| namespace { | ||
|
|
||
| void LoadFontFromList(tonic::Uint8List& font_data, | ||
| void LoadFontFromList(tonic::Uint8List& font_data, // NOLINT |
Contributor
There was a problem hiding this comment.
fyi: I believe you can say // NOLINT(name_of_lint) It might be easier to track these that way, up to you if you want to change it.
Contributor
|
Thanks greg! I thought this would take a while for us to migrate our stuff. I really appreciate it. |
gaaclarke
suggested changes
Jul 24, 2020
gaaclarke
left a comment
Contributor
There was a problem hiding this comment.
I thought about it a bit more and I think we really should make that change I mentioned about where the creation of the tonic::DartPersistentValue
Contributor
Author
|
Yeah, given the failing test, I agree with you. |
9abd32c to
ad7e459
Compare
Contributor
Author
|
@gaaclarke OK, take a look at picture.cc and let me know if that's what you had in mind. |
gaaclarke
reviewed
Jul 30, 2020
gaaclarke
left a comment
Contributor
There was a problem hiding this comment.
I'm still a bit uneasy about the change because of the comment on deleting the image_callback and how hard it is to reason about where the shared_ptr is actually being deleted.
If you can explain to me that it is deleting it as expected thats fine. Otherwise I'd try using std::unique_ptr and seeing where that leads you (might be a lot of work). Otherwise I think it's better to disable linting somehow for now.
| auto* dart_state = UIDartState::Current(); | ||
| tonic::DartPersistentValue* image_callback = | ||
| new tonic::DartPersistentValue(dart_state, raw_image_callback); | ||
| auto image_callback = std::make_shared<tonic::DartPersistentValue>(dart_state, raw_image_callback); |
Contributor
There was a problem hiding this comment.
I think this is good. FWIW I was thinking you could use unique_ptr. Then when you capture it in the lambda you can use std::move. It's slightly more efficient.
auto foo = std::make_unique<Foo>();
auto ui_task = fml::MakeCopyable([bar = std::move(foo)]() mutable {
});But I don't think that would have worked because ui_task is copied. There would have to be further refactoring to make that work.
| // on the UI thread | ||
| delete image_callback; | ||
| // on the UI thread. | ||
| image_callback.reset(); |
Contributor
There was a problem hiding this comment.
The comment above this reset is a bit ominous. With shared_ptr it's a bit harder to reason about whether the object is actually deleted here or not. I'm not sure that it is.
Contributor
Author
|
OK, I've switched to a |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 31, 2020
iskakaushik
pushed a commit
to flutter/flutter
that referenced
this pull request
Jul 31, 2020
* 4521162 Roll Skia from 39d1c1ebb607 to 3a608e5bc9d5 (1 revision) (flutter-team-archive/engine#20101) * b265bd2 Roll Skia from 3a608e5bc9d5 to 620bfa3fffba (1 revision) (flutter-team-archive/engine#20102) * 36c5f60 Roll Skia from 620bfa3fffba to 4866d0ad5f3c (1 revision) (flutter-team-archive/engine#20104) * 06fef5e Enable lazy-async-stacks by-default in all modes (2) (flutter-team-archive/engine#19270) * d71b11f Roll Fuchsia Linux SDK from ETPOTPepP... to ROYgzKMaF... (flutter-team-archive/engine#20105) * 42e4ebf Roll Skia from 4866d0ad5f3c to c34efe0da102 (1 revision) (flutter-team-archive/engine#20108) * 3a65b1a Roll Skia from c34efe0da102 to 8c7ecc1c06f4 (1 revision) (flutter-team-archive/engine#20109) * ad99f5e Roll Skia from 8c7ecc1c06f4 to 4f587337c306 (1 revision) (flutter-team-archive/engine#20110) * f0cc38f [web] Set correct defaults for text in canvas (flutter-team-archive/engine#20067) * 02f9ed9 Roll Fuchsia Mac SDK from 5dM55hp8B... to hzo88TZzN... (flutter-team-archive/engine#20113) * 34389f5 Roll Skia from 4f587337c306 to 96d6c6f04dcb (4 revisions) (flutter-team-archive/engine#20114) * 49a40fa Enabled linting on engine.cc (flutter-team-archive/engine#19981) * b955e15 Manual roll of Dart from 24c7666def...40fd1c456e (flutter-team-archive/engine#20092) * 145ef60 Remove confusing logs (flutter-team-archive/engine#20121) * 8fc3926 Roll Skia from 96d6c6f04dcb to 57d859edd3c4 (16 revisions) (flutter-team-archive/engine#20126) * ec9e17c Roll zlib (flutter-team-archive/engine#20119) * f288fe5 [web] Enable canvas measurement by default (flutter-team-archive/engine#19924) * 8464208 Add missing MouseCursorPlugin destroy call (flutter-team-archive/engine#19968) * 5498add Roll Skia from 57d859edd3c4 to 994ce8cf2300 (1 revision) (flutter-team-archive/engine#20129) * 9398bc4 Roll Skia from 994ce8cf2300 to 398c654ce7be (2 revisions) (flutter-team-archive/engine#20133) * d3bc43e Roll Fuchsia Linux SDK from ROYgzKMaF... to d4pESQYnB... (flutter-team-archive/engine#20132) * 4068918 Manual roll of Dart 40fd1c456e...7e8348f4ce (flutter-team-archive/engine#20125) * 4c49e0b Manual roll of Dart cb6ed67a73...7e8348f4ce (flutter-team-archive/engine#20135) * 4a3688e Roll Skia from 398c654ce7be to a4bbc9d8ec4f (1 revision) (flutter-team-archive/engine#20136) * b3c6fd3 Roll Fuchsia Mac SDK from hzo88TZzN... to 3XwiR_wVO... (flutter-team-archive/engine#20137) * adb5986 Manual roll of Dart 03e4737f31...cb6ed67a73 (flutter-team-archive/engine#20138) * 941c442 Add ALERT SoundType enum value (flutter-team-archive/engine#20139) * 19368ef Fix dartdocs of dart:ui (flutter-team-archive/engine#20140) * 9dd3d2e Roll Skia from a4bbc9d8ec4f to 94cefeff50d2 (1 revision) (flutter-team-archive/engine#20141) * 0fdb141 Roll Dart SDK from 03e4737f3115 to 59600f2b46ef (54 revisions) (flutter-team-archive/engine#20143) * 97d6ee2 Roll Skia from 94cefeff50d2 to 5ba6534884d9 (2 revisions) (flutter-team-archive/engine#20146) * da3d495 Roll Dart SDK from 59600f2b46ef to 04f4272546af (5 revisions) (flutter-team-archive/engine#20147) * 8229df8 Roll Skia from 5ba6534884d9 to e393c61a1563 (1 revision) (flutter-team-archive/engine#20148) * 8e1d48e Migrate some Dart_WeakPersistentHandle uses to Dart_FinalizableHandle (flutter-team-archive/engine#20107) * 27b61e7 Roll ANGLE to pick up warning fixes (flutter-team-archive/engine#19105) * 146d504 Roll Skia from e393c61a1563 to 3136789972ea (5 revisions) (flutter-team-archive/engine#20150) * 841b391 Roll Dart SDK from 04f4272546af to e87cb96bb89c (7 revisions) (flutter-team-archive/engine#20152) * e9334c9 Roll Skia from 3136789972ea to 5f2b2d6dc691 (5 revisions) (flutter-team-archive/engine#20153) * f2b02d8 [iOS] Fixes text input plugin crash (flutter-team-archive/engine#20127) * 3ed5893 Roll Skia from 5f2b2d6dc691 to 8cc118dce813 (2 revisions) (flutter-team-archive/engine#20154) * 7c5162e Roll Fuchsia Mac SDK from 3XwiR_wVO... to T2xc0OuiK... (flutter-team-archive/engine#20155) * e23e477 Lint fixes for fml, tools subdirs (flutter-team-archive/engine#19990) * f620eac Roll Dart SDK from e87cb96bb89c to bd528bfbd69d (8 revisions) (flutter-team-archive/engine#20158) * e1c9673 Fix targets in build_fuchsia_artifacts (flutter-team-archive/engine#19794) * c134e16 add information collection for safari bots (flutter-team-archive/engine#20123) * ee4d50c Revert "Enable lazy-async-stacks by-default in all modes (2) (#19270)" (flutter-team-archive/engine#20165) * 357b155 Roll Fuchsia Linux SDK from d4pESQYnB... to d_5wDVmBd... (flutter-team-archive/engine#20161) * a6cd3eb Fix lint errors in lib/ui (flutter-team-archive/engine#19988) * 280bbfc This makes the lint script use multiprocessing to speed it up. (flutter-team-archive/engine#19987)
Pragya007
pushed a commit
to Pragya007/flutter
that referenced
this pull request
Aug 11, 2020
* 4521162 Roll Skia from 39d1c1ebb607 to 3a608e5bc9d5 (1 revision) (flutter-team-archive/engine#20101) * b265bd2 Roll Skia from 3a608e5bc9d5 to 620bfa3fffba (1 revision) (flutter-team-archive/engine#20102) * 36c5f60 Roll Skia from 620bfa3fffba to 4866d0ad5f3c (1 revision) (flutter-team-archive/engine#20104) * 06fef5e Enable lazy-async-stacks by-default in all modes (2) (flutter-team-archive/engine#19270) * d71b11f Roll Fuchsia Linux SDK from ETPOTPepP... to ROYgzKMaF... (flutter-team-archive/engine#20105) * 42e4ebf Roll Skia from 4866d0ad5f3c to c34efe0da102 (1 revision) (flutter-team-archive/engine#20108) * 3a65b1a Roll Skia from c34efe0da102 to 8c7ecc1c06f4 (1 revision) (flutter-team-archive/engine#20109) * ad99f5e Roll Skia from 8c7ecc1c06f4 to 4f587337c306 (1 revision) (flutter-team-archive/engine#20110) * f0cc38f [web] Set correct defaults for text in canvas (flutter-team-archive/engine#20067) * 02f9ed9 Roll Fuchsia Mac SDK from 5dM55hp8B... to hzo88TZzN... (flutter-team-archive/engine#20113) * 34389f5 Roll Skia from 4f587337c306 to 96d6c6f04dcb (4 revisions) (flutter-team-archive/engine#20114) * 49a40fa Enabled linting on engine.cc (flutter-team-archive/engine#19981) * b955e15 Manual roll of Dart from 24c7666def...40fd1c456e (flutter-team-archive/engine#20092) * 145ef60 Remove confusing logs (flutter-team-archive/engine#20121) * 8fc3926 Roll Skia from 96d6c6f04dcb to 57d859edd3c4 (16 revisions) (flutter-team-archive/engine#20126) * ec9e17c Roll zlib (flutter-team-archive/engine#20119) * f288fe5 [web] Enable canvas measurement by default (flutter-team-archive/engine#19924) * 8464208 Add missing MouseCursorPlugin destroy call (flutter-team-archive/engine#19968) * 5498add Roll Skia from 57d859edd3c4 to 994ce8cf2300 (1 revision) (flutter-team-archive/engine#20129) * 9398bc4 Roll Skia from 994ce8cf2300 to 398c654ce7be (2 revisions) (flutter-team-archive/engine#20133) * d3bc43e Roll Fuchsia Linux SDK from ROYgzKMaF... to d4pESQYnB... (flutter-team-archive/engine#20132) * 4068918 Manual roll of Dart 40fd1c456e...7e8348f4ce (flutter-team-archive/engine#20125) * 4c49e0b Manual roll of Dart cb6ed67a73...7e8348f4ce (flutter-team-archive/engine#20135) * 4a3688e Roll Skia from 398c654ce7be to a4bbc9d8ec4f (1 revision) (flutter-team-archive/engine#20136) * b3c6fd3 Roll Fuchsia Mac SDK from hzo88TZzN... to 3XwiR_wVO... (flutter-team-archive/engine#20137) * adb5986 Manual roll of Dart 03e4737f31...cb6ed67a73 (flutter-team-archive/engine#20138) * 941c442 Add ALERT SoundType enum value (flutter-team-archive/engine#20139) * 19368ef Fix dartdocs of dart:ui (flutter-team-archive/engine#20140) * 9dd3d2e Roll Skia from a4bbc9d8ec4f to 94cefeff50d2 (1 revision) (flutter-team-archive/engine#20141) * 0fdb141 Roll Dart SDK from 03e4737f3115 to 59600f2b46ef (54 revisions) (flutter-team-archive/engine#20143) * 97d6ee2 Roll Skia from 94cefeff50d2 to 5ba6534884d9 (2 revisions) (flutter-team-archive/engine#20146) * da3d495 Roll Dart SDK from 59600f2b46ef to 04f4272546af (5 revisions) (flutter-team-archive/engine#20147) * 8229df8 Roll Skia from 5ba6534884d9 to e393c61a1563 (1 revision) (flutter-team-archive/engine#20148) * 8e1d48e Migrate some Dart_WeakPersistentHandle uses to Dart_FinalizableHandle (flutter-team-archive/engine#20107) * 27b61e7 Roll ANGLE to pick up warning fixes (flutter-team-archive/engine#19105) * 146d504 Roll Skia from e393c61a1563 to 3136789972ea (5 revisions) (flutter-team-archive/engine#20150) * 841b391 Roll Dart SDK from 04f4272546af to e87cb96bb89c (7 revisions) (flutter-team-archive/engine#20152) * e9334c9 Roll Skia from 3136789972ea to 5f2b2d6dc691 (5 revisions) (flutter-team-archive/engine#20153) * f2b02d8 [iOS] Fixes text input plugin crash (flutter-team-archive/engine#20127) * 3ed5893 Roll Skia from 5f2b2d6dc691 to 8cc118dce813 (2 revisions) (flutter-team-archive/engine#20154) * 7c5162e Roll Fuchsia Mac SDK from 3XwiR_wVO... to T2xc0OuiK... (flutter-team-archive/engine#20155) * e23e477 Lint fixes for fml, tools subdirs (flutter-team-archive/engine#19990) * f620eac Roll Dart SDK from e87cb96bb89c to bd528bfbd69d (8 revisions) (flutter-team-archive/engine#20158) * e1c9673 Fix targets in build_fuchsia_artifacts (flutter-team-archive/engine#19794) * c134e16 add information collection for safari bots (flutter-team-archive/engine#20123) * ee4d50c Revert "Enable lazy-async-stacks by-default in all modes (2) (flutter#19270)" (flutter-team-archive/engine#20165) * 357b155 Roll Fuchsia Linux SDK from d4pESQYnB... to d_5wDVmBd... (flutter-team-archive/engine#20161) * a6cd3eb Fix lint errors in lib/ui (flutter-team-archive/engine#19988) * 280bbfc This makes the lint script use multiprocessing to speed it up. (flutter-team-archive/engine#19987)
mingwandroid
pushed a commit
to mingwandroid/flutter
that referenced
this pull request
Sep 6, 2020
* 4521162 Roll Skia from 39d1c1ebb607 to 3a608e5bc9d5 (1 revision) (flutter-team-archive/engine#20101) * b265bd2 Roll Skia from 3a608e5bc9d5 to 620bfa3fffba (1 revision) (flutter-team-archive/engine#20102) * 36c5f60 Roll Skia from 620bfa3fffba to 4866d0ad5f3c (1 revision) (flutter-team-archive/engine#20104) * 06fef5e Enable lazy-async-stacks by-default in all modes (2) (flutter-team-archive/engine#19270) * d71b11f Roll Fuchsia Linux SDK from ETPOTPepP... to ROYgzKMaF... (flutter-team-archive/engine#20105) * 42e4ebf Roll Skia from 4866d0ad5f3c to c34efe0da102 (1 revision) (flutter-team-archive/engine#20108) * 3a65b1a Roll Skia from c34efe0da102 to 8c7ecc1c06f4 (1 revision) (flutter-team-archive/engine#20109) * ad99f5e Roll Skia from 8c7ecc1c06f4 to 4f587337c306 (1 revision) (flutter-team-archive/engine#20110) * f0cc38f [web] Set correct defaults for text in canvas (flutter-team-archive/engine#20067) * 02f9ed9 Roll Fuchsia Mac SDK from 5dM55hp8B... to hzo88TZzN... (flutter-team-archive/engine#20113) * 34389f5 Roll Skia from 4f587337c306 to 96d6c6f04dcb (4 revisions) (flutter-team-archive/engine#20114) * 49a40fa Enabled linting on engine.cc (flutter-team-archive/engine#19981) * b955e15 Manual roll of Dart from 24c7666def...40fd1c456e (flutter-team-archive/engine#20092) * 145ef60 Remove confusing logs (flutter-team-archive/engine#20121) * 8fc3926 Roll Skia from 96d6c6f04dcb to 57d859edd3c4 (16 revisions) (flutter-team-archive/engine#20126) * ec9e17c Roll zlib (flutter-team-archive/engine#20119) * f288fe5 [web] Enable canvas measurement by default (flutter-team-archive/engine#19924) * 8464208 Add missing MouseCursorPlugin destroy call (flutter-team-archive/engine#19968) * 5498add Roll Skia from 57d859edd3c4 to 994ce8cf2300 (1 revision) (flutter-team-archive/engine#20129) * 9398bc4 Roll Skia from 994ce8cf2300 to 398c654ce7be (2 revisions) (flutter-team-archive/engine#20133) * d3bc43e Roll Fuchsia Linux SDK from ROYgzKMaF... to d4pESQYnB... (flutter-team-archive/engine#20132) * 4068918 Manual roll of Dart 40fd1c456e...7e8348f4ce (flutter-team-archive/engine#20125) * 4c49e0b Manual roll of Dart cb6ed67a73...7e8348f4ce (flutter-team-archive/engine#20135) * 4a3688e Roll Skia from 398c654ce7be to a4bbc9d8ec4f (1 revision) (flutter-team-archive/engine#20136) * b3c6fd3 Roll Fuchsia Mac SDK from hzo88TZzN... to 3XwiR_wVO... (flutter-team-archive/engine#20137) * adb5986 Manual roll of Dart 03e4737f31...cb6ed67a73 (flutter-team-archive/engine#20138) * 941c442 Add ALERT SoundType enum value (flutter-team-archive/engine#20139) * 19368ef Fix dartdocs of dart:ui (flutter-team-archive/engine#20140) * 9dd3d2e Roll Skia from a4bbc9d8ec4f to 94cefeff50d2 (1 revision) (flutter-team-archive/engine#20141) * 0fdb141 Roll Dart SDK from 03e4737f3115 to 59600f2b46ef (54 revisions) (flutter-team-archive/engine#20143) * 97d6ee2 Roll Skia from 94cefeff50d2 to 5ba6534884d9 (2 revisions) (flutter-team-archive/engine#20146) * da3d495 Roll Dart SDK from 59600f2b46ef to 04f4272546af (5 revisions) (flutter-team-archive/engine#20147) * 8229df8 Roll Skia from 5ba6534884d9 to e393c61a1563 (1 revision) (flutter-team-archive/engine#20148) * 8e1d48e Migrate some Dart_WeakPersistentHandle uses to Dart_FinalizableHandle (flutter-team-archive/engine#20107) * 27b61e7 Roll ANGLE to pick up warning fixes (flutter-team-archive/engine#19105) * 146d504 Roll Skia from e393c61a1563 to 3136789972ea (5 revisions) (flutter-team-archive/engine#20150) * 841b391 Roll Dart SDK from 04f4272546af to e87cb96bb89c (7 revisions) (flutter-team-archive/engine#20152) * e9334c9 Roll Skia from 3136789972ea to 5f2b2d6dc691 (5 revisions) (flutter-team-archive/engine#20153) * f2b02d8 [iOS] Fixes text input plugin crash (flutter-team-archive/engine#20127) * 3ed5893 Roll Skia from 5f2b2d6dc691 to 8cc118dce813 (2 revisions) (flutter-team-archive/engine#20154) * 7c5162e Roll Fuchsia Mac SDK from 3XwiR_wVO... to T2xc0OuiK... (flutter-team-archive/engine#20155) * e23e477 Lint fixes for fml, tools subdirs (flutter-team-archive/engine#19990) * f620eac Roll Dart SDK from e87cb96bb89c to bd528bfbd69d (8 revisions) (flutter-team-archive/engine#20158) * e1c9673 Fix targets in build_fuchsia_artifacts (flutter-team-archive/engine#19794) * c134e16 add information collection for safari bots (flutter-team-archive/engine#20123) * ee4d50c Revert "Enable lazy-async-stacks by-default in all modes (2) (flutter#19270)" (flutter-team-archive/engine#20165) * 357b155 Roll Fuchsia Linux SDK from d4pESQYnB... to d_5wDVmBd... (flutter-team-archive/engine#20161) * a6cd3eb Fix lint errors in lib/ui (flutter-team-archive/engine#19988) * 280bbfc This makes the lint script use multiprocessing to speed it up. (flutter-team-archive/engine#19987)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This fixes all of the lint errors in lib/ui, except for a few (three, I think) where it would have changed the API, converting non-const references to pointers. For those, I just did NOLINT on the particular line instead of ignoring the whole file.