Bug 126237 – [iOS] Upstream WebCore/css changes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Daniel Bates]

Upstream the iOS related changes to WebCore/css.

[Daniel Bates]

Created attachment 220010 [details]
Patch

[WebKit Commit Bot]

Attachment 220010 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/DeprecatedStyleBuilder.cpp', u'Source/WebCore/css/MediaFeatureNames.h', u'Source/WebCore/css/MediaQueryEvaluator.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/html.css', u'Source/WebCore/css/mathml.css', u'Source/WebCore/css/mediaControlsiOS.css', u'Source/WebCore/css/svg.css', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/css/MediaQueryEvaluator.cpp:641:  video_playable_inlineMediaFeatureEval is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Comment on attachment 220010 [details]
Patch

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

I have one comment, shouldn't it be better to fix the composition-fill-color property usage before and land this after that without all those FIXME's? If that's not possible (too complicated) what about including a link to the tracking bug?

> Source/WebCore/css/MediaQueryEvaluator.cpp:641
> +static bool video_playable_inlineMediaFeatureEval(CSSValue*, RenderStyle*, Frame* frame, MediaFeaturePrefix)

CamelCase I guess. Also why are you passing all those arguments to the function if only frame is being used?

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 220010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220010&action=review
> 
> I have one comment, shouldn't it be better to fix the composition-fill-color property usage before and land this after that without all those FIXME's? If that's not possible (too complicated) what about including a link to the tracking bug?

It will be more straightforward to fix this up in a follow bug. Filed bug #126296. I'll update the comments to reference this bug number.

> 
> > Source/WebCore/css/MediaQueryEvaluator.cpp:641
> > +static bool video_playable_inlineMediaFeatureEval(CSSValue*, RenderStyle*, Frame* frame, MediaFeaturePrefix)
> 
> CamelCase I guess. Also why are you passing all those arguments to the function if only frame is being used?

Notice that the signature of this function is partially fixed by the preprocessor logic for defining a new CSS media query feature. Some of that logic is in MediaFeatureNames.h , <http://trac.webkit.org/browser/trunk/Source/WebCore/css/MediaFeatureNames.h?rev=148431#L34>. I suggest that we defer changing the name of this function to a follow up patch so that we can also simultaneously change the name of all the other CSS media query functions. For completeness, the name of this function, specifically its use of CamelCase, is consistent with the naming convention used for other media query feature functions (e.g. device_aspect_ratioMediaFeatureEval() -  <http://trac.webkit.org/browser/trunk/Source/WebCore/css/MediaQueryEvaluator.cpp?rev=160763#L269>).

With regards to the unused parameters, the parameter list of this function is fixed by its caller, MediaQueryEvaluator::eval(), <http://trac.webkit.org/browser/trunk/Source/WebCore/css/MediaQueryEvaluator.cpp?rev=160763#L715>. It turns out that this function only makes use of the passed Frame.

[Simon Fraser (smfr)]

Comment on attachment 220010 [details]
Patch

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

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2292
> +    // be float for sub-pixel kerning. See <rdar://problem/5020763>.

Wrong radar number?

Reference https://bugs.webkit.org/show_bug.cgi?id=20606 in the comment?

> Source/WebCore/css/MediaQueryEvaluator.cpp:635
> +    static wkDeviceClass deviceClass = iosDeviceClass();
> +    return deviceClass == wkDeviceClassiPhone || deviceClass == wkDeviceClassiPod;

Seems odd for this behavioral limitation to be hardcoded in WebCore. It should be a client decision; maybe exposed via ChromeClient?

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 220010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220010&action=review
> 
> > Source/WebCore/css/DeprecatedStyleBuilder.cpp:2292
> > +    // be float for sub-pixel kerning. See <rdar://problem/5020763>.
> 
> Wrong radar number?
> 
> Reference https://bugs.webkit.org/show_bug.cgi?id=20606 in the comment?

Will fix up comment to reference bug #20606.

> 
> > Source/WebCore/css/MediaQueryEvaluator.cpp:635
> > +    static wkDeviceClass deviceClass = iosDeviceClass();
> > +    return deviceClass == wkDeviceClassiPhone || deviceClass == wkDeviceClassiPod;
> 
> Seems odd for this behavioral limitation to be hardcoded in WebCore. It should be a client decision; maybe exposed via ChromeClient?

I agree. I hope you don't mind that I add a FIXME comment for now to fix this issue.

[Daniel Bates]

Committed r161286: <http://trac.webkit.org/changeset/161286>