Closed Bug 546605 Opened 13 years ago Closed 13 years ago
Some initialization and whitespace issues with the latest switch to QGraphics
The patch for bug 544216 introduced a few issues that should be fixed here.
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.
Attachment #427307 - Flags: review?(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.
Attachment #427307 - Flags: review?(romaxa) → review+
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?
Attachment #427337 - Flags: review?(romaxa)
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()?
Attachment #427337 - Flags: review?(romaxa) → review-
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.
this does fix the problem. Steffen can you take a look too? I am worried about the life of mWidget. mWidget->deleteLater(); + delete mWidget;
(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
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.
romaxa, can you fix the leak?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.