Bug 148062 – Refactor ViewGestureController swipe snapshot removal to be more platform-independent

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Refactor ViewGestureController swipe snapshot removal to be more platform-independent

[Tim Horton]

Created attachment 259106 [details]
Patch

[Tim Horton]

Mac is good; still have to test iOS.

[Tim Horton]

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

[Tim Horton]

Created attachment 259108 [details]
Patch

[Tim Horton]

Created attachment 259109 [details]
Patch

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&?

[Tim Horton]

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

[Tim Horton]

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

[Tim Horton]

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