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

Conversation

sjindel-google
Copy link
Contributor

The bug in the original CL was that we were not running replace_as_executable in OpenVmo if the namespace was not provided.

This was a divergence in behavior for MappedResource::LoadFromNamespace compared to the current implementation.

Copy link
Contributor

@chingjun chingjun left a comment

Choose a reason for hiding this comment

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

LGTM. I've ran a few tests internally with these changes as well, the tests I ran work fine.

dart_utils::Check(path[0] != '/', LOG_TAG);

if (namespc == nullptr) {
if (!VmoFromFilename(path, resource_vmo)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should this be dart_utils::VmoFromFilename for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're in the dart_utils namespace here, so I think the Google style guide allows (encourages?) using the unqualified name. At least, that's the convention we follow in Dart.

@sjindel-google sjindel-google merged commit 343d9f2 into master Jan 10, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 11, 2020
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jan 11, 2020
flutter/engine@7ef88f8...3d37d39

git log 7ef88f8..3d37d39 --first-parent --oneline
2020-01-11 [email protected] Roll src/third_party/skia 67d0f3fd725c..3723fb7e85bb (25 commits) (flutter/engine#15466)
2020-01-10 [email protected] Re-land "Use ELF for Dart AOT snapshots on Fuchsia. #13896" (flutter/engine#15360)


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 [email protected] 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
@chinmaygarde chinmaygarde deleted the sjindel.fuch_reland branch January 17, 2020 01:35
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
…lutter#15360)

The bug in the original CL was that we were not running replace_as_executable in OpenVmo if the namespace was not provided.

This was a divergence in behavior for MappedResource::LoadFromNamespace compared to the current implementation.
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020
…lutter#15360)

The bug in the original CL was that we were not running replace_as_executable in OpenVmo if the namespace was not provided.

This was a divergence in behavior for MappedResource::LoadFromNamespace compared to the current implementation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants