If you open a flash video on a tab and drag it to another window, the video is reset. It would be nice if the plugin state was kept when dragging a tab (Opera does this on Linux). (by the way, dragging a <video> does not reset its state)

Unfortunately, plugins use native widgets, and those can't be reparented on some OSes (Windows? I don't recall), so we can't do this in general. We might be able to special-case things here on an OS-by-OS basis, but it'll take some work, icky ifdefs, and we have bigger fish to fry first (like preserving all the non-plugin layout, say). This certainly won't happen until after view/widget elimination is done.

widgets can be reparented on Linux and Windows. Dunno if plugins can be reparented on Mac.

(In reply to comment #2) > widgets can be reparented on Linux and Windows. Dunno if plugins can be > reparented on Mac. Safari manages to maintain at least youtube state when dragging to a new tab, so I suspect so.

Also, Chrome and Safari both maintain youtube on windows.

(In reply to comment #2) > widgets can be reparented on Linux and Windows. Dunno if plugins can be > reparented on Mac. Plugins don't really have widgets on Mac so there isn't a problem.

The key thing here is that in the new plugin world we have to make sure that the plugin state is owned by the element, not the frame. That includes its native widget, if there is one. When we create a frame for the plugin (nsPluginFrame, say), that native widget would be reparented to the correct place. When there's no frame the widget will have to be parented to a hidden window or the toplevel window for the document (and hidden), or something like that. Although if we get to compositor first, then we can just have the widget always belong to the toplevel window since there won't be any other widgets to parent it to.

Well, the plugin-in-content thing (covered by other bugs) doesn't seem to be strictly needed here, since we do want d&d to preserve the frame tree long-term. We just didn't do it as a first cut because it introduced so much complexity.

Why do we want d&d to preserve the frame tree?

The main reasons would be to avoid the cost of the frame teardown and reconstruct and to not lose scroll positions. We might be able to do the latter with frame state restoration, but I'm not sure it actually works very well for anything other than the root scrollframe...

We can attach the scroll position to the content element as a property. Is the cost of frame teardown and reconstruct worth the complexity?

Right now, no, which is why we do it that way. If views and most subwidgets went away, so that the only complexity were to be reparenting of plug-in widgets, which would be necessary even if we don't keep the frame tree, then seems like it would be. Yes, that's a big change. So's moving plug-ins into content...

OK

I'm getting "File does not begin with '%PDF-'" when I drag a tab displaying a pdf off to it's own window. Is this related

Very likely, yes, if the temp file gets deleted when the plugin instance is destroyed.

what about preserving plugin state also for session restore? is this desirable?

It may be desirable, but it's definitely a different bug than this one, and a much more challenging one: it requires cooperation from plugins to serialize and deserialize their state correctly, including references to DOM objects that will have new addresses in the new process.

Is there any chance of getting this for Firefox 4? I would imagine most of the people who would know how to do this are busy with more important stuff :-(

This may be needed to support app-tabs, where a single app-tab can be visible in multiple browser windows. We would implement that by moving the real tab to the focused window and replacing the old tab with a fake screenshot.

Is that really the behaviour that we want? It seems like we should grey it out or something, at the least, since otherwise it won't be clear which one is most up to date if they're both visible. (Which window has focus is not always very obvious.)

We can keep the fake snapshot up to date (apart from windowed plugins). Or we could keep it static and gray it out. That's up to the UI folks. We still need to fix this bug.

(In reply to comment #27) > It seems like we should grey it out Yea, that's the current thinking. This bug doesn't affect doing that though.

Actually it seems to me swapping the frame trees might be a better approach for tab dragging and app-tabs. Post bug 130078, it's probably quite easy, and it would definitely be faster. Boris, is there a bug for that?

Blocking final? This bug kinda sets back tear-off tabs a bit.

I don't see a chance of this landing until electrolysis comes to Firefox. I'm assuming that's how the rest of the browsers do it. Does Opera have process separation?

This has almost nothing to do with e10s.

(In reply to comment #33) > I don't see a chance of this landing until electrolysis comes to Firefox. > I'm assuming that's how the rest of the browsers do it. Does Opera have process > separation? Don't make assumptions about things that you don't understand. Bugzilla is a bug tracker not a forum. Making these types of comments that also don't help developers fix the bug is putting yourself on the track to getting banned.

While this would be nice to fix, I think we have bigger fish to fry for 2.0...

We need this for app tabs (bug 551849), which is a major planned feature for Firefox 4, and it would be highly desired for tab multi-selection (bug 566510). Also, roc noted in comment #30 that after fixing bug 130078, which is blocking betaN, fixing this bug will be easy.

I think we should swap the frame trees for FF4.

Per the platform meeting yesterday, this needs to block. roc, can you assign?

Mats, can you take this? This needs to be top priority. The idea is to make nsSubdocumentFrame::Begin/EndSwapDocShells and the related code that actually swaps docshells swap the contained frame tree from one viewer to the other. Root views and plugin widgets will have to be reparented. But it seems to me it shouldn't be too hard.

If we swap the frame trees, does this still depend on bug 337801?

No.

I looked briefly at the code - preventing the presentation stuff from being destroyed seems fairly easy; I don't understand how to fix the root view/widget though, it has a null GetParent(). Where should it be hooked in?

I think we should assume bug 130078 has landed and fix this on top of that.

I have a patch that works smoothly on Linux. (on top of bug 130078.)

This patch should be applied on top of the bug 130078 patches: "hookup-vms", "hookup-vms-2" and "imp-remove-widget". I've tested it locally on Linux64, OSX and Windows XP, both IPP and OOPP (not all combinations though). TryServer builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mpalmgren@mozilla.com-c598140a3ac9/ Known problem: Print Preview is broken, it appears to be a regression of bug 130078 (handled in bug 584193).

Removing dependency as per comment 42; we've changed strategies in this bug such that we no longer depend on bug 337801

+ FrameProperties props = aFrame->Properties(); + PRBool found; + void* prop = props.Remove(DisplayItemDataProperty(), &found); + if (found) { + InternalDestroyDisplayItemData(aFrame, prop, PR_TRUE); + } This can just be aFrame->Properties().Delete(DisplayItemDataProperty()). Why is it necessary to hide the BaseWindows and then show them again? + mozilla::FrameLayerBuilder::DestroyDisplayItemDataFor(aFrame); Just use "using namespace mozilla;" + if (nsGkAtoms::placeholderFrame == child->GetType()) { + nsIFrame* outOfFlowFrame = + nsPlaceholderFrame::GetRealFrameForPlaceholder(child); + do { + DestroyDisplayItemDataForFrames(outOfFlowFrame); + } while ((outOfFlowFrame = outOfFlowFrame->GetNextContinuation())); + } + else { + DestroyDisplayItemDataForFrames(child); + } I don't think you need the placeholderFrame case here. We'll find all the frames on some list. The rest looks great!

Actually, a few other thoughts: -- the resize to 0,0 shouldn't be necessary, I would have thought. If you found it necessary on GTK, maybe something should be changed in nsWindow::SetParent. -- what triggers a resize reflow when a docshell is swapped into a container with a different size? -- needs tests!

(In reply to comment #48) > This can just be aFrame->Properties().Delete(DisplayItemDataProperty()). Fixed. > Why is it necessary to hide the BaseWindows and then show them again? It's not, I removed it. What if DocumentViewerImpl::mPreviousViewer is non-null though, could that cause problems? > Just use "using namespace mozilla;" Fixed. > I don't think you need the placeholderFrame case here. We'll find all the > frames on some list. Right, when starting from the root frame that holds (otherwise not). Fixed. (In reply to comment #49) > Actually, a few other thoughts: > -- the resize to 0,0 shouldn't be necessary, I would have thought. You're right, it was just a bug in nsWindow::SetParent on GTK. > -- what triggers a resize reflow when a docshell is swapped into a container > with a different size? This does: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#818 > -- needs tests! Fixed.

TryServer builds with "Patch rev. 2": http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mpalmgren@mozilla.com-b75b6698a67f/

+ if (rootFrame) + ::DestroyDisplayItemDataForFrames(rootFrame); {} + if (doc) + ::BeginSwapDocShellsForDocument(doc, nsnull); {} + if (doc) + ::EndSwapDocShellsForDocument(doc, nsnull); {} > What if DocumentViewerImpl::mPreviousViewer is non-null though, could that > cause problems? Yes, but actually the problem is worse than that. The issue is that *every* viewer in the docshell, including bfcached documents, needs to have its presentation swapped. I think we need to enumerate all view children of the innerView of the subdocument frame.

Changes from last version: * made nsSubDocumentFrame process all the child views * fixed the nits * rewrote the test

Comment on attachment 471181 [details] [diff] [review] Part 1, rev. 3 The code looks good. I think you should modify your test so that tabs are swapped to another window. That way we exercise widget reparenting. I assume you tested that this works correctly with bfcached documents now? It would actually be good if you could add that to your test somehow. I think we should test that you can load document A in tab T, then navigate to document B, then swap T to another window, then go back in T to document A, and then dispatching an event to T using sendMouseEvent is received correctly in document A. (If the presentation was not hooked up correctly, then I expect A would not receive the event.)

In fact you should probably test event dispatch for every document that you swap to a new window.

Hmm, this patch might break accessibility. We probably need to at least flush the accessibility trees for the swapped presentations. How can we do that?

In general while plugin window HWND and DOM tree are preserved then nothing that should be changed comes to my mind. Though we can't test the patch since accessibility is broken on trunk (bug 591874).

Added a test that opens a tab in a new window (replaceTabWithWindow), then navigates back in that tab, then test synthesizeMouse clicking. Added a few click tests to the previous same-window tab swapping as well.

Fantastic! Let's get this in!

Ok, I'm in the landing queue. I investigated the a11y situation a bit. As we reparent the views we recursively do nsViewManager::ReparentWidgets which calls SetParent on child widgets. If there is a a11y node for the content document then those shouldn't be a problem (assuming they are descendants of that). For top-level widgets we might need to reparent the corresponding a11y node. (not sure where those are stored in the a11y tree) (BTW, on GTK2 it's also stored in nsWindow::mRootAccessible).

Remember that subdocuments don't have widgets now. nsViewManager::ReparentWidgets isn't actually going to do anything because it won't find any widgets. We should actually remove it!

Right, I was thinking of the widget for popups, combobox dropdown etc. we may need to deal with a11y nodes for those, but let's do so in bug 591874. Filed bug 593365 on removing nsViewManager::ReparentWidgets Landed the patch briefly, but I had to backout since the added testcase caused some other test later in the browser-chrome suite to fail. Investigating...

Did you forget to close your window? Hmm, I guess ReparentWidgets does still have to reparent widgets for popups. Sigh.

The first sign of a problem actually occurs before my test runs so there's something missing/wrong with the code. The error message is: TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_bug561623.js | Console message: [JavaScript Error: "FullZoom.onLocationChange is not a function" {file: "chrome://browser/content/browser.js" line: 8402}] FullZoom.onLocationChange is null, here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4413 and here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4573

There's an existing bug in nsWindow::SetParent on GTK2 - it doesn't add the reparented child widget to the new parent's child list. This doesn't fix the --browser-chrome failure though. I believe the cause of that is due to a recent regression on trunk which made the patch here fail. The symptom of that regression is "FullZoom.onLocationChange is not a function", which is filed as bug 593378. I'll add additional comments there. I have a WIP for reparenting the native widget for popups, just need to test it on more platforms.

(In reply to comment #65) > I have a WIP for reparenting the native widget for popups, > just need to test it on more platforms. Can you add that to your test?

nsThebesDeviceContext::mWidget is intialized from FindContainerView() when a document viewer initialized. We need to update it (or get rid of it). http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesDeviceContext.h#162

Comment on attachment 473812 [details] [diff] [review] Part 3, rev. 1 What problems did you find that this fixes? Are we ready to land now?

(In reply to comment #68) > Comment on attachment 473812 [details] [diff] [review] > Part 3, rev. 1 > > What problems did you find that this fixes? Moving a tab containing a combobox, then closing the original window destroys that mWidget. Clicking on the combobox crashed when it tried to get the screen size through the device context, in nsFormControlFrame::GetUsableScreenRect(). > Are we ready to land now? Nope, still working on native widget reparenting on GTK2, which was harder than I first thought. I'm hoping to have a patch tomorrow.

It turned out 'mInitialized' is DEBUG only. This version removes it and uses mScreenManager != nsnull to test if we are initialized. No other changes.

Add nsIWidget::ReparentNativeWidget() to handle reparenting of top-level widgets.

Add a test that creates child-widget popups (panel.openPopupAtScreen) and synthesize events on them, then detach the tab (close the original window) and finally run the tests again in the new window. Strictly only to trigger assertions or crashes.

The patch set still doesn't pass browser-chrome suite though, I'm working on that. I noted that there are some existing problems with autocomplete popups for form fill (bug 477797). Probably not related to the test failure though...

Ok, so I found another broken test (bug 596022) and eventually a bug in nsISessionStore::setWindowState() that affects closed windows that were created by replaceTabWithWindow(), such as my tests do here. Filed bug 596592. Here's a temporary workaround for my tests. With this workaround, the patch set pass unit tests without causing test failures. There's still a problem with fairly consistent chrome and browser-chrome leaks reported for the tryserver runs I've done so far. Clicking the "Analyze the leak" link, it says: leaked 10-12 DOMWINDOW(s). I have no clue even where to start on that one. Suggestions anyone?

Hmm, there's a recent leak on trunk as well, bug 596603...

Comment on attachment 475510 [details] [diff] [review] Part 6, rev. 1 The workaround in bug 596592 is better.

Doh. Fix the leak. Here's the intradiff: < + nsPresContext* pc = nsnull; < + dv->GetPresContext(&pc); --- > + nsCOMPtr<nsPresContext> pc; > + dv->GetPresContext(getter_AddRefs(pc));

Add an empty PuppetWidget::ReparentNativeWidget() method to make it compile. It will never be called AFAIU.

I've submitted the above patches + the various test fixes to the TryServer. Should be ready to land as soon as the test results come in.

http://hg.mozilla.org/mozilla-central/rev/8d3d6240f81e http://hg.mozilla.org/mozilla-central/rev/f0993738b599 http://hg.mozilla.org/mozilla-central/rev/587c3e3d0080 http://hg.mozilla.org/mozilla-central/rev/6bcfca597af9 http://hg.mozilla.org/mozilla-central/rev/81922822080f http://hg.mozilla.org/mozilla-central/rev/e3c9e026315f

filed bug 597979 : Preserve plugin state in session restore, per comments 21-22.