Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220253002:
[Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap (Closed)
|
Created:
4 years, 4 months ago by anthonyhkf Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@CSSURLImageValue Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap
Those properties are: background-image, border-image-source, list-style-image, content, and shape-outside.
Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepted-types
BUG=545318
Committed: https://crrev.com/d76a6b0f5c29fe451128de078a82991954304393
Cr-Commit-Position: refs/heads/master@{#413403}
Patch Set 1 #Patch Set 2 : Add layout test #
Total comments: 15
Patch Set 3 : Add resource and remove keywords for list-style-image #Patch Set 4 : Add previous test #Patch Set 5 : Rebase #Patch Set 6 : Add tests for each image property and enable set for content #
Total comments: 1
Patch Set 7 : Fix tests #Patch Set 8 : Add comments #
Total comments: 7
Patch Set 9 : Use url & base64 images, set content to accept list of values #Patch Set 10 : Fixed typo #
Total comments: 8
Patch Set 11 : Renaming things #Patch Set 12 : Rebase #
Messages
Total messages: 44 (23 generated)
anthonyhkf@google.com changed reviewers: + ikilpatrick@chromium.org, meade@chromium.org
Looking pretty good, tests need a bit of work. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (left): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:10: }, "Can construct a new CSSURLImageValue object with url"); It's worth keeping this test as well, just put it above the new test you wrote (but make it use the 1x1 image). https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:15: c[c.length - 1] = 'css2.1/20110323/resources/1x1-green.png'; Let's make a new 1x1 image in our own directory so we don't have this weird cross-dependency on the css2.1 tests. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:21: var image1 = new Image(); Add a comment for why we are doing this. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:60: Remove extra newlines at end of file https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:234: list-style-image interpolable, inherited, custom_value, typedom_types=[Image], keywords=[disc|circle|square|decimal|decimal-leading-zero|lower-roman|upper-roman|lower-greek|lower-latin|upper-latin|armenian|georgian|lower-alpha|upper-alpha|none] Need to write a test for these keywords if you're adding them too :) Add a new file in the typedcssom/inlinestyle directory called list-style-image-keyword.html, and check that each of these keywords can be passed into the InlineStylePropertyMap, that they are returned from the InlineStylePropertyMap, and that the value of element.style.listStyleImage reflects the value put in. I did one of these partially for the transform property and rotations: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/typedcsso... You can use a structure mapping input to expected values, like in the test for CSSCalcLength: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/typedcsso.... In our case, we could do something like var values = ['disc', 'circle', 'square']; // test setting for (var i = 0; i < values.length; ++i) { test(function() { testElement.style = ''; testElement.styleMap.set('list-style-image', new CSSKeywordValue(values[i])); assert_equals(testElement.style.listStyleImage, values[i]); }, "Give this a real test name, " + values[i]); } // test getting for (var i = 0; i < values.length; ++i) { test(function() { testElement.style.listStyleImage = values[i]; var keyword = testElement.styleMap.get('list-style-image'); ... }, "Give this a real test name, " + values[i]); }
https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:19: document.getElementById("test-image1").styleMap.set('background-image', imageValue1); might be good to loop over all the possible property types here. cursor, content, etc. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:141: background-image interpolable, custom_all, typedom_types=[Image], keywords=[auto] I'm not really familiar with how this generated code works; but does this work with other custom types like: shape-outside, cursor, content, etc? (i think most of them are listed in this switch stmt). https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol...
https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:19: document.getElementById("test-image1").styleMap.set('background-image', imageValue1); On 2016/08/08 16:37:49, ikilpatrick wrote: > might be good to loop over all the possible property types here. > cursor, content, etc. Good idea! So far valid types are background-image, border-image-source and list-style-image https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:141: background-image interpolable, custom_all, typedom_types=[Image], keywords=[auto] On 2016/08/08 16:37:49, ikilpatrick wrote: > I'm not really familiar with how this generated code works; but does this work > with other custom types like: > > shape-outside, cursor, content, etc? > > (i think most of them are listed in this switch stmt). > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... In this CL, Anthony is just adding typedom_types for the types specified in the typed OM spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... The types in that switch statement are background-image - supported content - should be supported! cursor - I think we'll want a separate Cursor type since coordinates are compulsory when using URL for cursor list-style-image - supported border-image-source - supported ??? Not sure if we care -webkit-box-reflect -webkit-mask-box-image-source -webkit-mask-image shape-outside - could be supported! Summary: Anthony: Could you please try and see what happens if you add support to content and shape-outside? Take a look at the mozilla documenation to see what those should do: https://developer.mozilla.org/en/docs/Web/CSS/content https://developer.mozilla.org/en/docs/Web/CSS/shape-outside
I've added the 'shape-outside' and 'content'. But I need to change the StyleBuilderCustom.cpp to enable set for 'content' property, or else it will fail at the conversion from CSSValue to CSSValueList. Problem: when I want to set the 'content', if I try it locally, the image will be shown in the browser, but there will be a message indicating that it has been blocked by CORS policy because the origin is considered as 'file://'. I'm not sure about the origin in CQ dry run. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (left): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:10: }, "Can construct a new CSSURLImageValue object with url"); On 2016/08/08 06:49:01, Eddy wrote: > It's worth keeping this test as well, just put it above the new test you wrote > (but make it use the 1x1 image). Done. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:15: c[c.length - 1] = 'css2.1/20110323/resources/1x1-green.png'; On 2016/08/08 06:49:01, Eddy wrote: > Let's make a new 1x1 image in our own directory so we don't have this weird > cross-dependency on the css2.1 tests. Done. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:19: document.getElementById("test-image1").styleMap.set('background-image', imageValue1); On 2016/08/09 07:03:52, Eddy wrote: > On 2016/08/08 16:37:49, ikilpatrick wrote: > > might be good to loop over all the possible property types here. > > cursor, content, etc. > > Good idea! > > So far valid types are background-image, border-image-source and > list-style-image > > https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... Done. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:21: var image1 = new Image(); On 2016/08/08 06:49:01, Eddy wrote: > Add a comment for why we are doing this. Done. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:60: On 2016/08/08 06:49:01, Eddy wrote: > Remove extra newlines at end of file Done. https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSProperties.in (right): https://codereview.chromium.org/2220253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSProperties.in:141: background-image interpolable, custom_all, typedom_types=[Image], keywords=[auto] On 2016/08/09 07:03:52, Eddy wrote: > On 2016/08/08 16:37:49, ikilpatrick wrote: > > I'm not really familiar with how this generated code works; but does this work > > with other custom types like: > > > > shape-outside, cursor, content, etc? > > > > (i think most of them are listed in this switch stmt). > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > > In this CL, Anthony is just adding typedom_types for the types specified in the > typed OM spec: > https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... > > The types in that switch statement are > > background-image - supported > > content - should be supported! > > cursor - I think we'll want a separate Cursor type since coordinates are > compulsory when using URL for cursor > list-style-image - supported > > border-image-source - supported > > ??? Not sure if we care > -webkit-box-reflect > -webkit-mask-box-image-source > -webkit-mask-image > > shape-outside - could be supported! > > > Summary: > Anthony: Could you please try and see what happens if you add support to content > and shape-outside? Take a look at the mozilla documenation to see what those > should do: > https://developer.mozilla.org/en/docs/Web/CSS/content > https://developer.mozilla.org/en/docs/Web/CSS/shape-outside Done.
https://codereview.chromium.org/2220253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:48: image[i].src = url[i]; Oh, it's still wrong here :(
https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:12: return "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACoAAAAqCAYAAADFw8lbAAAIMUlEQVR42r3ZCVeTVxoHcD7LfAtn2LfqONae0zOnlh4piKBA2G2L0o5jtRoWWURkzYJQttrpoeOGoiC7ZRMSAmHfNwkh20tA7H+e+76ELQFeECfn/A8hC/nx3Hufm5s4OR3yojOuhjxrNSKxfAphGWM4naDFifgB+H7bD59v+nH8u36c/pcWoamjSC6dwfNWA5aW8Ben/9elQWVEgnwCvpd74RWnhWesEA+WGCHu63GL7oNrlBAXild0P67IJtGoNuGjAZvoj4ekDMOXquV9ScvHDhprD7VhXWyJpNvoZ9DNEbT0mo8OvGBckX6vnMQnbEgvCdkKtWE9HFR1NyiLc6Tw+1Xl9IdjO/rNOHOtD76X+vh5x+eQUFcH0L9FCPn8qhbdw9zhwNVtepyK7yGkBr5xvQTr24DasEcF/aukF26XelCvOuDcfdaqx/HoN/CJ6YFPrAY+BPXhodqPBmU5Fs2wRnHYVo0RJ+K64BPdTVAVQQkbp4E3X1Xtkc5ROyjFJVa9/zRYWFqRfh7fBa+IN/CKop/RKnjHqIXEUlXjqKpx2g9e9XtBj4X34tPvtXtDf5QNwzO8naCd8IrsgndUN7yjWdTwomngHaMhXJ9dNY8CemwLlOUH+ZRjbEUDh+B7A3C/8Bpekg6hqoT1iuwWKhvNqqrhK+tN2P2G3VHDt0GdRUDZ9dcaB302Um6ERLmMf8b/AfeLrfCUdPLhsVEqwqrhSZPdi1U1tpevrK2abKFFpA0ho2IM5dXjeFQziKJKDZILexB+6w9IbrYh/KdWSK53QPJTOyTSTkRJOxCZ3I0ISmSSSkhqDyJuqyg9iMnQ4IZieDu0vH4Z4UohoXkL8Aqug0doKzxoCnhSZT0juuDJY3sIq+GxHjG9OP5NH4+bmDWC4ziHmZ034EHVMOJTO/EtJT79DS7f6cKVuyok3O3BD/c0uJrTi2v5A7ghH8QN5TBuFY1CWjQGafE4+iaWN7HRRZtQFr9/q+B6rh7uoe3wCBewHjQFPCJV8IjqgQdhv/pRC/XgIo+xWDi0DXKQ11iQ+DsH6e/C9bYBCyzr4KGxRSQrtAIqtw/XCwZws3AEiYRJKZtCavk00ipmkPbLLNIfzCHj13mk/zqHypZFATq+sMYPebhiS2RmfBLaBJfgRrhRZd0lBJV0bWD9rvVifMbAAzqHLfR8C07ftuDT9ZxKEfIPSgTd1zXC/hkLZt8akf3bBGGmCTKHO/+ZQ+Zv88isfEtZwN1KHbL+q8M9locsi5R5AVpYb0WYYgeUEpAyCpfAerieb4Z7WDvcCOsmYb1VDdWQjkc+7+LwWeruSD7Jwu3VXRYeOzptItAcZWEdtIjsh3pkP2JZRM4TPXKrlpD31ID8KiPyn5kwOPsOTpfLOB7qKCdj2uES1AjX4Ga4hnVSZbtwl+akrZJikCwnk4XHdY8I2JoOPXIeE+gJgaoMlCVCGSB7bkRBtQkFL8yQV1ugeMlBWcOhWWuFU6jMgFD5Mp+d0JCstwRtgHNQE5xDmmmudmJy1sDPyZ3DvR+UJUJhhpmg84tmFDwzQE4wGcHkL81QvLDwKGXtMpSvrChkqVvB/fpVPO1chVNYvnED6iifJWh4qEtIC7UVNV9NtnAOivx7kplP+6CZr2rlayMKaznKMoGsKCJUEaG25v56SltWqKJ7IFku5pppE2ghbAsyy4Z4qPylPVAM8gRFWSsMf4PGguKG1W1xhLTF6SLD7IM9Ix2D8/kmlDyd4KGsBYlF7oQmVgoV7aDK7oa0g9Ztge4X76h2lDwZ56GsT9oBRSBZpDbokFl0NXnoBdkybNkNye7zT6O+V7o+9DU7gCKRJxI3h765zyKumhsVzTFhK3avfJc9LCymAXugGCRL55AAfdTBia5mOS00JwntQhdknKiE5BgwRfs62xYlCsdIG9ARMlK53p50JvzcYBVdzcesPV2hhh9SIBJKjyuqmuGryrZFtqBOikSeSjFDPba+4tUW0UiWlqE1OLHGygBiE67gMDAlvCDbFlk/dQjcgXzZLTxHM6xHae2iaGQhZXSeoJOLawjJ5w6ETaBRmFkQXphti2zH2Q3JhttWSfZGpqTOZIfcC1pQu+VkGkHzVCwyOF8ImzIDkwKAzTu247AVzdoPC7vOFg67jz1GNaiHtGT2QEPOqlmjfrcJrWha2QCIzXlKqJxDWcNmdR1lnBafrGoB/tJxFNdxB0KyTOneb3+XH5S/JArnKBdlwm5VSujHHaz1WPBzvQXXS3X4OnUWX0qnkPhAd2Dkwzar/ZmpomkZ5/Msu2L2S1De9gRSfz6b+hZ+STPwTxpBCbUjMUgbVPlqFdP6945PohLF0gcDz60nIMuAr27P4YvEKWQ9MuyNdFDN2p7V3c/2k7o1+OfqDwTbCbTlbKYefgSVZE1Tc1/ZF7gVWdayuv/HOlWd9EI55l1Be+H45Ao5m67DF7cm+XfpB0EW1ZqxYPyzQtTnT8U1Jjvsub2Suz2B2Sb4pcwhQTnPtyOxSMUrMyZ07w/2iR7DBuaaRON44HrYsH8pnUQxHRzFV9J0cKTt8rRjGecyFx2iduJsCcg2wy91HkkP9KKALBVNVvHDvdcCC89bouE0O4TtjD9VMzCNFlCjdf+hrrWitnvlaL94+IX6bEDmEgJyLLsiA9jcTKfz+lPTnkBl7QoqWy3UJ//8eN+OlDeYEZq9hK/vGbcjc2hu3jEgMoctoJVtQBsy/4UBz6mCU4edi4e5zOjXkEfn8ehCqnKGAWdpBzqTMovsKh2dUjnI6Jx+n+6vfM2hsc+K4dl3H4T7HySu84ZvNttQAAAAAElFTkSuQmCC"; Is it OK to use this image? If so I can just remove the resources folder that contains 1x1 pixel image
You also need to give your CL description a more descriptive title :) What does this CL do, other than the mechanical adding of typedom_types to CSSProperties.in? Which properties are you changing? https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:12: return "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACoAAAAqCAYAAADFw8lbAAAIMUlEQVR42r3ZCVeTVxoHcD7LfAtn2LfqONae0zOnlh4piKBA2G2L0o5jtRoWWURkzYJQttrpoeOGoiC7ZRMSAmHfNwkh20tA7H+e+76ELQFeECfn/A8hC/nx3Hufm5s4OR3yojOuhjxrNSKxfAphGWM4naDFifgB+H7bD59v+nH8u36c/pcWoamjSC6dwfNWA5aW8Ben/9elQWVEgnwCvpd74RWnhWesEA+WGCHu63GL7oNrlBAXild0P67IJtGoNuGjAZvoj4ekDMOXquV9ScvHDhprD7VhXWyJpNvoZ9DNEbT0mo8OvGBckX6vnMQnbEgvCdkKtWE9HFR1NyiLc6Tw+1Xl9IdjO/rNOHOtD76X+vh5x+eQUFcH0L9FCPn8qhbdw9zhwNVtepyK7yGkBr5xvQTr24DasEcF/aukF26XelCvOuDcfdaqx/HoN/CJ6YFPrAY+BPXhodqPBmU5Fs2wRnHYVo0RJ+K64BPdTVAVQQkbp4E3X1Xtkc5ROyjFJVa9/zRYWFqRfh7fBa+IN/CKop/RKnjHqIXEUlXjqKpx2g9e9XtBj4X34tPvtXtDf5QNwzO8naCd8IrsgndUN7yjWdTwomngHaMhXJ9dNY8CemwLlOUH+ZRjbEUDh+B7A3C/8Bpekg6hqoT1iuwWKhvNqqrhK+tN2P2G3VHDt0GdRUDZ9dcaB302Um6ERLmMf8b/AfeLrfCUdPLhsVEqwqrhSZPdi1U1tpevrK2abKFFpA0ho2IM5dXjeFQziKJKDZILexB+6w9IbrYh/KdWSK53QPJTOyTSTkRJOxCZ3I0ISmSSSkhqDyJuqyg9iMnQ4IZieDu0vH4Z4UohoXkL8Aqug0doKzxoCnhSZT0juuDJY3sIq+GxHjG9OP5NH4+bmDWC4ziHmZ034EHVMOJTO/EtJT79DS7f6cKVuyok3O3BD/c0uJrTi2v5A7ghH8QN5TBuFY1CWjQGafE4+iaWN7HRRZtQFr9/q+B6rh7uoe3wCBewHjQFPCJV8IjqgQdhv/pRC/XgIo+xWDi0DXKQ11iQ+DsH6e/C9bYBCyzr4KGxRSQrtAIqtw/XCwZws3AEiYRJKZtCavk00ipmkPbLLNIfzCHj13mk/zqHypZFATq+sMYPebhiS2RmfBLaBJfgRrhRZd0lBJV0bWD9rvVifMbAAzqHLfR8C07ftuDT9ZxKEfIPSgTd1zXC/hkLZt8akf3bBGGmCTKHO/+ZQ+Zv88isfEtZwN1KHbL+q8M9locsi5R5AVpYb0WYYgeUEpAyCpfAerieb4Z7WDvcCOsmYb1VDdWQjkc+7+LwWeruSD7Jwu3VXRYeOzptItAcZWEdtIjsh3pkP2JZRM4TPXKrlpD31ID8KiPyn5kwOPsOTpfLOB7qKCdj2uES1AjX4Ga4hnVSZbtwl+akrZJikCwnk4XHdY8I2JoOPXIeE+gJgaoMlCVCGSB7bkRBtQkFL8yQV1ugeMlBWcOhWWuFU6jMgFD5Mp+d0JCstwRtgHNQE5xDmmmudmJy1sDPyZ3DvR+UJUJhhpmg84tmFDwzQE4wGcHkL81QvLDwKGXtMpSvrChkqVvB/fpVPO1chVNYvnED6iifJWh4qEtIC7UVNV9NtnAOivx7kplP+6CZr2rlayMKaznKMoGsKCJUEaG25v56SltWqKJ7IFku5pppE2ghbAsyy4Z4qPylPVAM8gRFWSsMf4PGguKG1W1xhLTF6SLD7IM9Ix2D8/kmlDyd4KGsBYlF7oQmVgoV7aDK7oa0g9Ztge4X76h2lDwZ56GsT9oBRSBZpDbokFl0NXnoBdkybNkNye7zT6O+V7o+9DU7gCKRJxI3h765zyKumhsVzTFhK3avfJc9LCymAXugGCRL55AAfdTBia5mOS00JwntQhdknKiE5BgwRfs62xYlCsdIG9ARMlK53p50JvzcYBVdzcesPV2hhh9SIBJKjyuqmuGryrZFtqBOikSeSjFDPba+4tUW0UiWlqE1OLHGygBiE67gMDAlvCDbFlk/dQjcgXzZLTxHM6xHae2iaGQhZXSeoJOLawjJ5w6ETaBRmFkQXphti2zH2Q3JhttWSfZGpqTOZIfcC1pQu+VkGkHzVCwyOF8ImzIDkwKAzTu247AVzdoPC7vOFg67jz1GNaiHtGT2QEPOqlmjfrcJrWha2QCIzXlKqJxDWcNmdR1lnBafrGoB/tJxFNdxB0KyTOneb3+XH5S/JArnKBdlwm5VSujHHaz1WPBzvQXXS3X4OnUWX0qnkPhAd2Dkwzar/ZmpomkZ5/Msu2L2S1De9gRSfz6b+hZ+STPwTxpBCbUjMUgbVPlqFdP6945PohLF0gcDz60nIMuAr27P4YvEKWQ9MuyNdFDN2p7V3c/2k7o1+OfqDwTbCbTlbKYefgSVZE1Tc1/ZF7gVWdayuv/HOlWd9EI55l1Be+H45Ao5m67DF7cm+XfpB0EW1ZqxYPyzQtTnT8U1Jjvsub2Suz2B2Sb4pcwhQTnPtyOxSMUrMyZ07w/2iR7DBuaaRON44HrYsH8pnUQxHRzFV9J0cKTt8rRjGecyFx2iduJsCcg2wy91HkkP9KKALBVNVvHDvdcCC89bouE0O4TtjD9VMzCNFlCjdf+hrrWitnvlaL94+IX6bEDmEgJyLLsiA9jcTKfz+lPTnkBl7QoqWy3UJ//8eN+OlDeYEZq9hK/vGbcjc2hu3jEgMoctoJVtQBsy/4UBz6mCU4edi4e5zOjXkEfn8ehCqnKGAWdpBzqTMovsKh2dUjnI6Jx+n+6vfM2hsc+K4dl3H4T7HySu84ZvNttQAAAAAElFTkSuQmCC"; On 2016/08/10 07:02:13, anthonyhkf wrote: > Is it OK to use this image? If so I can just remove the resources folder that > contains 1x1 pixel image Both! You should test both absolute URLs and base-64 encoded images :) https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:688: } What's this for?
Description was changed from ========== [Typed-OM] Add Image typedom_types to properties Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... BUG=545318 ========== to ========== [Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap Those properties are: background-image, border-image-source, list-style-image, content, and shape-outside. Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... BUG=545318 ==========
https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:688: } On 2016/08/10 07:16:05, Eddy wrote: > What's this for? I cannot use "styleMap.set('content', ...)" if it isn't there. It will have runtime error when the toCSSValueList(value) is called because the 'value' is not a CSSValueList. And when I checked the class type of the 'value' variable, it was ImageClass, so I added those lines.
https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:688: } On 2016/08/10 07:26:09, anthonyhkf wrote: > On 2016/08/10 07:16:05, Eddy wrote: > > What's this for? > > I cannot use "styleMap.set('content', ...)" if it isn't there. It will have > runtime error when the toCSSValueList(value) is called because the 'value' is > not a CSSValueList. And when I checked the class type of the 'value' variable, > it was ImageClass, so I added those lines. Aside from the values 'none' and 'normal', content takes a list of values. So for now we can probably add supports_multiple in CSSProperties.in instead of having this code here.
https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:688: } On 2016/08/10 07:44:05, Timothy Loh wrote: > On 2016/08/10 07:26:09, anthonyhkf wrote: > > On 2016/08/10 07:16:05, Eddy wrote: > > > What's this for? > > > > I cannot use "styleMap.set('content', ...)" if it isn't there. It will have > > runtime error when the toCSSValueList(value) is called because the 'value' is > > not a CSSValueList. And when I checked the class type of the 'value' variable, > > it was ImageClass, so I added those lines. > > Aside from the values 'none' and 'normal', content takes a list of values. So > for now we can probably add supports_multiple in CSSProperties.in instead of > having this code here. Also, if a reviewer asks you to do something and you find out that it'll take extra code somewhere else, it's better to say to the reviewer "that requires extra changes that are unrelated to this CL, I'll do it in another CL", then have this discussion on the new CL. This avoids polluting the history of a simple CL with discussion about some random thing. The golden rule is "keep CLs small and focused". In this case, you were originally just adding things to CSSProperties.in and adding tests for them. Let's stick to just that.
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:12: return "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACoAAAAqCAYAAADFw8lbAAAIMUlEQVR42r3ZCVeTVxoHcD7LfAtn2LfqONae0zOnlh4piKBA2G2L0o5jtRoWWURkzYJQttrpoeOGoiC7ZRMSAmHfNwkh20tA7H+e+76ELQFeECfn/A8hC/nx3Hufm5s4OR3yojOuhjxrNSKxfAphGWM4naDFifgB+H7bD59v+nH8u36c/pcWoamjSC6dwfNWA5aW8Ben/9elQWVEgnwCvpd74RWnhWesEA+WGCHu63GL7oNrlBAXild0P67IJtGoNuGjAZvoj4ekDMOXquV9ScvHDhprD7VhXWyJpNvoZ9DNEbT0mo8OvGBckX6vnMQnbEgvCdkKtWE9HFR1NyiLc6Tw+1Xl9IdjO/rNOHOtD76X+vh5x+eQUFcH0L9FCPn8qhbdw9zhwNVtepyK7yGkBr5xvQTr24DasEcF/aukF26XelCvOuDcfdaqx/HoN/CJ6YFPrAY+BPXhodqPBmU5Fs2wRnHYVo0RJ+K64BPdTVAVQQkbp4E3X1Xtkc5ROyjFJVa9/zRYWFqRfh7fBa+IN/CKop/RKnjHqIXEUlXjqKpx2g9e9XtBj4X34tPvtXtDf5QNwzO8naCd8IrsgndUN7yjWdTwomngHaMhXJ9dNY8CemwLlOUH+ZRjbEUDh+B7A3C/8Bpekg6hqoT1iuwWKhvNqqrhK+tN2P2G3VHDt0GdRUDZ9dcaB302Um6ERLmMf8b/AfeLrfCUdPLhsVEqwqrhSZPdi1U1tpevrK2abKFFpA0ho2IM5dXjeFQziKJKDZILexB+6w9IbrYh/KdWSK53QPJTOyTSTkRJOxCZ3I0ISmSSSkhqDyJuqyg9iMnQ4IZieDu0vH4Z4UohoXkL8Aqug0doKzxoCnhSZT0juuDJY3sIq+GxHjG9OP5NH4+bmDWC4ziHmZ034EHVMOJTO/EtJT79DS7f6cKVuyok3O3BD/c0uJrTi2v5A7ghH8QN5TBuFY1CWjQGafE4+iaWN7HRRZtQFr9/q+B6rh7uoe3wCBewHjQFPCJV8IjqgQdhv/pRC/XgIo+xWDi0DXKQ11iQ+DsH6e/C9bYBCyzr4KGxRSQrtAIqtw/XCwZws3AEiYRJKZtCavk00ipmkPbLLNIfzCHj13mk/zqHypZFATq+sMYPebhiS2RmfBLaBJfgRrhRZd0lBJV0bWD9rvVifMbAAzqHLfR8C07ftuDT9ZxKEfIPSgTd1zXC/hkLZt8akf3bBGGmCTKHO/+ZQ+Zv88isfEtZwN1KHbL+q8M9locsi5R5AVpYb0WYYgeUEpAyCpfAerieb4Z7WDvcCOsmYb1VDdWQjkc+7+LwWeruSD7Jwu3VXRYeOzptItAcZWEdtIjsh3pkP2JZRM4TPXKrlpD31ID8KiPyn5kwOPsOTpfLOB7qKCdj2uES1AjX4Ga4hnVSZbtwl+akrZJikCwnk4XHdY8I2JoOPXIeE+gJgaoMlCVCGSB7bkRBtQkFL8yQV1ugeMlBWcOhWWuFU6jMgFD5Mp+d0JCstwRtgHNQE5xDmmmudmJy1sDPyZ3DvR+UJUJhhpmg84tmFDwzQE4wGcHkL81QvLDwKGXtMpSvrChkqVvB/fpVPO1chVNYvnED6iifJWh4qEtIC7UVNV9NtnAOivx7kplP+6CZr2rlayMKaznKMoGsKCJUEaG25v56SltWqKJ7IFku5pppE2ghbAsyy4Z4qPylPVAM8gRFWSsMf4PGguKG1W1xhLTF6SLD7IM9Ix2D8/kmlDyd4KGsBYlF7oQmVgoV7aDK7oa0g9Ztge4X76h2lDwZ56GsT9oBRSBZpDbokFl0NXnoBdkybNkNye7zT6O+V7o+9DU7gCKRJxI3h765zyKumhsVzTFhK3avfJc9LCymAXugGCRL55AAfdTBia5mOS00JwntQhdknKiE5BgwRfs62xYlCsdIG9ARMlK53p50JvzcYBVdzcesPV2hhh9SIBJKjyuqmuGryrZFtqBOikSeSjFDPba+4tUW0UiWlqE1OLHGygBiE67gMDAlvCDbFlk/dQjcgXzZLTxHM6xHae2iaGQhZXSeoJOLawjJ5w6ETaBRmFkQXphti2zH2Q3JhttWSfZGpqTOZIfcC1pQu+VkGkHzVCwyOF8ImzIDkwKAzTu247AVzdoPC7vOFg67jz1GNaiHtGT2QEPOqlmjfrcJrWha2QCIzXlKqJxDWcNmdR1lnBafrGoB/tJxFNdxB0KyTOneb3+XH5S/JArnKBdlwm5VSujHHaz1WPBzvQXXS3X4OnUWX0qnkPhAd2Dkwzar/ZmpomkZ5/Msu2L2S1De9gRSfz6b+hZ+STPwTxpBCbUjMUgbVPlqFdP6945PohLF0gcDz60nIMuAr27P4YvEKWQ9MuyNdFDN2p7V3c/2k7o1+OfqDwTbCbTlbKYefgSVZE1Tc1/ZF7gVWdayuv/HOlWd9EI55l1Be+H45Ao5m67DF7cm+XfpB0EW1ZqxYPyzQtTnT8U1Jjvsub2Suz2B2Sb4pcwhQTnPtyOxSMUrMyZ07w/2iR7DBuaaRON44HrYsH8pnUQxHRzFV9J0cKTt8rRjGecyFx2iduJsCcg2wy91HkkP9KKALBVNVvHDvdcCC89bouE0O4TtjD9VMzCNFlCjdf+hrrWitnvlaL94+IX6bEDmEgJyLLsiA9jcTKfz+lPTnkBl7QoqWy3UJ//8eN+OlDeYEZq9hK/vGbcjc2hu3jEgMoctoJVtQBsy/4UBz6mCU4edi4e5zOjXkEfn8ehCqnKGAWdpBzqTMovsKh2dUjnI6Jx+n+6vfM2hsc+K4dl3H4T7HySu84ZvNttQAAAAAElFTkSuQmCC"; On 2016/08/10 07:16:05, Eddy wrote: > On 2016/08/10 07:02:13, anthonyhkf wrote: > > Is it OK to use this image? If so I can just remove the resources folder that > > contains 1x1 pixel image > > Both! You should test both absolute URLs and base-64 encoded images :) Done.
https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:14: function urlImage() { This function name makes it sound like you're creating an image, but you're not. How about just "url()"? https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:20: function base64Image() { Likewise, how about base64Url() for this one? https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:25: var bg = new CSSURLImageValue(urlImage); Let's call this "image" instead of "bg", which is not descriptive for this test. https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:34: var div = document.getElementById("test-image1"); If you make the ID something without a dash in it, you can just refer to it by the ID. See testElement in https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/typedcsso...
https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:14: function urlImage() { On 2016/08/12 05:30:17, Eddy wrote: > This function name makes it sound like you're creating an image, but you're not. > How about just "url()"? Done. https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:20: function base64Image() { On 2016/08/12 05:30:17, Eddy wrote: > Likewise, how about base64Url() for this one? Done. https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:25: var bg = new CSSURLImageValue(urlImage); On 2016/08/12 05:30:17, Eddy wrote: > Let's call this "image" instead of "bg", which is not descriptive for this test. Done. https://codereview.chromium.org/2220253002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:34: var div = document.getElementById("test-image1"); On 2016/08/12 05:30:17, Eddy wrote: > If you make the ID something without a dash in it, you can just refer to it by > the ID. See testElement in > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/typedcsso... Done.
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
meade@chromium.org changed reviewers: + timloh@chromium.org
+Tim for OWNERS approval
lgtm
lgtm
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyhkf@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, ikilpatrick@chromium.org, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2220253002/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap Those properties are: background-image, border-image-source, list-style-image, content, and shape-outside. Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... BUG=545318 ========== to ========== [Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap Those properties are: background-image, border-image-source, list-style-image, content, and shape-outside. Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... BUG=545318 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap Those properties are: background-image, border-image-source, list-style-image, content, and shape-outside. Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... BUG=545318 ========== to ========== [Typed-OM] Add Image typedom_types to properties and enable them to be set with StyleMap Those properties are: background-image, border-image-source, list-style-image, content, and shape-outside. Spec: https://drafts.css-houdini.org/css-typed-om/#mapping-of-properties-to-accepte... BUG=545318 Committed: https://crrev.com/d76a6b0f5c29fe451128de078a82991954304393 Cr-Commit-Position: refs/heads/master@{#413403} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d76a6b0f5c29fe451128de078a82991954304393 Cr-Commit-Position: refs/heads/master@{#413403} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
