Closed
Bug 841350
Opened 12 years ago
Closed 12 years ago
Drag and drop a CTP tab in a new window makes the CTP icon disappear
Categories
(Core Graveyard :: Plug-ins, defect, P3)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla22
People
(Reporter: pauly, Assigned: keeler)
References
(Blocks 1 open bug)
Details
(Whiteboard: [CtPDefault:P3])
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
Gavin
:
review+
keeler
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
1. Enable CTP
2. Open 2 youtube videos in 2 tabs
3. Detach a tab to create a new FF window (or drag and drop the tab into an existing window)
AR: The CTP icon is gone after the drag & drop.
Reproducible on FF 19b6, 20.0a2 (2013-02-13), 21.0a1 (2013-02-13).
Not a regression, reproducible on FF 16.0.2, 17.0.1.
Updated•12 years ago
|
Priority: -- → P3
Whiteboard: [CtPDefault:P3]
Reporter | ||
Comment 1•12 years ago
|
||
May be related to bug 820448
Priority: P3 → --
Whiteboard: [CtPDefault:P3]
Reporter | ||
Updated•12 years ago
|
Priority: -- → P3
Whiteboard: [CtPDefault:P3]
Assignee | ||
Comment 2•12 years ago
|
||
The code that moves a tab from one browser to another doesn't take popup notifications into account. One option would be to re-architect popup notifications to be more tab-moving friendly. Another option would be to reshow any notifications we care about upon a tab being moved, as I've done here.
Attachment #714111 -
Flags: feedback?(jAwS)
Updated•12 years ago
|
Assignee: nobody → dkeeler
Comment 3•12 years ago
|
||
Comment on attachment 714111 [details] [diff] [review]
possible fix
Review of attachment 714111 [details] [diff] [review]:
-----------------------------------------------------------------
Tim, what do you think about the TabSwappedBrowsers event?
::: browser/base/content/browser-plugins.js
@@ +505,5 @@
> }, true);
> },
>
> + reshowClickToPlayNotification: function PH_reshowClickToPlayNotification(aBrowser) {
> + let browser = aBrowser ? aBrowser : gBrowser.selectedBrowser;
let browser = aBrowser || gBrowser.selectedBrowser;
@@ +513,5 @@
>
> // returns true if there is a plugin on this page that needs activation
> // and isn't in the "except these" list
> + _pluginNeedsActivationExceptThese: function PH_pluginNeedsActivationExceptThese(aExceptThese, aBrowser) {
> + let browser = aBrowser ? aBrowser : gBrowser.selectedBrowser;
let browser = aBrowser || gBrowser.selectedBrowser;
Attachment #714111 -
Flags: feedback?(ttaubert)
Attachment #714111 -
Flags: feedback?(jAwS)
Attachment #714111 -
Flags: feedback+
Comment 4•12 years ago
|
||
Comment on attachment 714111 [details] [diff] [review]
possible fix
Review of attachment 714111 [details] [diff] [review]:
-----------------------------------------------------------------
Moving a tab to a different window by using gBrowser.swapBrowsersAndCloseOther() fires pageshow events so we should actually have the call to reshowClickToPlayNotification() for free.
The right fix would be to register the 'pageshow' handler:
> gBrowser.addEventListener("pageshow", function(event) {
> // Filter out events that are not about the document load we are interested in
> if (content && event.target == content.document)
> setTimeout(pageShowEventHandlers, 0, event);
> }, true);
before we call swapBrowsersAndCloseOther() in the browser's delayed startup. So we only need to move the code above a couple of lines up.
Attachment #714111 -
Flags: feedback?(ttaubert) → feedback-
Assignee | ||
Comment 5•12 years ago
|
||
Well, I got this to work, but I had to relax the restriction that event.target == content.document. For some reason, when moving a tab from its own window to an existing window, event.target would never equal content.document. I tried to figure out why, but to no avail. Tim - is this okay?
Attachment #714111 -
Attachment is obsolete: true
Attachment #718081 -
Flags: feedback?(ttaubert)
Comment 6•12 years ago
|
||
So, for some reason, (event.target == content.document) is true if you check it on the next tick like here:
> gBrowser.addEventListener("pageshow", function(event) {
> setTimeout(function () {
> if (content && event.target == content.document)
> setTimeout(pageShowEventHandlers, 0, event);
> }, 0);
> }, true);
That's not a solution of course and I assume this is caused by docShell swapping magic and may be a platform bug.
Comment 7•12 years ago
|
||
In particular, content.document seems to change.
Comment 8•12 years ago
|
||
The problem is that the XULWindow::mPrimaryContentShell isn't up-to-date when the pageshow is fired. This field is updated here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#872
... but updateCurrentBrowser() is called *after* browser.swapDocShells() - which is what fires pageshow events. So... an easy solution would be to move the (content.document == event.target) check to the beginning of pageShowEventHandlers(). After setTimeout() (i.e. on the next tick) everything is as it should be.
Comment 9•12 years ago
|
||
Comment on attachment 718081 [details] [diff] [review]
patch
Review of attachment 718081 [details] [diff] [review]:
-----------------------------------------------------------------
Please take a look at comment #8. If you also add a nice comment why that check is there and that setTimeout() is important we should be fine.
Attachment #718081 -
Flags: feedback?(ttaubert)
Assignee | ||
Comment 10•12 years ago
|
||
Thanks for your help on this - I think it's ready for an actual review.
Attachment #718081 -
Attachment is obsolete: true
Attachment #719164 -
Flags: review?(ttaubert)
Comment 11•12 years ago
|
||
Comment on attachment 719164 [details] [diff] [review]
patch v2
Review of attachment 719164 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! r=me with the comment fixed.
::: browser/base/content/browser.js
@@ +1342,5 @@
>
> var isLoadingBlank = isBlankPageURL(uriToLoad);
>
> + gBrowser.addEventListener("pageshow", function(event) {
> + // The XULWindow::mPrimaryContentShell isn't up-to-date when pageshow
Please mention that this is only relevant when the pageshow event was fired by a swapFrameLoaders() call, i.e. when we swap the docShells of two tabs.
Attachment #719164 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Thanks! Carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=43c4a343d0f2
Attachment #719164 -
Attachment is obsolete: true
Attachment #719201 -
Flags: review+
Assignee | ||
Comment 13•12 years ago
|
||
Had to fix the test, but nothing else changed, so carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=789f706e3ee6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb9e70a38b2
Attachment #719201 -
Attachment is obsolete: true
Attachment #719705 -
Flags: review+
Comment 14•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 15•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #8)
> So... an easy solution would be to move
> the (content.document == event.target) check to the beginning of
> pageShowEventHandlers(). After setTimeout() (i.e. on the next tick)
> everything is as it should be.
This workaround likely caused bug 847070. Seems like we should fix the underlying event ordering issue rather than trying to work around it.
Assignee | ||
Comment 16•12 years ago
|
||
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•12 years ago
|
||
Comment 18•12 years ago
|
||
Ok, let's fix this for real. I investigated this a little more and think I found the actual issue here.
1) When dragging a tab off the strip into a new window, the <tab> element does not have the type="content-primary" attribute that selected tabs should have. That's easy to add for the first default tab in browser.xul.
2) When dragging a tab to another (already existing) window a new tab is created and the docShells are swapped. However, the newly created tab is not selected when the swapping starts so we must ensure that it is and just call updateCurrentBrowser() afterwards again.
The actual fix of registering the pageshow listener earlier is of course still needed.
Comment 19•12 years ago
|
||
Sorry for stealing this, David. I thought it might be easier to just submit the patch I wrote while looking for a new solution instead of explicitly writing down what needs to be done :) It'd be great if you can take a look at this and confirm that this fixes your problem!
Attachment #721302 -
Flags: review?(gavin.sharp)
Attachment #721302 -
Flags: feedback?(dkeeler)
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 721302 [details] [diff] [review]
patch v3
Review of attachment 721302 [details] [diff] [review]:
-----------------------------------------------------------------
Works for me! Thanks for taking this on. It would have taken me a while to figure out how to fix this properly. I'll upload a patch with just the test from the previous patch.
Attachment #721302 -
Flags: feedback?(dkeeler) → feedback+
Assignee | ||
Comment 21•12 years ago
|
||
This is just the test that was in patch v2.1. Carrying over r+.
Attachment #719705 -
Attachment is obsolete: true
Attachment #721357 -
Flags: review+
Comment 22•12 years ago
|
||
Comment on attachment 721302 [details] [diff] [review]
patch v3
Review of attachment 721302 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/browser.xul
@@ +909,5 @@
> flex="1"
> setfocus="false"
> tooltip="tabbrowser-tab-tooltip"
> stopwatchid="FX_TAB_CLICK_MS">
> + <tab class="tabbrowser-tab" selected="true" fadein="true" type="content-primary"/>
(In reply to Tim Taubert [:ttaubert] from comment #18)
> 1) When dragging a tab off the strip into a new window, the <tab> element
> does not have the type="content-primary" attribute that selected tabs should
> have. That's easy to add for the first default tab in browser.xul.
As I understand it, we don't set type="content-primary" on any <tab>. We only set it on <browser> elements. This change looks bogus to me.
Comment 23•12 years ago
|
||
Comment on attachment 721302 [details] [diff] [review]
patch v3
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>+ gBrowser.addEventListener("pageshow", function(event) {
Worth adding a comment that this needs to be done before the swapBrowsersAndCloseOther call below.
Attachment #721302 -
Flags: review?(gavin.sharp) → review+
Comment 24•12 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #22)
> As I understand it, we don't set type="content-primary" on any <tab>. We
> only set it on <browser> elements. This change looks bogus to me.
Thanks for catching this, it looks indeed bogus and unnecessary. The patch still works without it, not sure why it ended up in this patch.
Comment 25•12 years ago
|
||
This seems to reliably break browser_CTPScriptPlugin.js:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTPScriptPlugin.js | waited too long for popup notification to show (plugin_test_scriptedPopup3.html)
https://tbpl.mozilla.org/?tree=Try&rev=77ee2dfc7f7d
Assignee | ||
Comment 26•12 years ago
|
||
It turns out the test wasn't quite doing the right thing.
I've used this pattern elsewhere, so it might be worth going back and fixing those too...
Attachment #725029 -
Flags: review?(ttaubert)
Comment 27•12 years ago
|
||
Comment on attachment 725029 [details] [diff] [review]
fix browser_CTPScriptPlugin.js
Review of attachment 725029 [details] [diff] [review]:
-----------------------------------------------------------------
Ah, thanks for the quick fix! It probably makes sense to fix the other tests, too (in a follow-up). I'll push it all to try again.
Attachment #725029 -
Flags: review?(ttaubert) → review+
Comment 28•12 years ago
|
||
Looking good.
https://tbpl.mozilla.org/?tree=Try&rev=c13b36e2a1bc
Comment 29•12 years ago
|
||
Comment 30•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 31•12 years ago
|
||
Error: TypeError: this.updateCurrentBrowser is not a function
Source file: chrome://browser/content/tabbrowser.xml
Line: 3855
- this.updateCurrentBrowser(true);
+ this.tabbrowser.updateCurrentBrowser(true);
Comment 32•12 years ago
|
||
Oops :/ Thanks for noticing. I'll fix it right away.
Comment 33•12 years ago
|
||
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 34•12 years ago
|
||
Comment 35•12 years ago
|
||
Hrm, no tests failing for that one? We should fix that!
Reporter | ||
Comment 36•12 years ago
|
||
Verified fixed in nightly 22.0a1 (2013-03-24)
Status: RESOLVED → VERIFIED
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•