Bug 133186

Summary: -apple-system- styled element doesn't respond to system font size changes
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebKit2Assignee: Martin Hock <mhock>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, enrica, esprehn+autocc, glenn, kondapallykalyan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
enrica: review+
address comments
none
remove unneeded change to FontCacheIOS.mm none

Description Martin Hock 2014-05-22 11:09:32 PDT 
-apple-system- styled element doens't respond to system font size changes.
<rdar://problem/16583782>
Comment 1 Martin Hock 2014-05-22 11:13:45 PDT 
Created attachment 231899 [details]
patch
Comment 2 Enrica Casucci 2014-05-22 12:15:48 PDT 
Comment on attachment 231899 [details]
patch

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

Looks good to me, provided you address the comments.

> Source/WebCore/ChangeLog:3
> +        -apple-system- styled element doens't respond to system font size changes.

typo: doesn't. We normally use the same title for all ChangeLogs in the same patch.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:2272
> +		759CB837192DA9190012BC64 /* ControlStates.h in Headers */ = {isa = PBXBuildFile; fileRef = 311C08BC18E35D6800B65615 /* ControlStates.h */; settings = {ATTRIBUTES = (Private, ); }; };

What is this?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25764
> +				759CB837192DA9190012BC64 /* ControlStates.h in Headers */,

What is this change?

> Source/WebCore/rendering/RenderThemeIOS.mm:315
>  CFStringRef RenderThemeIOS::contentSizeCategory()

No need to use CFStringRef here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:165
> +    CFStringRef _userTextSize;

We normally use NSString.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1043
> +- (CFStringRef)_contentSizeCategory

Ditto. This way you don't need the cast on the return.
Comment 3 Martin Hock 2014-05-22 12:23:36 PDT 
(In reply to comment #2)
> (From update of attachment 231899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231899&action=review
> 
> Looks good to me, provided you address the comments.
> 
> > Source/WebCore/ChangeLog:3
> > +        -apple-system- styled element doens't respond to system font size changes.
> 
> typo: doesn't. We normally use the same title for all ChangeLogs in the same patch.

Oops, put a bad title in that one, thanks!

> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:2272
> > +		759CB837192DA9190012BC64 /* ControlStates.h in Headers */ = {isa = PBXBuildFile; fileRef = 311C08BC18E35D6800B65615 /* ControlStates.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> What is this?
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25764
> > +				759CB837192DA9190012BC64 /* ControlStates.h in Headers */,
> 
> What is this change?

This is needed in order to #import <WebCore/RenderThemeIOS.h> in WebPageIOS.mm. It's because RenderThemeIOS needs RenderTheme which in turn needs ControlStates.

> > Source/WebCore/rendering/RenderThemeIOS.mm:315
> >  CFStringRef RenderThemeIOS::contentSizeCategory()
> 
> No need to use CFStringRef here.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:165
> > +    CFStringRef _userTextSize;
> 
> We normally use NSString.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1043
> > +- (CFStringRef)_contentSizeCategory
> 
> Ditto. This way you don't need the cast on the return.

OK, I'll try getting rid of the CFStringRefs.
Comment 4 Martin Hock 2014-05-22 14:04:58 PDT 
Created attachment 231913 [details]
address comments
Comment 5 Martin Hock 2014-05-22 14:40:40 PDT 
Created attachment 231914 [details]
remove unneeded change to FontCacheIOS.mm
Comment 6 Martin Hock 2014-05-22 14:53:01 PDT 
Committed r169223: <http://trac.webkit.org/changeset/169223>