Bug 138755 - [CSS Font Loading] Switch to dispatching events asynchronously
Summary: [CSS Font Loading] Switch to dispatching events asynchronously
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
Depends on:
Blocks: 140249 135390 139355
  Show dependency treegraph
 
Reported: 2014-11-14 14:03 PST by Bear Travis
Modified: 2015-01-08 09:37 PST (History)
3 users (show)

See Also:


Attachments
Initial Patch (11.40 KB, patch)
2014-11-14 14:22 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Modified Patch (11.64 KB, patch)
2014-11-17 12:32 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updated Patch (11.61 KB, patch)
2014-11-17 13:55 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updated Patch (11.61 KB, patch)
2014-11-18 12:31 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Modifying timer (11.91 KB, patch)
2014-11-19 17:32 PST, Bear Travis
no flags Details | Formatted Diff | Diff
testing DRT fix (12.43 KB, patch)
2014-11-20 12:47 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Incorporating feedback (11.75 KB, patch)
2014-11-20 13:01 PST, Bear Travis
simon.fraser: review+
Details | Formatted Diff | Diff
Updated Patch, including test (13.06 KB, patch)
2014-11-20 15:56 PST, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.