Closed Bug 292948 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bryner)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Attached file Testcase
Note that if you click the "resize" button again after going back you can verify
that the resize handler is very much in place.
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: blazinglyfastback
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.
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.
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
Closed: 18 years ago
Resolution: --- → WORKSFORME
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 → ---
Attached patch patch (obsolete) — Splinter Review
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 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?
Attached patch patchSplinter Review
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)
Attachment #188967 - Flags: superreview?(darin)
Attachment #188967 - Flags: review?(darin)
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+
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+
I think this bug can truly be closed now.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 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.