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.

RESOLVED FIXED in mozilla2.0b12

Status

()

defect
RESOLVED FIXED
9 years ago
11 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Depends on 1 bug)

Trunk
mozilla2.0b12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted patch part 1 v2 from bug 617076 (obsolete) — Splinter Review
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.
Add "I think" to whatever I said in comment 0. :)
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.
Posted patch wip2 (obsolete) — Splinter Review
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
...
Yeah, I was thinking something like that. So you're saying that assertion still fires?
Yes, with the STR and stack in comment 3.
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?
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?
Posted patch fix v2Splinter Review
It seems we can only assert that when (aType >= Flush_Layout) though.
Assignee: nobody → matspal
Attachment #508039 - Attachment is obsolete: true
Attachment #508178 - Attachment is obsolete: true
Attachment #510800 - Flags: review?(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 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.
Attachment #510800 - Flags: review?(tnikkel) → review+
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.
Attachment #510800 - Flags: approval2.0?
Posted patch fix v3Splinter Review
with the nit fixed
Attachment #510800 - Flags: approval2.0?
Attachment #511350 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/0a9069e1e1b8
http://hg.mozilla.org/mozilla-central/rev/502b2bea2c4d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Summary: Make nsViewManager::ResizeView assert it's not called with a root view → 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.
Target Milestone: --- → mozilla2.0b12
Depends on: 634161
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.