1077085 - Tabs in a resized e10s window don't paint properly when selected

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) open bugzilla using a medium sized e10s window
2) click on the "New" bug page, wait for it to load
3) size the window larger
4) hit back button

result: the old cached page paints to the original window size, leaving blank areas around content.

Comment 1

[Jim Mathies [:jimm]]

reproduced on mac and win.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b17d9675c1b

I was a little nervous about removing that bounds check in PuppetWidget.. hopefully this doesn't blow up b2g tests. On desktop it's not an issue since Resize on the widget rarely gets called.

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8553289 [details] [diff] [review]
patch

So if I'm understanding the sequence of events correctly here mBounds is the old bounds on the document viewer, aBounds is the new bounds. mWindow already has the new bounds, but it will still call the WindowResized handler thanks to the PuppetWidget::Resize change? The WindowResized handler is nsView::WindowResized and that will call SetWindowDimensions on the view manager.

It seems like the mWindow && mAttachedToParent case is more like the !mWindow case in that we don't want to resize our widget, we want to resize our presentation inside the widget. So what if we made the mWindow && mAttachedToParent case follow the else branch (ie just directly call SetWindowDimensions and not mWidget->Resize)? Does that work? That avoids us sending a fake resize event to our widget.

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch

Yep, that fixes the problem as well, thanks.

Comment 6

[Jim Mathies [:jimm]]

I can clean this up a bit further by moving the checks on mPresContext && mViewManager down with the final resizeView check.

Comment 7

[Jim Mathies [:jimm]]

Attachment: patch

this looks a little cleaner.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8553729 [details] [diff] [review]
patch

Do we need the check if the old bounds are equal to the new bounds? SetWindowDimensions should be able to handle getting the same bounds set.

The old code had

if (mWindow && mAttachedToParent) {
  // do nothing
}
if (mWindow && !mAttachedToParent) {
  // resize the widget (which will eventually call something like SetWindowDimensions on the view manager)
}
if (!mWindow) {
  // call SetWindowDimensions on the view manager
}

The mWindow && mAttachedToParent case doesn't make sense to me, we should be resizing the document when the bounds are changed on the document viewer. I think this is the reason for the bug, and it doesn't seem to be specific to content processes (but I guess it only bites them).

So I think it should be more like

if (mWindow && !mAttachedToParent) {
  // resize the widget
} else {
  // call SetWindowDimensions on the view manager
}

Does that make sense? And fix the bug without causing any others?

Comment 9

[Jim Mathies [:jimm]]

(In reply to Timothy Nikkel (:tn) from comment #8)
> Comment on attachment 8553729 [details] [diff] [review]
> patch
> 
> Do we need the check if the old bounds are equal to the new bounds?
> SetWindowDimensions should be able to handle getting the same bounds set.
> 
> The old code had
> 
> if (mWindow && mAttachedToParent) {
>   // do nothing
> }
> if (mWindow && !mAttachedToParent) {
>   // resize the widget (which will eventually call something like
> SetWindowDimensions on the view manager)
> }
> if (!mWindow) {
>   // call SetWindowDimensions on the view manager
> }
> 
> The mWindow && mAttachedToParent case doesn't make sense to me, we should be
> resizing the document when the bounds are changed on the document viewer. I
> think this is the reason for the bug, and it doesn't seem to be specific to
> content processes (but I guess it only bites them).
> 
> So I think it should be more like
> 
> if (mWindow && !mAttachedToParent) {
>   // resize the widget
> } else {
>   // call SetWindowDimensions on the view manager
> }
> 
> Does that make sense? And fix the bug without causing any others?

This is old code, I really can't guess what the side effects will be. I'll test and push to try to see if something breaks there..

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

Comment 11

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f79b841078f7

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8554189 [details] [diff] [review]
patch

Hasn't broken anything yet in the try run. :)

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8554189 [details] [diff] [review]
patch

thanks

Comment 14

[Jim Mathies [:jimm]]

Attachment: patch r=tn

Comment 15

[Jim Mathies [:jimm]]

Attachment: patch r=tn

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/8510413a0356

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8510413a0356

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8510413a0356

Comment 19

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Fx38 and Fx39 due to bug 1150021.