| Summary: | Text Autosizing: Add basic framework | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | John Mellor <johnme> | ||||||||||||||||||
| Component: | Text | Assignee: | John Mellor <johnme> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Enhancement | CC: | abarth, dglazkov, eoconnor, eric, jchaffraix, kenneth, peter, satish, simon.fraser, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 84186 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
John Mellor
2012-06-08 06:51:28 PDT
Created attachment 146559 [details]
Patch
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. Unfortunately, I don't know much about the rendering tree. @jchaffraix: Would you be willing to review this patch? Created attachment 149936 [details]
Patch
Addressed abarth's comments.
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 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 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 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 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 on attachment 149936 [details]
Patch
John, would you be willing to post a new patch that addresses Eric's review comments?
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. Created attachment 150460 [details]
Patch
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 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 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 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 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 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. Created attachment 150571 [details]
Patch
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 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 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.
> "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
text autosizing. > text autosizing.
Works for me.
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 on attachment 150767 [details]
Patch
Forwarding Eric's r+. I assume you've addressed smfr's licensing comment as well.
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 on attachment 150767 [details] Patch Clearing flags on attachment: 150767 Committed r121866: <http://trac.webkit.org/changeset/121866> All reviewed patches have been landed. Closing bug. 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? Thank you simon for the review. It's always nice to have another pair of eyes. :) Happy (belated) 4th! Reopening to attach new patch. Created attachment 150967 [details]
Follow-up
Addressed smfr's review comments.
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 on attachment 150967 [details]
Follow-up
Thanks!
Comment on attachment 150967 [details] Follow-up Clearing flags on attachment: 150967 Committed r121920: <http://trac.webkit.org/changeset/121920> All reviewed patches have been landed. Closing bug. |