Last Comment Bug 756025 - Port |Bug 742753 - Click to Play should permit each element|
: Port |Bug 742753 - Click to Play should permit each element|
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.12
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-17 00:54 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2012-05-18 10:17 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.63 KB, patch)
2012-05-17 01:30 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Splinter Review
patch [Checkin: Comment 6] (2.39 KB, patch)
2012-05-18 03:07 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2012-05-17 00:54:55 PDT
Clicking a single plugin area in CTP mode should only enable that plugin instance and not all the others on the current page (activating all should still be possible using the doorhanger or notification bar).
Comment 1 Jens Hatlak (:InvisibleSmiley) 2012-05-17 01:30:22 PDT
Created attachment 624672 [details] [diff] [review]
patch

Test case: attachment 612565 [details]
Comment 2 neil@parkwaycc.co.uk 2012-05-17 02:32:02 PDT
Comment on attachment 624672 [details] [diff] [review]
patch

>+            var haveUnplayedPlugins = this.contentWindowUtils.plugins.some(function(plugin) {
>+              let objLoadingContent = plugin.QueryInterface(Components.interfaces.nsIObjectLoadingContent);
Nit: let at top-level function scope ;-) [but see below]

>+              return (plugin != aPlugin && !objLoadingContent.activated);
Nit: unnecessary ()s. [Also, confusing variable names or what?]

>+            var notification = PopupNotifications.getNotification("click-to-play-plugins",
>+                                                                  this.activeBrowser);
>+            if (notification && !haveUnplayedPlugins) {
>+              this.clickToPlayPromptShown = false;
>+              notification.remove();
>+            }
You need to call removePluginClickToPlayPrompt instead of manually removing the prompt, so it will work for both notificationboxes and doorhangers. (Of course you still need to check for haveUnplayedPlugins first.)

I wonder whether we should set the prompt shown variable to false.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2012-05-18 03:07:51 PDT
Created attachment 625049 [details] [diff] [review]
patch [Checkin: Comment 6]

> I wonder whether we should set the prompt shown variable to false.

I'm not sure what you meant - moving that to the remove method? For now I interpreted it as "maybe we don't want to set it to false in this case at all", reflected in the updated patch.
Comment 4 neil@parkwaycc.co.uk 2012-05-18 03:55:30 PDT
(In reply to Jens Hatlak from comment #3)
> > I wonder whether we should set the prompt shown variable to false.
> I'm not sure what you meant - moving that to the remove method?
Oversight on my part. Put it back wherever you like.
Comment 5 neil@parkwaycc.co.uk 2012-05-18 04:38:18 PDT
Comment on attachment 625049 [details] [diff] [review]
patch [Checkin: Comment 6]

[There seems to be a core bug here - I see entries for plugins that won't start (e.g. object/embed fallbacks generate two entries) but I eventually found a page that just had one embed and clicking it did remove the doorhanger.]
Comment 6 Jens Hatlak (:InvisibleSmiley) 2012-05-18 10:14:37 PDT
Comment on attachment 625049 [details] [diff] [review]
patch [Checkin: Comment 6]

http://hg.mozilla.org/comm-central/rev/f5b07be535e1
(with re-addition of this.clickToPlayPromptShown = false;)
Comment 7 Jens Hatlak (:InvisibleSmiley) 2012-05-18 10:17:05 PDT
(In reply to neil@parkwaycc.co.uk from comment #5)
> [There seems to be a core bug here - I see entries for plugins that won't
> start (e.g. object/embed fallbacks generate two entries) but I eventually
> found a page that just had one embed and clicking it did remove the
> doorhanger.]

I also found it strange that clicking all three plugin areas from the test case from comment 1 (which is actually from the base bug) did not remove the doorhanger, but at least it's consistent with FF. ;-) I will closely monitor all CTP-related bug fixes and see what happens there.

Note You need to log in before you can comment on or make changes to this bug.