Spun off from bug 570620. With the patches soon to be posted here, we replace platform widgets with fakes that work for painting with both fennec tilemanager and <browser remove> + cross-process layers. However, key events and focus are broken. I don't if/to what extent IME works on all fennec platforms, but it would be nice to get that working here too.

AFAIK, IME is pretty critical on N900 for example.

In the m.d.t.dom thread, Masayuki privately replied about IME with: ----- Each native IME event handler must do its jobs in same process because we cannot access to IME from another process. So, if a chrome process receives IME events, the chrome process's widget needs to handle the IME and to dispach Gecko events. (i.e., after that, the events should be redirected to the content process.) -----

Comment on attachment 460326 [details] [diff] [review] Implement a "puppet widget" stand-in for platform widgets, for content processes +nsresult +PuppetWidget::DispatchPaintEvent() +{ + NS_ABORT_IF_FALSE(!mDirtyRegion.IsEmpty(), "paint event logic messed up"); I'm playing with fennec-tile-less, loading google.com, and HW keys pressing aborting content process in this place.

(In reply to comment #5) > Comment on attachment 460326 [details] [diff] [review] > Implement a "puppet widget" stand-in for platform widgets, for content > processes > > +nsresult > +PuppetWidget::DispatchPaintEvent() > +{ > + NS_ABORT_IF_FALSE(!mDirtyRegion.IsEmpty(), "paint event logic messed up"); > > I'm playing with fennec-tile-less, loading google.com, and HW keys pressing > aborting content process in this place. That's very bad, but not enough info for me to debug. Can you get a widget paint/invalidate log? Set |user_pref("nglayout.debug.invalidate_dumping", true); user_pref("nglayout.debug.paint_dumping", true);|.

Comment on attachment 460325 [details] [diff] [review] Gut platform widgets from content processes If Show/Move are always going to have a 0/0 origin, could you just pass a width/height or nsPoint instead of the full rect? sr=me with that, or this way if it's somehow important to keep the full rect. We might need to deal with special cases for DOM coordinates like .clientWidth.

Updated to m-c tip.

Comment on attachment 460806 [details] [diff] [review] Gut platform widgets from content processes Oops, will carry over the sr+ pending comments on always rendering to <0, 0> in content processes.

Sneaking this in. Was useful for me.

+#ifdef MOZ_WIDGET_GTK2 + // sigh + gtk_init(NULL, NULL); +#endif + What's this about? + kWidgetCID, initDataPtr, nw, PR_TRUE, PR_FALSE, eContentTypeInherit, + (XRE_GetProcessType() == GeckoProcessType_Content) ? mParentWidget : nsnull); + mWindow->Create(IsContentProcess() ? aParentWidget : nsnull, + aNative, trect, ::HandleEvent, dx, nsnull, nsnull, aWidgetInitData); Why these changes? Having XRE_GetProcessType calls sprinkled throughout our code scares me a lot. Every time someone needs to use a Gecko subprocess for a new purpose, we have to add a new enum value and audit all those sites? - nsresult rv = CallCreateInstance(aClassIID, &mWindow); + nsresult rv; + if (IsContentProcess()) { + mWindow = nsIWidget::CreatePuppetWidget().get(); + rv = !mWindow ? NS_ERROR_NOT_AVAILABLE : NS_OK; + } else { + rv = CallCreateInstance(aClassIID, &mWindow); + } Seems like it might be better these days, if we have a parent widget, call a method on it to create a child, otherwise use CallCreateInstance. +using mozilla::layers::BasicShadowLayerManager; +using mozilla::layers::LayerManager; using namespace mozilla::layers? + nsCOMPtr<nsIWidget> widget = new mozilla::widget::PuppetWidget(); using namespace mozilla::widget?? explicit-::-itis is horrible IMHO +const uintptr_t PuppetWidget::kWindowHandle = 0xABC123bb;/*you and me girl*/ + aParent->GetNativeData(42) == reinterpret_cast<void*>(kWindowHandle)) && ??? Maybe we could get rid of all this if you take the suggestion above, we could make the standard create path go away for PuppetWidgets. + mSurface = gfxPlatform::GetPlatform() + ->CreateOffscreenSurface(gfxIntSize(1, 1), + gfxASurface::ImageFormatARGB32); Why do we need mSurface? Can't you have a null default target in setupLayerManager?

(In reply to comment #12) > +#ifdef MOZ_WIDGET_GTK2 > + // sigh > + gtk_init(NULL, NULL); > +#endif > + > > What's this about? > nsGTKToolkit assumes it has already been called (otherwise it crashes), and ContentChild::Init is unfortunately the most convenient place to call it. "// sigh" because I hoped we could do away with platform-specific widget code, but I had forgotten how much we depend on Gtk for rendering. Fixing that is way out of scope (obviously). > + kWidgetCID, initDataPtr, nw, PR_TRUE, PR_FALSE, eContentTypeInherit, > + (XRE_GetProcessType() == GeckoProcessType_Content) ? mParentWidget : > nsnull); > > + mWindow->Create(IsContentProcess() ? aParentWidget : nsnull, > + aNative, trect, ::HandleEvent, dx, nsnull, nsnull, > aWidgetInitData); > > Why these changes? > Part of the hack I discussed in bug 570620 comment 11. If we create static nsIWidget Create* functions and factory methods rather than going through XPCOM indirection, we could confine the hack to widget/. FWIW I wanted to change as little as possible to make content processes work, instead of reworking the way widgets are created, but the latter is definitely a better long-term solution. > Having XRE_GetProcessType calls sprinkled throughout our code scares me a lot. > Every time someone needs to use a Gecko subprocess for a new purpose, we have > to add a new enum value and audit all those sites? > I'm not quite sure what's scaring you; do you have more specific concerns? We do have an enum value per process type, and if something didn't work in the new subprocess type, it might trace back to one of these special cases, just like any other conditionally-called code. We do over-use XRE_GetProcessType in some places where I think we'd be better served by having a separate implementation file. > - nsresult rv = CallCreateInstance(aClassIID, &mWindow); > + nsresult rv; > + if (IsContentProcess()) { > + mWindow = nsIWidget::CreatePuppetWidget().get(); > + rv = !mWindow ? NS_ERROR_NOT_AVAILABLE : NS_OK; > + } else { > + rv = CallCreateInstance(aClassIID, &mWindow); > + } > > Seems like it might be better these days, if we have a parent widget, call a > method on it to create a child, otherwise use CallCreateInstance. > This sounds very good to me. > +using mozilla::layers::BasicShadowLayerManager; > +using mozilla::layers::LayerManager; > > using namespace mozilla::layers? > I saw a similar comment in bug 570620. The style guide doesn't cover this, shall we create the rule? IMHO we want to walk the line between two evils: on one side |using namespace foo;| unnecessarily pulling a bunch of decls into scope and setting up all files |using namespace foo;| for conflicts with decls later added to foo::, and on the other the old Eclipse-generated unmaintainable thousands of lines of |import foo.Bar;| for Java. My personal guideline is |using foo::Bar;| up to about 5 or so symbols, then either |using foo;| or seeing whether the namespace division should be changed. What's your personal rule? > + nsCOMPtr<nsIWidget> widget = new mozilla::widget::PuppetWidget(); > > using namespace mozilla::widget?? > > explicit-::-itis is horrible IMHO > The style guide doesn't cover this case either. My rule of thumb is to not |using foo::Bar;| for only one use, and add it for more than one. I don't particularly care about this though. > +const uintptr_t PuppetWidget::kWindowHandle = 0xABC123bb;/*you and me girl*/ > + aParent->GetNativeData(42) == reinterpret_cast<void*>(kWindowHandle)) && > > ??? Maybe we could get rid of all this if you take the suggestion above, we > could make the standard create path go away for PuppetWidgets. > Not a fan of the Jackson 5 ;)? Anyway, I just wanted a likely-unique constant for the hack above. Fixing the callsites to go through static nsIWidget functions or nsIWidget factory methods should make this go away. > + mSurface = gfxPlatform::GetPlatform() > + ->CreateOffscreenSurface(gfxIntSize(1, 1), > + gfxASurface::ImageFormatARGB32); > > Why do we need mSurface? Can't you have a null default target in > setupLayerManager? Without it, we hit "###!!! ASSERTION: CreateRenderingContext failure: 'Not Reached', file /home/cjones/mozilla/cedar/layout/base/nsPresShell.cpp, line 7477". I very much want to fix this; I had originally planned to do it as part of 570620, but then I saw that this small hack was enough to work around for the time being. Do you mind if we fix this in a followup? I filed bug 577791 about an API I think we'll need for content-process LayerManager. I can file another for fixing CreateRenderingContext or make 577791 be "widgetless layers".

(In reply to comment #13) > nsGTKToolkit assumes it has already been called (otherwise it crashes), and > ContentChild::Init is unfortunately the most convenient place to call it. "// > sigh" because I hoped we could do away with platform-specific widget code, but > I had forgotten how much we depend on Gtk for rendering. Fixing that is way > out of scope (obviously). Don't we call gtk_init from main() in nsAppRunner? > Part of the hack I discussed in bug 570620 comment 11. If we create static > nsIWidget Create* functions and factory methods rather than going through > XPCOM indirection, we could confine the hack to widget/. Let's do that. > FWIW I wanted to change as little as possible to make content processes > work, instead of reworking the way widgets are created, but the latter is > definitely a better long-term solution. We don't want to go out of control on a huge cleanup binge here, but I think it's definitely worth doing some cleanup as we go. > > Having XRE_GetProcessType calls sprinkled throughout our code scares me a > > lot. Every time someone needs to use a Gecko subprocess for a new purpose, > > we have to add a new enum value and audit all those sites? > > I'm not quite sure what's scaring you; do you have more specific concerns? We > do have an enum value per process type, and if something didn't work in the > new subprocess type, it might trace back to one of these special cases, just > like any other conditionally-called code. We do over-use XRE_GetProcessType > in some places where I think we'd be better served by having a separate > implementation file. For example, I see we have "Jetpack" process types. How do I, as a Gecko developer, have any idea what that means? At every place where we call XRE_GetProcessType, how do I know that the call site is doing the right thing for Jetpacks? For example, should Jetpack processes get puppet widgets or not? If we instead had a set of flags with meaningful names, e.g. "UsePuppetWidgets", and checked those instead of the process type, then it would be much more clear what's going on. > > +using mozilla::layers::BasicShadowLayerManager; > > +using mozilla::layers::LayerManager; > > > > using namespace mozilla::layers? > > I saw a similar comment in bug 570620. The style guide doesn't cover this, > shall we create the rule? IMHO we want to walk the line between two evils: on > one side |using namespace foo;| unnecessarily pulling a bunch of decls into > scope and setting up all files |using namespace foo;| for conflicts with decls > later added to foo::, Why not address this problem if and when it arises? It's easy enough to address in a local manner by backpedaling from "using namespace" to a series of "using" declarations, or by renaming identifiers, or using typedefs to create aliases. Remember it's only a problem if two namespaces define the same identifier *and* the file needs to use both identifiers. > What's your personal rule? Always use "using namespace" where it's safe to do so, i.e. in a compilation unit after all #includes. > The style guide doesn't cover this case either. My rule of thumb is to not > |using foo::Bar;| for only one use, and add it for more than one. I don't > particularly care about this though. I do. > Not a fan of the Jackson 5 ;)? Anyway, I just wanted a likely-unique constant > for the hack above. Fixing the callsites to go through static nsIWidget > functions or nsIWidget factory methods should make this go away. Let's do that. > > + mSurface = gfxPlatform::GetPlatform() > > + ->CreateOffscreenSurface(gfxIntSize(1, 1), > > + gfxASurface::ImageFormatARGB32); > > > > Why do we need mSurface? Can't you have a null default target in > > setupLayerManager? > > Without it, we hit "###!!! ASSERTION: CreateRenderingContext failure: 'Not > Reached', file /home/cjones/mozilla/cedar/layout/base/nsPresShell.cpp, line > 7477". I very much want to fix this; I had originally planned to do it as > part of 570620, but then I saw that this small hack was enough to work around > for the time being. Do you mind if we fix this in a followup? No, that's fine. I guess we will need a surface to create contexts with, but I suggest we put a single one in gfxPlatform.

(In reply to comment #14) > (In reply to comment #13) > > nsGTKToolkit assumes it has already been called (otherwise it crashes), and > > ContentChild::Init is unfortunately the most convenient place to call it. "// > > sigh" because I hoped we could do away with platform-specific widget code, but > > I had forgotten how much we depend on Gtk for rendering. Fixing that is way > > out of scope (obviously). > > Don't we call gtk_init from main() in nsAppRunner? > Yes, but content processes don't go through nsAppRunner, but rather an XRE API. > > > Having XRE_GetProcessType calls sprinkled throughout our code scares me a > > > lot. Every time someone needs to use a Gecko subprocess for a new purpose, > > > we have to add a new enum value and audit all those sites? > > > > I'm not quite sure what's scaring you; do you have more specific concerns? We > > do have an enum value per process type, and if something didn't work in the > > new subprocess type, it might trace back to one of these special cases, just > > like any other conditionally-called code. We do over-use XRE_GetProcessType > > in some places where I think we'd be better served by having a separate > > implementation file. > > For example, I see we have "Jetpack" process types. How do I, as a Gecko > developer, have any idea what that means? At every place where we call > XRE_GetProcessType, how do I know that the call site is doing the right thing > for Jetpacks? For example, should Jetpack processes get puppet widgets or not? > > If we instead had a set of flags with meaningful names, e.g. > "UsePuppetWidgets", and checked those instead of the process type, then it > would be much more clear what's going on. > Sure, I agree with that; I've tried to use that style elsewhere. I did mess up here with the IsContentProcess() helper, my fault. > > > +using mozilla::layers::BasicShadowLayerManager; > > > +using mozilla::layers::LayerManager; > > > > > > using namespace mozilla::layers? > > > > I saw a similar comment in bug 570620. The style guide doesn't cover this, > > shall we create the rule? IMHO we want to walk the line between two evils: on > > one side |using namespace foo;| unnecessarily pulling a bunch of decls into > > scope and setting up all files |using namespace foo;| for conflicts with decls > > later added to foo::, > > Why not address this problem if and when it arises? It's easy enough to address > in a local manner by backpedaling from "using namespace" to a series of "using" > declarations, or by renaming identifiers, or using typedefs to create aliases. > Remember it's only a problem if two namespaces define the same identifier *and* > the file needs to use both identifiers. > I'm OK with trying it this way. Basically you're betting on collisions not occurring, which should hopefully be mostly true for us. > > The style guide doesn't cover this case either. My rule of thumb is to not > > |using foo::Bar;| for only one use, and add it for more than one. I don't > > particularly care about this though. > > I do. > Interesting. Why? (just curious) > > > + mSurface = gfxPlatform::GetPlatform() > > > + ->CreateOffscreenSurface(gfxIntSize(1, 1), > > > + gfxASurface::ImageFormatARGB32); > > > > > > Why do we need mSurface? Can't you have a null default target in > > > setupLayerManager? > > > > Without it, we hit "###!!! ASSERTION: CreateRenderingContext failure: 'Not > > Reached', file /home/cjones/mozilla/cedar/layout/base/nsPresShell.cpp, line > > 7477". I very much want to fix this; I had originally planned to do it as > > part of 570620, but then I saw that this small hack was enough to work around > > for the time being. Do you mind if we fix this in a followup? > > No, that's fine. > > I guess we will need a surface to create contexts with, but I suggest we put a > single one in gfxPlatform. To create contexts from where, in what code?

(In reply to comment #15) > Yes, but content processes don't go through nsAppRunner, but rather an XRE > API. OK > > For example, I see we have "Jetpack" process types. How do I, as a Gecko > > developer, have any idea what that means? At every place where we call > > XRE_GetProcessType, how do I know that the call site is doing the right > > thing for Jetpacks? For example, should Jetpack processes get puppet > > widgets or not? > > > > If we instead had a set of flags with meaningful names, e.g. > > "UsePuppetWidgets", and checked those instead of the process type, then it > > would be much more clear what's going on. > > Sure, I agree with that; I've tried to use that style elsewhere. I did mess > up here with the IsContentProcess() helper, my fault. Yeah but I think we should have an interface like that that completely replaces XRE_GetProcessType. XRE_GetProcessType facilitates incomprehensible, non-extensible code. > I'm OK with trying it this way. Basically you're betting on collisions not > occurring, which should hopefully be mostly true for us. The cost of "losing the bet" in one file is about the same as the cost we'd incur all the time in each file if we try to avoid "using namespace", so I don't think of it as risky. > > > The style guide doesn't cover this case either. My rule of thumb is to > > > not |using foo::Bar;| for only one use, and add it for more than one. I > > > don't particularly care about this though. > > > > I do. > > Interesting. Why? (just curious) Because when I'm reading code, "mozilla::hooha::" is just irrelevant chaff. > > I guess we will need a surface to create contexts with, but I suggest we > > put a single one in gfxPlatform. > > To create contexts from where, in what code? Layout needs to create rendering contexts for measuring text (and sometimes paths). It creates those contexts from the widget. Contexts need destination surfaces, even if we don't actually draw to them. I'm saying we should have a single surface in gfxPlatform that layout can use to create contexts from, although I think continuing to go through nsIWidget to get those contexts is perfectly fine.

(In reply to comment #16) > (In reply to comment #15) > > I'm OK with trying it this way. Basically you're betting on collisions not > > occurring, which should hopefully be mostly true for us. > > The cost of "losing the bet" in one file is about the same as the cost we'd > incur all the time in each file if we try to avoid "using namespace", so I > don't think of it as risky. > I really doubt that any conflict would be restricted to a single file. > > > I guess we will need a surface to create contexts with, but I suggest we > > > put a single one in gfxPlatform. > > > > To create contexts from where, in what code? > > Layout needs to create rendering contexts for measuring text (and sometimes > paths). It creates those contexts from the widget. Contexts need destination > surfaces, even if we don't actually draw to them. I'm saying we should have a > single surface in gfxPlatform that layout can use to create contexts from, > although I think continuing to go through nsIWidget to get those contexts is > perfectly fine. Gotcha, thanks for the clarification.

(In reply to comment #7) > Comment on attachment 460325 [details] [diff] [review] > Gut platform widgets from content processes > > If Show/Move are always going to have a 0/0 origin, could you just pass a > width/height or nsPoint instead of the full rect? > Done. > sr=me with that, or this way if it's somehow important to keep the full rect. > We might need to deal with special cases for DOM coordinates like .clientWidth. I don't understand that case, but I can guess we can cross that bridge when we come to it.

Quick warning for the upcoming patch bomb: I found the logic in nsView::CreateWidget exceedingly opaque, so I refactored it in a series of small steps before cutting it up to suit PuppetWidget purposes. None of the early patches are at all nontrivial.

Should add that I'd like to land up to and including "part h" on m-c.

(In reply to comment #5) > Comment on attachment 460326 [details] [diff] [review] > Implement a "puppet widget" stand-in for platform widgets, for content > processes > > +nsresult > +PuppetWidget::DispatchPaintEvent() > +{ > + NS_ABORT_IF_FALSE(!mDirtyRegion.IsEmpty(), "paint event logic messed up"); > > I'm playing with fennec-tile-less, loading google.com, and HW keys pressing > aborting content process in this place. I reproduced this in tile-less fennec by clicking on google.com's input box and typing a letter; the crash happens just after the letter appears in the input box. (Hey, key events appear to kinda work there, that's a surprise to me.) Looking into it.

The code in v1 and v2 that I thought was cancelling the "paint task" was in actuality not. Fixed.

Comment on attachment 462645 [details] [diff] [review] part b: Remove nsIDeviceContext::SupportsNativeWidgets because it's not used meaningfully, and will be confusing in content processes nice

Comment on attachment 462647 [details] [diff] [review] part d: Simplify nsView::LoadWidget and return early if it fails Prefer to have success/failure results remain nsresult instead of PRBool (not bool!). It's clearer what success and failure are.

Comment on attachment 462651 [details] [diff] [review] part f: Split out window initialization code in preparation for multiple CreateWidget* methods PRBool, not bool

In part g, wouldn't it make more sense to have a "CreateChild" method that creates the object as well as initializing it?

(In reply to comment #36) > In part g, wouldn't it make more sense to have a "CreateChild" method that > creates the object as well as initializing it? I wanted to, but that means refactoring away the IID param, and I stopped short of that when I hit code like http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#322 . There's only so much I can take at once. I'm guessing you're holding the r+ ransom until that's gone too? ;) Sigh, I'll get the shovel back out.

Sorry, I didn't realize this was becoming too much.

No, I know you're right. Just venting some frustration with I intended to be kidding :).

That particular IID issue can be resolved internally to nsCocoaWindow/nsChildView::CreateChild, just by checking the window type and creating the right object. Whether you want to keep going in that direction is up to you. If you've had enough, just say so and we'll proceed with the patches you have.

Gah, wrong patch.

There's some remaining ugliness in this patch; happy to discuss better solutions. First, CreateChild() takes an aForceUseIWidgetParent param that says to not use the native widget for Create() even if the parent has one. This is to preserve existing behavior that I don't understand and am, TBH, scared to change ;). The second ugliness is hard-coding the special-cased NS_POPUP_CID for mac into nsBaseWidget::CreateChild(). I could have added an nsBaseWidget::CreatePopupWidget(), but it didn't quite seem worth the effort to me. Please advise.

This was mostly straightforward, except for the case where CreateWidget() is called and there's no suitable parent widget in the view hierarchy. I played through all the widget-creation scenarios in gdb that I could think of and never hit this case. I'd be more than happy to |return NS_FAILURE| or |ERROR()| there, if it's something that wouldn't happen in practice. Or, we could leave in this hacky special case until |rm -rf view/|. Please advise.

(In reply to comment #44) > This was mostly straightforward, except for the case where CreateWidget() is > called and there's no suitable parent widget in the view hierarchy. I was referring to this on IRC, was going to ask whether this can happen in nondegenerate cases (null aParent param and no other parent found in the view hierarchy).

Added an nsBaseWidget::CreateChildPopupWidget() interface to keep platform ifdefs out of src/xpwidget.

Updated to have nsIView::CreateWidget() return an error if a suitable parent widget isn't found in the view hierarchy.

Comment on attachment 463454 [details] [diff] [review] part j: Implement a "puppet widget" stand-in for platform widgets, for content processes + PRBool mEnabled; + PRBool mVisible; PRPackedBool

With the updated patch in comment 49, I get this assertion failure in reftests ###!!! ASSERTION: nsView::CreateWidget without suitable parent widget??: 'Error', file /home/cjones/mozilla/mozilla-central/view/src/nsView.cpp, line 727 nsView::CreateWidget(nsWidgetInitData*, int, int, nsContentType) (/home/cjones/mozilla/mozilla-central/view/src/nsView.cpp:728) nsIView::CreateWidget(nsWidgetInitData*, int, int, nsContentType) (/home/cjones/mozilla/mozilla-central/view/src/nsView.cpp:670) DocumentViewerImpl::MakeWindow(nsSize const&, nsIView*) (/home/cjones/mozilla/mozilla-central/layout/base/nsDocumentViewer.cpp:2348) DocumentViewerImpl::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, int, int, int) (/home/cjones/mozilla/mozilla-central/layout/base/nsDocumentViewer.cpp:895) DocumentViewerImpl::Init(nsIWidget*, nsIntRect const&) (/home/cjones/mozilla/mozilla-central/layout/base/nsDocumentViewer.cpp:695) nsExternalResourceMap::AddExternalResource(nsIURI*, nsIDocumentViewer*, nsILoadGroup*, nsIDocument*) (/home/cjones/mozilla/mozilla-central/content/base/src/nsDocument.cpp:814) nsExternalResourceMap::PendingLoad::OnStartRequest(nsIRequest*, nsISupports*) (/home/cjones/mozilla/mozilla-central/content/base/src/nsDocument.cpp:870) nsHttpChannel::CallOnStartRequest() (/home/cjones/mozilla/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:734) nsHttpChannel::ContinueProcessNormal(unsigned int) (/home/cjones/mozilla/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:1089) nsHttpChannel::ProcessNormal() (/home/cjones/mozilla/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:1027) nsHttpChannel::ProcessResponse() (/home/cjones/mozilla/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:913) nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (/home/cjones/mozilla/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:3567) nsInputStreamPump::OnStateStart() (/home/cjones/mozilla/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:441) nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/cjones/mozilla/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:397) nsInputStreamReadyEvent::Run() (/home/cjones/mozilla/mozilla-central/xpcom/io/nsStreamUtils.cpp:113) Rob tells me this is an external SVG document being loaded, and the document doesn't need a widget (at least, possibly after bug 585817). I would like to change this code path to force DocumentViewer to not create a widget. Boris/Rob, does setting a flag on the viewer in http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#883 sound like the right approach? I would check the flag somewhere near http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2321 .

Just check mDocument->GetDisplayDocument() there? That will be true if and only if the document is a resource document.

Will qfold this into part g on r+.

We'll need 585817 to remove rendering contexts' dependency on external documents having widgets.

Comment on attachment 464295 [details] [diff] [review] part 0.5: Add support for aInitData=NULL to the Windows nsWindow implementation hmm.. i never looked at this code in detail before, but the handling of aInitData here is very busted. sometimes we test for null, other times we freely access it. Not sure why, but I am glad you are cleaning it up! >- if (aInitData) >- mUnicodeWidget = aInitData->mUnicode; >+ nsWidgetInitData defaultInitData; >+ if (!aInitData) >+ aInitData = &defaultInitData; >+ aInitData is always non-null here. > nsIWidget *baseParent = aInitData && > (aInitData->mWindowType == eWindowType_dialog || > aInitData->mWindowType == eWindowType_toplevel || > aInitData->mWindowType == eWindowType_invisible) ? > nsnull : aParent; So this first test can be removed. Later in this same function we do something like: 568 } else if (nsnull != aInitData) { 569 // See if the caller wants to explictly set clip children and clip siblings I think that also has to change. Also, we can drop the test for aInitData here too: 580 mWnd = ::CreateWindowExW(extendedStyle, 581 aInitData && aInitData->mDropShadow ? 582 WindowPopupClass() : WindowClass(), I am not sure about the other patches -- could we also remove this test: http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsBaseWidget.cpp#215

(In reply to comment #56) > Comment on attachment 464295 [details] [diff] [review] > part 0.5: Add support for aInitData=NULL to the Windows nsWindow implementation > > hmm.. i never looked at this code in detail before, but the handling of > aInitData here is very busted. sometimes we test for null, other times we > freely access it. Not sure why, but I am glad you are cleaning it up! > > >- if (aInitData) > >- mUnicodeWidget = aInitData->mUnicode; > >+ nsWidgetInitData defaultInitData; > >+ if (!aInitData) > >+ aInitData = &defaultInitData; > >+ > > aInitData is always non-null here. > > > nsIWidget *baseParent = aInitData && > > (aInitData->mWindowType == eWindowType_dialog || > > aInitData->mWindowType == eWindowType_toplevel || > > aInitData->mWindowType == eWindowType_invisible) ? > > nsnull : aParent; > > So this first test can be removed. > > > Later in this same function we do something like: > > 568 } else if (nsnull != aInitData) { > 569 // See if the caller wants to explictly set clip children and clip > siblings > > I think that also has to change. > > Also, we can drop the test for aInitData here too: > > 580 mWnd = ::CreateWindowExW(extendedStyle, > 581 aInitData && aInitData->mDropShadow ? > 582 WindowPopupClass() : WindowClass(), > Thanks. > I am not sure about the other patches -- could we also remove this test: > > http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsBaseWidget.cpp#215 If all the other widget backends used this same kind of init code, we could, but gtk2 for example distributes the null-check guards like windows used to, and so would end up with aInitData==NULL there. I'd rather not fix all the other widget backends in this bug ;).

Fixed the other aInitData null checks. Double-checked that the default values don't change the logic in Create().

Comment on attachment 464578 [details] [diff] [review] part 0.5: Add support for aInitData=NULL to the Windows nsWindow implementation, v2 i'd probably see if I could move the mUnicodeWidget assignment to the rest of the members initalization, but this is much better than it was and I don't need to nitpick. thanks cjones.

With all these patches pushed to try on top of bug 585817, I see reftest failures on both OS X 10.5 and 10.6 in layout/reftests/bugs/580160-1.html: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281518630.1281519700.11721.gz I'm also seeing two other weird bugs with events on Mac that seem widget related http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281510743.1281511551.30078.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281512386.1281513486.7330.gz I'm cutting a mac build now to investigate whether it's my patches or bug 585817 (mine seem more likely), but if anyone has off-the-top-of-the-head ideas about where to look, I'd be grateful.

The reftest failure looks like an invalidation problem.

Blocks everything fennec+layers. Looks to be ready to go once bug 585817 gets reviews.

http://hg.mozilla.org/mozilla-central/rev/88a279b64473 http://hg.mozilla.org/mozilla-central/rev/9407827b5166 http://hg.mozilla.org/mozilla-central/rev/5452db293694 http://hg.mozilla.org/mozilla-central/rev/c5945b6a97ed http://hg.mozilla.org/mozilla-central/rev/7b17bcefb174 http://hg.mozilla.org/mozilla-central/rev/7735c00eabe9 http://hg.mozilla.org/mozilla-central/rev/353da995af6f http://hg.mozilla.org/mozilla-central/rev/5bf0b315a5aa http://hg.mozilla.org/mozilla-central/rev/f1af117d4598 http://hg.mozilla.org/mozilla-central/rev/037a5d6b376a http://hg.mozilla.org/mozilla-central/rev/90ad165ae21b The remaining patches will land on cedar once these look OK on m-c.

Round 2. http://hg.mozilla.org/mozilla-central/rev/4363ea3add87 http://hg.mozilla.org/mozilla-central/rev/d2e80a426678 http://hg.mozilla.org/mozilla-central/rev/91ab528ceb03 http://hg.mozilla.org/mozilla-central/rev/8f2104cbe0e0 http://hg.mozilla.org/mozilla-central/rev/1007fbdaad19 http://hg.mozilla.org/mozilla-central/rev/418247b1dc8a http://hg.mozilla.org/mozilla-central/rev/17ccf212ce68 http://hg.mozilla.org/mozilla-central/rev/1df224a59383 http://hg.mozilla.org/mozilla-central/rev/7852880685f8 http://hg.mozilla.org/mozilla-central/rev/2757103c7e89 http://hg.mozilla.org/mozilla-central/rev/95a39e236cc4

Comment on attachment 467918 [details] [diff] [review] Paper over Show() being called multiple times and triggering an assertion in the content process. This will all be reworked in the future r+. please file a new bug to clean this up. thanks cjones!

Filed bug 589337.

http://hg.mozilla.org/projects/cedar/rev/314680412c41 http://hg.mozilla.org/projects/cedar/rev/daeab9b5b99b http://hg.mozilla.org/projects/cedar/rev/a13fca21e6a8