Closed Bug 1077085 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
e10s m5+ ---
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
reproduced on mac and win.
OS: Windows 7 → All
Assignee: nobody → jmathies
Attached patch patch (obsolete) — Splinter Review
Attachment #8553289 - Flags: review?(tnikkel)
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 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.
Attached patch patch (obsolete) — Splinter Review
Yep, that fixes the problem as well, thanks.
Attachment #8553289 - Attachment is obsolete: true
Attachment #8553289 - Flags: review?(tnikkel)
Attachment #8553711 - Flags: review?(tnikkel)
I can clean this up a bit further by moving the checks on mPresContext && mViewManager down with the final resizeView check.
Attached patch patch (obsolete) — Splinter Review
this looks a little cleaner.
Attachment #8553711 - Attachment is obsolete: true
Attachment #8553711 - Flags: review?(tnikkel)
Attachment #8553729 - Flags: review?(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?
(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..
Attached patch patch (obsolete) — Splinter Review
Attachment #8553729 - Attachment is obsolete: true
Attachment #8553729 - Flags: review?(tnikkel)
Comment on attachment 8554189 [details] [diff] [review]
patch

Hasn't broken anything yet in the try run. :)
Attachment #8554189 - Flags: review?(tnikkel)
Comment on attachment 8554189 [details] [diff] [review]
patch

thanks
Attachment #8554189 - Flags: review?(tnikkel) → review+
Attached patch patch r=tn (obsolete) — Splinter Review
Attachment #8554189 - Attachment is obsolete: true
Attached patch patch r=tnSplinter Review
Attachment #8554252 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8510413a0356
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Depends on: 1150021
This was backed out from Fx38 and Fx39 due to bug 1150021.
QA Whiteboard: [good first verify][verify in Nightly only]
You need to log in before you can comment on or make changes to this bug.