RESOLVED FIXED 134667
Store ViewSnapshots directly on the WebBackForwardListItem
https://bugs.webkit.org/show_bug.cgi?id=134667
Summary Store ViewSnapshots directly on the WebBackForwardListItem
Tim Horton
Reported 2014-07-06 16:36:40 PDT
Recent back-forward changes mean that we can now avoid the UUID situation and instead store ViewSnapshots directly on the WebBackForwardListItem. This also means we can make the memory management of snapshots much smarter. <rdar://problem/17082639>
Attachments
patch (36.71 KB, patch)
2014-07-09 20:07 PDT, Tim Horton
no flags
patch (36.86 KB, patch)
2014-07-10 12:44 PDT, Tim Horton
mitz: review+
Tim Horton
Comment 1 2014-07-09 20:07:40 PDT
WebKit Commit Bot
Comment 2 2014-07-09 20:10:20 PDT
Attachment 234683 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:62: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:64: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:91: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:93: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:132: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:136: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:163: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 7 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
mitz
Comment 3 2014-07-09 20:19:41 PDT
Comment on attachment 234683 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=234683&action=review > Source/WebKit2/ChangeLog:13 > + prefering older snapshots. Additionally, we would not throw away snapshots when back forward items typo: “prefering” > Source/WebKit2/ChangeLog:75 > + Add create methods which take a image (or slot ID), and relevant sizes. typo: “methods” typo: “a image”
mitz
Comment 4 2014-07-09 20:42:36 PDT
Comment on attachment 234683 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=234683&action=review > Source/WebKit2/Shared/WebBackForwardListItem.h:61 > + void setSnapshot(ViewSnapshot* snapshot) { m_itemState.snapshot = snapshot; } Should this take a PassRefPtr? > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:-895 > - snapshot.imageSizeInBytes = snapshotSize.width * snapshotSize.height * 4; It’s surprising that these images’ rowBytes can be any multiple of 4. > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:46 > #define USE_RENDER_SERVER_VIEW_SNAPSHOTS true We normally use 1, why not here? > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:102 > + size_t m_imageSizeInBytes; Any reason why you removed the = 0 here but not from m_slotID? > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:133 > +protected: > + void didAddImageToSnapshot(ViewSnapshot&); > + void willRemoveImageFromSnapshot(ViewSnapshot&); > + Why protected? > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:141 > + SnapshotSet m_allSnapshotsWithImages; I think “all” is implied. We don’t call the GraphicsLayer member variable m_allChildren. > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:86 > + m_allSnapshotsWithImages.add(&snapshot); Can you assert that the snapshot is not already in the set? > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:92 > + m_allSnapshotsWithImages.remove(&snapshot); Can you assert that the snapshot is in the set? > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:143 > + for (auto& snapshot : m_allSnapshotsWithImages) > + snapshot->clearImage(); Is this iterating a set that’s being mutated? Why is this OK? > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:194 > + if (m_slotID) How can this be false?
mitz
Comment 5 2014-07-09 20:43:13 PDT
This looks good. I didn’t r+ because of my question about mutation during iteration.
Tim Horton
Comment 6 2014-07-10 09:29:13 PDT
Comment on attachment 234683 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=234683&action=review >> Source/WebKit2/Shared/WebBackForwardListItem.h:61 >> + void setSnapshot(ViewSnapshot* snapshot) { m_itemState.snapshot = snapshot; } > > Should this take a PassRefPtr? Certainly could! >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:-895 >> - snapshot.imageSizeInBytes = snapshotSize.width * snapshotSize.height * 4; > > It’s surprising that these images’ rowBytes can be any multiple of 4. We don't really have access to the image, so we can't tell its true size (at least, I don't know how). We could *assume* the same alignment properties as IOSurface, but since this "size in bytes" is just used for max cache size computation, it's OK that it's just an estimate. >> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:46 >> #define USE_RENDER_SERVER_VIEW_SNAPSHOTS true > > We normally use 1, why not here? Fine with me. >> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:102 >> + size_t m_imageSizeInBytes; > > Any reason why you removed the = 0 here but not from m_slotID? No, I meant to get that one too. >> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h:141 >> + SnapshotSet m_allSnapshotsWithImages; > > I think “all” is implied. We don’t call the GraphicsLayer member variable m_allChildren. Quite right. >> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:92 >> + m_allSnapshotsWithImages.remove(&snapshot); > > Can you assert that the snapshot is in the set? Both good plans. >> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:143 >> + snapshot->clearImage(); > > Is this iterating a set that’s being mutated? Why is this OK? It's not! Will fix. > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:175 > + clearImage(); This needs to be conditional on whether we still have an image. >> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:194 >> + if (m_slotID) > > How can this be false? createImageSlot is definitely not guaranteed to succeed, but we should probably check that in _takeViewSnapshot instead :D
Tim Horton
Comment 7 2014-07-10 12:44:49 PDT
WebKit Commit Bot
Comment 8 2014-07-10 12:47:13 PDT
Attachment 234720 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:156: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
mitz
Comment 9 2014-07-10 13:06:47 PDT
Comment on attachment 234720 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=234720&action=review > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:168 > + clearImage(); You said this needed to be conditional on whether there was an image.
Tim Horton
Comment 10 2014-07-10 13:30:14 PDT
(In reply to comment #9) > (From update of attachment 234720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234720&action=review > > > Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:168 > > + clearImage(); > > You said this needed to be conditional on whether there was an image. I did, but then I put the condition inside it :D
Tim Horton
Comment 11 2014-07-10 13:33:37 PDT
Note You need to log in before you can comment on or make changes to this bug.