Bug 113363 - Add a settings to disallow initializing background-size if background shorthand doesn't include it.
Summary: Add a settings to disallow initializing background-size if background shortha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-26 20:11 PDT by Yongjun Zhang
Modified: 2013-03-27 17:23 PDT (History)
10 users (show)

See Also:


Attachments
add a flag to avoid initialize background-size in background shorthand if shorthand doesn't include it. (12.00 KB, patch)
2013-03-26 20:54 PDT, Yongjun Zhang
simon.fraser: review-
Details | Formatted Diff | Diff
change shouldResetBackgroundSizeInBackgroundShorthand to legacyBackgroundSizeShorthandBehavior to make it clearer. (11.74 KB, patch)
2013-03-26 23:09 PDT, Yongjun Zhang
ddkilzer: review+
Details | Formatted Diff | Diff
Address review comments, change the name of settings to useLegacyBackgroundSizeShorthandBehavior. (11.83 KB, patch)
2013-03-27 09:45 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.