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

RESOLVED FIXED in Firefox 40

Status

()

Firefox
General
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
Firefox 38
x86_64
All
Points:
---

Firefox Tracking Flags

(e10sm5+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
reproduced on mac and win.
OS: Windows 7 → All
Assignee: nobody → jmathies
tracking-e10s: ? → m5+
(Assignee)

Comment 2

4 years ago
Created attachment 8553289 [details] [diff] [review]
patch
(Assignee)

Updated

4 years ago
Attachment #8553289 - Flags: review?(tnikkel)
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8553711 [details] [diff] [review]
patch

Yep, that fixes the problem as well, thanks.
Attachment #8553289 - Attachment is obsolete: true
Attachment #8553289 - Flags: review?(tnikkel)
Attachment #8553711 - Flags: review?(tnikkel)
(Assignee)

Comment 6

4 years ago
I can clean this up a bit further by moving the checks on mPresContext && mViewManager down with the final resizeView check.
(Assignee)

Comment 7

4 years ago
Created attachment 8553729 [details] [diff] [review]
patch

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?
(Assignee)

Comment 9

4 years ago
(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..
(Assignee)

Comment 10

4 years ago
Created attachment 8554189 [details] [diff] [review]
patch
Attachment #8553729 - Attachment is obsolete: true
Attachment #8553729 - Flags: review?(tnikkel)
(Assignee)

Comment 12

4 years ago
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+
(Assignee)

Comment 14

4 years ago
Created attachment 8554252 [details] [diff] [review]
patch r=tn
Attachment #8554189 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 8554253 [details] [diff] [review]
patch r=tn
Attachment #8554252 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8510413a0356
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8510413a0356
Status: NEW → RESOLVED
Last Resolved: 4 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.
status-firefox38: --- → wontfix
status-firefox39: --- → wontfix
status-firefox40: --- → fixed
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.