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

Conversation

dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Aug 17, 2023

This PR Rasterizer::Draw's discard_callback parameter, which is assigned by Shell, to Delegate, which is also implemented by Shell. This refactory makes the API cleaner.

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.

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.

Not sure if this was ready for review just yet but we ran into this in triage. LGTM after the nits.


auto rendered_scene = context.GetNextSceneImage();

printf("main 1\n");fflush(stdout);
Copy link
Member

Choose a reason for hiding this comment

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

Spurious printfs.


// |Rasterizer::Delegate|
bool ShouldDiscardLayerTree(int64_t view_id,
flutter::LayerTree& tree) override;
Copy link
Member

Choose a reason for hiding this comment

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

Can this be const?

Suggested change
flutter::LayerTree& tree) override;
const flutter::LayerTree& tree) override;

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

LGTM

@dkwingsmt dkwingsmt force-pushed the internalize-rasterizer-discarding branch from dae8622 to c491896 Compare August 23, 2023 02:12
@dkwingsmt dkwingsmt merged commit 48a5226 into flutter:main Aug 23, 2023
@dkwingsmt dkwingsmt deleted the internalize-rasterizer-discarding branch August 23, 2023 04:25
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 23, 2023
…133139)

flutter/engine@7d56840...72a0642

2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from ee7404db8273 to d0c369eaa637 (1 revision) (flutter/engine#44997)
2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from d0d2b7042bb9 to ee7404db8273 (1 revision) (flutter/engine#44995)
2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from eac8f8ea2660 to d0d2b7042bb9 (1 revision) (flutter/engine#44994)
2023-08-23 zanderso@users.noreply.github.com Revert "FontVariation.lerp, custom FontVariation constructors, and more documentation" (flutter/engine#44993)
2023-08-23 skia-flutter-autoroll@skia.org Roll Skia from aa208c8a2d60 to eac8f8ea2660 (1 revision) (flutter/engine#44992)
2023-08-23 dkwingsmt@users.noreply.github.com Move Rasterizer::Draw's discard_callback to Delegate (flutter/engine#44813)
2023-08-23 ian@hixie.ch FontVariation.lerp, custom FontVariation constructors, and more documentation (flutter/engine#43750)
2023-08-23 ian@hixie.ch Make web tests start with an empty title (flutter/engine#43846)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
This PR `Rasterizer::Draw`'s `discard_callback` parameter, which is
assigned by `Shell`, to `Delegate`, which is also implemented by
`Shell`. This refactory makes the API cleaner.

## 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].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants