Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599933002:
[Android] Stop using skia globals for font overriding in layout tests. (Closed)
Description[Android] Stop using skia globals for font overriding in layout tests.
The layout tests have been using globals in skia to provide custom
font files. Skia would like to get rid of those globals. This change
adds a chromium-side SkFontMgr::Factory for the layout tests to use
instead.
BUG=chromium:567947, skia:2817
Committed: https://crrev.com/b81742a8fd075e6cd228f4bf742744253aea769a
Cr-Commit-Position: refs/heads/master@{#441404}
Patch Set 1 #
Total comments: 4
Patch Set 2 : bungeman comments #Patch Set 3 : rebase #
Messages
Total messages: 30 (18 generated)
jbudorick@chromium.org changed reviewers: + bungeman@chromium.org
https://codereview.chromium.org/2599933002/diff/1/content/shell/app/blink_tes... File content/shell/app/blink_test_platform_support_android.cc (right): https://codereview.chromium.org/2599933002/diff/1/content/shell/app/blink_tes... content/shell/app/blink_test_platform_support_android.cc:42: custom.fIsolated = false; Not sure if you want isolated or not, isolated basically means that when the typefaces are created the files will be opened at it will hold onto the file descriptors. This is pretty much required when running inside the sandbox. Otherwise the files will be opened and closed as needed. https://codereview.chromium.org/2599933002/diff/1/skia/ext/fontmgr_default_an... File skia/ext/fontmgr_default_android.cc (right): https://codereview.chromium.org/2599933002/diff/1/skia/ext/fontmgr_default_an... skia/ext/fontmgr_default_android.cc:19: return g_default_fontmgr ? g_default_fontmgr : SkFontMgr_New_Android(nullptr); See fontmgr_default_linux.cc, you'll need to SkRef the g_default_fontmgr. I'll get these updated to sk_sp smart pointers soon. (This factory is supposed to return a ref, so it should be returning sk_sp <SkFontMgr> to enforce this.
https://codereview.chromium.org/2599933002/diff/1/content/shell/app/blink_tes... File content/shell/app/blink_test_platform_support_android.cc (right): https://codereview.chromium.org/2599933002/diff/1/content/shell/app/blink_tes... content/shell/app/blink_test_platform_support_android.cc:42: custom.fIsolated = false; On 2016/12/25 17:53:57, bungeman-chromium wrote: > Not sure if you want isolated or not, isolated basically means that when the > typefaces are created the files will be opened at it will hold onto the file > descriptors. This is pretty much required when running inside the sandbox. > Otherwise the files will be opened and closed as needed. For now, I'm just porting this over from https://codesearch.chromium.org/chromium/src/third_party/skia/src/ports/SkFon.... I'll have quite a bit more regarding how the font files are handled for layout tests in subsequent patches. https://codereview.chromium.org/2599933002/diff/1/skia/ext/fontmgr_default_an... File skia/ext/fontmgr_default_android.cc (right): https://codereview.chromium.org/2599933002/diff/1/skia/ext/fontmgr_default_an... skia/ext/fontmgr_default_android.cc:19: return g_default_fontmgr ? g_default_fontmgr : SkFontMgr_New_Android(nullptr); On 2016/12/25 17:53:57, bungeman-chromium wrote: > See fontmgr_default_linux.cc, you'll need to SkRef the g_default_fontmgr. I'll > get these updated to sk_sp smart pointers soon. (This factory is supposed to > return a ref, so it should be returning sk_sp <SkFontMgr> to enforce this. Done.
The CQ bit was checked by bungeman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm I'll update to smart pointers asap.
bungeman@google.com changed reviewers: + bungeman@google.com
OWNER lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jbudorick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com, bungeman@chromium.org Link to the patchset: https://codereview.chromium.org/2599933002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jbudorick@chromium.org changed reviewers: + peter@chromium.org
+peter for content/shell/ OWNERS
The CQ bit was checked by peter@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483552128915930,
"parent_rev": "6a7f22ff01bbe5bac3335e5ca36f1b6f3cd62676", "commit_rev":
"d7fd5c8d5142a76cd9851244efb486faa0af1be5"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Stop using skia globals for font overriding in layout tests. The layout tests have been using globals in skia to provide custom font files. Skia would like to get rid of those globals. This change adds a chromium-side SkFontMgr::Factory for the layout tests to use instead. BUG=chromium:567947,skia:2817 ========== to ========== [Android] Stop using skia globals for font overriding in layout tests. The layout tests have been using globals in skia to provide custom font files. Skia would like to get rid of those globals. This change adds a chromium-side SkFontMgr::Factory for the layout tests to use instead. BUG=chromium:567947,skia:2817 Review-Url: https://codereview.chromium.org/2599933002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Stop using skia globals for font overriding in layout tests. The layout tests have been using globals in skia to provide custom font files. Skia would like to get rid of those globals. This change adds a chromium-side SkFontMgr::Factory for the layout tests to use instead. BUG=chromium:567947,skia:2817 Review-Url: https://codereview.chromium.org/2599933002 ========== to ========== [Android] Stop using skia globals for font overriding in layout tests. The layout tests have been using globals in skia to provide custom font files. Skia would like to get rid of those globals. This change adds a chromium-side SkFontMgr::Factory for the layout tests to use instead. BUG=chromium:567947,skia:2817 Committed: https://crrev.com/b81742a8fd075e6cd228f4bf742744253aea769a Cr-Commit-Position: refs/heads/master@{#441404} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b81742a8fd075e6cd228f4bf742744253aea769a Cr-Commit-Position: refs/heads/master@{#441404} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
