Bug 132115 - WebKit2 View Gestures (Swipe): Discard snapshots made with a different view size/pixel density
Summary: WebKit2 View Gestures (Swipe): Discard snapshots made with a different view s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
: 132116 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-04-24 01:36 PDT by Tim Horton
Modified: 2014-04-28 14:07 PDT (History)
4 users (show)

See Also:


Attachments
patch (21.89 KB, patch)
2014-04-26 03:06 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
fix the build (22.09 KB, patch)
2014-04-26 10:46 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
fix the ML build (22.08 KB, patch)
2014-04-26 19:02 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
pulled crash fix into another bug (21.52 KB, patch)
2014-04-26 19:03 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-04-24 01:36:20 PDT
We shouldn't even bother trying to use swipe snapshots taken at a different view size or device pixel ratio, they always look terrible.

<rdar://problem/16681791>
Comment 1 Tim Horton 2014-04-26 02:52:53 PDT
*** Bug 132116 has been marked as a duplicate of this bug. ***
Comment 2 Tim Horton 2014-04-26 03:06:54 PDT
Created attachment 230239 [details]
patch
Comment 3 Tim Horton 2014-04-26 10:46:51 PDT
Created attachment 230244 [details]
fix the build
Comment 4 Tim Horton 2014-04-26 15:38:25 PDT
Comment on attachment 230244 [details]
fix the build

View in context: https://bugs.webkit.org/attachment.cgi?id=230244&action=review

> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:154
>      String oldSnapshotUUID = item->snapshotUUID();
> -    if (!oldSnapshotUUID.isEmpty())
> -        m_snapshotMap.remove(oldSnapshotUUID);
> +    if (!oldSnapshotUUID.isEmpty()) {
> +        const auto& oldSnapshotIter = m_snapshotMap.find(oldSnapshotUUID);
> +        if (oldSnapshotIter != m_snapshotMap.end()) {
> +            if (oldSnapshotIter->value.hasImage())
> +                m_snapshotsWithImagesCount--;
> +            m_snapshotMap.remove(oldSnapshotIter);
> +        }
> +    }
>  

Pulled this bit out into https://bugs.webkit.org/show_bug.cgi?id=132204
Comment 5 Tim Horton 2014-04-26 19:02:27 PDT
Created attachment 230250 [details]
fix the ML build
Comment 6 Tim Horton 2014-04-26 19:03:36 PDT
Created attachment 230251 [details]
pulled crash fix into another bug
Comment 7 Tim Horton 2014-04-28 10:30:17 PDT
Comment on attachment 230251 [details]
pulled crash fix into another bug

View in context: https://bugs.webkit.org/attachment.cgi?id=230251&action=review

> Source/WebKit2/ChangeLog:65
> +        Fix a bug where the count of snapshots with live images was too high
> +        because we were failing to decrement it when replacing a snapshot of
> +        an existing item with a fresh one.

get rid of the changelog part of this too (code change moved to a different bug)
Comment 8 Simon Fraser (smfr) 2014-04-28 10:48:48 PDT
Comment on attachment 230251 [details]
pulled crash fix into another bug

View in context: https://bugs.webkit.org/attachment.cgi?id=230251&action=review

> Source/WebKit2/ChangeLog:9
> +        To do this, we need an accurate picture of the topContentInset, both for the Web view,

"picture" in the context of snapshotting is ... confusing.

> Source/WebKit2/UIProcess/mac/ViewGestureController.h:137
> +    CALayer *determineSnapshotLayerParent();
> +    CALayer *determineLayerAdjacentToSnapshotForParent(SwipeDirection, CALayer *snapshotLayerParent);

Not sure why these aren't const.
Comment 9 Tim Horton 2014-04-28 14:07:59 PDT
http://trac.webkit.org/changeset/167905