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

Conversation

@fzyzcjy
Copy link
Contributor

@fzyzcjy fzyzcjy commented Mar 30, 2022

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
Please see flutter/flutter#100830

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#100830

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 30, 2022
@fzyzcjy fzyzcjy marked this pull request as ready for review March 30, 2022 12:59
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.

Just some nits, would defer to @flar for approval on DL code and tests but it all seems pretty fine to me.

@bdero @chinmaygarde FYI since this increases surface API that Impeller will end up needing to support if we land it.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

It looks good with a few minor fixes.

@fzyzcjy fzyzcjy requested a review from flar March 31, 2022 00:42
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Mar 31, 2022

The error seems to be:

02:50 �[32m+259�[0m�[33m ~6�[0m�[31m -1�[0m: test/html/paragraph/bidi_golden_test.dart: bidi with selection (DOM) �[1m�[31m[E]�[0m�[0m                                                                                                             
  TimeoutException after 0:00:30.000000: Test timed out after 30 seconds. See https://pub.dev/packages/test#timeouts
  /Users/chrome-bot/.pub-cache/hosted/pub.dartlang.org/test_api-0.4.1/lib/src/backend/invoker.dart 333:9   call
  org-dartlang-sdk:///lib/async/zone.dart 1418:39                                                          _rootRun
  org-dartlang-sdk:///lib/async/zone.dart 1327:34                                                          run
  /Users/chrome-bot/.pub-cache/hosted/pub.dartlang.org/test_api-0.4.1/lib/src/backend/invoker.dart 331:5   _handleError
  /Users/chrome-bot/.pub-cache/hosted/pub.dartlang.org/test_api-0.4.1/lib/src/backend/invoker.dart 326:8   _handleError
  /Users/chrome-bot/.pub-cache/hosted/pub.dartlang.org/test_api-0.4.1/lib/src/backend/invoker.dart 287:9   call
  org-dartlang-sdk:///lib/async/zone.dart 1426:12                                                          _rootRun
  org-dartlang-sdk:///lib/async/zone.dart 1327:34                                                          run
  /Users/chrome-bot/.pub-cache/hosted/pub.dartlang.org/test_api-0.4.1/lib/src/backend/invoker.dart 286:38  call
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 131:9                                  call
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/js_helper.dart 1818:14                                  invokeClosure
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/js_helper.dart 1852:1                                   <fn>
  
Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'pub run test --chain-stack-traces'.

Thus maybe unrelated to my PR?

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Mar 31, 2022

By the way, I see 25. Upload artifacts android-arm64-profile in the CI. So I wonder where is the uploaded artifacts? I want to have a try on them to ensure they are really correct when used in my app.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Apr 1, 2022

Hi, is there any updates?

@dnfield
Copy link
Contributor

dnfield commented Apr 1, 2022

There were some CI issues today or yesterday - not sure if they're resolved yet but might be worth just merging up to head and pushing a new commit to kick CI.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Apr 1, 2022

@flar @dnfield It is green now!

image

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Looks good to go pending a couple of minor changes:

  • Fix what looks like cut/paste errors in the dl_unittests.cc
  • Try to pare down the reliance on bounds padding in the canvas_unittests.cc (for Dilate/Erode the bounds should be nearly perfectly accurate so a padding tolerance of 21 pixels seems a bit overkill, hopefully the dilate/erode tests would not need bounds tolerance at all?)

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Apr 2, 2022

Everything seems green now, what should I do next?

image

@flar
Copy link
Contributor

flar commented Apr 2, 2022

Everything seems green now, what should I do next?

image

In general, we are moving to a state where all PRs have to use the "Waiting for tree to go green" label to get submitted. I seem to recall a requirement that developers need 2 approving reviews from authorized reviewers to be eligible to be pushed, but I don't see the checks asking for that. I'll add the label and see if it goes in.

@flar flar added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 2, 2022
@flar
Copy link
Contributor

flar commented Apr 2, 2022

If the bot comes along and removes the label, then tag @dnfield to complete a review and then the label should work.

Note that for now the tree is broken (the "luci-engine" failure in the list) so the label is currently in a wait state on that condition.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 2, 2022
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Apr 2, 2022

@flar I see.

@dnfield Hi could you please approve this PR such that it can be merged?

@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 4, 2022
@dnfield
Copy link
Contributor

dnfield commented Apr 4, 2022

Once the tree is open this should get landed by the bot.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Apr 4, 2022

@dnfield Thank you

@fluttergithubbot fluttergithubbot merged commit 9b3117a into flutter:main Apr 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 5, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Apr 5, 2022
* 9b3117a Create `ImageFilter.dilate`/`ImageFilter.erode` (flutter/engine#32334)

* 92a6ade Roll Fuchsia Mac SDK from m_-rjFvCk... to hJaq9O7XI... (flutter/engine#32402)

* 31fd1bb Roll Fuchsia Linux SDK from 5abhmXb9Q... to WdxX5Sqix... (flutter/engine#32403)

* f088801 Roll Skia from 5215ec1ab9cd to fd9c66e18030 (1 revision) (flutter/engine#32406)

* ac21195 Fix inconsistent enum/class private member naming (flutter/engine#32409)

* 75e7cfd Fix deltas when selection is active and composing begins on MacOS (flutter/engine#32412)

* 7e5989b Fix SemanticsAction naming consistency (flutter/engine#32411)

* 1b3e9dc Fix a crash when setting clipboardData to null on iOS (flutter/engine#32413)

* ef50b28 Roll Fuchsia Linux SDK from WdxX5Sqix... to PmeDIogNb... (flutter/engine#32422)

* b48d65e Roll Fuchsia Mac SDK from hJaq9O7XI... to WBAQhRswX... (flutter/engine#32423)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-web Code specifically for the web engine waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants