CSS Font Loading: FontFaceSet.ready should return the same promise each time

CSS Font Loading: FontFaceSet.ready should return the same promise each time

Use ScriptPromiseProperty to return same promise every time, until the
FontFaceSet switches from loaded to loading state. This is in line with
the spec, and also simplifies the code a bit.

TEST=fast/css/fontfaceset-ready.html
BUG=510434
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201857

[Kunihiko Sakamoto, 2015-09-01 08:39:55]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2015-09-01 08:42:31]

ksakamoto@chromium.org changed reviewers:
+ dominicc@chromium.org

[Kunihiko Sakamoto, 2015-09-01 08:42:31]

Dominic, could you take a look?

[dominicc (has gone to gerrit), 2015-09-01 18:27:26]

dominicc@chromium.org changed reviewers:
+ tabatkins@chromium.org

[dominicc (has gone to gerrit), 2015-09-01 18:29:15]

On 2015/09/01 at 08:42:31, ksakamoto wrote:
> Dominic, could you take a look?

Tab: Would you be interested in reviewing the layout test in this patch?

Sakamoto-san:

This looks nice!

Are there upstream W3C tests we could look at importing for this as well, in a
separate patch?

[dominicc (has gone to gerrit), 2015-09-02 00:14:07]

Oops, sending a message may have not sent comments. Here they are:

https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
File LayoutTests/fast/css/fontfaceset-ready.html (right):

https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
LayoutTests/fast/css/fontfaceset-ready.html:21: var newReady =
document.fonts.ready;
Exactly when does document.fonts.ready get reset? It would be good to
assert_equals (or set newReady if it is already reset) one line up.

https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
LayoutTests/fast/css/fontfaceset-ready.html:22: assert_not_equals(newReady,
oldReady,
Because resetting [[ReadyPromise]] happens at the same time as touching status,
could you add assertions about status here too?

https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
Source/core/css/FontFaceSet.cpp:259: if (!m_ready)
Pretty much everyone creates these lazily, and while they're to *free*, is it
worth it? Could you always initialize m_ready and simplify your code (no if
(m_ready); no m_ready &&). You might even be able to make the implementation of
status just check the state of the Promise for some things (eg if pending, then
status is loading) and simplify things even more.

[dominicc (has gone to gerrit), 2015-09-02 00:31:13]

On 2015/09/02 at 00:14:07, dominicc wrote:

...
>
https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
> Source/core/css/FontFaceSet.cpp:259: if (!m_ready)
> Pretty much everyone creates these lazily, and while they're to *free*, is it
worth it?

Gah! Tired :) I meant while they *aren't free*, I'm not sure it is worth doing
the lazy creation. The ScriptPromiseProperty already handles things lazily
per-world; the real cost is to have another lifecycle observer. It's not that
expensive.

[Kunihiko Sakamoto, 2015-09-02 05:05:42]

As far as I know there are no W3C tests for CSS font loading yet.

https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
File LayoutTests/fast/css/fontfaceset-ready.html (right):

https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
LayoutTests/fast/css/fontfaceset-ready.html:21: var newReady =
document.fonts.ready;
On 2015/09/02 00:14:06, dominicc wrote:
> Exactly when does document.fonts.ready get reset? It would be good to
> assert_equals (or set newReady if it is already reset) one line up.

That is when face.load() is called. Added another assertion above to make it
clear.

https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
LayoutTests/fast/css/fontfaceset-ready.html:22: assert_not_equals(newReady,
oldReady,
On 2015/09/02 00:14:06, dominicc wrote:
> Because resetting [[ReadyPromise]] happens at the same time as touching
status,
> could you add assertions about status here too?

Done.

https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
File Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
Source/core/css/FontFaceSet.cpp:259: if (!m_ready)
On 2015/09/02 00:14:06, dominicc wrote:
> Pretty much everyone creates these lazily, and while they're to *free*, is it
> worth it? Could you always initialize m_ready and simplify your code (no if
> (m_ready); no m_ready &&). You might even be able to make the implementation
of
> status just check the state of the Promise for some things (eg if pending,
then
> status is loading) and simplify things even more.

Done.
I think m_isLoading is still needed, since FontFaceSet.status is iniitally
"loaded" while .ready starts with a pending promise.

[dominicc (has gone to gerrit), 2015-09-07 04:38:27]

On 2015/09/02 at 05:05:42, ksakamoto wrote:
> As far as I know there are no W3C tests for CSS font loading yet.
> 
>
https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
> File LayoutTests/fast/css/fontfaceset-ready.html (right):
> 
>
https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
> LayoutTests/fast/css/fontfaceset-ready.html:21: var newReady =
document.fonts.ready;
> On 2015/09/02 00:14:06, dominicc wrote:
> > Exactly when does document.fonts.ready get reset? It would be good to
> > assert_equals (or set newReady if it is already reset) one line up.
> 
> That is when face.load() is called. Added another assertion above to make it
clear.
> 
>
https://codereview.chromium.org/1313853003/diff/20001/LayoutTests/fast/css/fo...
> LayoutTests/fast/css/fontfaceset-ready.html:22: assert_not_equals(newReady,
oldReady,
> On 2015/09/02 00:14:06, dominicc wrote:
> > Because resetting [[ReadyPromise]] happens at the same time as touching
status,
> > could you add assertions about status here too?
> 
> Done.
> 
>
https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
> File Source/core/css/FontFaceSet.cpp (right):
> 
>
https://codereview.chromium.org/1313853003/diff/20001/Source/core/css/FontFac...
> Source/core/css/FontFaceSet.cpp:259: if (!m_ready)
> On 2015/09/02 00:14:06, dominicc wrote:
> > Pretty much everyone creates these lazily, and while they're to *free*, is
it
> > worth it? Could you always initialize m_ready and simplify your code (no if
> > (m_ready); no m_ready &&). You might even be able to make the implementation
of
> > status just check the state of the Promise for some things (eg if pending,
then
> > status is loading) and simplify things even more.
> 
> Done.
> I think m_isLoading is still needed, since FontFaceSet.status is iniitally
"loaded" while .ready starts with a pending promise.

Great work! This looks like it not only corrects things but actually makes them
simpler.

[dominicc (has gone to gerrit), 2015-09-07 04:38:30]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2015-09-07 04:38:30]

lgtm

[commit-bot: I haz the power, 2015-09-07 04:38:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313853003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313853003/40001

[commit-bot: I haz the power, 2015-09-07 05:41:25]

Committed patchset #2 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201857