629823 - Use the right VM for the parent view in nsViewManager::ResizeView. Assert that the root view size is in sync with the pres context visible size.

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Mats Palmgren (inactive)]

Attachment: part 1 v2 from bug 617076

Follow-up from bug 617076 (the part 1 patch):

Timothy pointed out that nsViewManager::ResizeView shouldn't be called with
a root view.  I added an assertion for that which revealed that it triggers
all the time so I added "NS_FRAME_NO_MOVE_VIEW | NS_FRAME_NO_SIZE_VIEW"
flags in PresShell::DoReflow to avoid that. (see patch)

Timothy said this is wrong and we should instead fix whatever makes
that happen.

Comment 1

[Timothy Nikkel (:tnikkel)]

Add "I think" to whatever I said in comment 0. :)

Comment 2

[Timothy Nikkel (:tnikkel)]

Ok so nsViewManager::ResizeView is called on the root view all the time, but do we ever change the size of the root view this way? That is the case we don't want to happen.

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip2

You mean like so?

It occurs when opening Edit->Preferences on Linux:

###!!! ASSERTION: ResizeView is resizing the root view: 'view != mRootView', file view/src/nsViewManager.cpp, line 1349

#0  NS_DebugBreak_P
#1  nsViewManager::ResizeView at view/src/nsViewManager.cpp:1349
#2  PresShell::DoReflow at layout/base/nsPresShell.cpp:7869
#3  PresShell::ResizeReflowIgnoreOverride at layout/base/nsPresShell.cpp:2886
#4  DocumentViewerImpl::SizeToContent at layout/base/nsDocumentViewer.cpp:3207
#5  nsGlobalWindow::SizeToContent at dom/base/nsGlobalWindow.cpp:5263
#6  nsGlobalWindow::SizeToContent at dom/base/nsGlobalWindow.cpp:5241
#7  NS_InvokeByIndex_P
...

Comment 4

[Timothy Nikkel (:tnikkel)]

Yeah, I was thinking something like that. So you're saying that assertion still fires?

Comment 5

[Mats Palmgren (inactive)]

Yes, with the STR and stack in comment 3.

Comment 6

[Timothy Nikkel (:tnikkel)]

Ok, I see. The main reason I wanted that assertion was to make sure that the visible area stored on the prescontext is always in sync with the root view size. So I think we should just make ResizeView handle resizing a root view properly. Mats, I think you had a patch for that in the old bug?

Comment 7

[Timothy Nikkel (:tnikkel)]

And adding an assert that the root view's size matches the prescontext's visible area would be good, is the end PresShell::FlushPendingNotifications a good place for that?

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix v2

It seems we can only assert that when (aType >= Flush_Layout) though.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to comment #8)
> It seems we can only assert that when (aType >= Flush_Layout) though.

That makes sense because we only do the FlushDelayedResize(PR_TRUE) call if aType >= Flush_Layout (some of the time).

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 510800 [details] [diff] [review]
fix v2

>@@ -4906,16 +4909,27 @@ PresShell::FlushPendingNotifications(moz
>+          static_cast<nsView*>(rootView)->GetDimensions(viewSize);

Let's keep nsView inside of /view I think. So use whatever methods available on nsIView to get what you need.

Thanks Mats.

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 510800 [details] [diff] [review]
fix v2

I actually think we should take this. If zooming is involved then we could see the same type of problem as bug 617076. And it should be safe, we execute the same code, but in the right view manager with the right app units per dev pixel value.

Comment 12

[Mats Palmgren (inactive)]

Attachment: fix v3

with the nit fixed

Comment 13

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/0a9069e1e1b8
http://hg.mozilla.org/mozilla-central/rev/502b2bea2c4d