Closed Bug 769117 Opened 12 years ago Closed 8 years ago

Rewrite YouTube object embed with iframe so we can play HTML video instead of Flash

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- disabled
firefox47 --- fixed
relnote-firefox --- 47+

People

(Reporter: hub, Assigned: qdot)

References

Details

(Keywords: feature, flashplayer, html5)

Attachments

(2 files, 14 obsolete files)

Patch 1 (v13) - Rewrite youtube flash embeds to possibly use HTML5; r=hsivonen, r=bz
8.40 KB, patch
Details | Diff | Splinter Review
Patch 2 (v3) - Mochitests for youtube flash to html5 rewriting; r=bz r=hsivonen
3.42 KB, patch
Details | Diff | Splinter Review 
If there is no Flash plugin, replace embed by iframe for youtube, automatically. This will let Youtube use HTML5/WebM if available. This will make old style embed available without Flash.
Do we want to do site-specific behavior in Firefox? While it could be very interesting to have more users for of the media code, I don't think it is appropriate to address the flash vs. HTML5 issue this way. On the other hand, if we are waiting for Youtube people (and of course other video sites) to make the switch (that is, HTML5 video, then flash fallback).

Also, I don't think that Youtube has a solution to display video in HTML5 with ads at the beginning like they do for the flash video. Even if an extension can disable that (e.g. adblocks), we should not change an external site behavior _by default_ in a way that affect its revenue model.
Actually, iframe is just a different way to embed the content. In the case of youtube it will still use Flash if need / present. It is the smarter way to embed.

One of the reason I'm suggesting this is because a lot of third party "youtube embed" plugins for various CMS still use the old construct instead of the iframe.

When you click Share -> Embed, youtube gives you the iframe construct by default.
Safari on iOS employ a similar trick, albeit I'm not sure they even modify the DOM. They use it to display embedded Youtube videos in their own player.
I actually implemented this as an addon:

https://github.com/hfiguiere/no-flash/
Component: Audio/Video → Audio/Video: Playback
Keywords: flashplayer, html5
OS: Linux → All
Hardware: x86_64 → All
Version: 15 Branch → unspecified
Chris - who do we need to convince to make this into a thing?
Flags: needinfo?(cpeterson)
And just a quick note: we could extend this to vimeo (rare) and Dailymotion. I have support for both in no-flash if you are curious.
Kyle, jst said you will be looking at this bug. :)

Hubert already implemented this functionality in his no-flash addon. You would just need to figure out how we might cleanly integrate it into Firefox. Could we just bundle the no-flash addon in Firefox (like we do with PDF.js and Shumway) instead of reimplementing it in Gecko?
Assignee: nobody → kyle
Flags: needinfo?(cpeterson) → needinfo?(kyle)
Summary: Replace embed by iframe for youtube → Rewrite YouTube object embed with iframe so we can play HTML video instead of Flash
Hmm. We may be able to just throw it in browser/extensions, create a jar.mn file, and it might "just work". I'll do some more research on that.

Do we want a pref or anything to allow it be to turned off after it's integrated?
Flags: needinfo?(kyle) → needinfo?(cpeterson)
Flags: needinfo?(cpeterson)
Priority: -- → P2
A pref sounds like a good idea. Hubert might have an opinion about how much of this logic should be in Gecko vs the extension.
One thing I couldn't do, and maybe it is just me, is to avoid the door hanger about missing Flash plugin. Because we get triggered on the "PluginBindingAttached" event.

Not sure of the best way to solve this but I suspect it is either done from Gecko. A custom even on Flash. Or something like that.
It would be good to add a telemetry probe for YouTube object embeds (as part of this bug or a follow up bug). We can track the decline of object embeds and possibly remove this rewriting extension someday.
Ok, so there's something I overlooked in the first comment to this bug:

> If there is no Flash plugin, replace embed by iframe for youtube,

The "if there is no flash plugin" is the problem here. If we /do/ have a flash plugin, we load that before we hit the add-on code, and lose our access to the embed tag. So we'll need to figure out a way to either do the embed -> iframe before nsObjectLoadingContent starts parsing the tag, or else somehow ignore the embed tag if we see it's related to youtube and let it fall through to the add-ons.

Working on figuring out what the best way to do this is, will let you know.
Ok, having talked to a few people now, the best approach is probably going to be to do this back in the HTML parser. That way we can rewrite before the node object is created, meaning we won't trigger the plugin loader, and we also won't have to deal with DOM mutations. However, I'm not real sure how the parser works, so I'm still doing research there.
Well, :hsivonen is out until Oct 5, so not sure when I'm gonna hear back about parser solutions, so I'm trying quick and dirty for the moment just to see how bad it will be.

Here's a hideous WIP patch that at least does the deed. Just ripped the rewriting code out of hub's addon, threw it in PluginContent.jsm. This does at least give us the result we want, but it is slow and nasty. Because this doesn't trigger until pageLoad, if the user had a flash plugin, the plugin loads and displays, /then/ we rewrite the tag, which means we've got a repaint.

My current strategy is to figure out some sort of policy check in nsObjectLoadingContent where we can check mime type and origin, and just not load any plugin if its an embed, flash, and from youtube.com. This should speed things up considerably, but I still wouldn't call this a perfect solution by any means.
New version of work in progress patch. We can now detect whether we have an embed tag from youtube.com, and if we do, we block plugin loading, which speeds up rewriting. 

Unfortunately the fallback isn't firing quite right, so that's still something I need to fix, as right now we run the replacement algorithm on ALL pages with plugins, which isn't what we want. However, once that's fixed, this could end up being a workable solution just for getting telemetry if nothing else.
Attachment #8663871 - Attachment is obsolete: true
Ok, now have this set behind a pref, so I think it's ready for review. Next question, who's gonna be a good reviewer for this?
Attachment #8663949 - Attachment is obsolete: true
Comment on attachment 8664425 [details] [diff] [review]
Patch 1 (v3) - Rewrite youtube embeds to be iframes

Review of attachment 8664425 [details] [diff] [review]:
-----------------------------------------------------------------

Just some comments since you reused the my code. Nothing blocking.

::: browser/modules/PluginContent.jsm
@@ +1049,5 @@
> +          videoid: matches[1],
> +          processor: this.replaceYT
> +        };
> +      }
> +      return {

Also I am disappointed to see that only YouTube is rewritten. :-/

@@ +1082,5 @@
> +    container.parentNode.insertBefore(replacement, container);
> +    container.parentNode.removeChild(container);
> +  },
> +  updateYT: function () {
> +    var elements = this.content.document.getElementsByTagName("embed");

If it only uses <object> (I have seen it in a few occurences) the rewrite will not happen. No harm but it is a known limitation.
Comment on attachment 8664425 [details] [diff] [review]
Patch 1 (v3) - Rewrite youtube embeds to be iframes

Review of attachment 8664425 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsObjectLoadingContent.cpp
@@ +1499,5 @@
> +  if (!ok) {
> +    NS_WARNING("Could not parse plugin domain!");
> +    return false;
> +  }
> +  nsAutoCString domain("youtube.com");

If you look into the rewrite code, there is more than "youtube.com" as a domain.
(In reply to Hubert Figuiere [:hub] from comment #18)
> Comment on attachment 8664425 [details] [diff] [review]
> Patch 1 (v3) - Rewrite youtube embeds to be iframes
> 
> Review of attachment 8664425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some comments since you reused the my code. Nothing blocking.
> 
> ::: browser/modules/PluginContent.jsm
> @@ +1049,5 @@
> > +          videoid: matches[1],
> > +          processor: this.replaceYT
> > +        };
> > +      }
> > +      return {
> 
> Also I am disappointed to see that only YouTube is rewritten. :-/

Yeah, I pulled out vimeo/dailymotion because the scope of the bug didn't include them. They can certainly be added later.
Comment on attachment 8674469 [details] [diff] [review]
Patch 1 (v4) - Rewrite youtube embed tags to be iframes

I honestly have no idea if you're qualified to review this, but it's worth a shot. If not let me know and I'll find someone else.
Attachment #8674469 - Flags: review?(cpeterson)
Comment on attachment 8674469 [details] [diff] [review]
Patch 1 (v4) - Rewrite youtube embed tags to be iframes

Review of attachment 8674469 [details] [diff] [review]:
-----------------------------------------------------------------

f+ LGMT but I am not a peer for this code.

@ jimm: do you want to review this plugin patch?

::: browser/modules/PluginContent.jsm
@@ +1037,5 @@
> +      };
> +    }
> +    if (element.hasAttribute('src')) {
> +      var url = element.getAttribute('src');
> +      var matches = url.match('^(?:https?:)?\/\/(?:www\.)?youtube\.(?:googleapis\.)?com/v/([A-Za-z0-9_\-]{11})');

Can we point to an example URL from YouTube's Flash embed documentation? Do we know that videoid is always 11 characters long?

@@ +1068,5 @@
> +      Cu.reportError("Cannot extract youtube video id.");
> +      return ;
> +    }
> +
> +    var replacement = doc.createElement("iframe");

We should probably link to YouTube's iframe documentation here.

@@ +1081,5 @@
> +    container.parentNode.insertBefore(replacement, container);
> +    container.parentNode.removeChild(container);
> +  },
> +  updateYT: function (target) {
> +    var elements = target.getElementsByTagName("embed");

I think `embeds` might be more descriptive (and shorter! :) name than `elements`.

@@ +1086,5 @@
> +    // Every time we find a tag to replace, the embed will be replaced by an
> +    // iframe, so the list will shrink. However, if the page has non-youtube
> +    // embeds, we'll still iterate over those every time. I don't think this
> +    // affects performance too much though.
> +    for (var i = 0; i < elements.length; ++i) {

If the list shrinks, do we still want to increment i? Will we inadvertently skip the next embed in the list?
Attachment #8674469 - Flags: review?(jmathies)
Attachment #8674469 - Flags: review?(cpeterson)
Attachment #8674469 - Flags: feedback+
(In reply to Chris Peterson [:cpeterson] from comment #23)
> Comment on attachment 8674469 [details] [diff] [review]
> Patch 1 (v4) - Rewrite youtube embed tags to be iframes
> 
> @@ +1086,5 @@
> > +    // Every time we find a tag to replace, the embed will be replaced by an
> > +    // iframe, so the list will shrink. However, if the page has non-youtube
> > +    // embeds, we'll still iterate over those every time. I don't think this
> > +    // affects performance too much though.
> > +    for (var i = 0; i < elements.length; ++i) {
> 
> If the list shrinks, do we still want to increment i? Will we inadvertently
> skip the next embed in the list?

No, this isn't an iterator object, so we don't have to worry about it shrinking while we're going through it. It'll just hold references to elements that'll be gc'd when the array scopes out.
Adding comments, per cpeterson's review.
Attachment #8674469 - Attachment is obsolete: true
Attachment #8674469 - Flags: review?(jmathies)
Attachment #8675087 - Flags: review?(jmathies)
Comment on attachment 8675087 [details] [diff] [review]
Patch 1 (v5) - Rewrite youtube embed tags to be iframes

Review of attachment 8675087 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsObjectLoadingContent.cpp
@@ +3023,5 @@
> +  // If our fallback is to rewrite youtube links, fire that event now.
> +  if (aType == eFallbackRewrite) {
> +    nsIDocument* doc = thisContent->GetComposedDoc();
> +    if (doc && doc->IsActive()) {
> +      nsCOMPtr<nsIRunnable> ev = new nsSimplePluginEvent(doc,

Can't you dispatch the event on thisContent instead of doc? That way you could pass event.target directly to analyzeObject and skip going through updateYT and iterating over all plugin elements over and over again.
Massively simplified the functions in the javascript module thanks to :tobytailor's suggestions.
Attachment #8675087 - Attachment is obsolete: true
Attachment #8675087 - Flags: review?(jmathies)
Attachment #8677248 - Flags: review?(jmathies)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #14)
> Ok, having talked to a few people now, the best approach is probably going
> to be to do this back in the HTML parser. That way we can rewrite before the
> node object is created, meaning we won't trigger the plugin loader, and we
> also won't have to deal with DOM mutations. However, I'm not real sure how
> the parser works, so I'm still doing research there.

Kyle, why did we veer away from this approach? Sounds like you were missing some feedback on how to go about this from :hsivonen about this but he was out until Oct. 5th.
Oh, inertia more than anything, heh. I think I got a patch working as a proof of concept then just kept running with it. I'm ni'ing :hsivonen now.

Henri, we're looking at rewriting embed tags to iframe tags for youtube links in an effort to cut down on legacy flash content. The current patch for this uses a somewhat hacky replacement during plugin bring-up. I was curious if there was a way to do this in the HTML5 parser?
Flags: needinfo?(hsivonen)
Kyle, there is no diff between v6 and v5 of your patch.
Flags: needinfo?(kyle)
Uploaded patch with changes actually commited this time.
Attachment #8677248 - Attachment is obsolete: true
Attachment #8677248 - Flags: review?(jmathies)
Flags: needinfo?(kyle)
Flags: needinfo?(hsivonen)
Attachment #8677638 - Flags: review?(schneider)
Attachment #8677638 - Flags: review?(jmathies)
And now re-ni'ing hsivonen 'cause I hit the wrong button.
Flags: needinfo?(hsivonen)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #29)
> Henri, we're looking at rewriting embed tags to iframe tags for youtube
> links in an effort to cut down on legacy flash content. The current patch
> for this uses a somewhat hacky replacement during plugin bring-up. I was
> curious if there was a way to do this in the HTML5 parser?

If you really mean <embed> tags only and you don't need to rewrite <object> in a way that would involve looking at its <param> children, you could hack this the same way <keygen> is now hacked into a magic <select>:
https://dxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#342

Of course, you still need something else for sites that inject their Flash embeds via the DOM APIs later.

Speaking of APIs: Since both the patch here and the <keygen>-style hack change the DOM, is that OK? I.e. do sites not try to access the YouTube Flash player via JS APIs?

(FWIW, I think we should rewrite YouTube embeds to HTML5 even if Flash is installed. Definitely if installed but in click-to-play mode.)
Flags: needinfo?(hsivonen)
(In reply to Henri Sivonen (:hsivonen) from comment #33)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> #29)
> > Henri, we're looking at rewriting embed tags to iframe tags for youtube
> > links in an effort to cut down on legacy flash content. The current patch
> > for this uses a somewhat hacky replacement during plugin bring-up. I was
> > curious if there was a way to do this in the HTML5 parser?
> 
> If you really mean <embed> tags only and you don't need to rewrite <object>
> in a way that would involve looking at its <param> children, you could hack
> this the same way <keygen> is now hacked into a magic <select>:
> https://dxr.mozilla.org/mozilla-central/source/parser/html/
> nsHtml5TreeOperation.cpp#342
> 
> Of course, you still need something else for sites that inject their Flash
> embeds via the DOM APIs later.

I think we're mostly worried about static embed tags at the moment. We're seeing 1% session activations on these alone, so it'll be a good start.

> Speaking of APIs: Since both the patch here and the <keygen>-style hack
> change the DOM, is that OK? I.e. do sites not try to access the YouTube
> Flash player via JS APIs?

They certainly can, but I'm not sure they do that quite as much. Could be worth filing a followup though? ni'ing cpeterson for his opinion.

> 
> (FWIW, I think we should rewrite YouTube embeds to HTML5 even if Flash is
> installed. Definitely if installed but in click-to-play mode.)

That's the plan with this patch. If we see a youtube embed, we convert it, regardless of what plugins may/may not be installed already.
Flags: needinfo?(cpeterson)
Ok, so where this would go in the parser makes sense, and I think I have a basic idea of how to do it. 

That said, is it worth the extra difficulty of doing the url parsing/node replacement in C++ and burying it in the parser, versus doing this in the plugin code?
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #34)
> > Of course, you still need something else for sites that inject their Flash
> > embeds via the DOM APIs later.
> 
> I think we're mostly worried about static embed tags at the moment. We're
> seeing 1% session activations on these alone, so it'll be a good start.

Actually, it looks like YouTube's documentation recommends injecting the Flash player using SWFObject:

https://developers.google.com/youtube/js_api_reference#Embedding

It's acceptable for us to miss some Flash players, but I wonder how many we are missing if our telemetry and rewriter are only looking for static <object> embeds?


> > Speaking of APIs: Since both the patch here and the <keygen>-style hack
> > change the DOM, is that OK? I.e. do sites not try to access the YouTube
> > Flash player via JS APIs?
> 
> They certainly can, but I'm not sure they do that quite as much. Could be
> worth filing a followup though? ni'ing cpeterson for his opinion.

That is a great question. YouTube's embedded player (Flash or iframe) can be scripted from JS. Fortunately, I think you can detect whether the page intends to script the Flash player if the youtube.com URL includes the parameter "enablejsapi=1". In that case, we can choose not to rewrite the player.

https://developers.google.com/youtube/js_api_reference

You can test how your patch breaks YouTube Flash scripting on this YouTube demo page:

https://developers.google.com/youtube/youtube_player_demo


> > (FWIW, I think we should rewrite YouTube embeds to HTML5 even if Flash is
> > installed. Definitely if installed but in click-to-play mode.)
> 
> That's the plan with this patch. If we see a youtube embed, we convert it,
> regardless of what plugins may/may not be installed already.

Technically, we're not forcing HTML5 video. Kyle's patch rewrites YouTube Flash object embeds to YouTube iframe embeds, which lets YouTube decode whether to play Flash or HTML5 video.


(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #35)
> That said, is it worth the extra difficulty of doing the url parsing/node
> replacement in C++ and burying it in the parser, versus doing this in the
> plugin code?

We hope to remove this rewriting code some day, but I suspect that won't be until 2017 or later..
Flags: needinfo?(cpeterson)
(In reply to Chris Peterson [:cpeterson] from comment #36)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> > I think we're mostly worried about static embed tags at the moment. We're
> > seeing 1% session activations on these alone, so it'll be a good start.

Is our YouTube telemetry (bug 1207785) only measuring static <object> embed tags? The telemetry is counted in nsObjectLoadingContent::LoadObject(). Is that called when the HTML is parsed or when the plugin is loaded (e.g. including <object> elements injected by SWFObject)?


> Actually, it looks like YouTube's documentation recommends injecting the
> Flash player using SWFObject:
> 
> https://developers.google.com/youtube/js_api_reference#Embedding
> 
> It's acceptable for us to miss some Flash players, but I wonder how many we
> are missing if our telemetry and rewriter are only looking for static
> <object> embeds?

YouTube used to recommend using SWFObject to inject embedded videos, so it's safe to assume it's the most common method used. If our rewriter is in the HTML parser, I assume it would miss all those SWFObject embeds.


> > > Speaking of APIs: Since both the patch here and the <keygen>-style hack
> > > change the DOM, is that OK? I.e. do sites not try to access the YouTube
> > > Flash player via JS APIs?
> > 
> > They certainly can, but I'm not sure they do that quite as much. Could be
> > worth filing a followup though? ni'ing cpeterson for his opinion.
> 
> That is a great question. YouTube's embedded player (Flash or iframe) can be
> scripted from JS. Fortunately, I think you can detect whether the page
> intends to script the Flash player if the youtube.com URL includes the
> parameter "enablejsapi=1". In that case, we can choose not to rewrite the
> player.

Kyle, maybe we should expand our YouTube telemetry (bug 1207785) to also count how many YouTube <object> embeds include the `enablejsapi=1` parameter. I worry that many embeds might include the parameter, even if they aren't scripting the player from JS, because they copy/pasted some YouTube boilerplate code.

I can ask our friends at YouTube if they know how commonly `enablejsapi=1` is used.
Flags: needinfo?(kyle)
(In reply to Chris Peterson [:cpeterson] from comment #37)

> Is our YouTube telemetry (bug 1207785) only measuring static <object> embed
> tags? The telemetry is counted in nsObjectLoadingContent::LoadObject(). Is
> that called when the HTML is parsed or when the plugin is loaded (e.g.
> including <object> elements injected by SWFObject)?

So we call IsYoutubeEmbed during every LoadObject call, but it specifically checks for the existence of an embed node that the request is coming from. I just tested it with SWFObject and it doesn't seem to work. I'll look at adding testing for object nodes too and see what happens.

> YouTube used to recommend using SWFObject to inject embedded videos, so it's
> safe to assume it's the most common method used. If our rewriter is in the
> HTML parser, I assume it would miss all those SWFObject embeds.

Hmm, good point. Might be worth leaving things where they are for right now if that's the case (I'm not quite knowledgeable enough about all of this to say this concretely yet)

> Kyle, maybe we should expand our YouTube telemetry (bug 1207785) to also
> count how many YouTube <object> embeds include the `enablejsapi=1`
> parameter. I worry that many embeds might include the parameter, even if
> they aren't scripting the player from JS, because they copy/pasted some
> YouTube boilerplate code.

So we want to have an additional probe for enablejsapi also?
Flags: needinfo?(kyle)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #38)
> (In reply to Chris Peterson [:cpeterson] from comment #37)
> > Kyle, maybe we should expand our YouTube telemetry (bug 1207785) to also
> > count how many YouTube <object> embeds include the `enablejsapi=1`
> > parameter. I worry that many embeds might include the parameter, even if
> > they aren't scripting the player from JS, because they copy/pasted some
> > YouTube boilerplate code.
> 
> So we want to have an additional probe for enablejsapi also?

I think that is a good idea. If we cannot safely rewrite YouTube object embeds that specify `enablejsapi=1` (whether they script the YouTube player or not) and most of them do specify `enablejsapi=1`, then this rewriting feature would not be very useful. :\

I emailed our YouTube developer contacts to ask about enablejsapi, but I'm still waiting to hear back.
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #35)
> Ok, so where this would go in the parser makes sense, and I think I have a
> basic idea of how to do it. 
> 
> That said, is it worth the extra difficulty of doing the url parsing/node
> replacement in C++ and burying it in the parser, versus doing this in the
> plugin code?

did a decision get made here? need to know if this needs a review.
(In reply to Jim Mathies [:jimm] from comment #40)
> did a decision get made here? need to know if this needs a review.

Jim, you can probably delay your review, but Kyle should chime in here. We are still determining when and how this YouTube rewriting should happen.
Comment on attachment 8677638 [details] [diff] [review]
Patch 1 (v7) - Rewrite youtube embed tags to be iframes

Review of attachment 8677638 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing reviews until we figure out what we'll need.
Attachment #8677638 - Flags: review?(schneider)
Attachment #8677638 - Flags: review?(jmathies)
To rewrite <object> in the parser, you need to write the code in nsHtml5TreeOperation::DoneAddingChildren instead:
https://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#609

Note that that method gets called on more elements than just <object>, so to avoid QI'ing everything there, you could add a new <object>-specific tree op type around here:
https://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h#912

That said, I think it would be cleaner to do some sort of shadow DOM hack that covered all cases regardless of parser or DOM manipulation origin and didn't change the DOM visible to the scripts on the page.
The problem with shadow DOM here is that <object> and <embed> do their loads from the DOM node, not the presentation.  So they would still do it, even if we nerfed the presentation to show some anonymous elements via shadow DOM....
(In reply to Boris Zbarsky [:bz] from comment #44)
> The problem with shadow DOM here is that <object> and <embed> do their loads
> from the DOM node, not the presentation.  So they would still do it, even if
> we nerfed the presentation to show some anonymous elements via shadow DOM....

How troublesome would it be to have an nsObjectLoadingContent-specific non-Shadow DOM shadow iframe attached to nsObjectLoadingContent in such a way that nsObjectLoadingContent would know not to do its usual thing and layout would look at the shadow iframe?
It would probably be simpler to skip the shadow iframe and have the nsObjectLoadingContent take the codepath that <object type="text/html"> takes anyway, no?
(In reply to Boris Zbarsky [:bz] from comment #46)
> It would probably be simpler to skip the shadow iframe and have the
> nsObjectLoadingContent take the codepath that <object type="text/html">
> takes anyway, no?

Good point! Hacking YouTube nsObjectLoadingContent to divert to the text/html codepath with the in nsObjectLoadingContent itself seems like the best option to me. Way better than parser hacks or JS-based rewriting.
Kyle, we have some numbers from your YOUTUBE_EMBED_SEEN telemetry probe. The short story is that only about 0.15% of browser sessions see at least one Flash-only YouTube embed. Is it worth rewriting these Flash embeds for 0.15% of browser sessions? I think it's still worth rewriting, since you have already written the code. 0.15% is still a lot of Flash instantiations and we'll be fixing these videos for users without Flash.

The telemetry page shows that 0.36% of Beta 43 browser sessions have seen a Flash embed without enablejsapi=1, but that average is misleading because the first week of Beta 43 and Aurora 44 was not filtering out embeds with enablejsapi=1. If you look at the telemetry evolution view, you can see that after the enablejsapi=1 filter (bug 1218952) was uplifted on 2015-11-09, the reported YOUTUBE_EMBED_SEEN dropped from about 1% to 0.15%. This suggested that 85% of Flash-only embeds had enablejsapi=1 and should not be rewritten.

Aurora 44 evolution view: http://mzl.la/1jxZlU3 (drops with build 20151110)

Beta 43 evolution view: http://mzl.la/1jxZBlQ (drops with build 20151109)
Flags: needinfo?(kyle)
Turns out that our logic was flipped, so that 0.15% was showing us the number of sessions that saw unrewritable (embeds with enablejsapi=1 in the url) youtube embeds. Now, taking this with the 1% metric we were seeing before we changed what the probe was doing, this means 0.85% of embeds seen are rewritable.

Just to make sure, I've filed bug 1229971, which flips the logic and renames the probe, so we can make sure we're doing the right thing here.
Flags: needinfo?(kyle)
Would it be possible to generalize this to any Flash object? For example, by setting a header in the SWF response, any site could use this to replace old content, and YouTube would have some control over how the replacement happens.
@ Kyle, what do we need to finish before we land your patch?

@ magicalhobo, allowing the SWF host server to ask Firefox to rewrite third-party web pages is an interesting idea, but it could pose a security risk (e.g. malicious ad networks serving Flash ads that rewrite the web page)
Flags: needinfo?(kyle)
My thought was something like "X-Frame-Upgrade" which would specify a URL to load in an IFrame.
(In reply to Chris Peterson [:cpeterson] from comment #51)
> @ Kyle, what do we need to finish before we land your patch?

I'm researching the strategy talked about in comment 46 right now, once I get that figured out then we can proceed with reviews on either this or the js based rewriting.
Flags: needinfo?(kyle)
Ok, did my research, I get what comment 46 is talking about now. In nsObjectLoadingContent, if we see we're in an object or embed tag and loading a youtube URL, we change the url from "http://youtube.com/v/[video id]" tp "http://youtube.com/embed/[video id]", then divert to the eType_Document loader type, which will create an nsFrameLoader and do the right thing. I'm still figuring out how to deal with channels/listeners here, since we don't have one from the start like we expect, but the strategy at least makes sense, and will be cleaner than JS or parser hacks, as mentioned.
I've been working to implement the strategy in comment 46, but I'm running into some confusion in terms of how to make this take the Document loading path, versus the Plugin loading path. You'll have to excuse me if the way I describe this is a little odd, but it's what I've picked up via reading the code so far.

To treat this like an object tag with text/html, it looks like I also need to somehow attach the object/embed node in question to an nsHTTPChannel so that LoadObject is called via OnStartRequest, which sets up listeners and what not like nsFrameLoader requires. I'm not real sure where this happens in the code, though. I've been trying to trace the differences from the parser to nsObjectLoadingContent for the different tags (object/embed with flash versus object with text/html that uses nsFrameLoader), but I'm just not seeing it.

Assuming that made sense, have any advice?

(I'd be ni?'ing bz on this too but he's out 'til Jan 4th)
Flags: needinfo?(hsivonen)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (PTO UNTIL JAN 4. USE NEEDINFO?) from comment #56)
> I've been working to implement the strategy in comment 46, but I'm running
> into some confusion in terms of how to make this take the Document loading
> path, versus the Plugin loading path. You'll have to excuse me if the way I
> describe this is a little odd, but it's what I've picked up via reading the
> code so far.
> 
> To treat this like an object tag with text/html, it looks like I also need
> to somehow attach the object/embed node in question to an nsHTTPChannel so
> that LoadObject is called via OnStartRequest, which sets up listeners and
> what not like nsFrameLoader requires. I'm not real sure where this happens
> in the code, though. I've been trying to trace the differences from the
> parser to nsObjectLoadingContent for the different tags (object/embed with
> flash versus object with text/html that uses nsFrameLoader), but I'm just
> not seeing it.
> 
> Assuming that made sense, have any advice?

It's not exactly clear to me what the problem is. My apologies if my response is something you already tried.

It seems to me that the URL rewriting should happen right when the load machinery first asks the attributes for the URL to load. With the rewritten URL being the one that's fetched, YouTube should respond with text/html content, so the loading machinery would take the eType_Document path naturally. Right?

So:

1755   nsAutoString uriStr;
1756   // Different elements keep this in various locations
1757   if (isJava) {
1758     // Applet tags and embed/object with explicit java MIMEs have src/data
1759     // attributes that are not meant to be parsed as URIs or opened by the
1760     // browser -- act as if they are null. (Setting these attributes triggers a
1761     // force-load, so tracking the old value to determine if they have changed
1762     // is not necessary.)
1763   } else if (thisContent->NodeInfo()->Equals(nsGkAtoms::object)) {
1764     thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::data, uriStr);
1765   } else if (thisContent->NodeInfo()->Equals(nsGkAtoms::embed)) {
1766     thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::src, uriStr);
1767   } else {
1768     // Applet tags should always have a java MIME type at this point
1769     NS_NOTREACHED("Unrecognized plugin-loading tag");
1770   }
1771 

I'd add a check here to see if uriStr looks like a YouTube swf URL but |this| doesn't belong to a document on YouTube itself and if so, rewrite uriStr to the corresponding YouTube HTML5 embedding URL and assign "text/html" to newMime.

1772   // Note that the baseURI changing could affect the newURI, even if uriStr did
1773   // not change.
1774   if (!uriStr.IsEmpty()) {
1775     rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(newURI),
1776                                                    uriStr,
1777                                                    thisContent->OwnerDoc(),
1778                                                    newBaseURI);
1779     if (NS_SUCCEEDED(rv)) {
1780       NS_TryToSetImmutable(newURI);
1781     } else {
1782       stateInvalid = true;
1783     }
1784   }
Flags: needinfo?(hsivonen)
Thanks for the guidance Henri, that was exactly what I needed. Was just getting confused by the state machine inside nsObjectLoadingContent.

Here's a new version using only rewriting inside nsObjectLoadingContent. This is a WIP that could seriously use more cleaning and testing, but it seems to do the job well enough, and in far less code than the JS solution.

I'm gonna clean this up a bit then put it in for review. Not exactly sure how I'm going to write tests around this yet.
Attachment #8677638 - Attachment is obsolete: true
Comment on attachment 8704510 [details] [diff] [review]
Patch 1 (v8) - WIP: Rewrite youtube embed tags to iframes

Review of attachment 8704510 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #58)
> Thanks for the guidance Henri, that was exactly what I needed. Was just
> getting confused by the state machine inside nsObjectLoadingContent.
> 
> Here's a new version using only rewriting inside nsObjectLoadingContent.
> This is a WIP that could seriously use more cleaning and testing, but it
> seems to do the job well enough, and in far less code than the JS solution.

Nice!

> I'm gonna clean this up a bit then put it in for review. Not exactly sure
> how I'm going to write tests around this yet.

Some drive-by comments below (some of these may already be covered by your intent to clean this up):

::: dom/base/nsObjectLoadingContent.cpp
@@ +1483,4 @@
>  }
>  
>  bool
> +nsObjectLoadingContent::IsRewritableYoutubeEmbed(nsCOMPtr<nsIURI> aURI)

The type of the argument should be nsIURI*.

@@ +1514,1 @@
>    }

In the existing code,
if (!StringEndsWith(domain, currentBaseDomain)) {
looks wrong. It seems that if the embed is for e.g. "tube.com", it would match. It seems to me the check should be for equality instead.

@@ +1518,1 @@
>    // Only log urls that are rewritable, e.g. not using enablejsapi=1

Please add telemetry for assessing how common enablejsapi=1 is so that we can decide if it makes sense to put effort to emulating the JS API as a follow-up feature.

@@ +1782,5 @@
> +      // the path, and append it to the embed URL.
> +      nsCString uriPath;
> +      newURI->GetPath(uriPath);
> +      PRInt32 lastSlashLocation = uriPath.RFindChar('/');
> +      uriPath.Cut(0, lastSlashLocation);

Instead of calling Cut here, appending a substring three lines later should be a tiny bit more efficient.

@@ +1793,5 @@
> +                                                     newBaseURI);
> +      newMime = NS_LITERAL_CSTRING("text/html");
> +      newType = eType_Loading;
> +      mIsRewrittenYoutube = true;
> +    }

Please add an "else" branch to set mIsRewrittenYoutube to false to cover the theoretical case where a page uses JS to retarget a YouTube <object> to some other plug-in content later.

::: modules/libpref/init/all.js
@@ +5097,5 @@
>  // Allow customization of the fallback directory for file uploads
>  pref("dom.input.fallbackUploadDir", "");
> +
> +// Turn rewriting of youtube embeds on/off
> +pref("plugins.rewrite_youtube_embeds", true);

It seems that this pref isn't being checked and the telemetry is now tied to actual rewriting instead of rewritability, so checking would turn off the telemetry, too.
Thanks for the comments! Now you can see why I said it needed cleanup, heh. Was just posting the first thing I got working so make sure the basic solution didn't get lost. :)

(In reply to Henri Sivonen (:hsivonen) from comment #59)
> @@ +1518,1 @@
> >    // Only log urls that are rewritable, e.g. not using enablejsapi=1
> 
> Please add telemetry for assessing how common enablejsapi=1 is so that we
> can decide if it makes sense to put effort to emulating the JS API as a
> follow-up feature.
> 

We did this already, though somewhat by accident. If you check out the YOUTUBE_REWRITABLE_EMBED_SEEN probe on the telemetry page, it checks every time a session has seen at least one rewritable youtube embed without enablejsapi (currently at 1.11% of sessions for pre-release versions, since we didn't activate the probe on release). Due to accidentally flipping the logic when I first landed the probe, this ended up counting every enablejsapi=1 link for about a month during November/October, which showed that about 0.15% of sessions saw at least one embed with enablejsapi=1 (fixed in bug 1229971). I really don't think that's going to be enough to warrant trying to emulate around it and possibly breaking content, but cpeterson may be of a different opinion at some point. :)
Addressed Henri's comments, cleaned up code, added more comments.
Attachment #8704510 - Attachment is obsolete: true
Further simplified url replacement, added check to make sure we only do replaces on flash embeds.
Attachment #8704852 - Attachment is obsolete: true
Attachment #8705241 - Flags: review?(hsivonen)
Attachment #8705241 - Flags: review?(cpeterson)
I still have no idea how to write tests for this either, since it depends on an outside domain. I could probably rig something up using the flash test plugin to at least make sure we don't load the plugin somehow, but in terms of making sure the rewrite works, I'm not sure how I can fetch the URL back from the node in a test after it's rewritten.
Can you just add this domain to the list of domains our test server pretends to be?  We're already doing that for the publicly-routable example.org/com, www.itisatrap.org, various w3c-test.org stuff, etc.  See build/pgo/server-locations.txt
Yeah, that'll be a start, at least. Should get me a platform where I can start figuring out if I can retrieve the rewritten URL somehow.
Comment on attachment 8705241 [details] [diff] [review]
Patch 1 (v10) - Rewrite youtube flash embeds to possibly use HTML5

Review of attachment 8705241 [details] [diff] [review]:
-----------------------------------------------------------------

The logic LGTM, though I'm not familiar with the DOM bits.

::: dom/base/nsObjectLoadingContent.cpp
@@ +1802,5 @@
> +                              NS_LITERAL_STRING("/embed/"));
> +      rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(newURI),
> +                                                     uriStr,
> +                                                     thisContent->OwnerDoc(),
> +                                                     newBaseURI);

In an earlier version of your patch (v4), you copied some property values (like height and width) from the Flash embed's object element to the new replacement element. Is that no longer necessary if you are just rewriting the URL?

https://bugzilla.mozilla.org/attachment.cgi?id=8674469&action=diff
Attachment #8705241 - Flags: review?(cpeterson) → feedback+
Yeah, in the earlier version, we were creating a completely new element and had to transfer all of the information over. Now, we're working within the originating element, and the only thing that changes in that element is how object loading happens. So, the copying is no longer necessary. One of the many advantages of this new method. :)
More patch simplification, moving content detection logic to more obvious places.
Attachment #8705241 - Attachment is obsolete: true
Attachment #8705241 - Flags: review?(hsivonen)
Attachment #8705840 - Flags: review?(hsivonen)
Attachment #8705840 - Flags: review?(cpeterson)
Wrote a mochitest to check the innards of nsObjectLoadingContent. Would like to also be able to check displayedType, but I'm not quite sure how to detect when that changes from outside the object.
Attachment #8705841 - Flags: review?(hsivonen)
Attachment #8705841 - Flags: review?(cpeterson)
Ok, so something I've been thinking about while writing tests is handling of MIME type rewriting. We're effectively changing how we deal with embeds/objects that point to a certain URL, and avoiding using flash (treating it as we would text/html) even if the mime type is application/x-shockwave-flash. Is this going to present any problems to users? Just not sure if this might potentially mess with scripting or something.
Comment on attachment 8705841 [details] [diff] [review]
Patch 2 (v1) - Mochitests for youtube flash to html5 rewriting

Review of attachment 8705841 [details] [diff] [review]:
-----------------------------------------------------------------

f+ LGTM
Attachment #8705841 - Flags: review?(cpeterson) → feedback+
Comment on attachment 8705840 [details] [diff] [review]
Patch 1 (v11) - Rewrite youtube flash embeds to possibly use HTML5

Review of attachment 8705840 [details] [diff] [review]:
-----------------------------------------------------------------

f+ LGTM
Attachment #8705840 - Flags: review?(cpeterson) → feedback+
Comment on attachment 8705840 [details] [diff] [review]
Patch 1 (v11) - Rewrite youtube flash embeds to possibly use HTML5

Review of attachment 8705840 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if the EndsWith vs. Equals issue is either explained or replaced with EqualsLiteral.

::: dom/base/nsObjectLoadingContent.cpp
@@ +1514,3 @@
>    // See if URL is referencing youtube
> +  nsAutoCString youtubeBaseDomain("youtube.com");
> +  if (!StringEndsWith(currentBaseDomain, youtubeBaseDomain)) {

Why not
if (!currentBaseDomain.EqualsLiteral("youtube.com")) {
?

That is, why EndsWith instead of Equals?
Attachment #8705840 - Flags: review?(hsivonen) → review+
Comment on attachment 8705841 [details] [diff] [review]
Patch 2 (v1) - Mochitests for youtube flash to html5 rewriting

Looks like you forgot to hg add test_bug769117.html.
Attachment #8705841 - Flags: review?(hsivonen) → review-
(In reply to Henri Sivonen (:hsivonen) from comment #74)
> Comment on attachment 8705840 [details] [diff] [review]
> Patch 1 (v11) - Rewrite youtube flash embeds to possibly use HTML5
>
> Why not
> if (!currentBaseDomain.EqualsLiteral("youtube.com")) {
> ?
> 
> That is, why EndsWith instead of Equals?

Ah, that was my misunderstanding of what tldservice was returning. I'll fix it, thanks.
Actually attached all the files this time! \o/
Attachment #8705841 - Attachment is obsolete: true
Attachment #8706538 - Flags: review?(hsivonen)
Attachment #8706538 - Flags: review?(cpeterson)
Comment on attachment 8706537 [details] [diff] [review]
Patch 1 (v12) - Rewrite youtube flash embeds to possibly use HTML5; r=hsivonen

bz said he'd take a look too.
Attachment #8706537 - Flags: review?(bzbarsky)
Attachment #8706538 - Flags: review?(bzbarsky)
Comment on attachment 8706538 [details] [diff] [review]
Patch 2 (v2) - Mochitests for youtube flash to html5 rewriting

Review of attachment 8706538 [details] [diff] [review]:
-----------------------------------------------------------------

and the test file itself LGTM, too :)

::: dom/base/test/test_bug769117.html
@@ +25,5 @@
> +       embed.src = "https://mochitest.youtube.com/v/Xm5i5kbIXzc";
> +       embed.type = "application/x-shockwave-flash";
> +
> +       object.appendChild(embed);
> +       document.body.appendChild(object);

Why are creating the Flash embed dynamically? Can you declare it statically in this HTML file? That's how the embeds will appear in the wild, so it would be a more realistic test case.
Attachment #8706538 - Flags: review?(cpeterson) → feedback+
Comment on attachment 8706537 [details] [diff] [review]
Patch 1 (v12) - Rewrite youtube flash embeds to possibly use HTML5; r=hsivonen

>@@ -1776,6 +1794,22 @@ nsObjectLoadingContent::UpdateObjectParameters(bool aJavaURI)
>+      newType = eType_Loading;

Why bother?  This function doesn't read newType before writing it after this point.

In fact, the decl of newType could move all the way down to line 1938 or so without too much trouble, which would have made the compiler catch this assignment not being useful.

>+      mContentType = NS_LITERAL_CSTRING("text/html");

This _might_ be useful, but it's not clear to me why.  At the very least, please document...

>+      mRewrittenYoutubeEmbed = false;

This is all in the !uriStr.IsEmpty() case.  Shouldn't we set mRewrittenYoutubeEmbed to false even if uriStr.IsEmpty()?

>+  if (mRewrittenYoutubeEmbed) {
>+    return eType_Document;

This seems a bit weird.  What if there is a proxy returning non-HTML data there or something (far-fetched, I know).  Or just YouTube decides to return non-HTML data?

It seems like all we really want for purposes of this function is to pretend like "caps & eSupportDocuments" is true if mRewrittenYoutubeEmbed, right?  If that's not enough, please document clearly why it's not enough.

>+     * Used for identifying if we can rewrite a youtube flash embed to

s/if/whether/

r=me with those fixed.
Attachment #8706537 - Flags: review?(bzbarsky) → review+
Comment on attachment 8706538 [details] [diff] [review]
Patch 2 (v2) - Mochitests for youtube flash to html5 rewriting

>+       // This can go away once embed also is on WebIDL

Uh... <embed> _is_ on WebIDL.  HTMLEmbedElement.webidl says:

   HTMLEmbedElement implements MozObjectLoadingContent;

So you don't need this OBJLC thing at all.

>+       window.setTimeout(() =>

Please use SimpleTest.executeSoon if you just want to run this async.  But what really makes sure the actualType is updated at that point, since normally that depends on the server response?

Past that, I agree it's worth testing the case when the <embed>/<object> are inline and you just trigger off the load event instead of a 0-timeout.

I don't see what the point of "object" is in this testcase.  It's only the rewriting of the <embed> that's really being tested. Could use a separate test for rewriting of <object>.  And we probably do NOT want to rewrite <applet> even if someone points it to youtube (and why someone would do that, I dunno).

r=me with those issues fixed.
Attachment #8706538 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #81)
> Comment on attachment 8706537 [details] [diff] [review]
> Patch 1 (v12) - Rewrite youtube flash embeds to possibly use HTML5;
> r=hsivonen
> >+  if (mRewrittenYoutubeEmbed) {
> >+    return eType_Document;
> 
> This seems a bit weird.  What if there is a proxy returning non-HTML data
> there or something (far-fetched, I know).  Or just YouTube decides to return
> non-HTML data?
> 
> It seems like all we really want for purposes of this function is to pretend
> like "caps & eSupportDocuments" is true if mRewrittenYoutubeEmbed, right? 
> If that's not enough, please document clearly why it's not enough.

So my understanding was that the data at the rewritten URL needed to be fetched through the FrameLoader path (line 2361), which requires us to be eType_Document to reach. I'm not sure how just adding eSupportDocuments to the caps would achieve that, especially if we keep the mime type as application/x-shockwave-flash?
Flags: needinfo?(bzbarsky)
So I just paged in more of this code.  The basic idea is that this stuff detects the type at two places.  The first is before starting the load.  The second is when getting an OnStartRequest from necko; at that point we will call LoadObject _again_, and create a frameloader as needed.  This will use the actual type from the channel.  So in theory as long as YouTube returns an HTML type we would be fine.

That said, there are some complications around types detected as "plugin" types; if we start with one of those we do NOT allow the channel type to override.  Specifically, see the overrideChannelType stuff in nsObjectLoadingContent::UpdateObjectParameters and the fact that it gets set if typeHint == eType_Plugin, or if we have a plugin for the URI and newMime (which should not actually be an issue here, since I don't expect these URIs end in .swf or whatnot).

Given all that, we really do want to end up with typeHint being eType_Document.  But seems to me like we should be setting newMime to "text/html" to accomplish this, not setting mContentType.  newMime will get stored in mContentType anyway at what's currently line 2011 (the mType != eType_Loading && mContentType != newMime block) and I _think_ as long as you set newMime to "text/html" you won't need any changes to GetTypeOfContent, since it will see "text/html" coming in before the load and hence return eType_Document.  When the response starts coming in, it should likewise see "text/html" from the server and everything should be good, right?
Flags: needinfo?(bzbarsky)
Comment on attachment 8706538 [details] [diff] [review]
Patch 2 (v2) - Mochitests for youtube flash to html5 rewriting

Review of attachment 8706538 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/test/test_bug769117.html
@@ +12,5 @@
> +     SimpleTest.waitForExplicitFinish();
> +     /** Test for Bug 769117 **/
> +     "use strict";
> +     function onLoad () {
> +       function getAttr(obj, attr) {

Is this function called anywhere? If not, please remove.

@@ +25,5 @@
> +       embed.src = "https://mochitest.youtube.com/v/Xm5i5kbIXzc";
> +       embed.type = "application/x-shockwave-flash";
> +
> +       object.appendChild(embed);
> +       document.body.appendChild(object);

I think it's good to test dynamic insertion. Testing static markup, too, wouldn't hurt, but dynamic insertion should be the more complex case, so testing that is nice.
Attachment #8706538 - Flags: review?(hsivonen) → review+
(In reply to Boris Zbarsky [:bz] from comment #84)
> newMime will get
> stored in mContentType anyway at what's currently line 2011 (the mType !=
> eType_Loading && mContentType != newMime block) and I _think_ as long as you
> set newMime to "text/html" you won't need any changes to GetTypeOfContent,
> since it will see "text/html" coming in before the load and hence return
> eType_Document.  When the response starts coming in, it should likewise see
> "text/html" from the server and everything should be good, right?

Actually, we've still also got to change capabilities somewhere, like you mentioned in comment 81. In HTMLSharedObjectElement, we don't currently have eSupportDocuments in capabilities, so even if the MIME type is text/html, we'll get a eType_Null since the capabilities won't support documents.

Just to confirm, should we continue with the idea of special casing capabilities only for youtube embeds in GetTypeOfContent? I'm guessing we don't want all embeds to allow document types?
Forgot to ni? on comment 86.
Flags: needinfo?(bzbarsky)
Oh, right, you need to change the capabilities bitmask to allow text/html to work right.  And yes, you only want to do this for <embed> if it's a rewritten Youtube embed.  The simplest thing may actually be to change HTMLSharedObjectElement::GetCapabilities appropriately.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/df9aaad130a3
https://hg.mozilla.org/mozilla-central/rev/24e0645f9c31
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1240566
Depends on: 1240471
No longer depends on: 1240566
Component: Audio/Video: Playback → DOM: Core & HTML
Depends on: 1240566
No longer depends on: 1240566
Depends on: 1246088
Depends on: 1255918
Depends on: 1258053
Release Note Request (optional, but appreciated)

We should rel note this feature for 47. The code landed in 46, but we disabled it in 46 with bug 1255918. 47 is the first release with the feature enabled by default.

[Why is this notable]:

This feature will reduce the number of Flash plugin instances, which should improve Firefox stability, and move control of video playback experience from Flash to Firefox code we maintain.

[Suggested wording]:

New! Replace embedded Flash YouTube videos with HTML5 video

[Links (documentation, blog post, etc)]:

We have no documentation besides this bug.
status-firefox47: --- → fixed
relnote-firefox: --- → ?
Keywords: feature
How about: Embedded YouTube videos play with HTML5 if Flash is not installed
relnote-firefox: ? → 46+
Flags: needinfo?(cpeterson)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #97)
> How about: Embedded YouTube videos play with HTML5 if Flash is not installed

LGTM, though I would expand "HTML5" to "HTML5 video" just to be clear.
Flags: needinfo?(cpeterson)
Google just shipped YouTube Flash embed rewriting in Chrome 54:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/BW8g1iB0jLs/uddWuBroBAAJ
We may want to cross-check their behavior against ours...
Perhaps we should discuss standardizing this?  https://github.com/whatwg/html/issues/2390
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: