Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use createImageBitmap when available #7579
Use createImageBitmap when available #7579
Changes from 18 commits
96cbc18
b772d52
dca9ed1
824ceb1
e2108db
7336d96
e31eae3
3fe7b79
5cdd9e1
112c345
9e7fea4
bf46f89
7f5b46a
239d596
0964441
0b878e7
d2cc173
039975b
f8333b3
a6324e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a return statement and then you should move all of the following
then
calls up a level.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why? I'm assuming this is what you mean:
This will run the
.then(function(blob)
anyway even if weloadImageElement
and don't fetch the blob, and thenblob
will beundefined
, which we don't want, right?I can just move the
.then(deferred.resolve).otherwise(deferred.reject);
outside and I think that should work fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see
Resource.prototype.fetchImage
checks if the result is defined at each step in the promise chain to make sure it doesn't run when it terminates early like that. I can follow that pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if for some reason
supportsImageBitmapOptions
rejected, you would never calldeferred.reject
(ordeferred.resolve
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by this, please stop by if you still have questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I'm pretty sure I've got it locked down now. What I meant was, Resource has a similar promise chain when fetching an image. When we return undefined, all the subsequent then's are still going to run. So you need to add a check at each subsequent then() that the result is defined.
Here is where Resource is doing this:
https://github.com/AnalyticalGraphicsInc/cesium/blob/8bc364ce5eca3c42d84fe21386b0d5a50be82c2d/Source/Core/Resource.js#L886-L889
I just followed a similar pattern in
createImage
:https://github.com/AnalyticalGraphicsInc/cesium/blob/a6324e2bcf8b3547b4adb589e7ee789321997678/Source/Core/Resource.js#L1855-L1882
Notice that
deferred.resolve
is only called in the ImageBitmap codepath. SinceloadImageElement
calls it on theImage
ononload
.Will bump when CI passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole file could probably use some clean-up. Some of the (old) if checks make reading the code pretty confusing. No action on your part, and this is nothing you did; just mentioning it because I may open a small clean-up PR to take a pass (after merging this).