Implement CSSPropertySize in CSSStyleApplyProperty.
Created attachment 116311 [details] Patch
Comment on attachment 116311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116311&action=review > Source/WebCore/ChangeLog:8 > + This refactoring moves the implementation of the page size calculation into CSSStyleApplyPropwerty Typo: CSSStyleApplyProperty. > Source/WebCore/ChangeLog:11 > + No new tests / refactoing only. Typo: refactoring. > Source/WebCore/css/CSSStyleApplyProperty.cpp:680 > + static bool pageSizeFromName(CSSPrimitiveValue* pageSizeName, CSSPrimitiveValue* pageOrientation, Length& width, Length& height) I believe this function should be called getPageSizeFromName() since it doesn't return a "page size" directly, but rather fills in out arguments. > Source/WebCore/css/CSSStyleApplyProperty.cpp:692 > + if (!pageSizeName || pageSizeName->primitiveType() != CSSPrimitiveValue::CSS_IDENT) > + return false; The primitiveType() check here is redundant, since getIdent() would return 0 for non-CSS_IDENT values below and bail via the default case. > Source/WebCore/css/CSSStyleApplyProperty.cpp:733 > + if (pageOrientation->primitiveType() != CSSPrimitiveValue::CSS_IDENT) > + return false; Ditto. > Source/WebCore/css/CSSStyleApplyProperty.cpp:756 > + CSSValueListInspector inspector = value; We should use constructor syntax here: CSSValueListInspector inspector(value); Also, it appears that this site is the only user of the CSSValueListInspector class. Are there plans to expand its usage? Otherwise I think we could use CSSValueList methods directly here and remove the class. > Source/WebCore/css/CSSStyleApplyProperty.cpp:762 > + pageSizeType = PAGE_SIZE_RESOLVED; > + if (!inspector.first()->isPrimitiveValue() || !inspector.second()->isPrimitiveValue()) > + return; Nit: We should move the pageSizeType assignment to after the if statement to avoid unnecessary work in the early-return case. > Source/WebCore/css/CSSStyleApplyProperty.cpp:773 > + // The value order is guaranteed. See CSSParser::parseSizeParameter. Does this mean we could replace the primitive type checks in pageSizeFromName() with assertions? > Source/WebCore/css/CSSStyleSelector.h:339 > Length mmLength(double mm) const; > Length inchLength(double inch) const; These declarations should be removed now as well.
Comment on attachment 116311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116311&action=review >> Source/WebCore/ChangeLog:8 >> + This refactoring moves the implementation of the page size calculation into CSSStyleApplyPropwerty > > Typo: CSSStyleApplyProperty. done. >> Source/WebCore/ChangeLog:11 >> + No new tests / refactoing only. > > Typo: refactoring. done. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:680 >> + static bool pageSizeFromName(CSSPrimitiveValue* pageSizeName, CSSPrimitiveValue* pageOrientation, Length& width, Length& height) > > I believe this function should be called getPageSizeFromName() since it doesn't return a "page size" directly, but rather fills in out arguments. done. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:692 >> + return false; > > The primitiveType() check here is redundant, since getIdent() would return 0 for non-CSS_IDENT values below and bail via the default case. Nice. I recall noticing this earlier but appear to forgotten to include that cleanup. Done. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:733 >> + return false; > > Ditto. done. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:756 >> + CSSValueListInspector inspector = value; > > We should use constructor syntax here: > CSSValueListInspector inspector(value); > > Also, it appears that this site is the only user of the CSSValueListInspector class. Are there plans to expand its usage? Otherwise I think we could use CSSValueList methods directly here and remove the class. Switched to constructor syntax. I think the rest of the discussion is beyond the scope of this refactoring though. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:762 >> + return; > > Nit: We should move the pageSizeType assignment to after the if statement to avoid unnecessary work in the early-return case. I've moved it much further down. >> Source/WebCore/css/CSSStyleApplyProperty.cpp:773 >> + // The value order is guaranteed. See CSSParser::parseSizeParameter. > > Does this mean we could replace the primitive type checks in pageSizeFromName() with assertions? Already removed. >> Source/WebCore/css/CSSStyleSelector.h:339 >> Length inchLength(double inch) const; > > These declarations should be removed now as well. done.
Created attachment 116693 [details] Patch
Comment on attachment 116693 [details] Patch r=me
Comment on attachment 116693 [details] Patch Rejecting attachment 116693 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: succeeded at 1312 with fuzz 2 (offset 108 lines). patching file Source/WebCore/css/CSSStyleSelector.cpp Hunk #1 succeeded at 3626 (offset -132 lines). Hunk #2 FAILED at 4021. Hunk #3 succeeded at 3949 (offset -122 lines). 1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/css/CSSStyleSelector.cpp.rej patching file Source/WebCore/css/CSSStyleSelector.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/10668887
Created attachment 116837 [details] Patch for landing
Comment on attachment 116837 [details] Patch for landing Clearing flags on attachment: 116837 Committed r101317: <http://trac.webkit.org/changeset/101317>
All reviewed patches have been landed. Closing bug.