Bug 88655 - Text Autosizing: Add basic framework
Summary: Text Autosizing: Add basic framework
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: John Mellor
URL:
Keywords:
Depends on:
Blocks: FontBoosting
  Show dependency treegraph
 
Reported: 2012-06-08 06:51 PDT by John Mellor
Modified: 2012-07-05 12:57 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.45 KB, patch)
2012-06-08 06:56 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2012-06-28 06:13 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (13.42 KB, patch)
2012-07-02 12:02 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (1.26 MB, application/zip)
2012-07-02 13:06 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-06 (1.47 MB, application/zip)
2012-07-02 14:07 PDT, WebKit Review Bot
no flags Details
Patch (12.80 KB, patch)
2012-07-03 03:21 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Patch (12.83 KB, patch)
2012-07-04 04:07 PDT, John Mellor
no flags Details | Formatted Diff | Diff
Follow-up (6.12 KB, patch)
2012-07-05 11:41 PDT, John Mellor
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Mellor 2012-06-08 06:51:28 PDT
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.
Comment 1 John Mellor 2012-06-08 06:56:39 PDT
Created attachment 146559 [details]
Patch
Comment 2 Adam Barth 2012-06-08 11:24:37 PDT
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.
Comment 3 Adam Barth 2012-06-08 11:26:12 PDT
Unfortunately, I don't know much about the rendering tree.

@jchaffraix: Would you be willing to review this patch?
Comment 4 John Mellor 2012-06-28 06:13:54 PDT
Created attachment 149936 [details]
Patch

Addressed abarth's comments.
Comment 5 Eric Seidel (no email) 2012-06-28 08:50:02 PDT
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.
Comment 6 Eric Seidel (no email) 2012-06-28 08:50:34 PDT
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?
Comment 7 Eric Seidel (no email) 2012-06-28 09:05:31 PDT
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?
Comment 8 Eric Seidel (no email) 2012-06-28 09:10:19 PDT
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.
Comment 9 Eric Seidel (no email) 2012-06-28 09:15:56 PDT
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.
Comment 10 Adam Barth 2012-07-02 09:48:11 PDT
Comment on attachment 149936 [details]
Patch

John, would you be willing to post a new patch that addresses Eric's review comments?
Comment 11 John Mellor 2012-07-02 12:01:51 PDT
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.
Comment 12 John Mellor 2012-07-02 12:02:25 PDT
Created attachment 150460 [details]
Patch
Comment 13 WebKit Review Bot 2012-07-02 13:06:34 PDT
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
Comment 14 WebKit Review Bot 2012-07-02 13:06:39 PDT
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
Comment 15 WebKit Review Bot 2012-07-02 14:06:55 PDT
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
Comment 16 WebKit Review Bot 2012-07-02 14:07:01 PDT
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
Comment 17 Adam Barth 2012-07-02 15:18:59 PDT
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.  ;)
Comment 18 John Mellor 2012-07-03 03:20:34 PDT
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.
Comment 19 John Mellor 2012-07-03 03:21:30 PDT
Created attachment 150571 [details]
Patch
Comment 20 John Mellor 2012-07-03 07:46:07 PDT
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.
Comment 21 Eric Seidel (no email) 2012-07-03 11:46:58 PDT
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.
Comment 22 Simon Fraser (smfr) 2012-07-03 11:53:26 PDT
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.
Comment 23 Adam Barth 2012-07-03 13:32:11 PDT
> "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
Comment 24 Simon Fraser (smfr) 2012-07-03 14:22:04 PDT
text autosizing.
Comment 25 Adam Barth 2012-07-03 14:33:05 PDT
> text autosizing.

Works for me.
Comment 26 John Mellor 2012-07-04 04:07:37 PDT
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).
Comment 27 Adam Barth 2012-07-04 08:08:06 PDT
Comment on attachment 150767 [details]
Patch

Forwarding Eric's r+.  I assume you've addressed smfr's licensing comment as well.
Comment 28 John Mellor 2012-07-04 09:32:20 PDT
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.
Comment 29 WebKit Review Bot 2012-07-04 10:43:29 PDT
Comment on attachment 150767 [details]
Patch

Clearing flags on attachment: 150767

Committed r121866: <http://trac.webkit.org/changeset/121866>
Comment 30 WebKit Review Bot 2012-07-04 10:43:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Simon Fraser (smfr) 2012-07-05 10:20:42 PDT
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?
Comment 32 Eric Seidel (no email) 2012-07-05 10:27:19 PDT
Thank you simon for the review.  It's always nice to have another pair of eyes. :)  Happy (belated) 4th!
Comment 33 John Mellor 2012-07-05 11:41:19 PDT
Reopening to attach new patch.
Comment 34 John Mellor 2012-07-05 11:41:24 PDT
Created attachment 150967 [details]
Follow-up

Addressed smfr's review comments.
Comment 35 John Mellor 2012-07-05 11:42:06 PDT
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.
Comment 36 Simon Fraser (smfr) 2012-07-05 11:43:28 PDT
Comment on attachment 150967 [details]
Follow-up

Thanks!
Comment 37 WebKit Review Bot 2012-07-05 12:57:39 PDT
Comment on attachment 150967 [details]
Follow-up

Clearing flags on attachment: 150967

Committed r121920: <http://trac.webkit.org/changeset/121920>
Comment 38 WebKit Review Bot 2012-07-05 12:57:48 PDT
All reviewed patches have been landed.  Closing bug.