Make a more explicit call to fetch, including options such as destination and mode #359
Conversation
index.bs
Outdated
| <dfn>preferred artwork image</dfn> from <var>metadata</var>'s | ||
| {{MediaMetadata/artwork}} of the <a>active media session</a>. | ||
| </li> | ||
| <li>Let <var>request</var> be a new [=request=] whose URL is the |
There was a problem hiding this comment.
You should probably use [=request/URL=], [=request/mode=] and such here.
index.bs
Outdated
| {{MediaMetadata/artwork}} of the <a>active media session</a>. | ||
| </li> | ||
| <li>Let <var>request</var> be a new [=request=] whose URL is the | ||
| <a>preferred artwork image</a>'s {{MediaImage/src}}, destination |
There was a problem hiding this comment.
This state seems to come out of nowhere? Also, it looks like MediaImage/src is a string that needs to be parsed into a URL first? What happens if that fails?
There was a problem hiding this comment.
src is being parsed when setting the metadata. It is true it is a bit confusing since {{MediaImage/src}} here contains a properly parsed URL and not a USVString. Maybe there is a better way of doing things.
youennf
force-pushed
the
clarify-fetch-algorithm
branch
from
4def6b7 to
9dc2811
Compare
annevk
left a comment
annevk
left a comment
There was a problem hiding this comment.
I guess reworking that has to be a follow-up, but you can't just assign the result of the URL parser to a dictionary member that contains a string. It would be better if you had some specification-internal struct (https://infra.spec.whatwg.org/#structs) to hold the data after processing.
Another thing that seems broken is the amount of ambient state. The fetch image algorithm has no parameters, but somehow has access to all kinds of state.
But the change in question here seems reasonable amid the questionable things.
|
@steimelchrome, any thoughts?
Yep agreed.
Do you mean that this algorithm should take a MediaSession object as input for instance? |
|
Yeah probably, it needs something to start its operations from and communicate back to. |
steimelchrome
left a comment
steimelchrome
left a comment
There was a problem hiding this comment.
Sorry for the delay was OOO for a bit. Yep, this looks reasonable to me. Thanks!
3 participants
Fixes #346
Preview | Diff