Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(274)

Issue 1212943007: Fix spinning_mixed.dart, and resulting yak shave. (Closed)

Created:
5 years, 5 months ago by Hixie
Modified:
5 years, 5 months ago
Reviewers:
abarth-chromium
CC:
abarth-chromium, gregsimon, jackson_old, mojo-reviews_chromium.org, qsr+mojo_chromium.org
Base URL:
https://github.com/domokit/mojo.git@master
Target Ref:
refs/heads/master
Project:
mojo
Visibility:
Public.

Description

Fix spinning_mixed.dart, and resulting yak shave. The initial fix was updating Image to NetworkImage, which is the new name for the Widget that takes a URL to an image. However, this exposed an underlying bug: slots given to MultiChildRenderObjectWrapper children were not getting updated in the initial build. Initially, we don't have a next sibling for any of the children's roots, since we haven't yet synced anyone after the one we're syncing. So we pass null. However, as soon as we build the next one, the previous one now has an out of date slot: null means "append", but it needs to be "insert before the newly inserted root of the next child". So now we update the slot after each insertion. This returned the spinning_mixed demo to its original state, but that state was buggy: the image would expand to fit the button and push the text out of the button. To resolve that, this patch changes how RenderImage sizes an image that has no desired dimensions but has some constraints that make it impossible to show the image at full size: now, we try to maintain the aspect ratio and honour the constraints all at the same time. Also, if the constraints are tight we skip all that logic, because it'd be a waste of time. To help with the slot issue above, I needed to see the widget tree and the render tree and compare them (to see where the nodes were getting out of order), so I also extended Widget.toString() to dump a deep tree of the widget hierarchy. Also, while debugging all this I noticed we sometimes walk the tree into nodes that are null, which causes crashes. So to avoid that I added null-checks to certain walkChildren() functions. R=abarth@chromium.org Committed: https://chromium.googlesource.com/external/mojo/+/984df92e923fa77361e774532f84bbfaa6d7fa70

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+78 lines, -24 lines) Patch
M sky/sdk/example/widgets/spinning_mixed.dart View 3 chunks +3 lines, -3 lines 0 comments Download
M sky/sdk/lib/rendering/box.dart View 1 chunk +26 lines, -12 lines 2 comments Download
M sky/sdk/lib/widgets/widget.dart View 10 chunks +49 lines, -9 lines 0 comments Download

Messages

Total messages: 3 (1 generated)
abarth-chromium
lgtm https://codereview.chromium.org/1212943007/diff/1/sky/sdk/lib/rendering/box.dart File sky/sdk/lib/rendering/box.dart (right): https://codereview.chromium.org/1212943007/diff/1/sky/sdk/lib/rendering/box.dart#newcode1060 sky/sdk/lib/rendering/box.dart:1060: double maxHeight = math.min(math.max(constraints.minHeight, constraints.maxHeight), _image.height.toDouble()); Is this ...
5 years, 5 months ago (2015-07-07 00:11:15 UTC) #2
Hixie
5 years, 5 months ago (2015-07-07 00:31:26 UTC) #3
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as
984df92e923fa77361e774532f84bbfaa6d7fa70 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698