Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flag to ParagraphBuilder for rounding hack migration #43118

Merged

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Jun 22, 2023

The goal is to remove the rounding applied in skparagraph and in the framework: flutter/flutter#31707

The plumbing is done via a new static variable ParagraphBuilder.shouldDisableRoundingHack that toggles the rounding behavior in skparagraph and the flag is read by framework code. Application code and test code can either use ParagraphBuilder.setDisableRoundingHack or --dart-define="SKPARAGRAPH_REMOVE_ROUNDING_HACK=1" to opt-in.
Once the internal migration is finished the default value of the flag will be set to true.

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.

@LongCatIsLooong LongCatIsLooong marked this pull request as draft June 22, 2023 23:53
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Jun 22, 2023
@LongCatIsLooong LongCatIsLooong force-pushed the apply-rounding-hack-plumbing branch 3 times, most recently from 82a22bc to 207c9ec Compare June 23, 2023 06:35
@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review June 23, 2023 08:03
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

ParagraphStyle is part of dart:ui which is exposed directly to users, and so I am hesitant to add something temporary like this if we don't have to. I am assuming the plan is to remove this hack completely or shift it to the framework side, and this flag is needed in order to make changes in lock step with the framework. If so, it feels like maybe some sort of global flag that determines this behavior would be easier to remove than piping it through the paragraph style itself, which feels more disruptive to the API overall.

Also, for what it's worth, you probably can just not change any of the skwasm stuff at all. We don't run any framework tests that produce goldens or scubas in Google3 with the skwasm renderer yet, so there are less concerns about the migration there.

@LongCatIsLooong LongCatIsLooong marked this pull request as draft June 23, 2023 17:32
@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review June 23, 2023 20:11
@LongCatIsLooong
Copy link
Contributor Author

I removed skwasm-related changes and moved the global flag to dart:ui.

@LongCatIsLooong LongCatIsLooong changed the title Add a flag to ParagraphStyle for rounding hack migration Add a flag to ParagraphBuilder for rounding hack migration Jun 23, 2023
@chingjun
Copy link
Contributor

The approach that uses ParagraphBuilder.setDisableRoundingHack for internal migration LGTM

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2023
@auto-submit auto-submit bot merged commit f55f087 into flutter:main Jul 12, 2023
30 checks passed
@LongCatIsLooong LongCatIsLooong deleted the apply-rounding-hack-plumbing branch July 12, 2023 20:42
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 12, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
…130458)

flutter/engine@16e2ab7...1b1ccdd

2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

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 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
…130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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 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
...
LongCatIsLooong added a commit that referenced this pull request Jul 13, 2023
auto-submit bot pushed a commit that referenced this pull request Jul 13, 2023
…#43642)

Reverts #43118

The incorrect default value (`true` instead of  `false`) was used in the PR and that caused internal test failures. I'll add a test before trying to reland.
LongCatIsLooong added a commit to LongCatIsLooong/engine that referenced this pull request Jul 13, 2023
CaseyHillers pushed a commit to CaseyHillers/engine that referenced this pull request Jul 13, 2023
…flutter#43642)

Reverts flutter#43118

The incorrect default value (`true` instead of  `false`) was used in the PR and that caused internal test failures. I'll add a test before trying to reland.
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
…lutter#130458)

flutter/engine@16e2ab7...1b1ccdd

2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

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 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
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
…lutter#130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 ian@hixie.ch Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 chinmaygarde@google.com [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 chinmaygarde@google.com [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 jonahwilliams@google.com [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 jonahwilliams@google.com [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 flar@google.com move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 ian@hixie.ch Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 58529443+srujzs@users.noreply.github.com Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 31859944+LongCatIsLooong@users.noreply.github.com Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 john@johnmccutchan.com [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 flar@google.com Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 skia-flutter-autoroll@skia.org Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 skia-flutter-autoroll@skia.org Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 jonahwilliams@google.com [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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 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
...
auto-submit bot pushed a commit that referenced this pull request Jul 13, 2023
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
…#43118)

The goal is to remove the rounding applied in skparagraph and in the framework: flutter/flutter#31707

The plumbing is done via a new static variable `ParagraphBuilder.shouldDisableRoundingHack` that toggles the rounding behavior in skparagraph and the flag is read by framework code. Application code and test code can either use `ParagraphBuilder.setDisableRoundingHack` or `--dart-define="SKPARAGRAPH_REMOVE_ROUNDING_HACK=1"` to opt-in. 
Once the internal migration is finished the default value of the flag will be set to true.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
…flutter#43642)

Reverts flutter#43118

The incorrect default value (`true` instead of  `false`) was used in the PR and that caused internal test failures. I'll add a test before trying to reland.
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
…flutter#43642)

Reverts flutter#43118

The incorrect default value (`true` instead of  `false`) was used in the PR and that caused internal test failures. I'll add a test before trying to reland.
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
///
/// Do not rely on this getter as it exists for migration purposes only and
/// will soon be removed.
static bool get shouldDisableRoundingHack {
Copy link
Member

Choose a reason for hiding this comment

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

Can we mark this one as @deprecated to give a stronger signal that it is not to be used? That's how we usually mark temporary things for migration purposes in the framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
5 participants