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

[web] Environment variable to disable felt snapshot #13187

Merged
mdebbar merged 2 commits into from
Oct 17, 2019
Merged

[web] Environment variable to disable felt snapshot #13187

mdebbar merged 2 commits into from
Oct 17, 2019

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Oct 16, 2019

This PR also includes:

  • Fix a Chrome installation issue on Mac (after the yaml lock file format has changed).
  • Update README.md with instructions for doing felt tool development.

@mdebbar mdebbar added the platform-web Code specifically for the web engine label Oct 16, 2019
@mdebbar mdebbar requested review from nturgut and yjbanov October 16, 2019 22:25
@mdebbar mdebbar self-assigned this Oct 16, 2019
nturgut
nturgut approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@nturgut nturgut left a comment

Choose a reason for hiding this comment

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

Thanks Mouad!

lib/web_ui/dev/felt
echo "[Snapshot mode: off]"
# Running without snapshot means there is high likelyhood of local changes. In
# that case, let's clear the snapshot to avoid any surprises.
rm -f "$SNAPSHOT_PATH"
Copy link
Contributor

@nturgut nturgut Oct 17, 2019

Choose a reason for hiding this comment

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

suggestion: Shall we use "rm -rf"?

"-r" argument deletes recursively, may be we might want to use it?

Copy link
Contributor Author

@mdebbar mdebbar Oct 17, 2019

Choose a reason for hiding this comment

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

$SNAPSHOT_PATH and $STAMP_PATH point to files, not directories. The recursive flag -r only has an effect on directories, right?

yjbanov
yjbanov approved these changes Oct 17, 2019
View reviewed changes
lib/web_ui/dev/felt Outdated
install_deps
mkdir -p $DART_TOOL_DIR

echo "Creating the snapshot"
Copy link
Contributor

@yjbanov yjbanov Oct 17, 2019

Choose a reason for hiding this comment

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

This message seems redundant given the message above

lib/web_ui/dev/felt Outdated
"$DART_SDK_DIR/bin/dart" --snapshot="$SNAPSHOT_PATH" --packages="$WEB_UI_DIR/.packages" "$SCRIPT_PATH"
echo "$REVISION" > "$STAMP_PATH"
else
echo "[Snapshot mode: on] (using existing snapshot)"
Copy link
Contributor

@yjbanov yjbanov Oct 17, 2019

Choose a reason for hiding this comment

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

I think the default should be silent.

Copy link
Contributor Author

@mdebbar mdebbar Oct 17, 2019

Choose a reason for hiding this comment

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

I'm still debating this. On the one hand, like you said, the happy path should be silent. On the other hand, my concern is that someone would make changes in felt and forget to turn off snapshotting. Showing the message all the time makes it easier to realize that the snapshot mode is on.

I'll make it silent for now and see. If we start making a lot of mistakes, I'll "unsilent" it.

Copy link
Contributor

@yjbanov yjbanov Oct 17, 2019

Choose a reason for hiding this comment

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

Yeah, it depends on how often we'll be making changes to felt relative to using it. I'm not terribly attached to one way or the other.

@mdebbar mdebbar merged commit 8f621ad into flutter:master Oct 17, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 17, 2019
@engine-flutter-autoroll
8f621ad [web] Environment variable to disable felt snapshot (flutter/…
df83bb4 
…engine#13187)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 17, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 17, 2019
@engine-flutter-autoroll
8f621ad [web] Environment variable to disable felt snapshot (flutter/…
aee5893 
…engine#13187)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 17, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 18, 2019
@engine-flutter-autoroll
8f621ad [web] Environment variable to disable felt snapshot (flutter/…
7fbfcd0 
…engine#13187)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 18, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 18, 2019
@engine-flutter-autoroll
Roll engine 508146f..7a621a7 (26 commits) (#43017)
7da9561 
git@github.com:flutter/engine.git/compare/508146f0defb...7a621a7

git log 508146f..7a621a7 --no-merges --oneline
2019-10-18 skia-flutter-autoroll@skia.org Roll src/third_party/skia 20eafffd2d2f..b80d31f8cbe2 (4 commits) (flutter/engine#13226)
2019-10-18 skia-flutter-autoroll@skia.org Roll src/third_party/skia da29d70f1a59..20eafffd2d2f (1 commits) (flutter/engine#13223)
2019-10-18 skia-flutter-autoroll@skia.org Roll src/third_party/skia 63a387395751..da29d70f1a59 (11 commits) (flutter/engine#13221)
2019-10-18 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from WpvU_... to _G94w... (flutter/engine#13220)
2019-10-18 chinmaygarde@google.com Specify a human readable reason for an error from the embedder API. (flutter/engine#13218)
2019-10-18 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from _9Uy_... to KNygX... (flutter/engine#13219)
2019-10-17 iska.kaushik@gmail.com Reland ICU update to 64.2 (flutter/engine#13216)
2019-10-17 hterkelsen@users.noreply.github.com Use `window.devicePixelRatio` in the CanvasKit backend (flutter/engine#13192)
2019-10-17 gw280@google.com Re-enable WeakPtr ThreadChecker and fix associated failures (flutter/engine#12257)
2019-10-17 chinmaygarde@gmail.com Re-land "Custom compositor layers must take into account the device pixel ratio."
2019-10-17 chinmaygarde@gmail.com Add trace events around custom compositor callbacks. (flutter/engine#13212)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia 93e853bf2b83..63a387395751 (9 commits) (flutter/engine#13208)
2019-10-17 bkonyi@google.com Roll src/third_party/dart 9b3c7f64d8..a61c775db8 (5 commits)
2019-10-17 chinmaygarde@gmail.com Document //flutter/runtime/dart_snapshot.h (flutter/engine#13196)
2019-10-17 chinmaygarde@gmail.com Revert "Custom compositor layers must take into account the device pixel ratio. (#13193)" (flutter/engine#13211)
2019-10-17 50856934+nturgut@users.noreply.github.com wrap the text in text editing. This was causing a missalingment issue in textarea. (flutter/engine#13207)
2019-10-17 chinmaygarde@google.com Custom compositor layers must take into account the device pixel ratio. (flutter/engine#13193)
2019-10-17 mouad.debbar@gmail.com [web] Environment variable to disable felt snapshot (flutter/engine#13187)
2019-10-17 bkonyi@google.com Roll src/third_party/dart 9e636b5ab4..9b3c7f64d8 (5 commits)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0df7697235b4..93e853bf2b83 (1 commits) (flutter/engine#13205)
2019-10-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from ek5iQ... to WpvU_... (flutter/engine#13203)
2019-10-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 6j3Gw... to _9Uy_... (flutter/engine#13202)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6a19e03047cc..0df7697235b4 (1 commits) (flutter/engine#13200)
2019-10-17 bkonyi@google.com Roll src/third_party/dart 1e3e9ee04c..9e636b5ab4 (9 commits)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia f29cb70281d5..6a19e03047cc (5 commits) (flutter/engine#13198)
2019-10-17 bkonyi@google.com Roll src/third_party/dart f020ce5d23..1e3e9ee04c (12 commits)


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 franciscojma@google.com on the revert to ensure that a human
is aware of the problem.

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/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 508146f..7a621a7 (26 commits) (flutter#43017)
7bc1dbb 
git@github.com:flutter/engine.git/compare/508146f0defb...7a621a7

git log 508146f..7a621a7 --no-merges --oneline
2019-10-18 skia-flutter-autoroll@skia.org Roll src/third_party/skia 20eafffd2d2f..b80d31f8cbe2 (4 commits) (flutter/engine#13226)
2019-10-18 skia-flutter-autoroll@skia.org Roll src/third_party/skia da29d70f1a59..20eafffd2d2f (1 commits) (flutter/engine#13223)
2019-10-18 skia-flutter-autoroll@skia.org Roll src/third_party/skia 63a387395751..da29d70f1a59 (11 commits) (flutter/engine#13221)
2019-10-18 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from WpvU_... to _G94w... (flutter/engine#13220)
2019-10-18 chinmaygarde@google.com Specify a human readable reason for an error from the embedder API. (flutter/engine#13218)
2019-10-18 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from _9Uy_... to KNygX... (flutter/engine#13219)
2019-10-17 iska.kaushik@gmail.com Reland ICU update to 64.2 (flutter/engine#13216)
2019-10-17 hterkelsen@users.noreply.github.com Use `window.devicePixelRatio` in the CanvasKit backend (flutter/engine#13192)
2019-10-17 gw280@google.com Re-enable WeakPtr ThreadChecker and fix associated failures (flutter/engine#12257)
2019-10-17 chinmaygarde@gmail.com Re-land "Custom compositor layers must take into account the device pixel ratio."
2019-10-17 chinmaygarde@gmail.com Add trace events around custom compositor callbacks. (flutter/engine#13212)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia 93e853bf2b83..63a387395751 (9 commits) (flutter/engine#13208)
2019-10-17 bkonyi@google.com Roll src/third_party/dart 9b3c7f64d8..a61c775db8 (5 commits)
2019-10-17 chinmaygarde@gmail.com Document //flutter/runtime/dart_snapshot.h (flutter/engine#13196)
2019-10-17 chinmaygarde@gmail.com Revert "Custom compositor layers must take into account the device pixel ratio. (flutter#13193)" (flutter/engine#13211)
2019-10-17 50856934+nturgut@users.noreply.github.com wrap the text in text editing. This was causing a missalingment issue in textarea. (flutter/engine#13207)
2019-10-17 chinmaygarde@google.com Custom compositor layers must take into account the device pixel ratio. (flutter/engine#13193)
2019-10-17 mouad.debbar@gmail.com [web] Environment variable to disable felt snapshot (flutter/engine#13187)
2019-10-17 bkonyi@google.com Roll src/third_party/dart 9e636b5ab4..9b3c7f64d8 (5 commits)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0df7697235b4..93e853bf2b83 (1 commits) (flutter/engine#13205)
2019-10-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from ek5iQ... to WpvU_... (flutter/engine#13203)
2019-10-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 6j3Gw... to _9Uy_... (flutter/engine#13202)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6a19e03047cc..0df7697235b4 (1 commits) (flutter/engine#13200)
2019-10-17 bkonyi@google.com Roll src/third_party/dart 1e3e9ee04c..9e636b5ab4 (9 commits)
2019-10-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia f29cb70281d5..6a19e03047cc (5 commits) (flutter/engine#13198)
2019-10-17 bkonyi@google.com Roll src/third_party/dart f020ce5d23..1e3e9ee04c (12 commits)


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 franciscojma@google.com on the revert to ensure that a human
is aware of the problem.

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/+/master/autoroll/README.md
@mdebbar mdebbar deleted the felt_snapshot branch April 15, 2021 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@yjbanov yjbanov yjbanov approved these changes

@nturgut nturgut nturgut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mdebbar mdebbar

Labels

cla: yes platform-web Code specifically for the web engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mdebbar @yjbanov @nturgut @googlebot