Bug 113363

Summary: Add a settings to disallow initializing background-size if background shorthand doesn't include it.
Product: WebKit Reporter: Yongjun Zhang <yongjun_zhang>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, esprehn+autocc, koivisto, ktf.kim, macpherson, menard, mjs, ojan.autocc, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
Attachments:
Description Flags
add a flag to avoid initialize background-size in background shorthand if shorthand doesn't include it.
simon.fraser: review-
change shouldResetBackgroundSizeInBackgroundShorthand to legacyBackgroundSizeShorthandBehavior to make it clearer.
ddkilzer: review+
Address review comments, change the name of settings to useLegacyBackgroundSizeShorthandBehavior. none

Description Yongjun Zhang 2013-03-26 20:11:57 PDT
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.
Comment 1 Yongjun Zhang 2013-03-26 20:54:24 PDT
Created attachment 195213 [details]
add a flag to avoid initialize background-size in background shorthand if shorthand doesn't include it.
Comment 2 Yongjun Zhang 2013-03-26 21:03:37 PDT
<rdar://problem/12520345>
Comment 3 Simon Fraser (smfr) 2013-03-26 21:12:32 PDT
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.
Comment 4 Yongjun Zhang 2013-03-26 23:09:54 PDT
Created attachment 195229 [details]
change shouldResetBackgroundSizeInBackgroundShorthand to legacyBackgroundSizeShorthandBehavior to make it clearer.
Comment 5 David Kilzer (:ddkilzer) 2013-03-27 09:03:21 PDT
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.
Comment 6 Simon Fraser (smfr) 2013-03-27 09:07:32 PDT
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.
Comment 7 David Kilzer (:ddkilzer) 2013-03-27 09:08:30 PDT
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.)
Comment 8 David Kilzer (:ddkilzer) 2013-03-27 09:09:32 PDT
(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.
Comment 9 Yongjun Zhang 2013-03-27 09:10:59 PDT
(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.
Comment 10 Yongjun Zhang 2013-03-27 09:45:10 PDT
Created attachment 195344 [details]
Address review comments, change the name of settings to useLegacyBackgroundSizeShorthandBehavior.
Comment 11 WebKit Review Bot 2013-03-27 17:23:33 PDT
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>
Comment 12 WebKit Review Bot 2013-03-27 17:23:38 PDT
All reviewed patches have been landed.  Closing bug.