Bug 120173 - [iOS] Upstream changes to WebCore/style
Summary: [iOS] Upstream changes to WebCore/style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks: 139946
  Show dependency treegraph
 
Reported: 2013-08-22 14:35 PDT by Daniel Bates
Modified: 2016-03-11 11:03 PST (History)
3 users (show)

See Also:


Attachments
Patch (4.89 KB, patch)
2013-08-22 14:39 PDT, Daniel Bates
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2013-08-22 14:35:09 PDT
Upstream iOS-specific changes to WebCore/style. We should look to clean up and find a better place for the content observer code.
Comment 1 Daniel Bates 2013-08-22 14:39:37 PDT
Created attachment 209395 [details]
Patch
Comment 2 Darin Adler 2013-08-23 09:44:37 PDT
Comment on attachment 209395 [details]
Patch

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

Looks fine. The merged code isn’t great, but we can improve it later; good to have it upstreamed as a start.

> Source/WebCore/style/StyleResolveTree.cpp:474
> +    CheckForVisibilityChangeOnRecalcStyle checkForVisibilityChange(current, current->renderStyle());

Seems like a strange way to plug this in. Maybe we can come up with something better or at least explain why this is platform-specific.

> Source/WebCore/style/StyleResolveTree.cpp:542
> +            if (settings->fontFallbackPrefersPictographs() && document->styleResolverIfExists())
> +                documentStyle->font().update(document->styleResolverIfExists()->fontSelector());

Would be nice to put style resolver into a local rather than evaluating it twice.
Comment 3 Daniel Bates 2013-08-29 10:30:11 PDT
(In reply to comment #2)
> [...]
> > Source/WebCore/style/StyleResolveTree.cpp:474
> > +    CheckForVisibilityChangeOnRecalcStyle checkForVisibilityChange(current, current->renderStyle());
> 
> Seems like a strange way to plug this in. Maybe we can come up with something better or at least explain why this is platform-specific.

I agree. Filed bug #120478 to fix this issue.

> 
> > Source/WebCore/style/StyleResolveTree.cpp:542
> > +            if (settings->fontFallbackPrefersPictographs() && document->styleResolverIfExists())
> > +                documentStyle->font().update(document->styleResolverIfExists()->fontSelector());
> 
> Would be nice to put style resolver into a local rather than evaluating it twice.

Will fix before landing.
Comment 4 Daniel Bates 2013-08-29 10:33:58 PDT
Committed r154820: <http://trac.webkit.org/changeset/154820>