546605 - Some initialization and whitespace issues with the latest switch to QGraphicsWidgets

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Steffen Imhof]

The patch for bug 544216 introduced a few issues that should be fixed here.

Comment 1

[Steffen Imhof]

Attachment: Patch to add a missing nsnull member assignment and remove some trailing whitespace

There was a nsnull assignment missing in the early out when the graphics view could not be created.
Some indentation issues were fixed and I also removed all trailing whitespace from nsWindow.* and mozqwidget.* even those occurences not introduced by the latest patch.

Comment 2

[Oleg Romashin (:romaxa)]

Comment on attachment 427307 [details] [diff] [review]
Patch to add a missing nsnull member assignment and remove some trailing whitespace 

This is ok, but there are one more problem:
We have GetViewWidget which may return NULL,
But in rest of code, which is using this function, in some places we are checking GetViewWidget with if (GetViewWidget()) check, and then calling GetViewWidget() again...
In many places we are not checking return result at all.

Comment 3

[Steffen Imhof]

Attachment: Additional patch to make GetViewWidget() more clear (and a few more whitespace issues)

Good point, with this patch GetViewWidget() never returns nsnull anymore, so I also removed the one place where it was checked for.
My rationale for that is, that every nsWindow has to live in some QGraphicsView to be shown. So either it has a mScene with one view (toplevel), or it has a parent that can be asked (child/paintarea).

I tried it with Fennec and Firefox and it seems the assertion is in fact never triggered, but maybe I am missing some case here like invisible windows for something?

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 427337 [details] [diff] [review]
Additional patch to make GetViewWidget() more clear (and a few more whitespace issues)

from a qt/e10s build:

###!!! ASSERTION: Top level window without scene encountered!: 'mScene || mParent', file /home/dougt/builds/e10sQt/electrolysis/widget/src/qt/nsWindow.cpp, line 730


For child widgets, there isn't a widget parent provided to the Create() method any longer.  Is there a way we could just the the MozQWidget's parent in GetViewWidget()?

Comment 5

[Steffen Imhof]

Ok, I guess the assertion was a bit short-sighted in light of e10s.

We could use the MozQWidget's parent/child structure to get to the view, but in the e10s case I think we would not get the view that we want.
It's probably possible for MozQWidgets to not be in a view at all if they live in some child process, and even if they are, that would be some kind of dummy view, that is not shown directly on the screen. So I'll restructure the patch in the opposite direction allowing for MozQWidgets not having a view and checking everywhere before using it.

In a lot of places I think this makes sense because the GetViewWidget() is used to make a connection between a toplevel nsWindow and its surrounding view (resizing, size modes, and so on). Things we have to watch out for might be raising windows or grabbing the mouse which I'm not sure how they would work over the process boundaries.

But if needed we should do that in another bug.

Also I'd like to get rid of the mScene member and instead just use the scene() pointer from the MozQWidget.

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: This should be ok, to fix crash problem

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Fixed apply on m-c sources

Comment 8

[Doug Turner (:dougt)]

this does fix the problem.  Steffen can you take a look too?  I am worried about the life of mWidget.  

         mWidget->deleteLater();
+        delete mWidget;

Comment 9

[Oleg Romashin (:romaxa)]

(In reply to comment #8)
> this does fix the problem.  Steffen can you take a look too?  I am worried
> about the life of mWidget.  
> 
>          mWidget->deleteLater();
> +        delete mWidget;

oh yep, this is just debugging stuff... I was experimenting but forgot to remove

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Fixed patch

Comment 11

[Steffen Imhof]

There is a small lifetime issue with the QGraphicsScene I think, because that will never get deleted. Other than that, I'd say it's fine.

Comment 12

[Doug Turner (:dougt)]

romaxa, can you fix the leak?

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Fixed leak

Comment 14

[Oleg Romashin (:romaxa)]

Fixed
http://hg.mozilla.org/mozilla-central/rev/eb5e823a2995