Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222863002:
[Typed-OM] Enable getting CSSURLImageValue from stylemap (Closed)
|
Created:
4 years, 4 months ago by anthonyhkf Modified:
4 years, 3 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@CSSProperties_Image Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed-OM] Enable getting CSSURLImageValue from stylemap
BUG=545318
Committed: https://crrev.com/6fcceab2ad6b25e6015097ab6033ec4977f06387
Cr-Commit-Position: refs/heads/master@{#415567}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add more properties to test #Patch Set 3 : Rebase and add tests for each property #
Total comments: 5
Patch Set 4 : Remove backgroundImage test from unsupported-properties, simplify test to get from StyleMap #
Total comments: 2
Patch Set 5 : Change tests for unsupported properties #
Total comments: 2
Patch Set 6 : Rebase #Patch Set 7 : Remove additional newline #Patch Set 8 : Changed quotes for consistency #Patch Set 9 : Rebase #
Total comments: 2
Messages
Total messages: 40 (25 generated)
anthonyhkf@google.com changed reviewers: + ikilpatrick@chromium.org, meade@chromium.org
On 2016/08/08 05:08:30, anthonyhkf wrote: I added the create function that accepts a const CSSImageValue*. It will be called in the StyleValueFactory when getting the image value. If I use the create function that accepts URL, the state will be "unloaded".
The CQ bit was checked by meade@chromium.org 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Looks like you have some merge conflicts making all the bots red :( Try git rebase-update? https://codereview.chromium.org/2222863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2222863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:10: function isSame(obj1, obj2) { As discussed, it's better to just do the assert_equals again instead of having this function.
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...
https://codereview.chromium.org/2222863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2222863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:10: function isSame(obj1, obj2) { On 2016/08/08 06:57:13, Eddy wrote: > As discussed, it's better to just do the assert_equals again instead of having > this function. Done.
https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (left): https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:73: // add an Image object to know if the image has been loaded Might as well keep this comment, even if it is the same as the other. https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:36: var imageProperties = ["background-image", "border-image-source", "list-style-image", "content", "shape-outside"]; Probably from a dependent CL, but put this right at the top so that it's easy to find (not sure which dependent CL to put this comment in...). https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:130: div4.styleMap.set(imageProperties[i], imageValue4); For this test, it'd be clearer if you set the properties using the old OM: div4.style[<property>] = 'url(' + base64Image()' + ')';
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The tests for getting backgroundImage from unsupported-properties are removed because now we can get CSSURLImageValue from StyleMap. https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:36: var imageProperties = ["background-image", "border-image-source", "list-style-image", "content", "shape-outside"]; On 2016/08/11 03:57:26, Eddy wrote: > Probably from a dependent CL, but put this right at the top so that it's easy to > find (not sure which dependent CL to put this comment in...). Done. https://codereview.chromium.org/2222863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:130: div4.styleMap.set(imageProperties[i], imageValue4); On 2016/08/11 03:57:26, Eddy wrote: > For this test, it'd be clearer if you set the properties using the old OM: > > div4.style[<property>] = 'url(' + base64Image()' + ')'; 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.
Almost there. Need to get my CL landed first (sorry D:) https://codereview.chromium.org/2222863002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html (right): https://codereview.chromium.org/2222863002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/cssURLImageValue.html:127: div4.style[imageProperties[i]] = 'url(' + url4 + ')'; For JS style we use curly braces ({}) everywhere, including one-line for loops. https://codereview.chromium.org/2222863002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/unsupported-properties.html (left): https://codereview.chromium.org/2222863002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/unsupported-properties.html:30: }, 'Setting the same property using the result of getting an unknown value works'); Uhhh don't delete these tests, just change it to something else that's not supported.
lgtm https://codereview.chromium.org/2222863002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/unsupported-properties.html (right): https://codereview.chromium.org/2222863002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/unsupported-properties.html:15: testElement.style.backgroundColor = "green"; nit: single quote for consistently.
https://codereview.chromium.org/2222863002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/unsupported-properties.html (right): https://codereview.chromium.org/2222863002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/unsupported-properties.html:15: testElement.style.backgroundColor = "green"; On 2016/08/23 00:50:52, ikilpatrick wrote: > nit: single quote for consistently. 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.
meade@chromium.org changed reviewers: + esprehn@chromium.org
lgtm +Elliott for OWNERS
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 https://codereview.chromium.org/2222863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp (right): https://codereview.chromium.org/2222863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:46: return CSSURLImageValue::create(imageValue.valueWithURLMadeAbsolute()); Note this causes two allocations for every image value because valueWithURLMadeAbsolute always allocates a new CSSImageValue. That's probably fine for now, but we might want to reconsider someday.
Thanks everyone for the reviews :) https://codereview.chromium.org/2222863002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp (right): https://codereview.chromium.org/2222863002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:46: return CSSURLImageValue::create(imageValue.valueWithURLMadeAbsolute()); On 2016/08/30 21:39:19, esprehn wrote: > Note this causes two allocations for every image value because > valueWithURLMadeAbsolute always allocates a new CSSImageValue. That's probably > fine for now, but we might want to reconsider someday. Acknowledged.
The CQ bit was checked by anthonyhkf@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ikilpatrick@chromium.org, meade@chromium.org Link to the patchset: https://codereview.chromium.org/2222863002/#ps160001 (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.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Enable getting CSSURLImageValue from stylemap BUG=545318 ========== to ========== [Typed-OM] Enable getting CSSURLImageValue from stylemap BUG=545318 Committed: https://crrev.com/6fcceab2ad6b25e6015097ab6033ec4977f06387 Cr-Commit-Position: refs/heads/master@{#415567} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6fcceab2ad6b25e6015097ab6033ec4977f06387 Cr-Commit-Position: refs/heads/master@{#415567} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
