Bug 13631 - Page Cache should support pages with frames
Summary: Page Cache should support pages with frames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks: 4123 20806
  Show dependency treegraph
 
Reported: 2007-05-08 19:50 PDT by Brady Eidson
Modified: 2009-09-03 16:57 PDT (History)
4 users (show)

See Also:


Attachments
Refactor related code out of DocumentLoader and enhance logging (8.44 KB, patch)
2009-08-17 17:25 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Cleanup and scaffolding - no change in behavior (9.97 KB, patch)
2009-09-01 10:58 PDT, Brady Eidson
sam: review+
Details | Formatted Diff | Diff
A next step - more of the handling of the "CachedFrame tree" (7.36 KB, patch)
2009-09-02 09:40 PDT, Brady Eidson
sam: review+
Details | Formatted Diff | Diff
Another step - getting close! (9.14 KB, patch)
2009-09-02 16:09 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Almost there... (5.33 KB, patch)
2009-09-02 17:19 PDT, Brady Eidson
darin: review-
Details | Formatted Diff | Diff
Address Darin's comments (6.80 KB, patch)
2009-09-02 17:38 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Enable back/forward cache for pages with frames (along with some layout tests) (11.15 KB, patch)
2009-09-03 16:39 PDT, Brady Eidson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2007-05-08 19:50:38 PDT
The back/forward cache needs to work with pages that have frames

<rdar://problem/4886592>
Comment 1 Brady Eidson 2009-08-17 17:22:36 PDT
Back to working on this lately.  It will be coming in baby steps, trying to make the code a little more sane as we get closer to flipping the switch.
Comment 2 Brady Eidson 2009-08-17 17:25:25 PDT
Created attachment 35002 [details]
Refactor related code out of DocumentLoader and enhance logging
Comment 3 Eric Seidel (no email) 2009-08-17 18:13:51 PDT
Comment on attachment 35002 [details]
Refactor related code out of DocumentLoader and enhance logging

It's now possible to test the page cache (with the setting override stuff).  Dumi (I think?) added the first page-cache test the other day.  Seems we should add one for this change too.
Comment 4 Brady Eidson 2009-08-17 18:24:32 PDT
If he added page cache layout tests, that's great.

In this situation, I wish you could've told me *where* he added these, because I personally don't have time to watch all the commits go by on a day to day basis and don't know where to look.

Regardless, since this patch is a pure refactoring and has zero change in behavior, testing the change in behavior for regressions is a non-starter.  

I did run the layout tests and there were no new failures.

Putting back up for review.
Comment 5 Brady Eidson 2009-08-17 18:25:14 PDT
Comment on attachment 35002 [details]
Refactor related code out of DocumentLoader and enhance logging

Layout test not possible, zero behavior change.  Putting back up for review.
Comment 6 Brady Eidson 2009-08-17 18:28:38 PDT
Sam Weinig reviewed this over my shoulder.
Comment 7 Brady Eidson 2009-08-17 18:38:55 PDT
This cleanup landed in r47407.
Comment 8 Brady Eidson 2009-09-01 10:30:43 PDT
Updated Radar URL: <rdar://problem/3541409>
Comment 9 Brady Eidson 2009-09-01 10:58:29 PDT
Created attachment 38871 [details]
Cleanup and scaffolding - no change in behavior
Comment 10 Brady Eidson 2009-09-01 11:14:31 PDT
Landed in http://trac.webkit.org/changeset/47943
Comment 11 Brady Eidson 2009-09-02 09:40:10 PDT
Created attachment 38928 [details]
A next step - more of the handling of the "CachedFrame tree"

Again, this doesn't change current behavior, and is just another step in landing my work piece-by-piece.
Comment 12 Brady Eidson 2009-09-02 12:33:26 PDT
This morning's patch landed in http://trac.webkit.org/changeset/47985
More coming...
Comment 13 Brady Eidson 2009-09-02 13:17:19 PDT
http://trac.webkit.org/changeset/47989 is related to this work, as well.
Comment 14 Brady Eidson 2009-09-02 16:09:54 PDT
Created attachment 38944 [details]
Another step - getting close!
Comment 15 Brady Eidson 2009-09-02 17:08:24 PDT
http://trac.webkit.org/changeset/47999
Comment 16 Brady Eidson 2009-09-02 17:19:12 PDT
Created attachment 38951 [details]
Almost there...
Comment 17 Darin Adler 2009-09-02 17:26:17 PDT
Comment on attachment 38951 [details]
Almost there...

Should use an enum instead of adding a new bool argument.

It's true that in one place the argument is a boolean expression, not a constant, but I still think that these confusing functions can be made less so with enums.

In fact, I think you could combine the sendUnload and sendPagehide bool arguments into a single enum if you name the values carefully.
Comment 18 Brady Eidson 2009-09-02 17:38:52 PDT
Created attachment 38955 [details]
Address Darin's comments
Comment 19 Brady Eidson 2009-09-02 17:56:00 PDT
Landed in http://trac.webkit.org/changeset/48001

I'll flip the switch sometime tomorrow, after I get a chance to update and build on Windows.
Comment 20 Brady Eidson 2009-09-03 16:10:55 PDT
The patch was reviewed over my shoulder, so it's not in the list, but:
http://trac.webkit.org/changeset/48034
Comment 21 Brady Eidson 2009-09-03 16:39:13 PDT
Created attachment 39019 [details]
Enable back/forward cache for pages with frames (along with some layout tests)
Comment 22 Brady Eidson 2009-09-03 16:57:48 PDT
Landed in http://trac.webkit.org/changeset/48036