Specify a human readable reason for an error from the embedder API. #13218

chinmaygarde · 2019-10-17T23:01:13Z

cbracken · 2019-10-17T23:04:21Z

shell/platform/embedder/embedder.cc

-  FML_LOG(ERROR) << "Returning error '" << name << "' (" << code
-                 << ") from Flutter Embedder API call to '" << function
-                 << "'. Origin: " << file << ":" << line;
+  const auto file_base = (::strrchr(file, '/') ? strrchr(file, '/') + 1 : file);


Are the paths handed to us in file normalised? i.e. will this be an issue on Windows?

No. Yes. See https://godbolt.org/z/F6SO6q. Fixed.

cbracken · 2019-10-17T23:06:01Z

shell/platform/embedder/embedder.cc

-                 << "'. Origin: " << file << ":" << line;
+  const auto file_base = (::strrchr(file, '/') ? strrchr(file, '/') + 1 : file);
+  char error[256] = {};
+  snprintf(error, (sizeof(error) / sizeof(char)) - 1,


sizeof(char) is 1 by definition (unless someone's sneakily updated the spec). In theory there's a small advantage to leaving it if we ever changed to wchar or something, it's a nice reminder I suppose.

cbracken · 2019-10-17T23:09:45Z

shell/platform/embedder/embedder.cc

-    return LOG_EMBEDDER_ERROR(kInvalidLibraryVersion);
+    return LOG_EMBEDDER_ERROR(
+        kInvalidLibraryVersion,
+        "Flutter engine version the embedder was built for did not match the "


nit: Since we're using the a few words in I'd use "The Flutter engine version..." to make this read a bit better. I had to backtrack and brain-reset once when I read this the first time :)

I suppose you could do something like "Flutter embedder version mismatch. The engine version ..."?

cbracken · 2019-10-17T23:11:55Z

shell/platform/embedder/embedder.cc

-    FML_LOG(WARNING) << "Invalid renderer config.";
-    return LOG_EMBEDDER_ERROR(kInvalidArguments);
+    return LOG_EMBEDDER_ERROR(kInvalidArguments,
+                              "The renderer configuration was not valid.");


Desperate grasp at avoiding a line-wrap since github doesn't show an 80-col marker: would s/not valid/invalid/ get you to 80 cols by any chance?

Nope. But I updated it anyway.

cbracken

Nice!

jason-simmons · 2019-10-17T23:10:08Z

shell/platform/embedder/embedder.cc

-                 << "'. Origin: " << file << ":" << line;
+  const auto file_base = (::strrchr(file, '/') ? strrchr(file, '/') + 1 : file);
+  char error[256] = {};
+  snprintf(error, (sizeof(error) / sizeof(char)) - 1,


Use sizeof(error) here

I wanted to keep space for the null terminator.

Nevermind. The man page says this is unnecessary.

jason-simmons · 2019-10-17T23:16:34Z

shell/platform/embedder/embedder.cc

-  FML_LOG(ERROR) << "Returning error '" << name << "' (" << code
-                 << ") from Flutter Embedder API call to '" << function
-                 << "'. Origin: " << file << ":" << line;
+  const auto file_base = (::strrchr(file, '/') ? strrchr(file, '/') + 1 : file);


Consider adding a function to fml (similar to fml::paths::GetDirectoryName) that can handle each platform's path separators

I went for a smaller fix. If the same routine is required in any other spots, I will move to fml.

…r API. (flutter/engine#13218)

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

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

Specify the a human readable reason for an error from the embedder API.

a1d1701

Fixes flutter/flutter#42480

chinmaygarde requested a review from jason-simmons October 17, 2019 23:01

googlebot added the cla: yes label Oct 17, 2019

chinmaygarde requested a review from cbracken October 17, 2019 23:01

cbracken reviewed Oct 17, 2019

View reviewed changes

chinmaygarde changed the title ~~Specify the a human readable reason for an error from the embedder API.~~ Specify a human readable reason for an error from the embedder API. Oct 17, 2019

cbracken reviewed Oct 17, 2019

View reviewed changes

cbracken approved these changes Oct 17, 2019

View reviewed changes

jason-simmons reviewed Oct 17, 2019

View reviewed changes

chinmaygarde added 2 commits October 17, 2019 16:31

Address PR comments.

fa644cf

Address PR comment.

d0d2adc

chinmaygarde merged commit c57ffbf into flutter:master Oct 18, 2019

chinmaygarde deleted the better_error_messages branch October 18, 2019 00:29

engine-flutter-autoroll mentioned this pull request Oct 18, 2019

Roll engine 508146f0defb..7a621a7f7e4f (26 commits) flutter/flutter#43017

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 18, 2019

c57ffbf Specify a human readable reason for an error from the embedde…

d92e935

…r API. (flutter/engine#13218)

Specify a human readable reason for an error from the embedder API. #13218

Specify a human readable reason for an error from the embedder API. #13218

Uh oh!

Conversation

chinmaygarde commented Oct 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbracken Oct 17, 2019 •

edited

Loading