Report error when instantiating CanvasKit network image #22159

harryterkelsen · 2020-10-28T02:25:25Z

Description

If there is an error when performing the HttpRequest for a CanvasKit network image, report it to the
framework for proper handling.

Related Issues

Fixes flutter/flutter#65537

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change.

flutter-dashboard · 2020-10-28T02:25:28Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

yjbanov

Is this testable?

yjbanov · 2020-10-28T16:43:01Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

    } else {
-      final SkData skData = _skAnimatedImage.encodeToData(); //defaults to PNG 100%
+      final SkData skData =
+          _skAnimatedImage.encodeToData(); //defaults to PNG 100%


nit: add space after //

yjbanov · 2020-10-28T16:48:17Z

lib/web_ui/lib/src/engine/canvaskit/image.dart

  ).then((html.HttpRequest response) {
+    if (response.status != 200) {
+      completer.completeError(
+          'Network image request failed with status: ${response.status}');


Given the amount of confusion this is causing, I think it's worth elaborating on what might be going on here. Alternatively, we could have a wiki page that explains CORS and link to it from here.

CORS exceptions are caught in the onError handler below. This catches "successful" requests which aren't status 200. For CORS exceptions an error is dumped to the console explaining that the image to be downloaded didn't have CORS headers set.

yjbanov

LGTM if LGT CI

…ter/engine#22159)

* c597333 Roll Skia from f548a028ce70 to c21902c0d3cc (46 revisions) (flutter/engine#22224) * 37d766c Fix includes to start with shell (flutter/engine#22227) * 1ad6765 [web] Fixes canvas pixelation and overallocation due to transforms. (flutter/engine#22160) * 9f9fc1f Roll Skia from c21902c0d3cc to 9615bcf71f2a (1 revision) (flutter/engine#22226) * 0faa72e Roll Dart SDK from fed66f60a3bc to 25ef5dc559cf (1 revision) (flutter/engine#22225) * 172a393 Report image diff status for iOS scenario golden tests (flutter/engine#22230) * fddabca updating integration tests version. (flutter/engine#22235) * a36bcdc Roll Fuchsia Mac SDK from mhak7e_o6... to 8SkbMXJJ9... (flutter/engine#22231) * 6a331d3 Roll Skia from 9615bcf71f2a to d5e6368fffd0 (8 revisions) (flutter/engine#22234) * 9b34207 Fixing semantics borders on mobile web (flutter/engine#21856) * 4e9459e Refactored the FlutterEngine to make it easier to implement spawn functionality (flutter/engine#21890) * fa77e68 disable AppLifecycleTests (flutter/engine#22236) * 153775b update golden (flutter/engine#22247) * 50dbe85 [web] fix hot restart type error (flutter/engine#22248) * c8cf09a Roll Skia from d5e6368fffd0 to 7585a65ac709 (7 revisions) (flutter/engine#22237) * 68e2e46 Roll Fuchsia Mac SDK from 8SkbMXJJ9... to Pz4ZHZrUp... (flutter/engine#22246) * d3182bc Roll Dart SDK from 25ef5dc559cf to 5acb5fcf84cb (4 revisions) (flutter/engine#22243) * 9b4bb20 makes android semanticsnode to ignore hittest if it is not focusable (flutter/engine#22205) * 3c7a54e Roll Fuchsia Linux SDK from sNx8qabBn... to QqGvMWaYk... (flutter/engine#22244) * eea98b2 Roll Skia from 7585a65ac709 to dffd20efe95c (14 revisions) (flutter/engine#22250) * 46e3bba Defer Windows arrow key and delete handling (flutter/engine#22207) * 14437d6 fix _getArrayBuffer signature (flutter/engine#22251) * 8defec6 Fix nullability issue with Image.network (flutter/engine#22252) * 67d55ed Roll Dart SDK from 5acb5fcf84cb to a9d583383410 (4 revisions) (flutter/engine#22255) * 7c8f57c Report error when instantiating CanvasKit network image (flutter/engine#22159) * 9945db3 Remove the metrics task from cirrus. (flutter/engine#22240) * bd19181 Add braces on if statements to match linter style (flutter/engine#22130) * e9c62e7 do not print in _computePixelDensity (flutter/engine#22257) * 2617101 Roll Dart SDK from a9d583383410 to d2577410a501 (1 revision) (flutter/engine#22258) * 3d194fa Switch macOS embedding to proc table embedder API (flutter/engine#21811) * 31b6f0b Roll Fuchsia Mac SDK from Pz4ZHZrUp... to 6yEx5GNGG... (flutter/engine#22262) * 78a0181 Roll Fuchsia Linux SDK from QqGvMWaYk... to oLF1FW-gC... (flutter/engine#22264) * ccdb681 WeakPersistentHandle migration (flutter/engine#19843) * ce0a30c Roll Dart SDK from 52783837369d to b43baaaa477d (723 revisions) (flutter/engine#22265) * 59b01e0 [web] Fix repaint logic for cullrect,transform changes (flutter/engine#22273)

Report error when instantiating CanvasKit network image

4c115b5

harryterkelsen requested a review from yjbanov October 28, 2020 02:25

google-cla bot added the cla: yes label Oct 28, 2020

yjbanov reviewed Oct 28, 2020

View reviewed changes

Add test

a2421b0

yjbanov approved these changes Oct 29, 2020

View reviewed changes

chinmaygarde added the platform-web Code specifically for the web engine label Oct 29, 2020

harryterkelsen requested a review from yjbanov October 29, 2020 23:28

Merge branch 'master' into canvaskit-network-image-error

e35221f

yjbanov approved these changes Nov 2, 2020

View reviewed changes

harryterkelsen added 2 commits November 2, 2020 13:06

Merge branch 'master' into canvaskit-network-image-error

2c1c887

Fix merge issue

9843bdc

harryterkelsen merged commit 7c8f57c into flutter:master Nov 2, 2020

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 7c8f57c07d24 (25 revisions) flutter/flutter#69623

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

9bdb567

…ter/engine#22159)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 26171019992a (29 revisions) flutter/flutter#69637

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

409c38b

…ter/engine#22159)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 31b6f0b31a48 (31 revisions) flutter/flutter#69648

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

cd03d17

…ter/engine#22159)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 78a018174088 (32 revisions) flutter/flutter#69657

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

f3bea7d

…ter/engine#22159)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69672

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

4bb07ad

…ter/engine#22159)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69692

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

c74ff79

…ter/engine#22159)

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 59b01e0e5a3f (35 revisions) flutter/flutter#69709

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

7c8f57c Report error when instantiating CanvasKit network image (flut…

e44434b

…ter/engine#22159)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Report error when instantiating CanvasKit network image (flutter#22159)

d359991

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Report error when instantiating CanvasKit network image #22159

Report error when instantiating CanvasKit network image #22159

Uh oh!

harryterkelsen commented Oct 28, 2020

Uh oh!

flutter-dashboard bot commented Oct 28, 2020

Uh oh!

yjbanov left a comment

Uh oh!

yjbanov Oct 28, 2020

Uh oh!

harryterkelsen Oct 29, 2020

Uh oh!

yjbanov Oct 28, 2020

Uh oh!

harryterkelsen Oct 29, 2020

Uh oh!

yjbanov left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Report error when instantiating CanvasKit network image #22159

Report error when instantiating CanvasKit network image #22159

Uh oh!

Conversation

harryterkelsen commented Oct 28, 2020

Description

Related Issues

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Oct 28, 2020

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

yjbanov Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

harryterkelsen Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

yjbanov Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

harryterkelsen Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yjbanov left a comment •

edited

Loading