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

Conversation

@zhongwuzw
Copy link
Member

If we don't use callback cache, every time we start the app, the log prints "Runner[667:97662] [VERBOSE0:callback_cache.cc(136)] Could not parse callback cache, aborting restore", which may lead to misleading.

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.

@zhongwuzw zhongwuzw requested a review from bkonyi October 25, 2022 10:31
@flutter-dashboard
Copy link

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.

@bkonyi
Copy link
Contributor

bkonyi commented Oct 26, 2022

Thanks for the contribution! Just waiting for sign-off from @Hixie to see if we're okay not adding a test for this.

@Hixie
Copy link
Contributor

Hixie commented Oct 26, 2022

Ideally we would repurpose some of our integration tests over on the framework repo to trigger the condition that results in this output, and verify that the logs are entirely clear of extraneous messages. We have a few tests that check things like this already, at least on Android. cc @jmagman who may know precisely which tests would be best suited to use for this.

@jmagman
Copy link
Member

jmagman commented Oct 26, 2022

I'm not familiar with how this cache works to know which tests might catch this, what are the cases where would it be empty?

@zhongwuzw
Copy link
Member Author

zhongwuzw commented Oct 27, 2022

I'm not familiar with how this cache works to know which tests might catch this, what are the cases where would it be empty?

@jmagman Seems most cases of users should be empty, only when navigator restorable method use it , like https://github.com/flutter/flutter/blob/d9a2229babbc7e28cabd0ad42470c527a6cd9d3e/packages/flutter/lib/src/widgets/navigator.dart#L5386 and https://github.com/flutter/flutter/blob/d9a2229babbc7e28cabd0ad42470c527a6cd9d3e/packages/flutter/lib/src/widgets/navigator.dart#L4461 whitch calles ui.PluginUtilities.getCallbackHandle

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Even though this is a verbose log, I don't think it adds any value. I'd be down to just removing it as it isn't actionable.

@jmagman
Copy link
Member

jmagman commented Oct 27, 2022

Even though this is a verbose log, I don't think it adds any value. I'd be down to just removing it as it isn't actionable.

SGTM, @zhongwuzw can you remove the Could not parse callback cache log?

@zhongwuzw
Copy link
Member Author

Even though this is a verbose log, I don't think it adds any value. I'd be down to just removing it as it isn't actionable.

SGTM, @zhongwuzw can you remove the Could not parse callback cache log?

@jmagman Done. :)

@zhongwuzw zhongwuzw requested a review from jmagman October 28, 2022 02:52
@zhongwuzw zhongwuzw requested a review from jmagman October 31, 2022 03:38
@zhongwuzw zhongwuzw added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 1, 2022
@auto-submit auto-submit bot merged commit f721db6 into flutter:main Nov 1, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 1, 2022
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants