[Font Load Events] Implement FontFaceSet methods

[Font Load Events] Implement FontFaceSet methods

* A new method match() returns font faces that match a given font string
* checkFont() is renamed to check()
* loadFont() is translated to load(), which returns Promise that fulfills
  when all loads complete
* notifyWhenFontsReady() is translated to ready(), which returns Promise
* Boolean 'loading' attribute is changed to string 'status' attribute

spec: http://dev.w3.org/csswg/css-font-load-events/#FontFaceSet-interface

BUG=53213
TEST=fast/css

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158006

[Kunihiko Sakamoto, 2013-09-18 06:04:31]

yhirano, could you take a look at FontFaceSet.cpp to see if I am using Promise
in the right way?

[yhirano, 2013-09-18 11:49:44]

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.cpp:276: ScriptPromise promise =
resolver->promise();
You should call ScriptPromiseResolver::detachPromise if you don't access the
promise object via the resolver again.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.cpp:357: }
What happens if face == NULL for all f in the loop?
The resolver seems never to be resolved nor rejected.
You can call reject in LoadFontPromiseResolver::error because a fulfill / reject
/ resolve call to a fulfilled / rejected / resolved resolver shall be ignored.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.cpp:358: return resolver->promise();
You should call ScriptPromiseResolver::detachPromise if you don't access the
promise object via the resolver again.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSet.h
File Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.h:122: Vector<RefPtr<LoadFontPromiseResolver> >
m_pendingLoadPromises;
I don't think m_pendingLoadPromises is a right name because it is not a list of
Promises.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.h:123: Vector<OwnPtr<FontsReadyPromiseResolver> >
m_readyPromises;
ditto

[Kunihiko Sakamoto, 2013-09-19 01:14:25]

Thanks for the comments.
PTAL

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.cpp:276: ScriptPromise promise =
resolver->promise();
On 2013/09/18 11:49:45, yhirano wrote:
> You should call ScriptPromiseResolver::detachPromise if you don't access the
> promise object via the resolver again.

Changed FontsReadyPromiseResolver::promise() detaches the promise.

FWIW it would be useful if ScriptPromiseResolver provided such a utility
function.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.cpp:357: }
On 2013/09/18 11:49:45, yhirano wrote:
> What happens if face == NULL for all f in the loop?
Then FontFaceSet::scheduleResolve is called inside
LoadFontPromiseResolver::error, so the promise will be resolved eventually.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.cpp:358: return resolver->promise();
On 2013/09/18 11:49:45, yhirano wrote:
> You should call ScriptPromiseResolver::detachPromise if you don't access the
> promise object via the resolver again.

Now LoadFontPromiseResolver::promise() does that.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSet.h
File Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.h:122: Vector<RefPtr<LoadFontPromiseResolver> >
m_pendingLoadPromises;
On 2013/09/18 11:49:45, yhirano wrote:
> I don't think m_pendingLoadPromises is a right name because it is not a list
of
> Promises.

Renamed to m_pendingLoadResolvers.

https://codereview.chromium.org/23717059/diff/4001/Source/core/css/FontFaceSe...
Source/core/css/FontFaceSet.h:123: Vector<OwnPtr<FontsReadyPromiseResolver> >
m_readyPromises;
On 2013/09/18 11:49:45, yhirano wrote:
> ditto

Changed to m_readyResolvers.

[yhirano, 2013-09-19 04:00:15]

lgtm

[Kunihiko Sakamoto, 2013-09-19 04:03:33]

Thank you Hirano-san.

Eric, could you please take a look?

[eseidel, 2013-09-19 04:11:18]

I'm not a promises expert, but otherwise lgtm.

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/CSSSegmen...
File Source/core/css/CSSSegmentedFontFace.cpp (right):

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/CSSSegmen...
Source/core/css/CSSSegmentedFontFace.cpp:199: Vector<RefPtr<FontFace> >
fontFaces;
Thanks to the Named Return Value Optimization this will not copy the vector when
returning.

Slightly fragile.  But should work.

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
Source/core/css/FontFaceSet.cpp:392: if (fontString.isEmpty())
I believe (!fontString) is equivalent, but this is fine too.

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
File Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
Source/core/css/FontFaceSet.h:41: // Mac OS X 10.6 SDK defines check() macro
that interfares with our check() method
We've worked around this in the past using ImplementedAs=_check in the idl
bindings.  This is also OK.

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
Source/core/css/FontFaceSet.h:76: String status() const { return (m_loadingCount
> 0 || m_shouldFireDoneEvent) ? "loading" : "loaded"; }
AtomicString?

[Kunihiko Sakamoto, 2013-09-19 04:49:31]

Thank you Eric for the quick turnaround!
I got lgtm from a promises expert (yhirano) so this should be okay :)

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
Source/core/css/FontFaceSet.cpp:392: if (fontString.isEmpty())
On 2013/09/19 04:11:18, eseidel wrote:
> I believe (!fontString) is equivalent, but this is fine too.

!s is equivalent to !s.isNull(), but we don't want to pass empty string to
CSSParser::parseValue().

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
File Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
Source/core/css/FontFaceSet.h:41: // Mac OS X 10.6 SDK defines check() macro
that interfares with our check() method
On 2013/09/19 04:11:18, eseidel wrote:
> We've worked around this in the past using ImplementedAs=_check in the idl
> bindings.  This is also OK.

I see.

https://codereview.chromium.org/23717059/diff/18001/Source/core/css/FontFaceS...
Source/core/css/FontFaceSet.h:76: String status() const { return (m_loadingCount
> 0 || m_shouldFireDoneEvent) ? "loading" : "loaded"; }
On 2013/09/19 04:11:18, eseidel wrote:
> AtomicString?

Done.

[commit-bot: I haz the power, 2013-09-19 04:50:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ksakamoto@chromium.org/23717059/28001

[commit-bot: I haz the power, 2013-09-19 06:08:23]

Change committed as 158006