Bug 138755

Summary: [CSS Font Loading] Switch to dispatching events asynchronously
Product: WebKit Reporter: Bear Travis <betravis>
Component: WebCore Misc.Assignee: Bear Travis <betravis>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mmaxfield, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 140249, 135390, 139355    
Attachments:
Description Flags
Initial Patch
none
Modified Patch
none
Updated Patch
none
Updated Patch
none
Modifying timer
none
testing DRT fix
none
Incorporating feedback
simon.fraser: review+
Updated Patch, including test none

Description Bear Travis 2014-11-14 14:03:54 PST
The FontLoader code should be refactored to take advantage of EventSender rather than maintaining its own event dispatch timing logic.
Comment 1 Bear Travis 2014-11-14 14:22:04 PST
Created attachment 241625 [details]
Initial Patch
Comment 2 Bear Travis 2014-11-17 12:17:54 PST
I was originally going to use EventSender, but it was not really designed for this use case. Would ideally be able to reuse something like the GenericEventQueue, but I need to fire the callbacks at the appropriate time as well, so I'm bundling the timer logic into FontLoader.
Comment 3 Bear Travis 2014-11-17 12:32:17 PST
Created attachment 241734 [details]
Modified Patch
Comment 4 Bear Travis 2014-11-17 13:55:28 PST
Created attachment 241740 [details]
Updated Patch
Comment 5 Bear Travis 2014-11-18 12:31:11 PST
Created attachment 241802 [details]
Updated Patch
Comment 6 Bear Travis 2014-11-19 17:32:52 PST
Created attachment 241913 [details]
Modifying timer
Comment 7 Simon Fraser (smfr) 2014-11-20 10:06:01 PST
Comment on attachment 241913 [details]
Modifying timer

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

> Source/WebCore/css/FontLoader.cpp:172
>      Vector<RefPtr<Event> > pendingEvents;

No need for the space between > and >

> Source/WebCore/css/FontLoader.cpp:185
> +        Vector<RefPtr<VoidCallback> > callbacks;

No need for space between > and >

> Source/WebCore/css/FontLoader.h:98
> +    void timerFired(Timer&) { firePendingEvents(); }

Please rename to pendingEventsTimerFired().

> Source/WebCore/css/FontLoader.h:110
> +    std::unique_ptr<Timer> m_pendingEventTimer;

We normally just store Timers by value.
Comment 8 Bear Travis 2014-11-20 12:47:08 PST
Created attachment 241970 [details]
testing DRT fix
Comment 9 Bear Travis 2014-11-20 12:49:06 PST
Comment on attachment 241970 [details]
testing DRT fix

This probably needs to be broken up for the DRT fix on Windows, but I want to make sure it passes the bots first. So not yet ready for review.
Comment 10 Bear Travis 2014-11-20 13:01:34 PST
Created attachment 241971 [details]
Incorporating feedback
Comment 11 Simon Fraser (smfr) 2014-11-20 13:55:57 PST
Comment on attachment 241971 [details]
Incorporating feedback

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

> Source/WebCore/ChangeLog:10
> +        to EventSender or the GenericEventQueue. Several bugs have popped
> +        up in the past because FontLoader has been sending events during

Would be nice to add a testcase for that specific behavior (since it's very bad if it happens).
Comment 12 Bear Travis 2014-11-20 15:56:19 PST
Created attachment 241998 [details]
Updated Patch, including test

Adding one of the test cases that caused the need for this refactoring.
Comment 13 WebKit Commit Bot 2014-11-21 10:43:10 PST
Comment on attachment 241998 [details]
Updated Patch, including test

Clearing flags on attachment: 241998

Committed r176453: <http://trac.webkit.org/changeset/176453>
Comment 14 WebKit Commit Bot 2014-11-21 10:43:14 PST
All reviewed patches have been landed.  Closing bug.