Bug 37514

Created attachment 79940 [details] patch Proposal for css3 background-position.

Attachment 79940 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css3/css3-background-position-..." exit_code: 1 Source/WebCore/rendering/style/FillLayer.h:180: Extra space between int and m_xOrigin [whitespace/declaration] [3] Source/WebCore/rendering/style/FillLayer.h:181: Extra space between int and m_yOrigin [whitespace/declaration] [3] Source/WebCore/rendering/style/FillLayer.cpp:23: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 79942 [details] patch2 fix style issue.

Comment on attachment 79942 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=79942&action=review > Source/WebCore/css/CSSParser.cpp:2567 > +static bool isBackgroundPositionCKeyword(RefPtr<CSSPrimitiveValue> value) You mean to use a raw pointer here. Please read http://www.webkit.org/coding/RefPtr.html

Comment on attachment 79942 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=79942&action=review WE need to find ways to share more code here. You have so many copy/paste code paths. > Source/WebCore/css/CSSParser.cpp:2601 > + } else { > + valueX = CSSPrimitiveValue::create(Pair::create(CSSPrimitiveValue::createIdentifier(CSSValueLeft), value)); > + } No {} on one line clauses. Seems we might want to wrap this in a helper method, since you seem to do the same thing in all 3 of these cases, and at least the CSSPrimativeValue::create call could be shared. > Source/WebCore/css/CSSParser.cpp:2605 > +static PassRefPtr<CSSPrimitiveValue> createBackgroundPositionY(PassRefPtr<CSSPrimitiveValue> pValue, bool usePair = true) Soooo much duplicated code :(

It looks like this patch might be abandoned. Unless someone objects, I'll find someone to take a stab at making the requested changes.

<rdar://problem/10218267>

It would be good to make progress on this, because the Transforms spec may end up wanting to re-use the same parsing behavior for transform-origin and perspective-origin.

Created attachment 135521 [details] Proposed patch

Comment on attachment 135521 [details] Proposed patch Attachment 135521 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12324576 New failing tests: ietestcenter/css3/bordersbackgrounds/background_position_three_four_values.htm

Created attachment 135525 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 135560 [details] Proposed patch

Comment on attachment 135560 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135560&action=review It's difficult to actually see all that you changed because this change is huge. I would advise you to split it into a refactoring that aligns the code with what you want and the real change that introduces the new syntax. As-is, it's definitely not a good change due to the un-intended changes, the loss of CSSValuePool and everything I have missed as I haven't spend much time looking at whether the extended grammar matches the spec. > Source/WebCore/ChangeLog:3 > + Background-position parsing more than 2 values This should be the bug number here. If you just run Tools/Scripts/prepare-ChangeLog, it would fill this part. > Source/WebCore/ChangeLog:24 > + Create value from keyword left/top - 0%, center - 50%, right/bottom - 100%. Your ChangeLog is lacking the details: what are you allowing as part of this change? > Source/WebCore/css/CSSParser.cpp:3283 > + return CSSPrimitiveValue::createIdentifier(id); AFAICT you want to use CSSValuePool as much as possible as it does some internal caching so this should go through CSSValuePool. > Source/WebCore/css/CSSParser.cpp:3291 > +void CSSParser::parseFillPosition(CSSParserValueList* valueList, RefPtr<CSSValue>& value1, RefPtr<CSSValue>& value2, unsigned int numValues) parseFillPosition is shared with -webkit-mask-position and several other properties are also using the function. This is never mentioned in your change that this is a side effect and the lack of testing makes me think you didn't see that. I haven't found some standard information about -webkit-mask-position so I cannot tell if this is a desirable change. Each properties needs to be evaluated as you may be deviating from the existing standards. > Source/WebCore/css/CSSParser.cpp:3301 > + if (values.size() == numValues) I am not sure what is the use of numValues here. First the name is so vague as to be confusing (what does it represents? Could you use that to rename the variable to something helping). Then I would have expected it to reflect something (like the size of some vectors or passed in variable) but it bears no relation to anything. Third it seems completely arbitrary that the default value is 0 (for which btw this condition will never be true as you always append something above). > Source/WebCore/css/CSSParser.cpp:-3297 > - // Only one value was specified. If that value was not a keyword, then it sets the x position, and the y position > - // is simply 50%. This is our default. > - // For keywords, the keyword was either an x-keyword (left/right), a y-keyword (top/bottom), or an ambiguous keyword (center). > - // For left/right/center, the default of 50% in the y is still correct. Removing useful comments is really NOT OK. This comment explains why the magic constant 50% below in the '1' case. > Source/WebCore/rendering/style/FillLayer.cpp:25 > +#include "CSSValueKeywords.h" That's a layering violation. The whole purpose of the rendering/style/ classes is to hold the computed style but also to isolate the rendering code from the CSS code. The proper way is to introduce an enum in RenderStyleConstants.h to hold the computed values. > LayoutTests/ChangeLog:14 > + * css3/calc/resources/smiley.png: Added. Where did you get this file? You need the right to this image for us to integrate it and I am not sure this is the case here. Preferably re-use an existing image. > LayoutTests/ChangeLog:15 > + * platform/chromium/test_expectations.txt: Skip background-position test Why is that fine to make one of the IE test to fail? You need a good justification here before skipping it. Also such changes need to be done on *all* platforms that have a baseline, not on the only EWS that runs the test. > LayoutTests/css3/calc/background-position.html:1 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> We usually prefer the HTML5 doctype for WebKit-specific test: <!DOCTYPE html> > LayoutTests/css3/calc/background-position.html:2 > +<html lang="en"> No lang please. > LayoutTests/css3/calc/background-position.html:5 > + <style type="text/css"> No type. > LayoutTests/css3/calc/background-position.html:6 > + .case { width: 128px; height: 128px; float: left; border: solid; margin: 0.2em; } You could easily remove the case class and use instead the div tag selector (div { .... }) > LayoutTests/css3/calc/background-position.html:7 > + .a, .b, .c, .d, .e, .f, .g, .h, .i, .j, .k, .l, .m, .n, .o, .p, .r, .s, .t, .u, .v, .w, .x1, .x2, .x3, .x4, .x5, .x6 no .y? Anyway, it's pretty confusing to use just letters for those classes and it doesn't make the test case easy to read. > LayoutTests/css3/calc/background-position.html:14 > + .case .a { background-position: left 25% top 25%; } Not a huge fan of the combined selectors here: .a should be enough no?

Created attachment 138085 [details] Draft patch Thanks for reviewing. This is smaller patch with helper functions and parsing code. I will add testcases in next patch.

Hi Julien, can you please review the patch?

Comment on attachment 138085 [details] Draft patch View in context: https://bugs.webkit.org/attachment.cgi?id=138085&action=review To be frank, I think your approach of coming and implementing the whole grammar (even worse the grammars of different CSS values) is going to be painful: you have little experience in WebKit and > 20k patches from someone with little experience in the WebKit way have little change to get landed (also they are a reviewer time sink) Think about this incrementally: which step as small as possible can you take towards supporting the full grammar while getting your change tested! It's important to keep your changes small and tested: this helps reviewers assess the quality of your change. My take here would be to add parsing support for one <position> first, then add the needed rendering. Once this is done, scale to 2 <position>, ... Also this bug is about 'background-position', it is good that you are thinking about other values but don't get ahead of your self: getting 'background-position' is enough work for now. I haven't read the whole patch because I think it should be split and tested. > Source/WebCore/ChangeLog:11 > + Currently parser supports only parsing of two values for background-position property. > + According to CSS specification http://www.w3.org/TR/css3-background/#the-background-position, > + it can take upto four values. This parsing function can also be reused for parsing other > + properties like -transform-origin, -perspective-origin, -webkit-mask-position and -radial-gradient. Saying what grammar you *do* support would be helpful instead of having to extract it from the code. > Source/WebCore/ChangeLog:13 > + No functionality change. Tests to be added in next patch. Technically this is a change in functionality. It's not exposed in JavaScript as you don't implement the getComputedStyle part though. Pushing the tests in another bug is not going to work as we expect changes in functionality to be tested. > Source/WebCore/css/CSSParser.cpp:3225 > + if (value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) { > + int key = value->getIdent(); > + return key == CSSValueLeft || key == CSSValueRight > + || key == CSSValueTop || key == CSSValueBottom || key == CSSValueCenter; > + } > + return false; This should be: return isBackgroundKeywordX(value) || isBackgroundKeyword(value) || isBackgroundKeywordC(value); > Source/WebCore/css/CSSParser.cpp:3243 > + if (value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) { > + int key = value->getIdent(); > + return key == CSSValueTop || key == CSSValueBottom; > + } > + return false; We prefer early return (not repeated): if (!value->isIdent()) return false; int identity = value->getIdent(); return identity == ...; (btw, |key| is not a good variable name as it doesn't bear any meaning) > Source/WebCore/css/CSSParser.cpp:3246 > +static bool isBackgroundKeywordC(CSSPrimitiveValue* value) The naming doesn't cut. I guess C as Center but please don't abbreviate needlessly your names. > Source/WebCore/rendering/style/FillLayer.h:208 > + bool m_xOriginSet : 1; > + bool m_yOriginSet : 1; The whole pattern of having a bool for being set + the value above makes me think we should have some structure to do that for us. We would need to be smart about it.

Comment on attachment 138085 [details] Draft patch View in context: https://bugs.webkit.org/attachment.cgi?id=138085&action=review Thanks for reviewing. Rendering code for one or two <position> is already present. So my approach here was to reduce three or four <position> into two <position>. There is a special case when offset in pixels from right or bottom is specified where rendering support is needed. I will upload a patch which only handles background-position and addressing review comments along with test cases. >> Source/WebCore/ChangeLog:11 >> + properties like -transform-origin, -perspective-origin, -webkit-mask-position and -radial-gradient. > > Saying what grammar you *do* support would be helpful instead of having to extract it from the code. I will make changes such that only background-position specification is handled. Other properties will be handled in different bug to keep the patch smaller and easier for review. >> Source/WebCore/ChangeLog:13 >> + No functionality change. Tests to be added in next patch. > > Technically this is a change in functionality. It's not exposed in JavaScript as you don't implement the getComputedStyle part though. Pushing the tests in another bug is not going to work as we expect changes in functionality to be tested. This is a draft patch where only parsing functions are written but not called from anywhere. As per your previous suggestion, I was splitting up bigger patch into parsing code and real patch that introduces the change. I meant to add tests to this bug itself when real patch would be uploaded as this draft patch itself was getting bigger. >> Source/WebCore/css/CSSParser.cpp:3225 >> + return false; > > This should be: > > return isBackgroundKeywordX(value) || isBackgroundKeyword(value) || isBackgroundKeywordC(value); Done. >> Source/WebCore/css/CSSParser.cpp:3243 >> + return false; > > We prefer early return (not repeated): > if (!value->isIdent()) > return false; > int identity = value->getIdent(); > return identity == ...; > > (btw, |key| is not a good variable name as it doesn't bear any meaning) Done. >> Source/WebCore/css/CSSParser.cpp:3246 >> +static bool isBackgroundKeywordC(CSSPrimitiveValue* value) > > The naming doesn't cut. I guess C as Center but please don't abbreviate needlessly your names. Done. >> Source/WebCore/rendering/style/FillLayer.h:208 >> + bool m_yOriginSet : 1; > > The whole pattern of having a bool for being set + the value above makes me think we should have some structure to do that for us. We would need to be smart about it. Right. But shouldn't that be done as different patch as this patch is already getting bigger and we would be touching code related to other fill properties?

Created attachment 140488 [details] WIP patch with two position values WIP patch addressing review comments

Comment on attachment 140488 [details] WIP patch with two position values I am going to break full patch down into different bugs instead of doing it incrementally in same bug.

(In reply to comment #19) > (From update of attachment 140488 [details]) > I am going to break full patch down into different bugs instead of doing it incrementally in same bug. This bug has been inactive since May, If nobody objects, I'd like to take it to actually implement the feature. I landed new tests already to backup the existing implementation.

(In reply to comment #20) > (In reply to comment #19) > > (From update of attachment 140488 [details] [details]) > > I am going to break full patch down into different bugs instead of doing it incrementally in same bug. > > This bug has been inactive since May, If nobody objects, I'd like to take it to actually implement the feature. I landed new tests already to backup the existing implementation. Sure.

I think we can close this bug now. The support is now enabled by default on all WebKit port.