-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Process data uri images synchronously #4105
Merged
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
536c90e
If image source is already in data uri form, process synchronously
jonmmease 0a397c4
Apply indentation
jonmmease 611064d
lint fix
jonmmease 86a3fc0
Restore "data:" string
jonmmease c310c92
Don't create promise or invoke onload for data uri images
jonmmease dd585b2
Add test that a canvas element is not created when image is a data uri
jonmmease 4a1afbd
Restructure test to avoid setTimeout
jonmmease File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
d.source
should always be a string at this stage, so you could use:instead.
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.
👍
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.
d.source
can be large though, isn'tslice(0, 5)
going to be a lot faster thanindexOf
?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.
That's a good point @alexcjohnson. @etpinard, anything you don't like about
slice
here?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.
It's not obvious to me why
slice
would be faster thanindexOf() === 0
. Has anyone tested that out on jsperf before?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.
My thinking was that If the string starts with
'data:'
, thenindexOf()
would terminate right away. But if not,indexOf()
would search the full string and not bail out until it reaches the end, whereas slice never needs to look past the first 5 characters. If the string doesn't start withdata:
, then I guess it should be a URL which hopefully isn't terribly long. So it might not matter either way.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 guess in as far as the two options we care about are a short string starting
http
and a long string starting
data:
,indexOf
should be fine. The case to worry about would be a long string that doesn't startdata:
... but we don't expect that and wouldn't care if it's slow.jsperf shows a win for
indexOf
https://jsperf.com/slice-vs-indexof-acj but I don't trust it, there's no way it could actually doindexOf
in 2 clock cycles, compiler must have turned it into a constant. This isn't happening in a loop so either option is fast enough to be not worth worrying about.