Bug 88655 – Text Autosizing: Add basic framework

• Component: Text
• Status: RESOLVED FIXED

[John Mellor]

This adds a highly simplified foundation that subsequent Font Boosting patches can build upon. I've refactored this code (since the earlier combined diff uploaded to bug 84186) to touch as few files as possible.

[John Mellor]

Created attachment 146559 [details]
Patch

[Adam Barth]

Comment on attachment 146559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146559&action=review

> Source/WebCore/rendering/FontBoosting.cpp:47
> +PassOwnPtr<FontBoosting> FontBoosting::create(Document* document)
> +{
> +    return adoptPtr(new FontBoosting(document));
> +}

We usually just put the implementation of "create" functions like this in the header.

> Source/WebCore/rendering/FontBoosting.h:46
> +private:
> +    explicit FontBoosting(Document*);

Normally we just put this sort of declaration at the top of the normal "private" section below.

[Adam Barth]

Unfortunately, I don't know much about the rendering tree.

@jchaffraix: Would you be willing to review this patch?

[John Mellor]

Created attachment 149936 [details]
Patch

Addressed abarth's comments.

[Eric Seidel (no email)]

Comment on attachment 149936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149936&action=review

> Source/WebCore/rendering/FontBoosting.cpp:114
> +// FIXME: There must be a standard way of doing this...
> +PassRefPtr<RenderStyle> FontBoosting::cloneRenderStyleWithState(const RenderStyle* currentStyle)

I'm not sure what the goal is here.  I'm surprised you need to write your own.

[Eric Seidel (no email)]

Comment on attachment 149936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149936&action=review

> Source/WebCore/rendering/FontBoosting.cpp:49
> +    for (RenderObject* descendant = traverseNext(layoutRoot, 0, layoutRoot); descendant; descendant = traverseNext(descendant, 0, layoutRoot)) {

Maybe the filter should be the last argument, so it can be implicit?

[Eric Seidel (no email)]

Comment on attachment 149936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149936&action=review

> Source/WebCore/rendering/FontBoosting.cpp:119
> +    if (currentStyle->childrenAffectedByForwardPositionalRules())
> +        newStyle->setChildrenAffectedByForwardPositionalRules();

It looks like thsi code also exists in Element::recalcStyle.  We should abstract that code out and share it.

RenderStyle::copyStateBitsFromStyle(style) maybe?

[Eric Seidel (no email)]

Comment on attachment 149936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149936&action=review

> Source/WebCore/rendering/FontBoosting.h:43
> +class FontBoosting {

Nit:  We normally give classes "action" names, like "FontBooster" in this case.  Although I'm not sure if the "font boosting" term is standard.  You would know better than I.

[Eric Seidel (no email)]

Comment on attachment 149936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149936&action=review

> Source/WebCore/rendering/FontBoosting.cpp:60
> +    const float windowWidthCssPx = 320; // FIXME: Hook this up to the actual window width. For now this is useful for testing on desktop.
> +    float multiplier = block->logicalWidth() / windowWidthCssPx; // FIXME: This is overly simplistic.

I think we should hook this up to the window width now.  We can still test things on desktop by making the window less wide.

> Source/WebCore/rendering/FontBoosting.cpp:78
> +    style->font().update(style->font().fontSelector());

I wonder how much work goes on inside of update() here and if I shoudl be worried that this could end up restyling us or similar.

> Source/WebCore/rendering/FontBoosting.cpp:90
> +RenderObject* FontBoosting::traverseNext(RenderObject* current, RenderObjectFilter filter, const RenderObject* stayWithin)

I would reorder the args here, I think I mentioned that somewhere else.

[Adam Barth]

Comment on attachment 149936 [details]
Patch

John, would you be willing to post a new patch that addresses Eric's review comments?

[John Mellor]

Comment on attachment 149936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149936&action=review

>> Source/WebCore/rendering/FontBoosting.cpp:49
>> +    for (RenderObject* descendant = traverseNext(layoutRoot, 0, layoutRoot); descendant; descendant = traverseNext(descendant, 0, layoutRoot)) {
> 
> Maybe the filter should be the last argument, so it can be implicit?

Done.

>> Source/WebCore/rendering/FontBoosting.cpp:60
>> +    float multiplier = block->logicalWidth() / windowWidthCssPx; // FIXME: This is overly simplistic.
> 
> I think we should hook this up to the window width now.  We can still test things on desktop by making the window less wide.

Done. I added a HACK_FORCE_FONT_BOOSTING_ON_DESKTOP ifdef allowing you to override this to 320x480 for testing on desktop (making the window less wide isn't the same, since narrows the initial containing block, hence causes text to wrap etc).

>> Source/WebCore/rendering/FontBoosting.cpp:78
>> +    style->font().update(style->font().fontSelector());
> 
> I wonder how much work goes on inside of update() here and if I shoudl be worried that this could end up restyling us or similar.

It's also used in RenderStyle::setBlendedFontSize (and seems to work in practice).

>> Source/WebCore/rendering/FontBoosting.cpp:90
>> +RenderObject* FontBoosting::traverseNext(RenderObject* current, RenderObjectFilter filter, const RenderObject* stayWithin)
> 
> I would reorder the args here, I think I mentioned that somewhere else.

Done.

>> Source/WebCore/rendering/FontBoosting.cpp:114
>> +PassRefPtr<RenderStyle> FontBoosting::cloneRenderStyleWithState(const RenderStyle* currentStyle)
> 
> I'm not sure what the goal is here.  I'm surprised you need to write your own.

I'm not sure - I think this might have been an optimization since we know that we're putting the style back in the same place it was before. For now I'll just use RenderStyle::clone for this, which seems to work as well.

>> Source/WebCore/rendering/FontBoosting.cpp:119
>> +        newStyle->setChildrenAffectedByForwardPositionalRules();
> 
> It looks like thsi code also exists in Element::recalcStyle.  We should abstract that code out and share it.
> 
> RenderStyle::copyStateBitsFromStyle(style) maybe?

The code in Element::recalcStyle has significant differences. It additionally handles affectedByHoverRules, affectedByActiveRules and affectedByDragRules, but on the other hand it doesn't handle lastChildState, firstChildState, childIndex and affectedByEmpty. It's not clear what we should do here (though as mentioned above, just using clone seems to work well enough).

>> Source/WebCore/rendering/FontBoosting.h:43
>> +class FontBoosting {
> 
> Nit:  We normally give classes "action" names, like "FontBooster" in this case.  Although I'm not sure if the "font boosting" term is standard.  You would know better than I.

"Font Boosting" is the general term we've used (both internally and externally) in talking about this approach, so there is some value in keeping that name for the class, though I'm happy to change it if you still think it should be an action.

[John Mellor]

Created attachment 150460 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150460 [details]
Patch

Attachment 150460 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13129221

New failing tests:
compositing/geometry/abs-position-inside-opacity.html
compositing/geometry/fixed-in-composited.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/compositing-visible-descendant.html
accessibility/aria-labelledby-on-input.html
animations/additive-transform-animations.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/self-painting-layers.html
animations/cross-fade-webkit-mask-box-image.html
animations/3d/change-transform-in-end-event.html
animations/missing-values-last-keyframe.html
http/tests/cache/cancel-during-revalidation-succeeded.html
compositing/geometry/ancestor-overflow-change.html
animations/cross-fade-list-style-image.html
animations/missing-values-first-keyframe.html
animations/cross-fade-background-image.html
compositing/geometry/clipping-foreground.html
animations/cross-fade-border-image-source.html
compositing/geometry/fixed-position-composited-page-scale-down.html
animations/cross-fade-webkit-mask-image.html
compositing/color-matching/image-color-matching.html
compositing/color-matching/pdf-image-match.html

[WebKit Review Bot]

Created attachment 150465 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 150460 [details]
Patch

Attachment 150460 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13136186

New failing tests:
compositing/geometry/abs-position-inside-opacity.html
compositing/geometry/fixed-in-composited.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/compositing-visible-descendant.html
accessibility/aria-labelledby-on-input.html
animations/additive-transform-animations.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/self-painting-layers.html
animations/cross-fade-webkit-mask-box-image.html
animations/3d/change-transform-in-end-event.html
animations/missing-values-last-keyframe.html
http/tests/cache/cancel-during-revalidation-succeeded.html
compositing/geometry/ancestor-overflow-change.html
animations/cross-fade-list-style-image.html
animations/missing-values-first-keyframe.html
animations/cross-fade-background-image.html
compositing/geometry/clipping-foreground.html
animations/cross-fade-border-image-source.html
compositing/geometry/fixed-position-composited-page-scale-down.html
animations/cross-fade-webkit-mask-image.html
compositing/color-matching/image-color-matching.html
compositing/color-matching/pdf-image-match.html

[WebKit Review Bot]

Created attachment 150473 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

Comment on attachment 150460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150460&action=review

> Source/WebCore/rendering/FontBoosting.cpp:28
> +#include "InspectorInstrumentation.h"

What happens if ENABLE(INSPECTOR) is false?

> Source/WebKit/chromium/features.gypi:164
> -          'ENABLE_FONT_BOOSTING=0',
> +          'ENABLE_FONT_BOOSTING=1',

Presumably we don't want to actually enable this on desktop until we've got a setting for turning it off.  ;)

[John Mellor]

Comment on attachment 150460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150460&action=review

>> Source/WebCore/rendering/FontBoosting.cpp:28
>> +#include "InspectorInstrumentation.h"
> 
> What happens if ENABLE(INSPECTOR) is false?

Oops, test code snuck into the patch. I'll reupload a clean version.

[John Mellor]

Created attachment 150571 [details]
Patch

[John Mellor]

Comment on attachment 150460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150460&action=review

>>> Source/WebCore/rendering/FontBoosting.cpp:28
>>> +#include "InspectorInstrumentation.h"
>> 
>> What happens if ENABLE(INSPECTOR) is false?
> 
> Oops, test code snuck into the patch. I'll reupload a clean version.

Oops, that reply was intended for your other comment. This code should be fine - if ENABLE(INSPECTOR) is false, then InspectorImplementation.h defines InspectorInstrumentation::shouldApplyScreenWidthOverride to always return false, as expected. This works the same way as for DOMWindow::innerWidth et al.

>> Source/WebKit/chromium/features.gypi:164
>> +          'ENABLE_FONT_BOOSTING=1',
> 
> Presumably we don't want to actually enable this on desktop until we've got a setting for turning it off.  ;)

Oops, this was the test code I was referring to. I've reuploaded a clean version.

[Eric Seidel (no email)]

Comment on attachment 150571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150571&action=review

This LGTM.  I'm excited for the next patch which makes this into a runtime setting so we can actually test this!

> Source/WebCore/rendering/FontBoosting.cpp:19
> +/*
> + * Copyright (C) 2012 Google Inc. All rights reserved.
> + * Copyright (C) 2012 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */

Did you intend to make this LGPL instead of BSD?  It doesn't matter, but I think most new code from Google is BSD.... Or did this come from Apple somehow?  (iOS?)  Again, it doesn't matter, just checking.

[Simon Fraser (smfr)]

Comment on attachment 150571 [details]
Patch

"font boosting" is a chromium marketing term. Please don't use that for class and file names.

Please also ensure that the license in new files are appropriate for those files containing code from iOS open source files.

[Adam Barth]

> "font boosting" is a chromium marketing term. Please don't use that for class and file names.

Do you have thoughts on a better name?  Some quick brainstorming:

* text scaling
* text boosting
* font scaling
* text autosizing
* embiggenizer

[Simon Fraser (smfr)]

text autosizing.

[Adam Barth]

> text autosizing.

Works for me.

[John Mellor]

Created attachment 150767 [details]
Patch

Renamed the algorithm from Font Boosting to Text Autosizing, and renamed the class to Text Autosizer in line with Eric Seidel's earlier suggestion to use an action name. This patch depends on the rename patch posted to bug 87394 (so there will probably be spurious bot failures in the meantime).

[Adam Barth]

Comment on attachment 150767 [details]
Patch

Forwarding Eric's r+.  I assume you've addressed smfr's licensing comment as well.

[John Mellor]

Comment on attachment 150767 [details]
Patch

> I assume you've addressed smfr's licensing comment as well.

Yes, I kept TextAutosizer.cpp as LGPL to facilitate any future use of iOS code.

[WebKit Review Bot]

Comment on attachment 150767 [details]
Patch

Clearing flags on attachment: 150767

Committed r121866: <http://trac.webkit.org/changeset/121866>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 150767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150767&action=review

> Source/WebCore/rendering/TextAutosizer.cpp:45
> +bool TextAutosizer::boostSubtree(RenderObject* layoutRoot)

You still have "boost" terminology here.

> Source/WebCore/rendering/TextAutosizer.cpp:53
> +#ifdef HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP
> +    IntSize windowSize(320, 480);

Ick. is the inspector stuff not enough?

> Source/WebCore/rendering/TextAutosizer.cpp:57
> +    IntSize windowSize = mainFrame->view()->visibleContentRect(includeScrollbars).size(); // FIXME: Check that this is always in logical (density-independent) pixels (see wkbug.com/87440).

Seems like that comment should not apply to OpenSource; it's Android-specific.

> Source/WebCore/rendering/TextAutosizer.cpp:68
> +void TextAutosizer::boostBlock(RenderBlock* block, LayoutSize windowSize)

Boost again. The size should be a const&.

> Source/WebCore/rendering/TextAutosizer.cpp:80
> +void TextAutosizer::boostText(RenderText* text, float multiplier)

Boost.

> Source/WebCore/rendering/TextAutosizer.cpp:100
> +bool TextAutosizer::treatAsInline(const RenderObject* renderer)
> +{
> +    return !renderer->isRenderBlock() || renderer->isListItem() || renderer->isInlineBlockOrInlineTable();
> +}
> +

Should this be a method on RenderObject?

[Eric Seidel (no email)]

Thank you simon for the review.  It's always nice to have another pair of eyes. :)  Happy (belated) 4th!

[John Mellor]

Reopening to attach new patch.

[John Mellor]

Created attachment 150967 [details]
Follow-up

Addressed smfr's review comments.

[John Mellor]

Comment on attachment 150767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150767&action=review

Thanks for the review Simon. It's really useful to have you and Kenneth giving feedback on these patches :)

>> Source/WebCore/rendering/TextAutosizer.cpp:45
>> +bool TextAutosizer::boostSubtree(RenderObject* layoutRoot)
> 
> You still have "boost" terminology here.

Renamed to process(Subtree|Block|Text).

>> Source/WebCore/rendering/TextAutosizer.cpp:53
>> +    IntSize windowSize(320, 480);
> 
> Ick. is the inspector stuff not enough?

The inspector stuff below is purely to know whether we should include scrollbars or not (copied from DOMWindow::innerWidth) - it doesn't tell us any width information.

The testing patch that followed this (bug 90561) added Settings::setTextAutosizingWindowSizeOverride, as a clean way for layout tests (and perhaps in future the inspector's device metrics emulation) to indicate what device size they are testing.

This hack was purely for running the browser standalone for manual testing. It got removed by the testing patch, and replaced with an ifdef in Settings.cpp that adjusts the default values of the settings. I'd be happy to remove that if you want (since I can always reapply it locally when testing); it just seemed it might make it easier for others wanting to try out Text Autosizing.

>> Source/WebCore/rendering/TextAutosizer.cpp:57
>> +    IntSize windowSize = mainFrame->view()->visibleContentRect(includeScrollbars).size(); // FIXME: Check that this is always in logical (density-independent) pixels (see wkbug.com/87440).
> 
> Seems like that comment should not apply to OpenSource; it's Android-specific.

That issue affects the upstream open source implementation, and may affect other ports too, so it seemed a reasonable concern to document...

>> Source/WebCore/rendering/TextAutosizer.cpp:68
>> +void TextAutosizer::boostBlock(RenderBlock* block, LayoutSize windowSize)
> 
> Boost again. The size should be a const&.

Done (also changed this to an IntSize since it's not actually in CSS pixels).

>> Source/WebCore/rendering/TextAutosizer.cpp:80
>> +void TextAutosizer::boostText(RenderText* text, float multiplier)
> 
> Boost.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:100
>> +
> 
> Should this be a method on RenderObject?

Possibly. A future patch is going to make this method more complicated and more text-autosizing-specific; perhaps we should revisit this then. I've added a FIXME as a reminder.

[Simon Fraser (smfr)]

Comment on attachment 150967 [details]
Follow-up

Thanks!

[WebKit Review Bot]

Comment on attachment 150967 [details]
Follow-up

Clearing flags on attachment: 150967

Committed r121920: <http://trac.webkit.org/changeset/121920>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.