Resize events not fired on fastback to a document with resize handlers

RESOLVED FIXED

Status

()

Core
Document Navigation
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: bz, Assigned: Brian Ryner (not reading))

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Bug 274784 comment 16 says:

----------------------------------------------------------------------------
> Actually, what happens if I leave a page, resize the window, then go back
> to the page?

There's an onresize event.
----------------------------------------------------------------------------

This doesn't happen with bfcache.  Testcase coming up.
Note that if you click the "resize" button again after going back you can verify
that the resize handler is very much in place.
(Assignee)

Comment 3

13 years ago
I don't get said alert on WinXP even with bfcache off.  So, I don't believe this
is a bfcache regression.  Removing dependency (and someone else can feel free to
take this if it's important that it be fixed... I have too much on my plate
right now to deal with non-regression bugs).
No longer blocks: 274784
The point is that the onresize event only needs to happen if the DOM has been
preserved.  If a new one is constructed, its onload or the script execution will
get the correct size.
Blocks: 274784
Created attachment 186713 [details]
testcase showing why this is a problem
Did I ever say this was a regression?  This bug is about an initial design
criterion for bfcache (as described in the comment quoted in comment 0) that was
never addressed in the patches that went in.
(Assignee)

Comment 7

13 years ago
It was addressed though -- dbaron's testcase works as expected.  There is code
in RestorePresentation that does exactly what you quoted in comment 0. 
Resolving WORKSFORME.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WORKSFORME
Created attachment 187995 [details]
David's testcase in an iframe
Sorry, reopening.  David's testcase works, even in an iframe, but my original
testcase in this bug does not work.  I don't see the difference between that and
putting David's testcase in an iframe, but there it is.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 10

13 years ago
Created attachment 188967 [details] [diff] [review]
patch

This patch makes the resize handler get fired on the original testcase.  The
reason it was not is actually that fastback was not caching the presentation
due to bogus checking of the load type flags (it was deciding it couldn't cache
if any of the high bits were set).

However, I would appreciate it in the future if people could refrain from
reopening bugs such as this without first verifying that a problem exists.  The
comment about script handlers needing to see resize events when the DOM is
cached is all well and good, except that no one checked that the DOM was even
cached in the "broken" testcase.  If the DOM is cached, we fire the resize
events, as I've said.  If it's not, then the document will be loaded at its new
size and nothing will need to see a resize.  So the only problem exposed by
this testcase was a case where we should be caching and are not.
Attachment #188967 - Flags: superreview?(darin)
Attachment #188967 - Flags: review?(darin)
All I could tell was that I had fastback enabled, that the testcase should have
been getting cached based on the testcase setup, and that the load event was not
firing.  I didn't really have a chance to debug whether the caching happened...
 I probably should have looked for the time to do that.

That said, what exact load flag bit is a problem here?  Is it the STOP_CONTENT
flag because we're setting location?  Generally in docshell load types are just
numbers; using '|' on them is a little odd (for example, LOAD_LINK completely
subsumes LOAD_NORMAL when you do that) and makes it hard to tell exactly what's
being tested for.

Put another way, do you really want this code to trigger for LOAD_REFRESH?  Or
for LOAD_BYPASS_HISTORY?

Comment 12

13 years ago
Comment on attachment 188967 [details] [diff] [review]
patch

LOAD_NORMAL, LOAD_HISTORY, and LOAD_LINK are mutually exclusive, no?  These are
meant to be used in a case-switch statement, so checking them in a bitwise
expression like this seems wrong or at least odd.  Would it be better to test
for LOAD_CMD_NORMAL and LOAD_CMD_HISTORY instead?  Should you be using
LOAD_TYPE_HAS_FLAGS to test for nsIWebNavigation::LOAD_FLAGS_IS_LINK?
(Assignee)

Comment 13

13 years ago
Created attachment 189129 [details] [diff] [review]
patch

Ok, how about if we just check explicitly for the LOAD_STOP_CONTENT and
LOAD_STOP_CONTENT_AND_REPLACE types, which I think are what I was after
originally...
Attachment #188967 - Attachment is obsolete: true
Attachment #189129 - Flags: superreview?(darin)
Attachment #189129 - Flags: review?(darin)

Updated

13 years ago
Attachment #188967 - Flags: superreview?(darin)
Attachment #188967 - Flags: review?(darin)

Comment 14

13 years ago
Comment on attachment 189129 [details] [diff] [review]
patch

r+sr=darin with tabs replaced with spaces.
Attachment #189129 - Flags: superreview?(darin)
Attachment #189129 - Flags: superreview+
Attachment #189129 - Flags: review?(darin)
Attachment #189129 - Flags: review+
(Assignee)

Comment 15

13 years ago
Comment on attachment 189129 [details] [diff] [review]
patch

requesting approval
Attachment #189129 - Flags: approval1.8b4?
Comment on attachment 189129 [details] [diff] [review]
patch

a=me with tabs expanded.

/be
Attachment #189129 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 17

13 years ago
I think this bug can truly be closed now.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
Looks like this could have hurt btek by 20ms. Maybe it doesn't matter
I bet that the pageload tests set document.location to run.  So all that's
happening is that now we're actually testing the performance impact of bfcache
on pageload, probably for the first time ever.  Tp is also up about 10-20ms on
luna...
You need to log in before you can comment on or make changes to this bug.