CSS Font Loading: make FontFaceSet Setlike

CSS Font Loading: make FontFaceSet Setlike

Declare FontFaceSet IDL with setlike<FontFaceSet> to add
entries(), keys(), and values().

This patch adds entries(), keys(), and values() methods,
and switches to use Iterable's implementation for forEach()
and has().

BUG=392075
Committed: https://crrev.com/7d9f3ae0f1993db746e41659627be241d3586270
Cr-Commit-Position: refs/heads/master@{#354742}

[Takashi Toyoshima, 2015-10-15 07:53:43]

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

[Takashi Toyoshima, 2015-10-16 05:10:04]

toyoshim@chromium.org changed reviewers:
+ yhirano@chromium.org

[Takashi Toyoshima, 2015-10-16 05:31:32]

ksakamoto@ for WebFonts, and yhirano@ for Setlike.
Can you guys take a look this?

[Takashi Toyoshima, 2015-10-16 06:36:34]

Sorry. I noticed one bug after requesting the review. I already fixed one point
in the latest patch set.

https://codereview.chromium.org/1409433003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:85: bool
hasForBinding(ScriptState*, const PassRefPtrWillBeRawPtr<FontFace>&,
ExceptionState&) const;
I noticed the type was wrong here.
The last patch set just changed the type PassRefPtrWillBeRawPtr here.

[Kunihiko Sakamoto, 2015-10-16 08:07:12]

> This patch also changes the second argument of
> ForEachCallback to conform the latest spec.
>
> https://drafts.csswg.org/css-font-loading/#callbackdef-foreachcallback

I think this ForEachCallback is a leftover from when the spec had its own Set
methods, introduced here: https://hg.csswg.org/drafts/rev/5d3f09766bba

Firefox calls the callback with [FontFace, FontFace, FontFaceSet], as ES6 Set
does. We should match this behavior.

[Takashi Toyoshima, 2015-10-16 08:50:36]

Thanks. So reverting the forEach behavior change can make things simple.
PTAL Patch Set 7. Now we can use Iterable.h without any change.

[Kunihiko Sakamoto, 2015-10-16 09:50:07]

lgtm with nits

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html
(right):

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html:36:
shouldBeFalse('document.fonts.has(nonCssConnectedFace)');
Can you move the rest of this function (line 36-40) to line 83? I think there's
no point checking them 4 times.

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html:43:
function callback(face, index, set) {
Second parameter is no longer an index.

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:539: bool
FontFaceSet::IterationSource::next(ScriptState* scriptState,
RefPtrWillBeMember<FontFace>& key, RefPtrWillBeMember<FontFace>& value,
ExceptionState& exceptionState)
nit: omit unused parameter names

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:124: IterationSource(const
WillBeHeapVector<RefPtrWillBeMember<FontFace>> fontFaces)
nit: explicit, and maybe a reference parameter?

[Takashi Toyoshima, 2015-10-16 10:07:03]

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html
(right):

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html:36:
shouldBeFalse('document.fonts.has(nonCssConnectedFace)');
On 2015/10/16 09:50:07, Kunihiko Sakamoto wrote:
> Can you move the rest of this function (line 36-40) to line 83? I think
there's
> no point checking them 4 times.

Done.

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html:43:
function callback(face, index, set) {
On 2015/10/16 09:50:07, Kunihiko Sakamoto wrote:
> Second parameter is no longer an index.

Done.

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.cpp (right):

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.cpp:539: bool
FontFaceSet::IterationSource::next(ScriptState* scriptState,
RefPtrWillBeMember<FontFace>& key, RefPtrWillBeMember<FontFace>& value,
ExceptionState& exceptionState)
On 2015/10/16 09:50:07, Kunihiko Sakamoto wrote:
> nit: omit unused parameter names

Done.

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:87: bool
hasForBinding(ScriptState*, const PassRefPtrWillBeRawPtr<FontFace>&,
ExceptionState&) const;
The second argument seems to be FontFace* here.

https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:124: IterationSource(const
WillBeHeapVector<RefPtrWillBeMember<FontFace>> fontFaces)
On 2015/10/16 09:50:07, Kunihiko Sakamoto wrote:
> nit: explicit, and maybe a reference parameter?

Done.

[Takashi Toyoshima, 2015-10-16 10:08:17]

toyoshim@chromium.org changed reviewers:
+ haraken@chromium.org
- yhirano@chromium.org

[Takashi Toyoshima, 2015-10-16 10:10:27]

+haraken@ for OWNER approval.
move yhirano@ from R to CC (as optional reviewer) because now this change does
not contain any Iterable.h modification.

[haraken, 2015-10-16 13:15:14]

https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:130:
WillBeHeapVector<RefPtrWillBeMember<FontFace>> m_fontFaces;

You need to define:

DEFINE_INLINE_TRACE()
{
  visitor->trace(m_fontFace);
}

Otherwise, oilpan builds will fail. Would you check if this CL compiles with
'enable_oilpan=1'?

[Takashi Toyoshima, 2015-10-19 05:23:35]

PTAL

https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:130:
WillBeHeapVector<RefPtrWillBeMember<FontFace>> m_fontFaces;
Oh, I didn't compile it with 'enable_oilpan=1'. Are there no trybots for
enable_oilpan=1 yet?

Anyway, now it passed the oilpan build.

[haraken, 2015-10-19 07:13:08]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-10-19 07:13:08]

LGTM

+tkent for API owner review on the IDL change.

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:122: class IterationSource
final : public FontFaceSetIterable::IterationSource {

Add WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED().

[tkent, 2015-10-19 07:30:47]

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (left):

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.idl:44: void
forEach(FontFaceSetForEachCallback callback, optional any thisArg);
forEach was already shipped, right?  We need UseCounter data and
intent-to-remove.

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.idl:45: [RaisesException] boolean
has(FontFace fontFace);
Why do you remove |has| and rename C++ has to hasForBinding?

[Takashi Toyoshima, 2015-10-19 07:51:44]

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:122: class IterationSource
final : public FontFaceSetIterable::IterationSource {
Done.
Any guideline to use the macro? Should I use it always as possible if the class
isn't shipped for oilpan?

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (left):

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.idl:44: void
forEach(FontFaceSetForEachCallback callback, optional any thisArg);
It was shipped, but this change doesn't remove it.

The new line 'readonly setlike<FontFace>' automatically supplements it.
It is equivalent to following lines.

  void forEach(FontFaceSetForEachCallback callback, optional any thisArg);
  [RaiseException] boolean has(FontFace fontFace);
  [RaiseException] Iterator entries();
  [RaiseException] Iterator keys();
  [RaiseException] Iterator values();

So this change does not remove anything, but adds entries(), keys(), and
values().

They are tested by fontfaceset-set-operations.html I modified in this CL.

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.idl:45: [RaisesException] boolean
has(FontFace fontFace);
Same. *forBinding is special method that is called auto-generated v8 binding
methods. They are mainly implemented in bindings/core/v8/Iterable.h and
FontFaceSet inherits it. Only FontFaceSet has to do is implement
startIteration(). Iterable.h does everything that are needed to implement
setlike related methods.

[Takashi Toyoshima, 2015-10-19 07:55:08]

Sorry, s/Only FontFaceSet has to do/All FontFaceSet has to do/.

[haraken, 2015-10-19 07:56:05]

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.h (right):

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.h:122: class IterationSource
final : public FontFaceSetIterable::IterationSource {
On 2015/10/19 07:51:44, Takashi Toyoshima wrote:
> Done.
> Any guideline to use the macro? Should I use it always as possible if the
class
> isn't shipped for oilpan?

Sorry, I noticed that FontFaceSetIterable::IterationSource is already
GarbageCollectedFinalized (i.e. on oilpan's heap in both non-oilpan and oilpan).
So you don't need any macro here.

[tkent, 2015-10-19 07:59:53]

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFaceSet.idl (left):

https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFaceSet.idl:44: void
forEach(FontFaceSetForEachCallback callback, optional any thisArg);
On 2015/10/19 at 07:51:44, Takashi Toyoshima wrote:
> It was shipped, but this change doesn't remove it.
> 
> The new line 'readonly setlike<FontFace>' automatically supplements it.
> It is equivalent to following lines.
> 
>   void forEach(FontFaceSetForEachCallback callback, optional any thisArg);
>   [RaiseException] boolean has(FontFace fontFace);
>   [RaiseException] Iterator entries();
>   [RaiseException] Iterator keys();
>   [RaiseException] Iterator values();
> 
> So this change does not remove anything, but adds entries(), keys(), and
values().
> 
> They are tested by fontfaceset-set-operations.html I modified in this CL.

I see.  So, the web-exposed changes is trivial.  LGTM to ship.

[Takashi Toyoshima, 2015-10-19 08:03:05]

Thanks. I reverted the FastAlloc change for the comment #18, and updated the
change description to clarify IDL changes.

[Takashi Toyoshima, 2015-10-19 08:06:36]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-10-19 08:06:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
ksakamoto@chromium.org, haraken@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1409433003/#ps200001
(title: "revert FastAlloc")

[commit-bot: I haz the power, 2015-10-19 08:06:57]

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

[commit-bot: I haz the power, 2015-10-19 09:24:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-10-19 09:25:15]

Patchset 11 (id:??) landed as
https://crrev.com/7d9f3ae0f1993db746e41659627be241d3586270
Cr-Commit-Position: refs/heads/master@{#354742}