Set default SVG image font settings

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

[pdr., 2015-05-17 08:32:19]

pdr@chromium.org changed reviewers:
+ ed@chromium.org, esprehn@chromium.org, fs@opera.com

[Erik Dahlström, 2015-05-17 20:20:15]

https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphic...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphic...
Source/core/svg/graphics/SVGImage.cpp:441: if (!Page::ordinaryPages().isEmpty())
{
Under what circumstances can ordinaryPages be empty?

https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphic...
Source/core/svg/graphics/SVGImage.cpp:442: Settings& defaultSettings =
(*Page::ordinaryPages().begin())->settings();
This means svg images can get a different set of defaults if the first page gets
closed, right? If so, then that's not completely ideal, but it's still way
better than the current situation.

[fs, 2015-05-18 09:19:26]

lgtm

[pdr., 2015-05-19 01:44:09]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[pdr., 2015-05-19 01:44:09]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1128143003/#ps80001
(title: "Assert that ordinary pages is not empty")

[commit-bot: I haz the power, 2015-05-19 01:44:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128143003/80001

[pdr., 2015-05-19 02:02:29]

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/graphic...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphic...
Source/core/svg/graphics/SVGImage.cpp:441: if (!Page::ordinaryPages().isEmpty())
{
On 2015/05/17 at 20:20:15, Erik Dahlström wrote:
> Under what circumstances can ordinaryPages be empty?

I don't think it is possible to have ordinaryPages be empty when we hit this
code because some ordinary page needs to trigger the svg image load. I've added
an assert here to enforce that.

https://codereview.chromium.org/1128143003/diff/60001/Source/core/svg/graphic...
Source/core/svg/graphics/SVGImage.cpp:442: Settings& defaultSettings =
(*Page::ordinaryPages().begin())->settings();
On 2015/05/17 at 20:20:15, Erik Dahlström wrote:
> This means svg images can get a different set of defaults if the first page
gets closed, right? If so, then that's not completely ideal, but it's still way
better than the current situation.

The defaults for these specific settings shouldn't change on a per-page basis so
I think we're good in that case. This patch doesn't support dynamic changes of
these values though.

[esprehn, 2015-05-19 02:13:56]

This makes me a little nervous, ordinaryPages can contain any random tab, I
guess these settings never vary per domain so this is safe?

If this is a safe thing to do we should really have the embedder set the
defaults at process startup into static variables before any pages are ever
created and pages should just take the defaults from the statics.

[commit-bot: I haz the power, 2015-05-19 02:28:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-19 02:28:25]

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/6...)

[pdr., 2015-05-23 23:49:31]

On 2015/05/19 at 02:13:56, esprehn wrote:
> This makes me a little nervous, ordinaryPages can contain any random tab, I
guess these settings never vary per domain so this is safe?
> 
> If this is a safe thing to do we should really have the embedder set the
defaults at process startup into static variables before any pages are ever
created and pages should just take the defaults from the statics.

I see what you mean about it being scary to pull settings from a random page but
I don't quite understand your solution. Isn't WebPreferences effectively a
static variable in the embedder with these font settings already? The issue is
just plumbing the settings down to a detached Page. There is an extensions API
for changing these settings (chrome.fontSettings) that Falkin's advanced font
settings extension uses. The font settings look to be browser-wide and not
scoped to a domain/page.

What do you think about just making it more clear that font settings are global
on all pages? For example, we could introduce a static FontSettings struct
(GenericFontFamilySettings + minFontSize + minLogicalFontSize + defaultFontSize
+ defaultFixedFontSize) on Settings and add comments describing why it's static
unlike the other settings. It would then be trivial to pull these settings
directly from Page in SVGImage.

[esprehn, 2015-05-27 06:08:05]

On 2015/05/23 at 23:49:31, pdr wrote:
> On 2015/05/19 at 02:13:56, esprehn wrote:
> > This makes me a little nervous, ordinaryPages can contain any random tab, I
guess these settings never vary per domain so this is safe?
> > 
> > If this is a safe thing to do we should really have the embedder set the
defaults at process startup into static variables before any pages are ever
created and pages should just take the defaults from the statics.
> 
> I see what you mean about it being scary to pull settings from a random page
but I don't quite understand your solution. Isn't WebPreferences effectively a
static variable in the embedder with these font settings already? The issue is
just plumbing the settings down to a detached Page. There is an extensions API
for changing these settings (chrome.fontSettings) that Falkin's advanced font
settings extension uses. The font settings look to be browser-wide and not
scoped to a domain/page.
> 
> What do you think about just making it more clear that font settings are
global on all pages? For example, we could introduce a static FontSettings
struct (GenericFontFamilySettings + minFontSize + minLogicalFontSize +
defaultFontSize + defaultFixedFontSize) on Settings and add comments describing
why it's static unlike the other settings. It would then be trivial to pull
these settings directly from Page in SVGImage.

I'm saying if WebSettings is actually static then we should create a static
Settings object inside blink, and a SettingsClient that the embedder needs to
hook up and remove the Page -> Settings association.

I think that's the real bug, Settings should be a static thing where you do:

Settings::instance() to get the shared instance, and anyone using settings
implements class SettingsObservers { virtual void settingsChanged(Settings&
newSettings) { } }, and does Settings::addObserver(*this) and removeObserver in
their destructors.

[esprehn, 2015-05-27 06:08:48]

btw this is lgtm, the hack makes me sad though.

[pdr., 2015-05-27 22:27:00]

On 2015/05/27 at 06:08:48, esprehn wrote:
> btw this is lgtm, the hack makes me sad though.

Thanks for the writeup. Agreed on all points.

[pdr., 2015-05-27 22:27:03]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-05-27 22:27:04]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1128143003/#ps100001
(title: "Page::ordinaryPages can be empty in tests (ImageResourceTest)")

[commit-bot: I haz the power, 2015-05-27 22:27:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128143003/100001

[commit-bot: I haz the power, 2015-05-28 00:37:58]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196021