575567 - Drop X[Parent/Child] widget embedding from remote browser

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Kill XEmbed for Qt... (I think it make sense for GTK builds too)

I'm using patches from bug 130078 which does not have widgets structure...

and IPC nsFrameLoader::ShowRemoteFrame fails on 
http://mxr.mozilla.org/projects-central/source/electrolysis/content/base/src/nsFrameLoader.cpp#767

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 454814 [details] [diff] [review]
Kill XEmbed for Qt... (I think it make sense for GTK builds too)

I'm not at all familiar how bug 130078 changes this all.

Comment 2

[Oleg Romashin (:romaxa)]

(In reply to comment #1)
> (From update of attachment 454814 [details] [diff] [review])
> I'm not at all familiar how bug 130078 changes this all.

that bug is removing multiple nsIWidgets structure, and making only one nsIWidget per window.. and this code just cannot find nsIWidget for some nsIView...

I would suggest to remove all this XEmbed code completely... because it does not make any sense

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 454814 [details] [diff] [review]
Kill XEmbed for Qt... (I think it make sense for GTK builds too)

So could you still explain how the widget handling works with the patch in e10s. How is the child process connected to parent?

Comment 4

[Oleg Romashin (:romaxa)]

> So could you still explain how the widget handling works with the patch in
> e10s. How is the child process connected to parent?

Rendering working with remote layers or canvas..

Should it be connected somehow else?

If yes then I think XEmbed is not good way to connect child and parent process... also I think child process should be widgetless in general.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 454814 [details] [diff] [review]
Kill XEmbed for Qt... (I think it make sense for GTK builds too)

I think someone more familiar with widget handling should review this.

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Drop Widget embedding completely and stop confusing people.

Currently we  are initializing some weird  X-Embedding stuff for browser element... that brings some confusing about how remote browser works, and bring some unexpected behavior...

I think we should handle remote tab views the same way for all platforms... using remote layers environment.

Comment 7

[Benjamin Smedberg]

How does focus work? Currently we focus the child widget in order to deliver keyboard events. Will the child process have *any* widget?

Comment 8

[Oleg Romashin (:romaxa)]

> How does focus work? Currently we focus the child widget in order to deliver

Focus events should be delivered from parent -> child, the same way as mouse,key and other events.

> keyboard events. Will the child process have *any* widget?
I hope no real widget... probably only nsIWidget wrapper class without real gdk/qt/win widgets...

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 457841 [details] [diff] [review]
Drop Widget embedding completely and stop confusing people.

This patch make same remote browser initialization for all platforms.
XEmbedding approach is not going to be used in fennec 2.0
And currently XEmbedding on Gtk/Win brings not correct expose events which are activating ShadowLayer tree... and on Qt/Android shadowLayer tree not activated at all, because XEmbedding disabled there.

I think we should disable XEmbed and create implementation for Focus/ShadowTreeActivate events.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #8)
> > keyboard events. Will the child process have *any* widget?
> I hope no real widget... probably only nsIWidget wrapper class without real
> gdk/qt/win widgets...

I talked to roc about that approach at the summit, and he's not in favor of fake widgets.  I'm still pretty confident we can de-widgetize the relevant code here.  (Or at least teach it to deal with widget=NULL.)

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm afraid I don't have the requisite widget-fu to know if these changes will work.  Isn't not parenting the widgets to the chrome process's window going to cause problems?  I think it would.

I definitely like the massive block of removed code here, but I'm not sure what you're trying to accomplish with this intermediate step.  Maybe energy is better focused on nuking widgets from content processes entirely?

Comment 12

[Oleg Romashin (:romaxa)]

> better focused on nuking widgets from content processes entirely?

yep, that is the next part... with current patch we still have real widget on child  process... next part  is replace parent native widget with something like win=0x12345, and make sure that gtk/qt-nsIWidget can be created and work without real platform widgets. 

(also microb on N900 does not have any real widgets in child process)

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We should have taken this patch, my mistake.

http://hg.mozilla.org/mozilla-central/rev/be857a136ffd

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 457841 [details] [diff] [review]
Drop Widget embedding completely and stop confusing people.

Ok, this is already fixed in cedar... 582057