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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that if you click the "resize" button again after going back you can verify
that the resize handler is very much in place.

Comment 3

[Brian Ryner (not reading)]

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).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase showing why this is a problem

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Brian Ryner (not reading)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: David's testcase in an iframe

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Brian Ryner (not reading)]

Attachment: 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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

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?

Comment 13

[Brian Ryner (not reading)]

Attachment: 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...

Comment 14

[Darin Fisher]

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

r+sr=darin with tabs replaced with spaces.

Comment 15

[Brian Ryner (not reading)]

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

requesting approval

Comment 16

[Brendan Eich [:brendan]]

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

a=me with tabs expanded.

/be

Comment 17

[Brian Ryner (not reading)]

I think this bug can truly be closed now.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like this could have hurt btek by 20ms. Maybe it doesn't matter

Comment 19

[Boris Zbarsky [:bzbarsky]]

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...