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

Issue 1128143003: Set default SVG image font settings (Closed)

Created:
5 years, 2 months ago by pdr.
Modified:
5 years, 2 months ago
Reviewers:
esprehn, fs, Erik Dahlström
CC:
blink-reviews, krit, f(malita), fs, gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney
Target Ref:
refs/remotes/origin/master
Project:
blink
Visibility:
Public.

Description

Set default SVG image font settings When SVG is loaded as an image, we create a detached Page which doesn't get default font settings from the embedder. This patch copies Settings from an existing Page so SVG has sensible defaults. This fixes two long-standing bugs: 1) The genericFontFamilySettings setting contains font fallback values that are needed to properly lookup sans-serif glyphs on OSX. 2) An unreadable font-size would be used if no font-size were set. Another approach is to plumb these settings through from the requesting Page when creating the SVGImage in ImageResource (i.e., ImageResource would query it's m_loader which has a FetchContext which references the requesting Page). This proved to be messy because data:uri images do not have a loader which means a loader needs to be introduced or a second codepath is needed from ResourceFetcher to ImageResource. I think the approach in this patch ends up being cleaner. With this patch, svg/as-background-image/svg-as-background-{1,3,6}* no longer outputs sans-serif text on linux and windows. This matches OSX as well as Gecko. BUG=167760, 348436 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196021

Patch Set 1 #

Patch Set 2 : Add a test of an svg image document and an externally loaded file #

Patch Set 3 : Update expectations #

Patch Set 4 : Update expectations #

Total comments: 4

Patch Set 5 : Assert that ordinary pages is not empty #

Patch Set 6 : Page::ordinaryPages can be empty in tests (ImageResourceTest) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+135 lines, -0 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-font-settings-external.html View 1 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-font-settings-external-expected.html View 1 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-font-size.html View 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-font-size-expected.html View 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-monospace-font.html View 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-monospace-font-expected.html View 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-sans-serif-font.html View 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/default-sans-serif-font-expected.html View 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/svg/as-image/resources/default-font-settings.svg View 1 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/svg/custom/default-font-settings.svg View 1 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/svg/custom/default-font-settings-expected.html View 1 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/svg/graphics/SVGImage.cpp View 1 2 3 4 5 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (6 generated)
pdr.
5 years, 2 months ago (2015-05-17 10:03:24 UTC) #2
Erik Dahlström
https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphics/SVGImage.cpp File Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphics/SVGImage.cpp#newcode441 Source/core/svg/graphics/SVGImage.cpp:441: if (!Page::ordinaryPages().isEmpty()) { Under what circumstances can ordinaryPages be ...
5 years, 2 months ago (2015-05-17 20:20:15 UTC) #3
fs
lgtm
5 years, 2 months ago (2015-05-18 09:19:26 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128143003/80001
5 years, 2 months ago (2015-05-19 01:44:24 UTC) #7
pdr.
Thanks for the reviews! If there are no further comments, I'll commit this tomorrow. https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphics/SVGImage.cpp ...
5 years, 2 months ago (2015-05-19 02:02:29 UTC) #8
esprehn
This makes me a little nervous, ordinaryPages can contain any random tab, I guess these ...
5 years, 2 months ago (2015-05-19 02:13:56 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/62058)
5 years, 2 months ago (2015-05-19 02:28:25 UTC) #11
pdr.
On 2015/05/19 at 02:13:56, esprehn wrote: > This makes me a little nervous, ordinaryPages can ...
5 years, 2 months ago (2015-05-23 23:49:31 UTC) #12
esprehn
On 2015/05/23 at 23:49:31, pdr wrote: > On 2015/05/19 at 02:13:56, esprehn wrote: > > ...
5 years, 2 months ago (2015-05-27 06:08:05 UTC) #13
esprehn
btw this is lgtm, the hack makes me sad though.
5 years, 2 months ago (2015-05-27 06:08:48 UTC) #14
pdr.
On 2015/05/27 at 06:08:48, esprehn wrote: > btw this is lgtm, the hack makes me ...
5 years, 2 months ago (2015-05-27 22:27:00 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128143003/100001
5 years, 2 months ago (2015-05-27 22:27:19 UTC) #18
commit-bot: I haz the power
5 years, 2 months ago (2015-05-28 00:37:58 UTC) #19
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196021

Powered by Google App Engine
This is Rietveld 408576698