Remove unsafe getFontMetrics methods

Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
BUG=655815
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/fdcbab80bc37108f6e03d6906f27831228690350
Cr-Commit-Position: refs/heads/master@{#425437}

[eae, 2016-10-13 22:17:13]

Description was changed from

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
==========

to

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[eae, 2016-10-13 22:17:51]

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

[commit-bot: I haz the power, 2016-10-13 22:18:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eae, 2016-10-13 22:20:38]

Description was changed from

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
BUG=655815
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2016-10-13 22:54:31]

lgtm

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1696: return -1;
Is -1 the correct error value? I see LayoutTheme::baselinePosition() which is
also used as a return value in this method returns 0 in a different
less-error-ish case (!o->isBox()).

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1768: if
(!haveNormalFlowChild && hasLineIfEmpty() && fontData) {
Perhaps worth putting fontData first to save on checking others if not available
since hasLineIfEmpty() is a virtual method. Though obviously this situation
should be very rare so not super impactful.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2494: return -1;
But I see we use -1 here and a number of cases below, so I'm inferring the
answer to my previous question is, '-1 is the right return value.'

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1356: return 0;
But then here 0 is the right value?

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngineBaseline.cpp
(right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngineBaseline.cpp:37:
const SimpleFontData* fontData = m_font.primaryFont();
Double checking, this method has |font| param, but we use m_font here, which
matches code below, but, like, why was this code ignoring the parameter?

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.h (left):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:2517: const FontMetrics&
getFontMetrics() const;
Discussed in person -- an alternative approach that would save on code verbosity
at callsites could be to have something like a final-fallback dummy
PrimaryFont/FontMetrics that is implemented purely in code and returns
equivalent of zero/empty values, and return that here if we failed to load even
the single final fallback font. And then, add a separate method callsites can
use to check explicitly whether there's a real font available for retrieving
metrics or we've had to fall back to the dummy case.

However, we'd like to try this change as written for now. The above could lead
to new code not being written in a thoughtful manner, and so having a pointer
where it's clear it could be null will require authors to consider this case.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/exported/WebFont.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebFont.cpp:50: return fontData ?
fontData->getFontMetrics().ascent() : 0;
Could be worth a helper method to use here and in variants below just for
maintenance purposes.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/Font.h (left):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/Font.h:126: const FontMetrics&
getFontMetrics() const {
Suggest adding documentation to Font::primaryFont() method to explain generally
why we can end up with no primary font. Naively it's not clear how this would
happen, and whether there's a layer of logic elsewhere that makes sure we fall
back to some super-general default, or if instead all logic has to make sure to
essentially deal with a situation where there is no font and so no text can
pragmatically be laid out/painted.

[eae, 2016-10-13 23:09:58]

Thanks for the review!

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1696: return -1;
Yeah, -1 is the magic value for unknown baseline.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1768: if
(!haveNormalFlowChild && hasLineIfEmpty() && fontData) {
On 2016/10/13 22:54:30, wkorman wrote:
> Perhaps worth putting fontData first to save on checking others if not
available
> since hasLineIfEmpty() is a virtual method. Though obviously this situation
> should be very rare so not super impactful.

Acknowledged.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutInline.cpp:1356: return 0;
On 2016/10/13 22:54:30, wkorman wrote:
> But then here 0 is the right value?

Changed to -1, good catch.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/exported/WebFont.cpp (right):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/exported/WebFont.cpp:50: return fontData ?
fontData->getFontMetrics().ascent() : 0;
On 2016/10/13 22:54:31, wkorman wrote:
> Could be worth a helper method to use here and in variants below just for
> maintenance purposes.

Good idea.

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/Font.h (left):

https://codereview.chromium.org/2416033003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/Font.h:126: const FontMetrics&
getFontMetrics() const {
On 2016/10/13 22:54:31, wkorman wrote:
> Suggest adding documentation to Font::primaryFont() method to explain
generally
> why we can end up with no primary font. Naively it's not clear how this would
> happen, and whether there's a layer of logic elsewhere that makes sure we fall
> back to some super-general default, or if instead all logic has to make sure
to
> essentially deal with a situation where there is no font and so no text can
> pragmatically be laid out/painted.

Done, thank you.

[eae, 2016-10-13 23:10:02]

The CQ bit was checked by eae@chromium.org

[eae, 2016-10-13 23:10:02]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2416033003/#ps20001
(title: "Address wkroman suggestions")

[commit-bot: I haz the power, 2016-10-13 23:10:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 23:16:34]

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

[commit-bot: I haz the power, 2016-10-13 23:16:35]

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

[haraken, 2016-10-13 23:47:48]

On 2016/10/13 23:16:35, commit-bot: I haz the power wrote:
> 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...)

modules/ and web/ LGTM

[eae, 2016-10-14 18:33:37]

On 2016/10/13 23:47:48, haraken wrote:
> On 2016/10/13 23:16:35, commit-bot: I haz the power wrote:
> > 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...)
> 
> modules/ and web/ LGTM

Thanks!

[eae, 2016-10-14 18:33:50]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-10-14 18:34:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 19:48:20]

Description was changed from

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
BUG=655815
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
BUG=655815
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-10-14 19:48:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-14 19:51:27]

Description was changed from

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
BUG=655815
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Remove unsafe getFontMetrics methods

Remove the ComputedStyle::getFontMetrics and Font::getFontMetrics helper
methods as they both assume that the primaryFont method always returns a
valid SimpleFontDataObject. That assumption is both incorrect and unsafe
as it can return nullptr in case fallback to the last-resort-font fails.

By replacing the convince calls with explicit calls and null checks that
type of problems becomes more apparent and can be handled appropriately.

R=wkorman@chromium.org
BUG=655815
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/fdcbab80bc37108f6e03d6906f27831228690350
Cr-Commit-Position: refs/heads/master@{#425437}
==========

[commit-bot: I haz the power, 2016-10-14 19:51:29]

Patchset 2 (id:??) landed as
https://crrev.com/fdcbab80bc37108f6e03d6906f27831228690350
Cr-Commit-Position: refs/heads/master@{#425437}

[drott, 2016-10-18 10:45:00]

Nice.