Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1878843002:
Add option to specify font fallback when creating the skia font manager (Closed)
|
Created:
4 years, 8 months ago by Ilya Kulshin Modified:
4 years, 8 months ago CC:
reviews_skia.org, reed1, bsalomon_chromium Base URL:
https://chromium.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd option to specify font fallback when creating the skia font manager
This originally existed as https://codereview.chromium.org/1740533003/,
but then got reverted for causing perf problems at startup. This change
avoids that by allowing callers to specify their own font fallback, and
only uses the system fallback if a fallback is needed and none was
provided.
This is part 1 of a three part change.
1: https://codereview.chromium.org/1878843002/
Adds support for specifying a font fallback in skia
2: https://codereview.chromium.org/1846433005/
Implements the fallback proxy in Chromium
3: https://codereview.chromium.org/1883483002/
Adds code to blink to call skia's fallback API
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1878843002
Committed: https://skia.googlesource.com/skia/+/82497f9300432375cb9fd0e0ceca011ea7dce847
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix indent #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== Add option to specify font fallback when creating the skia font manager BUG=skia: ========== to ========== Add option to specify font fallback when creating the skia font manager BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add option to specify font fallback when creating the skia font manager BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify font fallback when creating the skia font manager Add option to specify font fallback when creating the skia font manager. This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add option to specify font fallback when creating the skia font manager Add option to specify font fallback when creating the skia font manager. This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify font fallback when creating the skia font manager Add option to specify font fallback when creating the skia font manager. This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002 Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Add option to specify font fallback when creating the skia font manager Add option to specify font fallback when creating the skia font manager. This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002 Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify font fallback when creating the skia font manager Add option to specify font fallback when creating the skia font manager. This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
kulshin@chromium.org changed reviewers: + bungeman@chromium.org
ptal. The previous attempt at this change (https://codereview.chromium.org/1740533003/) was reverted because it was causing performance problems at startup. I am now using a different approach that avoids loading the system fallback if the caller provides their own.
bungeman@google.com changed reviewers: + bungeman@google.com - bungeman@chromium.org
https://codereview.chromium.org/1878843002/diff/1/src/ports/SkFontMgr_win_dw.cpp File src/ports/SkFontMgr_win_dw.cpp (right): https://codereview.chromium.org/1878843002/diff/1/src/ports/SkFontMgr_win_dw.... src/ports/SkFontMgr_win_dw.cpp:282: SkASSERT(fFactory2.get()); nit: Skia uses 4 space indents. https://codereview.chromium.org/1878843002/diff/1/src/ports/SkFontMgr_win_dw.... src/ports/SkFontMgr_win_dw.cpp:779: } Hmmm... I see. The idea is that while making the transition we aren't using this anyway, so get the default here if needed. If we tried to get the default in SkFontMgr_New_DirectWrite then we'd end up trying to get one, which we don't want. So after Chromium provides an IDWriteFontFallback to SkFontMgr_New_DirectWrite, this default code could be moved out to the constructor (since Chromium would no longer be going down the path which would create the default).
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1878843002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1878843002/1
I know it's a bit late for this now, but if you'd like to run these sorts of Skia changes though the Chromium bots along with the Chromium/Blink changes ahead of time, I just put up some instructions on how to do so at https://skia.org/dev/chrome/multi_repo_trybots .
The CQ bit was unchecked by bungeman@google.com
Description was changed from ========== Add option to specify font fallback when creating the skia font manager Add option to specify font fallback when creating the skia font manager. This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify font fallback when creating the skia font manager This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1878843002/diff/1/src/ports/SkFontMgr_win_dw.cpp File src/ports/SkFontMgr_win_dw.cpp (right): https://codereview.chromium.org/1878843002/diff/1/src/ports/SkFontMgr_win_dw.... src/ports/SkFontMgr_win_dw.cpp:282: SkASSERT(fFactory2.get()); On 2016/04/14 21:48:39, bungeman1 wrote: > nit: Skia uses 4 space indents. Done. https://codereview.chromium.org/1878843002/diff/1/src/ports/SkFontMgr_win_dw.... src/ports/SkFontMgr_win_dw.cpp:779: } On 2016/04/14 21:48:39, bungeman1 wrote: > Hmmm... I see. The idea is that while making the transition we aren't using this > anyway, so get the default here if needed. If we tried to get the default in > SkFontMgr_New_DirectWrite then we'd end up trying to get one, which we don't > want. > > So after Chromium provides an IDWriteFontFallback to SkFontMgr_New_DirectWrite, > this default code could be moved out to the constructor (since Chromium would no > longer be going down the path which would create the default). Yes, after Chromium starts providing the fallback, Chromium wouldn't end up in this block. However I'm not sure about other users of Skia. We've seen in Chromium that creating the fallback is potentially expensive, so I'm somewhat reluctant to create it automatically in the constructor if it might not even be needed.
Description was changed from ========== Add option to specify font fallback when creating the skia font manager This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify font fallback when creating the skia font manager This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
bungeman@google.com changed reviewers: + reed@google.com
lgtm, reed can you take a quick look at the include change?
lgtm
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1878843002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1878843002/20001
Message was sent while issue was closed.
Description was changed from ========== Add option to specify font fallback when creating the skia font manager This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add option to specify font fallback when creating the skia font manager This originally existed as https://codereview.chromium.org/1740533003/, but then got reverted for causing perf problems at startup. This change avoids that by allowing callers to specify their own font fallback, and only uses the system fallback if a fallback is needed and none was provided. This is part 1 of a three part change. 1: https://codereview.chromium.org/1878843002/ Adds support for specifying a font fallback in skia 2: https://codereview.chromium.org/1846433005/ Implements the fallback proxy in Chromium 3: https://codereview.chromium.org/1883483002/ Adds code to blink to call skia's fallback API GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/82497f9300432375cb9fd0e0ceca011ea7dce847 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/82497f9300432375cb9fd0e0ceca011ea7dce847 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
