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:

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

Yongjun Zhang

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yongjun Zhang

Comment 1 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.

Yongjun Zhang

Comment 2 2013-03-26 21:03:37 PDT

<rdar://problem/12520345>

Simon Fraser (smfr)

Comment 3 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.

Yongjun Zhang

Comment 4 2013-03-26 23:09:54 PDT

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

David Kilzer (:ddkilzer)

Comment 5 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.

Simon Fraser (smfr)

Comment 6 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.

David Kilzer (:ddkilzer)

Comment 7 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.)

David Kilzer (:ddkilzer)

Comment 8 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.

Yongjun Zhang

Comment 9 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.

Yongjun Zhang

Comment 10 2013-03-27 09:45:10 PDT

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

WebKit Review Bot

Comment 11 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>

WebKit Review Bot

Comment 12 2013-03-27 17:23:38 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.