Closed Bug 812562 Opened 12 years ago Closed 12 years ago

click-to-play blocklisted plugins: reshow urlbar notification as with normal click-to-play

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla20

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file, 2 obsolete files)

When navigating back, click-to-play events aren't re-fired*, so we manually check and see if we need to re-show the popup notification (albeit in the dismissed state, so we just see the icon in the urlbar). However, this code assumes if "plugins.click_to_play" is false, it doesn't need to do anything. With the advent of click-to-play blocklisting, this is no longer the case.

*This might change with bug 800018, but we'll cross that bridge when we come to it.
Attached patch patch (obsolete) — Splinter Review
Jared, if you could have a look at this, that'd be great.
As a side-note, I refactored browser/.../test/browser_pluginnotification.js a bit with the idea of stopping that file from growing so large.
Attachment #682565 - Flags: review?(jaws)
Comment on attachment 682565 [details] [diff] [review]
patch

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

r=me with the explicit plugins.click_to_play=false set in the test.

::: browser/base/content/test/browser_bug812562.js
@@ +42,5 @@
> +function prepareTest(nextTest, url) {
> +  gNextTest = nextTest;
> +  gTestBrowser.contentWindow.location = url;
> +}
> +// Tests that the going back will reshow the notification for click-to-play 

nit: trailing whitespace.

@@ +44,5 @@
> +  gTestBrowser.contentWindow.location = url;
> +}
> +// Tests that the going back will reshow the notification for click-to-play 
> +// blocklisted plugins (part 1/4)
> +function test1a() {

This test should explicitly set plugins.click_to_play=false since that is what it is expecting.
Attachment #682565 - Flags: review?(jaws) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Fixed issues. Carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=8493a4c41628
Attachment #682565 - Attachment is obsolete: true
Attachment #682664 - Flags: review+
Attached patch patch v3Splinter Review
That had some issues, so I actually ended up getting rid of that fake blocklist business and replacing it with using the actual blocklist. I'd appreciate another look - thanks!

Try run: https://tbpl.mozilla.org/?tree=Try&rev=90fb28428f94
Attachment #682664 - Attachment is obsolete: true
Attachment #683816 - Flags: review?(jaws)
Attachment #683816 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/ff0511be54a6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 815954
Depends on: 816059
Can you please tell me how to test this fix?
This should work on windows:
1. install an old version of flash (<10.2.9999)
2. replace "addons.mozilla.org" with "addons-dev.allizom.org" in the pref extensions.blocklist.url
3. evaluate `Components.classes["@mozilla.org/extensions/blocklist;1"].getService(Components.interfaces.nsITimerCallback).notify(null);' in the error console
4. visit a page with flash on it (verify that it's ctp-blocked and there is a urlbar notification)
5. navigate to a page without flash (in the same tab)
6. go back (verify that the urlbar notification shows up again)
Thanks David for explaining. But:

(In reply to David Keeler from comment #8)
> This should work on windows:
> 1. install an old version of flash (<10.2.9999)
why applies only to versions <10.2 ?

> 2. replace "addons.mozilla.org" with "addons-dev.allizom.org" in the pref
> extensions.blocklist.url
shouldn't be the same on production?

> 4. visit a page with flash on it (verify that it's ctp-blocked and there is
> a urlbar notification)
what notification exactly? Are you talking about that old info-bar saying the plugin is outdated? As far as I know it was removed after FF 16. I'm only seeing the CTP specific doorhanger near the location bar.

> 5. navigate to a page without flash (in the same tab)
> 6. go back (verify that the urlbar notification shows up again)
I don't see any differences before and after the fix.
Sorry - I was just trying to be specific about how to get a plugin to be click-to-play blocklisted. Let me try again:

1. Get a plugin to be click-to-play blocklisted
2. Visit a page with that plugin (verify the CTP-specific doorhanger is in the urlbar)
3. Navigate to a page without that plugin
4. Go back (verify that the CTP-specific doorhanger shows up again - before this was fixed, if the plugin was not regular click-to-play, it wouldn't show up)
Works fine on latest nightly, but I'm also not able to reproduce the issue before the fix (tested on Nightly 2012-11-17, Nightly 2012-11-26). A test url would help, if not much of a trouble.
Any youtube video should do the trick (if flash is ctp-blocked). Then for the page without a plugin, I've just been using www.google.com.
I'm still not able to reproduce the issue on the affected builds. Anyway, I don't think this makes any difference since everything looks ok after the fix.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.