Fix #99: Update the attachment references to lead to HTML51 local mode #143

wolenetz · 2016-08-09T23:27:15Z

Note that the old "stalled" and "progress" event logic is allowed (but not required)
in a non-normative note (even though the logic which does such events is in the
now-skipped remote mode section) since that particular behavior change
would have been substantive, IMHO. We already have a VNext MSE spec
bug #88 which will eventually clarify behavior around 'stalled' and
'progress' events.

edit: included "progress" in this msg to inform eventual squash commit msg, since 3c44929 now also mentions 'progress' events.

Note that the old "stalled" event logic is allowed in a non-normative note (even though the logic which does such stalled events is in the now-skipped remote mode section) since that particular behavior change would have been substantive, IMHO. We already have a VNext MSE spec bug #88 which will eventually clarify behavior around 'stalled' and 'progress' events.

…ibute

...including those in EndOfStream. Also, made the wording around replacing the first step of the resource fetch algorithm more clear IMHO.

wolenetz · 2016-08-09T23:54:31Z

@jdsmith3000 @mwatson2 @tidoust: I believe you are all out of the office this week. This PR follows the route outlined by each of @tidoust, @mwatson2, and @foolip in comments in #99.

I would like to merge this ASAP to meet MSE timeline. Given that I believe this follows the editors' agreed-upon implementation route, I would like at least one other person to look this over before I merge it. @foolip or @paulbrucecotton, would you please take a look and see if this raises any concerns? I kept the substantive changes out of this by noting the same in non-normative notes (specifically, 'stalled' MAY still occur; and srcObject MAY not reflect the src-attribute (or child src-attribute) attached MediaSource object).
Barring any comments otherwise, I'll plan to merge this Wednesday Aug 10 before noon PDT.

edit: Useful link for reviewers: http://services.w3.org/htmldiff?doc1=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2Fba1a409a311ad642dd787adcb1535eda3c95e268%2Findex.html&doc2=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2F55b92b1aee07f6490295bad8f6097b62babc6d3c%2Findex.html

(The changes are within the "Attaching to a media element" and "End of stream algorithm" sections.)

wolenetz · 2016-08-10T00:12:55Z

Updated link for reviewers (including 7fa73b0): http://services.w3.org/htmldiff?doc1=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2Fba1a409a311ad642dd787adcb1535eda3c95e268%2Findex.html&doc2=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2F7fa73b0b802523aab078c2f92bdbe1b6db81755c%2Findex.html

paulbrucecotton · 2016-08-10T00:55:18Z

I would like to merge this ASAP to meet MSE timeline.

@wolenetz - As much as I appreciate your attempts to close out the remaining MSE GitHub issues ASAP, maybe a better plan would be for you to leave this PR open for a longer review and for you to work on the outstanding comments and problems with the MSE test suite on WPT? The test suite and test results are as much the "long pole" to getting to PR as the GitHub issues are.

/paulc

foolip · 2016-08-10T07:58:21Z

index.html

@@ -1782,7 +1786,7 @@ <h4 id="h-mediasource-detach" resource="#h-mediasource-detach"><span property="x
            <a href="https://www.w3.org/TR/html51/webappapis.html#queuing">Queue a task</a> to <a href="https://www.w3.org/TR/html51/infrastructure.html#fire">fire a simple event</a> named <code><a href="#dom-evt-sourceclose">sourceclose</a></code> at the <a href="#idl-def-mediasource" class="internalDFN" data-link-type="dfn"><code>MediaSource</code></a>.</li>
        </ol>
        <div class="note">
-          <div class="note-title marker" aria-level="5" role="heading" id="h-note20"><span>Note</span></div>
+          <div class="note-title marker" aria-level="5" role="heading" id="h-note21"><span>Note</span></div>


I see no internal links to #h-note*, so I guess these are linked to from somewhere else? Aren't they meant to be stable? If nobody is actually expected to link to these, I'd just drop the ids to make diffs like this smaller :)

Oh, index.html is generated, media-source-respec.html is the input. Too bad they show up in the other order in diffs :)

Yes, the regeneration upon note insertion/removal makes for a bunch of lines in the index.html diff. I suppose the link tag is generated in case someone might wish to make a link into a document revision to a particular note, though those numbers can obviously change significantly across revisions.

wolenetz · 2016-08-10T17:47:45Z

9310bf8 addresses all comments so far.
Updated htmldiff link to assist review: http://services.w3.org/htmldiff?doc1=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2Fba1a409a311ad642dd787adcb1535eda3c95e268%2Findex.html&doc2=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2F9310bf8b095dbe654f313de4a29ee1859d26f9a7%2Findex.html

foolip · 2016-08-10T20:07:43Z

media-source-respec.html


+          <p>If the <a def-id="resource-fetch-algorithm"></a> was invoked with a media provider object that is a MediaSource object or a URL record whose object is a MediaSource object, then let mode be local, skip the first step in the <a def-id="resource-fetch-algorithm"></a> which may otherwise set mode to remote, and run the following steps at the beginning of the <a def-id="Otherwise-mode-is-local"></a> section of the <a def-id="resource-fetch-algorithm"></a>.
+          <p class="note">The <a def-id="resource-fetch-algorithm"></a>[[HTML51]]'s first step is expected to eventually align with selecting local mode for URL records whose objects are media provider objects. The intent is that if the HTMLMediaElement's <code>src</code> attribute or selected child <code>&lt;source&gt;</code>'s <code>src</code> attribute is an absolute URL matching a <a def-id="MediaSource-object-URL"></a> when the respective <code>src</code> attribute was last changed, then that MediaSource object is used as the media provider object and current media resource in the local mode logic in the <a def-id="resource-fetch-algorithm"></a>. This also means that the remote mode logic that includes observance of any preload attribute is skipped when a MediaSource object is attached. Even with that eventual change to [[HTML51]], the execution of the following steps at the beginning of the local mode logic is still required when the current media resource is a MediaSource object.</p>


Two issues on the same source line.

A bit odd with the spec ref is between "resource fetch algorithm" and the trailing 's, maybe skip the ref entirely or put if after?

Maybe "blob: URL" instead of "absolute URL"?

sgtm. I'll fix both. I used "absolute URL" since that's what the current w3c html 5.1 terminology is, but blob: is even more specific, and this is already in a non-normative note describing the real intent.

foolip · 2016-08-10T20:19:29Z

OK, I've finished reviewing the whole change, on the assumption that it'll be squashed into a single commit so I didn't pay attention to the later commit messages. LGTM, I think it does the job, and since the difference between this sometimes taking the remote branch is observable, it's important to get all implementations aligned.

(And again, I'm sorry that integrating with HTML is such a pain for you, but I'll do what I can to make it less terrible.)

Addresses: #143 (diff) #143 (diff) #143 (diff)

wolenetz · 2016-08-10T20:55:16Z

3c44929 addressess all comments so far. Updated htmldiff link to assist review is: http://services.w3.org/htmldiff?doc1=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2Fba1a409a311ad642dd787adcb1535eda3c95e268%2Findex.html&doc2=http%3A%2F%2Frawgit.com%2Fw3c%2Fmedia-source%2F3c449292ab57363913384b4a00593c47f64e4cdf%2Findex.html

foolip · 2016-08-10T21:43:51Z

I agree that having srcObject automatically reflect the object inside a blob: URL assigned to the src attribute would be neat and at least occasionally useful. Simply extracting and exposing the object in srcObject at the beginning of the resource selection algorithm is probably low risk, better than the src attribute idea I had. The weird bit might be what the currentSrc attribute should then return. These details are probably not insurmountable, if you think something like that's a good idea and would implement if spec'd, feel free to file an HTML spec issue or send a PR of how you'd like it work.

(Top-level since it'd otherwise be in collapsed comments.)

wolenetz · 2016-08-11T21:30:25Z

Note, this and PR #146 modify similar region of the spec, so merge conflict resolution will be necessary when they're landed.

foolip · 2016-08-12T12:16:43Z

This PR LGTM, can it be merged so reviewing the other one is cleaner?

jdsmith3000 · 2016-08-15T21:42:48Z

media-source-respec.html

          <dl class="switch">
            <dt>If <a def-id="readyState"></a> is NOT set to <a def-id="closed"></a></dt>
-            <dd>Run the <a def-id="media-data-cannot-be-fetched"></a> steps of the <a def-id="resource-fetch-algorithm"></a>.</dd>
+            <dd>Run the <a def-id="media-data-cannot-be-fetched"></a> steps of the <a def-id="resource-fetch-algorithm"></a>'s <a def-id="media-data-processing-steps-list"></a>.</dd>


This addition LGTM.

jdsmith3000 · 2016-08-15T21:47:41Z

I made a minor comment about how the added/clarified steps are spliced in. Otherwise, the change PR LGTM as well.

wolenetz added 3 commits August 9, 2016 16:23

Note that srcObject MAY not reflect MediaSource attached via src attr…

442198a

…ibute

Update references to include media-data-processing-steps-list linkages

55b92b1

...including those in EndOfStream. Also, made the wording around replacing the first step of the resource fetch algorithm more clear IMHO.

Move two of the new notes out of the "Otherwise" steps

7fa73b0

foolip reviewed Aug 10, 2016
View reviewed changes

Clarify the replacement first step of resource fetch algorithm

9310bf8

wolenetz mentioned this pull request Aug 10, 2016

The fetch step in HTML5.1's resource fetch algorithm no longer exists #99

Closed

foolip reviewed Aug 10, 2016
View reviewed changes

Address multiple review comments

3c44929

Addresses: #143 (diff) #143 (diff) #143 (diff)

wolenetz mentioned this pull request Aug 10, 2016

Allow retrieval, via HTMLMediaElement.srcObject, of media provider object (even if not assigned media provider object) w3c/html#555

Closed

wolenetz mentioned this pull request Aug 11, 2016

Fix #104: Clarify that attachment is asynchronous #146

Merged

jdsmith3000 reviewed Aug 15, 2016
View reviewed changes

wolenetz added 2 commits August 16, 2016 13:03

Address jdsmith3000@'s PR review comment

53d00c5

Merge branch 'gh-pages' into fix_99

4d6c775

wolenetz merged commit 61c687f into w3c:gh-pages Aug 16, 2016

wolenetz deleted the fix_99 branch August 16, 2016 20:08

plehegar mentioned this pull request Aug 18, 2016

Treat blob: URLs as local in the resource selection algorithm (media elements) w3c/html#550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #99: Update the attachment references to lead to HTML51 local mode #143

Fix #99: Update the attachment references to lead to HTML51 local mode #143

wolenetz commented Aug 9, 2016 •

edited

Loading

wolenetz commented Aug 9, 2016 •

edited

Loading

wolenetz commented Aug 10, 2016 •

edited

Loading

paulbrucecotton commented Aug 10, 2016

foolip Aug 10, 2016

foolip Aug 10, 2016

wolenetz Aug 10, 2016

wolenetz commented Aug 10, 2016

foolip Aug 10, 2016

wolenetz Aug 10, 2016

foolip commented Aug 10, 2016

wolenetz commented Aug 10, 2016

foolip commented Aug 10, 2016

wolenetz commented Aug 11, 2016

foolip commented Aug 12, 2016

jdsmith3000 Aug 15, 2016

jdsmith3000 commented Aug 15, 2016


		<p>If the <a def-id="resource-fetch-algorithm"></a> was invoked with a media provider object that is a MediaSource object or a URL record whose object is a MediaSource object, then let mode be local, skip the first step in the <a def-id="resource-fetch-algorithm"></a> which may otherwise set mode to remote, and run the following steps at the beginning of the <a def-id="Otherwise-mode-is-local"></a> section of the <a def-id="resource-fetch-algorithm"></a>.
		<p class="note">The <a def-id="resource-fetch-algorithm"></a>[[HTML51]]'s first step is expected to eventually align with selecting local mode for URL records whose objects are media provider objects. The intent is that if the HTMLMediaElement's <code>src</code> attribute or selected child <code><source></code>'s <code>src</code> attribute is an absolute URL matching a <a def-id="MediaSource-object-URL"></a> when the respective <code>src</code> attribute was last changed, then that MediaSource object is used as the media provider object and current media resource in the local mode logic in the <a def-id="resource-fetch-algorithm"></a>. This also means that the remote mode logic that includes observance of any preload attribute is skipped when a MediaSource object is attached. Even with that eventual change to [[HTML51]], the execution of the following steps at the beginning of the local mode logic is still required when the current media resource is a MediaSource object.</p>

Fix #99: Update the attachment references to lead to HTML51 local mode #143

Fix #99: Update the attachment references to lead to HTML51 local mode #143

Conversation

wolenetz commented Aug 9, 2016 • edited Loading

wolenetz commented Aug 9, 2016 • edited Loading

wolenetz commented Aug 10, 2016 • edited Loading

paulbrucecotton commented Aug 10, 2016

foolip Aug 10, 2016

Choose a reason for hiding this comment

foolip Aug 10, 2016

Choose a reason for hiding this comment

wolenetz Aug 10, 2016

Choose a reason for hiding this comment

wolenetz commented Aug 10, 2016

foolip Aug 10, 2016

Choose a reason for hiding this comment

wolenetz Aug 10, 2016

Choose a reason for hiding this comment

foolip commented Aug 10, 2016

wolenetz commented Aug 10, 2016

foolip commented Aug 10, 2016

wolenetz commented Aug 11, 2016

foolip commented Aug 12, 2016

jdsmith3000 Aug 15, 2016

Choose a reason for hiding this comment

jdsmith3000 commented Aug 15, 2016

wolenetz commented Aug 9, 2016 •

edited

Loading

wolenetz commented Aug 9, 2016 •

edited

Loading

wolenetz commented Aug 10, 2016 •

edited

Loading