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

Document //flutter/runtime/dart_snapshot.h #13196

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

Document //flutter/runtime/dart_snapshot.h #13196

chinmaygarde merged 2 commits into from
Oct 17, 2019

Conversation

@chinmaygarde
Copy link
Contributor

@chinmaygarde chinmaygarde commented Oct 17, 2019

jason-simmons
jason-simmons reviewed Oct 17, 2019
View reviewed changes
runtime/dart_snapshot.h Outdated
/// the host snapshot the state of the Dart heap at specific points
/// and package the same with the Flutter application. When the Dart
/// VM on the target is configured to run AOT compiled Dart code,
/// the tools also compile the developers Flutter application code
Copy link
Member

@jason-simmons jason-simmons Oct 17, 2019

Choose a reason for hiding this comment

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

typo: "developers" -> "developer's"

Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
/// to target specific machine code (instructions). This class deals
/// with the mapping of both these buffers at runtime on the target.
///
/// A flutter application typically needs two instances of this
Copy link
Member

@jason-simmons jason-simmons Oct 17, 2019

Choose a reason for hiding this comment

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

typo: "flutter" -> "Flutter"

runtime/dart_snapshot.h Outdated
/// These snapshots are typically memory-mapped at runtime, or,
/// referenced directly as symbols present in Mach or ELF binaries.
///
/// In case of the core snapshot, the snapshot is collected when the
Copy link
Member

@jason-simmons jason-simmons Oct 17, 2019

Choose a reason for hiding this comment

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

typo: "In case" -> "In the case"

Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
///
/// A flutter application typically needs two instances of this
/// class at runtime to run Dart code. One instance is for the VM
/// and is also called the "core snapshot". The other is the isolate
Copy link
Member

@jason-simmons jason-simmons Oct 17, 2019

Choose a reason for hiding this comment

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

remove "also"

Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
public:
//----------------------------------------------------------------------------
/// When referencing the heap component of the core snapshot via the dynamic
/// linker, the symbols name is expected to be this constant.
Copy link
Member

@jason-simmons jason-simmons Oct 17, 2019

Choose a reason for hiding this comment

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

typo: "symbols" -> "symbol's"
(same for the other snapshot symbols)

I think this would be clearer with phrasing like: "The symbol name of the heap data in the core snapshot dynamic library"

Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
/// the core snapshot.
///
/// @attention Depending on the runtime mode of the Flutter application and
/// the target the Flutter is running on, a complex fallback
Copy link
Member

@jason-simmons jason-simmons Oct 17, 2019

Choose a reason for hiding this comment

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

typo: "target the" -> "target that"

(also applies to VMSnapshotFromSettings and IsolateSnapshotFromSettings)

Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

chinmaygarde
chinmaygarde commented Oct 17, 2019
View reviewed changes
Copy link
Contributor Author

@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.

Addressed all comments.

runtime/dart_snapshot.h Outdated
/// the host snapshot the state of the Dart heap at specific points
/// and package the same with the Flutter application. When the Dart
/// VM on the target is configured to run AOT compiled Dart code,
/// the tools also compile the developers Flutter application code
Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
///
/// A flutter application typically needs two instances of this
/// class at runtime to run Dart code. One instance is for the VM
/// and is also called the "core snapshot". The other is the isolate
Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
/// These snapshots are typically memory-mapped at runtime, or,
/// referenced directly as symbols present in Mach or ELF binaries.
///
/// In case of the core snapshot, the snapshot is collected when the
Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
public:
//----------------------------------------------------------------------------
/// When referencing the heap component of the core snapshot via the dynamic
/// linker, the symbols name is expected to be this constant.
Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

runtime/dart_snapshot.h Outdated
/// the core snapshot.
///
/// @attention Depending on the runtime mode of the Flutter application and
/// the target the Flutter is running on, a complex fallback
Copy link
Contributor Author

@chinmaygarde chinmaygarde Oct 17, 2019

Choose a reason for hiding this comment

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

Done.

@chinmaygarde chinmaygarde merged commit c147edd into flutter:master Oct 17, 2019
@chinmaygarde chinmaygarde deleted the document_dartsnapshot branch October 17, 2019 20:20
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 18, 2019
@engine-flutter-autoroll
c147edd Document //flutter/runtime/dart_snapshot.h (flutter/engine#13196
545c8d9 
)
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cbracken cbracken Awaiting requested review from cbracken

1 more reviewer

@jason-simmons jason-simmons jason-simmons approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chinmaygarde @jason-simmons @googlebot