STR: - enable remote tabs, remote auto start - restart and visit a page with plugins expected: you should get a click-to-play overlay on plugin windows results: you get a gray background without the activate message/link

The fix was reverted and now it isn't working again.

So e10s-m2 will ship with this not fixed?

I can work on this, though I won't be working on it full time so I can't promise how soon I can finish. (If anyone else wants to help out, just let me know.)

This is patch is about 1/3 complete and not very functional yet, but it's getting there.

Slightly farther along, but broken by the fact that Services.perms.addFromPrincipal is not available in the content process, while content.document.nodePrincipal is not available in the parent process.

Basic click-to-play functionality now works in e10s windows! Still need to port other types of notifications and behaviors, and fix some breakage in non-e10s windows.

Fixed some more bugs; click-to-play now works in both e10s and non-e10s windows. Overall this is about 50% done. Most of the remaining work is in the "install" and "play preview" code.

Hi Matt! the problem when E10S is enabled now is you can not switch between "Ask to activate, Always activate, never activate" or "check for update, view recent update...." in the about:addons tab!! let me know if you want me to file a bug file for that, cheers

Now with working click-to-install and fixes for some errors. TODO: Play-preview and crash plugin notifications. (In reply to Achwaq Khalid from comment #10) > the problem when E10S is enabled now is you can not switch between "Ask to > activate, Always activate, never activate" or "check for update, view recent > update...." in the about:addons tab!! > > let me know if you want me to file a bug file for that, cheers Yes, that sounds like a separate bug. Could you file a new bug report, please? (Note: I couldn't reproduce that bug in my nightly build on Linux.)

Almost done. I think the only thing left to touch is crash reporting.

Holla Matt here i just reported it https://bugzilla.mozilla.org/show_bug.cgi?id=1031606

A fully functioning patch! Next steps: Rebase on m-c; fix mochitest failures.

looking forward to check it! Flashblock was really misbehaving recently

I believe this patch is ready to review, although I still need to make a bunch of test changes (which I'll do as a separate patch). Review notes: * This patch uses "hg cp" for the code that was split into multiple files, so you can compare each new file against the original. * The majority of code in browser-plugins.js was using non-prefixed identifiers (e.g. "browser" instead of "aBrowser") so I tried consistently follow that style in the new JSM file, and take some steps toward consistency in other code I added. * I changed the name of "_setNotificationIcon" to "updateHiddenPluginUI" since it changes more than just the icon.

Comment on attachment 8448888 [details] [diff] [review] patch I'm going to delegate this to gfritzsche.

I will probably not get to this today. I will see that i get to looking over it this week though.

No rush; it will take me a while to get through the tests since I'm doing this in my spare time. (Anyone want to help? ;) And the underlying code is not churning much at all. Sorry for the giant patch; I didn't find a good way to split it into smaller logical pieces.

Comment on attachment 8448888 [details] [diff] [review] patch This looks good to me, although I've really only looked at the IPC aspects. Right now I think the main challenge is to find a way to make this easier to review. It seems like it might be possible to split out the "core" features from things that are more optional. For example, an initial patch without plugin installation or crash reporting would probably be simpler. I'm not sure if it would help in this case, but here's how I split up the patches for session restore. First I split out all the content process-specific code into a separate JSM but without using the message manager. It's a more straightforward refactoring that way, and it doesn't make the code async, so you generally don't have to fix up all the tests. Then I slowly introduced the message manager between the two pieces of code. As a side note, you should be able to send principals over the message manager once bug 1032592 lands.

(In reply to Bill McCloskey (:billm) from comment #20) > This looks good to me, although I've really only looked at the IPC aspects. > Right now I think the main challenge is to find a way to make this easier to > review. It seems like it might be possible to split out the "core" features > from things that are more optional. For example, an initial patch without > plugin installation or crash reporting would probably be simpler. Attachment 8443747 [details] [diff] is basically this, except that enough stuff was broken in each of my intermediate "WIP" patches to make them kind of iffy candidates for review. > I'm not sure if it would help in this case, but here's how I split up the > patches for session restore. First I split out all the content > process-specific code into a separate JSM but without using the message > manager. Unfortunately I didn't do anything nearly so sane in my actual development process, but I could try to retroactively split the big patch up into phases like this if it would help gfritze review it. > As a side note, you should be able to send principals over the message > manager once bug 1032592 lands. Thanks!

(In reply to Matt Brubeck (:mbrubeck) from comment #21) > Unfortunately I didn't do anything nearly so sane in my actual development > process, but I could try to retroactively split the big patch up into phases > like this if it would help gfritze review it. Splitting it up would be helpful, but please don't go for it if it takes too much effort. The major blocker for me to start here are other high-priority bugs.

Note that I just touched browser-plugins.js in bug 1009760 which added crash-report support for a new type of plugin. It was a very simple change though: We still receive a PluginCrashed event for those plugins, however they don't have a related DOM object. So the target of the event is the `window`, and on that case we go directly to the "show infobar" path.

Matt, do you intend to break this into multiple patches or rebase it soon? Or should i plan on making a first pass already?

(In reply to Georg Fritzsche [:gfritzsche] from comment #24) > Matt, do you intend to break this into multiple patches or rebase it soon? > Or should i plan on making a first pass already? I don't have any current plans to split up the patch, though I can do it if it would significantly speed reviewing. I'm happy to rebase if the patch no longer applies. I do think it's ready for a first pass whenever you have bandwidth. Thanks!

Ok, i'll try to get to a first pass here next week - sorry for the long delay.

Comment on attachment 8448888 [details] [diff] [review] patch Review of attachment 8448888 [details] [diff] [review]: ----------------------------------------------------------------- A first pass here looks good. I didn't try to trace important paths yet, but it looks sensible. ::: browser/base/content/browser-plugins.js @@ +70,5 @@ > + return Services.scriptSecurityManager.getSystemPrincipal(); > + } > + let uri = Services.io.newURI(remotePrincipal.URI.spec, null, null); > + return Services.scriptSecurityManager > + .getAppCodebasePrincipal(uri, remotePrincipal.appId, remotePrincipal.isInBrowserElement); There should be more places that need to pass principals - does this have potential to be a general helper? @@ +210,2 @@ > if (!browser.missingPlugins) > browser.missingPlugins = new Map(); Hm, is this a COWS |browser|? Do we need to set on that? ::: browser/base/content/browser.js @@ -241,5 @@ > - // The PluginClickToPlay events are not fired when navigating using the > - // BF cache. |persisted| is true when the page is loaded from the > - // BF cache, so this code reshows the notification if necessary. > - if (persisted) > - gPluginHandler.reshowClickToPlayNotification(); Do we still need to worry about this? Is there an equivalent for this in this patch?

Hey mbrubeck, Not sure if you're reading bugmail while on PTO, but was wondering if you wanted me to take the helm to drive this bug to completion seeing as how you've been doing it in your spare cycles while working on Servo. Let me know! All the best, -Mike

Tentatively nabbing until I hear back from mbrubeck...

This has bitrotted a bit, especially due to the work with GMP from bug 1009760. I'm cleaning this up right now...

(In reply to Mike Conley (:mconley) from comment #29) > Not sure if you're reading bugmail while on PTO, but was wondering if you > wanted me to take the helm to drive this bug to completion seeing as how > you've been doing it in your spare cycles while working on Servo. Yes, that's great. Thanks!

(In reply to Georg Fritzsche [:gfritzsche] from comment #28) > There should be more places that need to pass principals - does this have > potential to be a general helper? If I remember correctly, there is/was work in progress to add general principal-passing support to the platform, which should make the code here unnecessary. > > - // The PluginClickToPlay events are not fired when navigating using the > > - // BF cache. |persisted| is true when the page is loaded from the > > - // BF cache, so this code reshows the notification if necessary. > > - if (persisted) > > - gPluginHandler.reshowClickToPlayNotification(); > > Do we still need to worry about this? Is there an equivalent for this in > this patch? Yes, this logic has been moved to the "onPageShow" method in PluginContent.jsm.

Alright, I've got all tests passing now. I'll have a patch posted tomorrow once I clean things up a bit (and figure out a nice way of splitting up this massive change).

So with this test, all mochitest-browser tests under browser/base/content/test/plugins now pass. I had to do some gross hacks to make some of the tests pass, and I'm going to try to refactor some of that a bit. I'm also going to try to split this patch up into smaller chunks: 1) Changes to browser-plugins.js 2) Changes to browser-plugins.js after it was copied to PluginsContent.jsm, and other things to get the new PluginsContent.jsm module built. 3) Changes to other bits of browser 4) Changes to tests I'll have patches 1, 2 and 3 up today, and can probably get some reviews kicked off. For 4, it might have to wait until next Tuesday.

This is the difference between the old browser-plugins.js and PluginContent.jsm. This should help with reviewing PluginContent.jsm, which is really just the old browser-plugins.js, but heavily modified to send messages instead.

Comment on attachment 8481499 [details] [diff] [review] Part 1: Update browser-plugins to use message passing for plugin events. r=? I've broken down the overall change here by "subject". The patches I've posted are not atomic, unfortunately, so you might need to refer back and forth to the other ones to get the full pictures. Basically, we've introduced a PluginContent.jsm that content scripts can instantiate which listens for plugin events, and CTP stuff, and sends messages up to the parent for it to react. The parent can also send messages back down to the child about things like a plugin was activated, or a context menu was clicked, etc. So anything that has directly fiddled with content has been removed from browser-plugins, and put into PluginContent.jsm instead. The majority of _this_ patch is those removals, but also changes for sending and receiving messages from the child. jaws, I've seen you reviewing patches in the past for browser-plugins stuff - let me know if you're not the right guy to review this, or if you're swamped with other things and I'll redirect.

Comment on attachment 8481500 [details] [diff] [review] Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=? So this is the other half of the puzzle, where we introduce PluginContent.jsm which hears / reacts to Plugin events in content, and sends and receives messages from the parent process. This is basically browser-plugins.js, but rejigged so that it's a JSM that returns objects that can do these reactions per browser. Naturally, this script gets full access to the content DOM, but no access to the browser chrome - it just sends messages to it's browser-plugins counterpart. Giant green walls of text suck to review, and like I said - this is basically a rejigged browser-plugins.js, so I've also attached a diff between this new PluginContent.jsm and the original browser-plugins.js so you can see what exactly changed: https://bugzilla.mozilla.org/attachment.cgi?id=8481518 You'll probably want to look at that instead, and then just flag this patch r+ or r- depending on what you find. Again, jaws, pointing this at you based on the log of browser-plugins.js, but if you're overloaded, just say the word.

Comment on attachment 8481499 [details] [diff] [review] Part 1: Update browser-plugins to use message passing for plugin events. r=? Hey Georg - jaws said you'd be the right guy to review these. Mind taking a look?

Comment on attachment 8481500 [details] [diff] [review] Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=? Same with this one? (Note my comment to jaws in comment 46).

Comment on attachment 8481518 [details] [diff] [review] Difference between browser-plugins.js and PluginContent.jsm Review of attachment 8481518 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty sane. Thanks for this diff, that was helpful to get started on. I still want to cross-read the two actual patches and peek into the test patches, but generally this looks good. ::: browser-plugins.js @@ +28,2 @@ > PREF_SESSION_PERSIST_MINUTES: "plugin.sessionPermissionNow.intervalInMinutes", > PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays", These two are unused here. @@ +601,5 @@ > } > }, > > + reshowClickToPlayNotification: function () { > + // Ignore events that aren't from the main document. This doesn't match the following code. @@ +633,5 @@ > }, > > /** > + * Manually activate the plugins that would have been automatically > + * activated. Out of the original context this is not quite clear - can you clarify a bit? @@ +917,5 @@ > let isShowing = false; > > if (plugin) { > // If there's no plugin (an <object> or <embed> element), this call is > + // for a window-global plugin. I©n this case, there's no overlay to show. Copy'n'paste massacre? ;) @@ +939,5 @@ > + }); > + // Remove the notfication when the page is reloaded. > + doc.defaultView.top.addEventListener("unload", event => { > + this.hideNotificationBar("plugin-crashed"); > + }, false); Doesn't it make more sense to handle this kind of cleanup on the chrome side, in case of content crashes etc.? @@ +945,3 @@ > } > > + // TODO: Make this this.setupPluginOverlay? What's up with this?

Comment on attachment 8481500 [details] [diff] [review] Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=? Review of attachment 8481500 [details] [diff] [review]: ----------------------------------------------------------------- Moving the comments over here. ::: browser/modules/PluginContent.jsm @@ +25,5 @@ > +} > + > +PluginContent.prototype = { > + PREF_SESSION_PERSIST_MINUTES: "plugin.sessionPermissionNow.intervalInMinutes", > + PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays", These two are unused here. @@ +601,5 @@ > + } > + }, > + > + reshowClickToPlayNotification: function () { > + // Ignore events that aren't from the main document. This doesn't match the following code. @@ +633,5 @@ > + }, > + > + /** > + * Manually activate the plugins that would have been automatically > + * activated. Out of the original context this is not quite clear - can you clarify a bit? @@ +917,5 @@ > + let isShowing = false; > + > + if (plugin) { > + // If there's no plugin (an <object> or <embed> element), this call is > + // for a window-global plugin. I©n this case, there's no overlay to show. Copy'n'paste massacre? ;) @@ +939,5 @@ > + }); > + // Remove the notfication when the page is reloaded. > + doc.defaultView.top.addEventListener("unload", event => { > + this.hideNotificationBar("plugin-crashed"); > + }, false); Doesn't it make more sense to handle this kind of cleanup on the chrome side, in case of content crashes etc.? @@ +943,5 @@ > + }, false); > + } > + } > + > + // TODO: Make this this.setupPluginOverlay? What's up with this?

Comment on attachment 8485178 [details] [diff] [review] Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=? (In reply to Georg Fritzsche [:gfritzsche] from comment #55) > Comment on attachment 8481500 [details] [diff] [review] > Part 2: Introduce a new PluginContent.jsm that lets content scripts message > plugin events to the parent. r=? > > Review of attachment 8481500 [details] [diff] [review]: > ----------------------------------------------------------------- > > Moving the comments over here. > > ::: browser/modules/PluginContent.jsm > @@ +25,5 @@ > > +} > > + > > +PluginContent.prototype = { > > + PREF_SESSION_PERSIST_MINUTES: "plugin.sessionPermissionNow.intervalInMinutes", > > + PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays", > > These two are unused here. > Removed. > @@ +601,5 @@ > > + } > > + }, > > + > > + reshowClickToPlayNotification: function () { > > + // Ignore events that aren't from the main document. > > This doesn't match the following code. > Not sure what you mean. What doesn't match? The style? If so, which parts, exactly? It's going over my head. :) > @@ +633,5 @@ > > + }, > > + > > + /** > > + * Manually activate the plugins that would have been automatically > > + * activated. > > Out of the original context this is not quite clear - can you clarify a bit? > Did my best to clear that up. > @@ +917,5 @@ > > + let isShowing = false; > > + > > + if (plugin) { > > + // If there's no plugin (an <object> or <embed> element), this call is > > + // for a window-global plugin. I©n this case, there's no overlay to show. > > Copy'n'paste massacre? ;) > Fixed! > @@ +939,5 @@ > > + }); > > + // Remove the notfication when the page is reloaded. > > + doc.defaultView.top.addEventListener("unload", event => { > > + this.hideNotificationBar("plugin-crashed"); > > + }, false); > > Doesn't it make more sense to handle this kind of cleanup on the chrome > side, in case of content crashes etc.? > Yes, perhaps - although I think that might be better as a follow-up. Does that work with you? > @@ +943,5 @@ > > + }, false); > > + } > > + } > > + > > + // TODO: Make this this.setupPluginOverlay? > > What's up with this? Removed the TODO _ the setupPluginOverlay didn't go quietly into a function hanging off of PluginContent, since it expected much in its closure.

Comment on attachment 8485180 [details] [diff] [review] Make mozapps CTP tests work again. r=? A small review, I swear! :D

Comment on attachment 8485180 [details] [diff] [review] Make mozapps CTP tests work again. r=? Review of attachment 8485180 [details] [diff] [review]: ----------------------------------------------------------------- *mumble mumble ... whitepace differences... reviewboard...*

Side note: i will probably get to the reviews here again in the next two days. (In reply to Mike Conley (:mconley) - Needinfo me! from comment #60) > > @@ +601,5 @@ > > > + } > > > + }, > > > + > > > + reshowClickToPlayNotification: function () { > > > + // Ignore events that aren't from the main document. > > > > This doesn't match the following code. > > > > Not sure what you mean. What doesn't match? The style? If so, which parts, > exactly? It's going over my head. :) There are two places that use the same comment - i think this one is just a left-over, as the following code doesn't try to ignore any events. > > @@ +939,5 @@ > > > + }); > > > + // Remove the notfication when the page is reloaded. > > > + doc.defaultView.top.addEventListener("unload", event => { > > > + this.hideNotificationBar("plugin-crashed"); > > > + }, false); > > > > Doesn't it make more sense to handle this kind of cleanup on the chrome > > side, in case of content crashes etc.? > > > > Yes, perhaps - although I think that might be better as a follow-up. Does > that work with you? Sounds good.

Gentle review ping?

Comment on attachment 8485178 [details] [diff] [review] Part 2: Introduce a new PluginContent.jsm that lets content scripts message plugin events to the parent. r=? Review of attachment 8485178 [details] [diff] [review]: ----------------------------------------------------------------- Side-note: Nice to see things getting cleaned up here in the process! ::: browser/modules/PluginContent.jsm @@ +598,5 @@ > + } > + }, > + > + reshowClickToPlayNotification: function () { > + // Ignore events that aren't from the main document. This seems to be copy/paste left-over. @@ +630,5 @@ > + }, > + > + /** > + * Activate the plugins that the user has specified that they > + * wanted activated. Nitpicking here: Just "Activate the plugins that the user has specified" is enough?

Comment on attachment 8485177 [details] [diff] [review] Part 1: Update browser-plugins to use message passing for plugin events. r=? Review of attachment 8485177 [details] [diff] [review]: ----------------------------------------------------------------- Phew, i don't see any real issues here. ::: browser/base/content/browser-plugins.js @@ +10,5 @@ > PREF_PERSISTENT_DAYS: "plugin.persistentPermissionAlways.intervalInDays", > > + init: function () { > + const mm = window.messageManager; > + mm.addMessageListener("PluginContent:ShowClickToPlayNotification", this); Looking at this pattern in this file again, can't we just put the message names in a const array and avoid explicitly specifying them twice in init() and uninit()? @@ +77,5 @@ > + case "openHelpPage": > + case "openPluginUpdatePage": > + this[msg.data.name].apply(this); > + break; > + } Add a warning here for unexpected names?

Comment on attachment 8481507 [details] [diff] [review] Part 3: Misc. browser fixups to deal with the new browser-plugins.js Whoops, I never set a reviewer on this. It's short, I promise. :)

I'll note that I _might_ be seeing a new random orange showing up from this patch in browser_CTP_data_urls.js on Linux debug: https://tbpl.mozilla.org/?tree=Try&rev=32fff51c2bf9 :/

Comment on attachment 8488056 [details] [diff] [review] Fix an intermittent orange in browser_pluginnotification.js. r=? Super tiny patch.

Moving old M2 P1 bugs to M3.

Comment on attachment 8481507 [details] [diff] [review] Part 3: Misc. browser fixups to deal with the new browser-plugins.js Review of attachment 8481507 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/content.js @@ +492,5 @@ > > ContentLinkHandler.init(this); > > +// TODO: Load this lazily so the JSM is run only if a relevant event/message fires. > +let pluginContent = new PluginContent(global); Please file a follow-up here.

Comment on attachment 8485203 [details] [diff] [review] Make browser plugin tests work again. r=? Review of attachment 8485203 [details] [diff] [review]: ----------------------------------------------------------------- oh these tests would be so much tidier if they were Tasks ::: browser/base/content/test/plugins/browser_CTP_crashreporting.js @@ +97,5 @@ > > let tab = gBrowser.loadOneTab("about:blank", { inBackground: false }); > gTestBrowser = gBrowser.getBrowserForTab(tab); > + let mm = gTestBrowser.messageManager; > + mm.loadFrameScript("data:,(" + frameScript.toString() + ")();", true); true should probably be false, to not let this framescript load for other browsers.. although I think that it doesn't make a difference if you're using the mm from this specific browser. but for good measure, switch it to false

Thank you so much for the reviews everybody! I've addressed all of the feedback, and pushed: remote: https://hg.mozilla.org/integration/fx-team/rev/5747034f2bcc I'm filing the follow-ups that were requested now, and will link to them shortly.

Backed out for mochitest-bc permafail. https://hg.mozilla.org/integration/fx-team/rev/cba8fb1e0e37 https://tbpl.mozilla.org/php/getParsedLog.php?id=48113491&tree=Fx-Team

It's actually a random orange it seems (and one I thought I'd fixed) - I just happened to have a particularly unlucky roll of the dice. I'm able to reproduce the failure running the test with --run-until-failure. I'm starting investigations.

My current theory here is that the click-to-play notification is sometimes not updated to deal with new content before the test grabs the notification and clicks a button in it. So I think we need to make sure each test makes sure the notification gets dismissed by browsing to a different URI before proceeding.

This was backed out (see comment 77)

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #81) > This was backed out (see comment 77) Sorry about that.

Backout was merged over in https://hg.mozilla.org/mozilla-central/rev/cba8fb1e0e37 tonight. Sorry for the confusion.

The test failure that I'm trying to fix seems to be centered on a race involving the click to play PopupNotifications - specifically, it looks like Test 24a's PopupNotification can sometimes set permission to "allow now" for the plugin on the system principal, instead of the proper principal which has the origin http://127.0.0.1:8000. Narrowing in on the race now.

Comment on attachment 8490787 [details] [diff] [review] Put a stake through the heart of browser_pluginnotifications.js orange So I think I figured out the orange. Test 23 in browser_pluginnotification.js works by testing what happens when a plugin that has been activated changes types. We should deactivate the plugin in that case. The last part of that test ensures that the plugin is not activated, and then we move on to Test 24a. Here's the rub though - once PluginContent.jsm detects that the plugin has changed types and been deactivated, it sends a message up to the parent saying, "We should probably show a Click-to-play notification for this thing", and passes along the principal of the page it's on (which is a chrome URI for Test 23). The principal for a chrome URI is the System Principal. Test 24a however is pointed at a local HTTP server, and has a different principal. By the time that test starts running, the message that the framescript sent to display the Click-to-play notification gets received and shown, even though we're no longer viewing the page it was meant for. That's why activating the plugin there sets the permission for the wrong principal - it sets it for the system principal instead of the local HTTP server principal. This patch adds a check at the top of showClickToPlay notification to ensure that the principal that is passed from the framescript matches the principal for the browser content, and bails if they're not the same. With this patch, I've run the tests with --run-until-failure several times and not hit the failure once.

There was some rebasing needed which I'm not sure if I did it correctly, so I preferred to send it to try first: https://tbpl.mozilla.org/?tree=Try&rev=26e0c1f7f812 If it comes out green I'll land this for mconley as he is on TRIBE :)

That push went total orange due to a small rebase snafu - re-pushed to try: remote: https://tbpl.mozilla.org/?tree=Try&rev=db6db827b59a

Not all the builds are in but I'm pretty convinced. remote: https://hg.mozilla.org/integration/fx-team/rev/c07c6f96d613

So, lovely, things are coming in green, and it looks like my patch fixes the frequent orange for click-to-play on non-e10s! That's the good news. The bad news is because I spent so much time focusing on getting the test to go green, I didn't check to see if click-to-play in e10s (which this patch is all about) actually still works. My fix actually breaks click-to-play for e10s mode because in that mode, we attempt to do a comparison of a nsIPrincipal with an nsIPrincipal CPOW, which is a no go. However, because all of this foundational work is both large, reviewed, doesn't break non-e10s click-to-play and a dependency for some PFS clean-up work, I'm opting to keep the patch landed, but work on a follow-up patch to fix the nsIPrincipal stuff. So stay tuned for that.

Mike, I'm just curious: do you know how far we are from having click-to-play tests working in e10s? Even having just a few running would be pretty cool.

(In reply to Bill McCloskey (:billm) from comment #92) > Mike, I'm just curious: do you know how far we are from having click-to-play > tests working in e10s? Even having just a few running would be pretty cool. Having glanced at quite a few e10s tests, I think the main barrier is that the tests directly manipulate plugin elements (like activating them) by using contentDocument. We could go for a short-term win by using contentDocumentAsCPOW and then updating each test (I'd love to get these all updated to use add_task, to avoid the callback hell). I can test the plausibility of that theory tomorrow.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #93) > Having glanced at quite a few e10s tests, I think the main barrier is that > the tests directly manipulate plugin elements (like activating them) by > using contentDocument. We could go for a short-term win by using > contentDocumentAsCPOW and then updating each test (I'd love to get these all > updated to use add_task, to avoid the callback hell). browser-chrome tests are run in the context of an add-on. Therefore, all the add-on shims are in force. So contentDocument will just work inside test code. The same is true for the other usual ways of accessing content.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #91) > The bad news is because I spent so much time focusing on getting the test to > go green, I didn't check to see if click-to-play in e10s (which this patch > is all about) actually still works. My fix actually breaks click-to-play for > e10s mode because in that mode, we attempt to do a comparison of a > nsIPrincipal with an nsIPrincipal CPOW, which is a no go. Hm, so this means we're also missing some test-coverage if this patch went green?

(In reply to Georg Fritzsche [:gfritzsche] from comment #96) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #91) > > The bad news is because I spent so much time focusing on getting the test to > > go green, I didn't check to see if click-to-play in e10s (which this patch > > is all about) actually still works. My fix actually breaks click-to-play for > > e10s mode because in that mode, we attempt to do a comparison of a > > nsIPrincipal with an nsIPrincipal CPOW, which is a no go. > > Hm, so this means we're also missing some test-coverage if this patch went > green? Indeed - almost none of the mochitest-browser tests are being run with e10s enabled. We're working on it (see bug 984139).

With bug 1069567 fixed, this CTP in e10s windows should be working now.

Verified on latest Nightly 35.0a1 (buildID: 20140921030208): - on Windows 7 64bit and Mac OSX 10.9.5 - it works fine - on Ubuntu 32bit: the click-to-play overlay is correctly displayed, but after that, even if I select to allow a plugin, the video/game doesn't play - I filled bug 1070981 for this.

Bugnotes: http://people.mozilla.org/~mconley2/bugnotes/bug-899347.html