Bug 113363 – Add a settings to disallow initializing background-size if background shorthand doesn't include it.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yongjun Zhang]

For backward-compatibility, we should add a setting to avoid initializing background-size, if the background shorthand doesn't include background-size.  This settings could be adopted by WebKit clients (not the browser) as needed.

[Yongjun Zhang]

Created attachment 195213 [details]
add a flag to avoid initialize background-size in background shorthand if shorthand doesn't include it.

[Yongjun Zhang]

<rdar://problem/12520345>

[Simon Fraser (smfr)]

Comment on attachment 195213 [details]
add a flag to avoid initialize background-size in background shorthand if shorthand doesn't include it.

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

I think it would be clearer to refer to "legacyBackgroundSizeShorthandBehavior" or something, for the various settings and flags.

> Source/WebCore/css/CSSParser.cpp:287
> +    , shouldResetBackgroundSizeInBackgroundShorthand(!document->settings() || document->settings()->shouldResetBackgroundSizeInBackgroundShorthand())

Shame we fetch settings so many times in this code.

[Yongjun Zhang]

Created attachment 195229 [details]
change shouldResetBackgroundSizeInBackgroundShorthand to legacyBackgroundSizeShorthandBehavior to make it clearer.

[David Kilzer (:ddkilzer)]

Comment on attachment 195213 [details]
add a flag to avoid initialize background-size in background shorthand if shorthand doesn't include it.

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

>> Source/WebCore/css/CSSParser.cpp:287
>> +    , shouldResetBackgroundSizeInBackgroundShorthand(!document->settings() || document->settings()->shouldResetBackgroundSizeInBackgroundShorthand())
> 
> Shame we fetch settings so many times in this code.

We'd have to move the initialization code into the body of the method to fix this.

[Simon Fraser (smfr)]

Comment on attachment 195229 [details]
change shouldResetBackgroundSizeInBackgroundShorthand to legacyBackgroundSizeShorthandBehavior to make it clearer.

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

> Source/WebCore/css/CSSParser.h:600
> +    bool legacyBackgroundSizeShorthandBehavior() const;

Sorry to suggest another name change, but I think it would be clearer to say useLegacyBackgroundSizeShorthandBehavior() everywhere.

[David Kilzer (:ddkilzer)]

Comment on attachment 195229 [details]
change shouldResetBackgroundSizeInBackgroundShorthand to legacyBackgroundSizeShorthandBehavior to make it clearer.

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

r=me

> Source/WebCore/css/CSSParserMode.h:77
>      bool needsSiteSpecificQuirks;
>      bool enforcesCSSMIMETypeInNoQuirksMode;
> +    bool legacyBackgroundSizeShorthandBehavior;

Nit:  There are 10 bool variables in CSSParserContext now.  Seems like we could make them all bitfields and save some bytes on the size of the struct (as long as they're all initialized properly).  (Not for this patch.)

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (From update of attachment 195229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195229&action=review
> 
> r=me

I think we should implement Simon's name change to useLegacyBackgroundSizeShorthandBehavior() as well.

[Yongjun Zhang]

(In reply to comment #6)
> (From update of attachment 195229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195229&action=review
> 
> > Source/WebCore/css/CSSParser.h:600
> > +    bool legacyBackgroundSizeShorthandBehavior() const;
> 
> Sorry to suggest another name change, but I think it would be clearer to say useLegacyBackgroundSizeShorthandBehavior() everywhere.

no worries, useLegacyBackgroundSizeShorthandBehavior does sound better.  I will make the change.

[Yongjun Zhang]

Created attachment 195344 [details]
Address review comments, change the name of settings to useLegacyBackgroundSizeShorthandBehavior.

[WebKit Review Bot]

Comment on attachment 195344 [details]
Address review comments, change the name of settings to useLegacyBackgroundSizeShorthandBehavior.

Clearing flags on attachment: 195344

Committed r147034: <http://trac.webkit.org/changeset/147034>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.