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

Reduce code-duplication a bit and add more error context across SkiaGoldClient. - #51426

Merged
matanlurey merged 1 commit into
flutter-team-archive:mainfrom
matanlurey:engine-skia-gold-throw-refactor
Mar 14, 2024
Merged

Reduce code-duplication a bit and add more error context across SkiaGoldClient.#51426
matanlurey merged 1 commit into
flutter-team-archive:mainfrom
matanlurey:engine-skia-gold-throw-refactor

Conversation

@matanlurey

Copy link
Copy Markdown
Contributor
  • Replaced manual StringBuffer()..writeln('stdout: ...') with a single SkiaGoldProcessError constructor.
  • Updated tests to make sure it's working.

/cc @dnfield @jonahwilliams FYI only.

@matanlurey
matanlurey requested review from gaaclarke and mdebbar March 14, 2024 20:04
import 'package:path/path.dart' as path;
import 'package:process/process.dart';

import 'src/errors.dart';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these uri's have the package in them instead of being relative paths?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly either way (would prefer a lint).

It appears we're intentionally opting out. I'll file a follow-up issue to suggest opting back in?

Screenshot 2024-03-14 at 1 26 25 PM

@matanlurey
matanlurey merged commit ec4cfe4 into flutter-team-archive:main Mar 14, 2024
@matanlurey
matanlurey deleted the engine-skia-gold-throw-refactor branch March 14, 2024 21:55
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2024
auto-submit Bot pushed a commit to flutter/flutter that referenced this pull request Mar 15, 2024
flutter-team-archive/engine@622b372...995d890

2024-03-14 matanlurey@users.noreply.github.com Add some header-goodies for et. (flutter-team-archive/engine#51434)
2024-03-14 jonahwilliams@google.com [Impeller] revert glyph atlas texture recycling. (flutter-team-archive/engine#51428)
2024-03-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add DisplayList Region and Transform benchmarks to CI (#51429)" (flutter-team-archive/engine#51432)
2024-03-14 flar@google.com Add DisplayList Region and Transform benchmarks to CI (flutter-team-archive/engine#51429)
2024-03-14 skia-flutter-autoroll@skia.org Roll Dart SDK from 2bc8b222d01f to 70ca2323a702 (1 revision) (flutter-team-archive/engine#51430)
2024-03-14 matanlurey@users.noreply.github.com Reduce code-duplication a bit and add more error context across `SkiaGoldClient`. (flutter-team-archive/engine#51426)
2024-03-14 zanderso@users.noreply.github.com [et] build and run commands disable RBE with a flag or when not available (flutter-team-archive/engine#51404)
2024-03-14 matej.knopp@gmail.com Fix flakiness in FlutterVSyncWaiterTest.VSyncWorks and FlutterDisplayLinkTest.WorkaroundForFB13482573 (flutter-team-archive/engine#51405)

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 bdero@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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@mdebbar

mdebbar commented Mar 15, 2024

Copy link
Copy Markdown
Contributor

Belated LGTM. Thanks @matanlurey for the cleanup!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants