Bug 134667 – Store ViewSnapshots directly on the WebBackForwardListItem

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

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>

[Tim Horton]

Created attachment 234683 [details]
patch

[WebKit Commit Bot]

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.

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”

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?

This looks good. I didn’t r+ because of my question about mutation during iteration.

[Tim Horton]

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]

Created attachment 234720 [details]
patch

[WebKit Commit Bot]

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.

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]

(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]

http://trac.webkit.org/changeset/170974