Bug 148062 - Refactor ViewGestureController swipe snapshot removal to be more platform-independent
Summary: Refactor ViewGestureController swipe snapshot removal to be more platform-ind...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-15 16:07 PDT by Tim Horton
Modified: 2015-08-16 17:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (44.96 KB, patch)
2015-08-15 16:09 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (44.98 KB, patch)
2015-08-15 16:34 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (44.99 KB, patch)
2015-08-15 16:47 PDT, Tim Horton
mitz: 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 2015-08-15 16:07:50 PDT
Refactor ViewGestureController swipe snapshot removal to be more platform-independent
Comment 1 Tim Horton 2015-08-15 16:09:00 PDT
Created attachment 259106 [details]
Patch
Comment 2 Tim Horton 2015-08-15 16:09:33 PDT
Mac is good; still have to test iOS.
Comment 3 Tim Horton 2015-08-15 16:11:46 PDT
Logging looks like this:

Swipe Snapshot Removal (0.00 ms) - start
Swipe Snapshot Removal (0.14 ms) - (re)started watchdog timer for 5 seconds
Swipe Snapshot Removal (828.31 ms) - outstanding event occurred: VisuallyNonEmptyLayout 
Swipe Snapshot Removal (828.45 ms) - deferring removal; had oustanding events: RenderTreeSizeThreshold MainFrameLoad SubresourceLoads 
Swipe Snapshot Removal (828.51 ms) - (re)started watchdog timer for 3 seconds
Swipe Snapshot Removal (844.20 ms) - outstanding event occurred: RenderTreeSizeThreshold 
Swipe Snapshot Removal (844.34 ms) - deferring removal; had oustanding events: MainFrameLoad SubresourceLoads 
Swipe Snapshot Removal (2046.35 ms) - outstanding event occurred: MainFrameLoad 
Swipe Snapshot Removal (2046.46 ms) - deferring removal; had oustanding events: SubresourceLoads 
Swipe Snapshot Removal (2046.52 ms) - outstanding event occurred: SubresourceLoads 
Swipe Snapshot Removal (2046.59 ms) - removed snapshot


Swipe Snapshot Removal (0.00 ms) - start
Swipe Snapshot Removal (0.12 ms) - (re)started watchdog timer for 5 seconds
Swipe Snapshot Removal (86.57 ms) - outstanding event occurred: MainFrameLoad 
Swipe Snapshot Removal (86.68 ms) - deferring removal; had oustanding events: VisuallyNonEmptyLayout RenderTreeSizeThreshold SubresourceLoads 
Swipe Snapshot Removal (86.74 ms) - wait for event cancelled: VisuallyNonEmptyLayout 
Swipe Snapshot Removal (86.80 ms) - deferring removal; had oustanding events: RenderTreeSizeThreshold SubresourceLoads 
Swipe Snapshot Removal (86.86 ms) - outstanding event occurred: SubresourceLoads 
Swipe Snapshot Removal (86.91 ms) - deferring removal; had oustanding events: RenderTreeSizeThreshold 
Swipe Snapshot Removal (270.75 ms) - outstanding event occurred: RenderTreeSizeThreshold 
Swipe Snapshot Removal (270.91 ms) - removed snapshot
Comment 4 Tim Horton 2015-08-15 16:34:22 PDT
Created attachment 259108 [details]
Patch
Comment 5 Tim Horton 2015-08-15 16:47:24 PDT
Created attachment 259109 [details]
Patch
Comment 6 mitz 2015-08-16 16:45:58 PDT
Comment on attachment 259109 [details]
Patch

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

> Source/WebKit2/UIProcess/ViewGestureController.cpp:51
> +static HashMap<uint64_t, WebKit::ViewGestureController*>& viewGestureControllersForAllPages()
> +{
> +    // The key in this map is the associated page ID.
> +    static NeverDestroyed<HashMap<uint64_t, WebKit::ViewGestureController*>> viewGestureControllers;
> +    return viewGestureControllers.get();
> +}

Can this go inside the namespace WebKit block?

> Source/WebKit2/UIProcess/ViewGestureController.cpp:151
> +String ViewGestureController::SnapshotRemovalTracker::eventsDescription(ViewGestureController::SnapshotRemovalTracker::Events event)

This can be a static function, perhaps even a file-static function.

> Source/WebKit2/UIProcess/ViewGestureController.cpp:153
> +    String description;

StringBuilder?

> Source/WebKit2/UIProcess/ViewGestureController.cpp:238
> +bool ViewGestureController::SnapshotRemovalTracker::cancelOutstandingEvent(Events event)
> +{
> +    ASSERT(hasOneBitSet(event));
> +
> +    if (!(m_outstandingEvents & event))
> +        return false;
> +
> +    log("wait for event cancelled: " + eventsDescription(event));
> +
> +    m_outstandingEvents &= ~event;
> +
> +    fireRemovalCallbackIfPossible();
> +    return true;
> +}

Strange to have two functions that differ only in the message they log.

> Source/WebKit2/UIProcess/ViewGestureController.cpp:256
> +        log("removed snapshot");

“removing”? “will remove”? log after making the callback?

> Source/WebKit2/UIProcess/mac/ViewGestureController.h:170
> +        void log(String);

const String&?
Comment 7 Tim Horton 2015-08-16 17:44:30 PDT
(In reply to comment #6)
> Comment on attachment 259109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259109&action=review
> 
> > Source/WebKit2/UIProcess/ViewGestureController.cpp:151
> > +String ViewGestureController::SnapshotRemovalTracker::eventsDescription(ViewGestureController::SnapshotRemovalTracker::Events event)
> 
> This can be a static function, perhaps even a file-static function.

It can't be file static because SnapshotRemovalTracker is private. But static, yes. And everything else, yes. Thanks!
Comment 8 Tim Horton 2015-08-16 17:52:45 PDT
(In reply to comment #6)
> Comment on attachment 259109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259109&action=review
>
> Strange to have two functions that differ only in the message they log.

I really want to keep separate function names for clarity at callsites, but I refactored it so the majority of the code is only in one place.
Comment 9 Tim Horton 2015-08-16 17:53:48 PDT
http://trac.webkit.org/changeset/188521