Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(124)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 22839023: Add support for the object-position CSS property. (Closed)

Created:
7 years, 3 months ago by mstensho (USE GERRIT)
Modified:
7 years, 3 months ago
Reviewers:
pdr., ojan
CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), jeez, eae+blinkwatch, dglazkov+blink, leviw+renderwatch, dstockwell, Timothy Loh, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, Steve Block, dino_apple.com, Eric Willigers
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Add support for the object-position CSS property. This is hidden behind an "experimental" runtime flag named "ObjectFitPosition", together with object-fit. This is an implementation of object-position as described in http://www.w3.org/TR/2012/CR-css3-images-20120417/#object-position Object-position is used to offset replaced content within its content box. Painting is always clipped against the content box, regardless of the "overflow" property. This property is useful together with object-fit (to achieve a difference between content box size and replaced content size, so that specifying alignment is interesting), but can also be used on its own. BUG=236333 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157745

Patch Set 1 #

Total comments: 11

Patch Set 2 : Address code review issues #

Patch Set 3 : Rebase master (the buildbots couldn't apply the patch, although I had no problems locally) #

Patch Set 4 : Rebase master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+838 lines, -32 lines) Patch
M LayoutTests/animations/resources/animation-test-helpers.js View 1 chunk +13 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position.html View 1 chunk +49 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-expected.html View 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-svg.html View 1 1 chunk +50 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-svg-expected.html View 1 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-with-fit-contain.html View 1 chunk +50 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-with-fit-contain-expected.html View 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-with-fit-cover.html View 1 chunk +50 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-with-fit-cover-expected.html View 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-with-fit-none.html View 1 chunk +50 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/object-position-with-fit-none-expected.html View 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/parsing-object-position.html View 1 chunk +80 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/parsing-object-position-expected.txt View 1 chunk +52 lines, -0 lines 0 comments Download
A + LayoutTests/fast/css/resources/circle-small.svg View 1 1 chunk +2 lines, -2 lines 0 comments Download
A LayoutTests/transitions/object-position-transition.html View 1 chunk +34 lines, -0 lines 0 comments Download
A LayoutTests/transitions/object-position-transition-expected.txt View 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/webexposed/css-properties-as-js-properties-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 3 chunks +9 lines, -1 line 0 comments Download
M Source/core/css/CSSParser.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSParser-in.cpp View 1 2 3 3 chunks +18 lines, -2 lines 0 comments Download
M Source/core/css/CSSProperties.in View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSProperty.cpp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSPropertyNames.in View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/Pair.h View 1 chunk +29 lines, -12 lines 0 comments Download
M Source/core/css/resolver/StyleBuilderCustom.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/resolver/StyleBuilderCustom.cpp View 1 2 3 2 chunks +10 lines, -0 lines 0 comments Download
M Source/core/page/RuntimeCSSEnabled.cpp View 1 2 3 2 chunks +5 lines, -1 line 0 comments Download
M Source/core/page/UseCounter.cpp View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/page/animation/CSSPropertyAnimation.cpp View 1 2 3 2 chunks +7 lines, -0 lines 0 comments Download
A + Source/core/platform/LengthPoint.h View 1 2 3 1 chunk +36 lines, -8 lines 0 comments Download
M Source/core/rendering/RenderImage.cpp View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderReplaced.cpp View 1 2 3 2 chunks +5 lines, -5 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.h View 1 2 3 3 chunks +3 lines, -0 lines 0 comments Download
M Source/core/rendering/style/RenderStyle.cpp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/style/StyleRareNonInheritedData.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/rendering/style/StyleRareNonInheritedData.cpp View 1 2 3 3 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
mstensho (USE GERRIT)
Note about this implementation: My initial idea was to represent object-position as a shorthand internally, ...
7 years, 3 months ago (2013-08-26 16:56:47 UTC) #1
pdr.
I think this looks quite nice. https://codereview.chromium.org/22839023/diff/1/LayoutTests/fast/css/object-position-expected.html File LayoutTests/fast/css/object-position-expected.html (right): https://codereview.chromium.org/22839023/diff/1/LayoutTests/fast/css/object-position-expected.html#newcode1 LayoutTests/fast/css/object-position-expected.html:1: <!DOCTYPE html> These ...
7 years, 3 months ago (2013-08-27 20:07:18 UTC) #2
mstensho (USE GERRIT)
Only a couple of comments for discussion for now. Will follow up with actual code ...
7 years, 3 months ago (2013-08-27 20:23:10 UTC) #3
mstensho (USE GERRIT)
I've addressed all code review issues except the one in Pair. Not sure what to ...
7 years, 3 months ago (2013-08-29 11:16:12 UTC) #4
mstensho (USE GERRIT)
https://codereview.chromium.org/22839023/diff/1/Source/core/css/Pair.h File Source/core/css/Pair.h (right): https://codereview.chromium.org/22839023/diff/1/Source/core/css/Pair.h#newcode83 Source/core/css/Pair.h:83: , m_identicalValuesPolicy(DropIdenticalValues) { } If the memory usage increase ...
7 years, 3 months ago (2013-08-29 13:09:30 UTC) #5
pdr.
On 2013/08/29 13:09:30, Morten Stenshorne wrote: > https://codereview.chromium.org/22839023/diff/1/Source/core/css/Pair.h > File Source/core/css/Pair.h (right): > > https://codereview.chromium.org/22839023/diff/1/Source/core/css/Pair.h#newcode83 ...
7 years, 3 months ago (2013-08-29 22:41:46 UTC) #6
ojan
lgtm
7 years, 3 months ago (2013-09-12 21:33:34 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/22839023/13001
7 years, 3 months ago (2013-09-12 21:33:46 UTC) #8
commit-bot: I haz the power
Failed to apply patch for Source/core/page/RuntimeCSSEnabled.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 3 months ago (2013-09-12 21:33:59 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/22839023/24001
7 years, 3 months ago (2013-09-13 07:59:20 UTC) #10
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 3 months ago (2013-09-13 09:10:55 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/22839023/24001
7 years, 3 months ago (2013-09-13 09:25:52 UTC) #12
commit-bot: I haz the power
Change committed as 157745
7 years, 3 months ago (2013-09-13 10:27:19 UTC) #13
mstensho (USE GERRIT)
7 years, 3 months ago (2013-09-20 15:41:21 UTC) #14
Message was sent while issue was closed. 
On 2013/09/13 10:27:19, I haz the power (commit-bot) wrote:
> Change committed as 157745

This caused assertion failures all over the place, and was reverted by ajuma in
https://codereview.chromium.org/23465021/

I'll follow up with a small additional change to fix this in
https://codereview.chromium.org/24077007/

Powered by Google App Engine
This is Rietveld 408576698