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}
Thanks. So reverting the forEach behavior change can make things simple. PTAL Patch Set 7. ...
2 years, 6 months ago
(2015-10-16 08:50:36 UTC)
#6
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
lgtm with nits https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html File third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html (right): https://codereview.chromium.org/1409433003/diff/120001/third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html#newcode36 third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html:36: shouldBeFalse('document.fonts.has(nonCssConnectedFace)'); Can you move the rest ...
2 years, 6 months ago
(2015-10-16 09:50:07 UTC)
#7
LGTM +tkent for API owner review on the IDL change. https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.h File third_party/WebKit/Source/core/css/FontFaceSet.h (right): https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.h#newcode122 ...
2 years, 6 months ago
(2015-10-19 07:13:08 UTC)
#14
https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.h File third_party/WebKit/Source/core/css/FontFaceSet.h (right): https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.h#newcode122 third_party/WebKit/Source/core/css/FontFaceSet.h:122: class IterationSource final : public FontFaceSetIterable::IterationSource { Done. Any ...
2 years, 6 months ago
(2015-10-19 07:51:44 UTC)
#16
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
Sorry, s/Only FontFaceSet has to do/All FontFaceSet has to do/.
2 years, 6 months ago
(2015-10-19 07:55:08 UTC)
#17
Sorry, s/Only FontFaceSet has to do/All FontFaceSet has to do/.
haraken
https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.h File third_party/WebKit/Source/core/css/FontFaceSet.h (right): https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.h#newcode122 third_party/WebKit/Source/core/css/FontFaceSet.h:122: class IterationSource final : public FontFaceSetIterable::IterationSource { On 2015/10/19 ...
2 years, 6 months ago
(2015-10-19 07:56:05 UTC)
#18
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
https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.idl File third_party/WebKit/Source/core/css/FontFaceSet.idl (left): https://codereview.chromium.org/1409433003/diff/160001/third_party/WebKit/Source/core/css/FontFaceSet.idl#oldcode44 third_party/WebKit/Source/core/css/FontFaceSet.idl:44: void forEach(FontFaceSetForEachCallback callback, optional any thisArg); On 2015/10/19 at ...
2 years, 6 months ago
(2015-10-19 07:59:53 UTC)
#19
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
Thanks. I reverted the FastAlloc change for the comment #18, and updated the change description ...
2 years, 6 months ago
(2015-10-19 08:03:05 UTC)
#20
Thanks. I reverted the FastAlloc change for the comment #18, and updated the
change description to clarify IDL changes.
Takashi Toyoshima
The CQ bit was checked by toyoshim@chromium.org
2 years, 6 months ago
(2015-10-19 08:06:36 UTC)
#21
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
2 years, 6 months ago
(2015-10-19 08:06:57 UTC)
#23
Issue 1409433003: CSS Font Loading: make FontFaceSet Setlike
(Closed)
Created 2 years, 6 months ago by Takashi Toyoshima
Modified 2 years, 6 months ago
Reviewers: tkent, haraken, Kunihiko Sakamoto
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 20