768 bytes, text/html
975 bytes, text/html; charset=UTF-8
589 bytes, text/html
729 bytes, patch
Darin Fisher: review+
Darin Fisher: superreview+
|Details | Diff | Splinter Review|
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.
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.
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
Last Resolved: 13 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 → ---
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.
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?
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...
Comment on attachment 189129 [details] [diff] [review] patch r+sr=darin with tabs replaced with spaces.
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
Last Resolved: 13 years ago → 13 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.