Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 1409433003: CSS Font Loading: make FontFaceSet Setlike (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 9 months ago by Takashi Toyoshima
Modified:
1 year, 9 months ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, Inactive, chromium-reviews, dglazkov+blink, rwlbuis, vivekg_samsung, vivekg, yhirano
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : done #

Patch Set 3 : fix layout tests #

Patch Set 4 : fix ValueIterator problems #

Patch Set 5 : update expectation #

Patch Set 6 : oilpan type fix #

Total comments: 1

Patch Set 7 : revert forEach behavior (that makes it easy!) #

Total comments: 9

Patch Set 8 : review #7 #

Total comments: 2

Patch Set 9 : fix oilpan build #

Total comments: 8

Patch Set 10 : FastAlloc #

Patch Set 11 : revert FastAlloc #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -60 lines) Patch
M third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations.html View 1 2 3 4 5 6 7 2 chunks +46 lines, -16 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/fontfaceset-set-operations-expected.txt View 1 2 3 4 5 6 7 2 chunks +29 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/FontFaceSet.h View 1 2 3 4 5 6 7 8 10 5 chunks +27 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/css/FontFaceSet.cpp View 1 2 3 4 5 6 7 3 chunks +27 lines, -33 lines 0 comments Download
M third_party/WebKit/Source/core/css/FontFaceSet.idl View 1 2 1 chunk +2 lines, -4 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 25 (6 generated)
Takashi Toyoshima
ksakamoto@ for WebFonts, and yhirano@ for Setlike. Can you guys take a look this?
1 year, 9 months ago (2015-10-16 05:31:32 UTC) #3
Takashi Toyoshima
Sorry. I noticed one bug after requesting the review. I already fixed one point in ...
1 year, 9 months ago (2015-10-16 06:36:34 UTC) #4
Kunihiko Sakamoto
> This patch also changes the second argument of > ForEachCallback to conform the latest ...
1 year, 9 months ago (2015-10-16 08:07:12 UTC) #5
Takashi Toyoshima
Thanks. So reverting the forEach behavior change can make things simple. PTAL Patch Set 7. ...
1 year, 9 months ago (2015-10-16 08:50:36 UTC) #6
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 ...
1 year, 9 months ago (2015-10-16 09:50:07 UTC) #7
Takashi Toyoshima
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)'); On 2015/10/16 09:50:07, Kunihiko Sakamoto wrote: > Can ...
1 year, 9 months ago (2015-10-16 10:07:03 UTC) #8
Takashi Toyoshima
+haraken@ for OWNER approval. move yhirano@ from R to CC (as optional reviewer) because now ...
1 year, 9 months ago (2015-10-16 10:10:27 UTC) #10
haraken
https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Source/core/css/FontFaceSet.h File third_party/WebKit/Source/core/css/FontFaceSet.h (right): https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Source/core/css/FontFaceSet.h#newcode130 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); ...
1 year, 9 months ago (2015-10-16 13:15:14 UTC) #11
Takashi Toyoshima
PTAL https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Source/core/css/FontFaceSet.h File third_party/WebKit/Source/core/css/FontFaceSet.h (right): https://codereview.chromium.org/1409433003/diff/140001/third_party/WebKit/Source/core/css/FontFaceSet.h#newcode130 third_party/WebKit/Source/core/css/FontFaceSet.h:130: WillBeHeapVector<RefPtrWillBeMember<FontFace>> m_fontFaces; Oh, I didn't compile it with ...
1 year, 9 months ago (2015-10-19 05:23:35 UTC) #12
haraken
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 ...
1 year, 9 months ago (2015-10-19 07:13:08 UTC) #14
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); forEach was already ...
1 year, 9 months ago (2015-10-19 07:30:47 UTC) #15
Takashi Toyoshima
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 ...
1 year, 9 months ago (2015-10-19 07:51:44 UTC) #16
Takashi Toyoshima
Sorry, s/Only FontFaceSet has to do/All FontFaceSet has to do/.
1 year, 9 months ago (2015-10-19 07:55:08 UTC) #17
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 ...
1 year, 9 months ago (2015-10-19 07:56:05 UTC) #18
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 ...
1 year, 9 months ago (2015-10-19 07:59:53 UTC) #19
Takashi Toyoshima
Thanks. I reverted the FastAlloc change for the comment #18, and updated the change description ...
1 year, 9 months ago (2015-10-19 08:03:05 UTC) #20
commit-bot: I haz the power
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
1 year, 9 months ago (2015-10-19 08:06:57 UTC) #23
commit-bot: I haz the power
Committed patchset #11 (id:200001)
1 year, 9 months ago (2015-10-19 09:24:29 UTC) #24
commit-bot: I haz the power
1 year, 9 months ago (2015-10-19 09:25:15 UTC) #25
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/7d9f3ae0f1993db746e41659627be241d3586270
Cr-Commit-Position: refs/heads/master@{#354742}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973