Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795173002:
Do not show image placeholders for CSS sprites (Closed)
DescriptionDo not show image placeholders for CSS sprites
This CL recognizes if CSS background image is used for CSS sprites, and
shows the full image instead of image placeholders.
sprite
BUG=678792
Review-Url: https://codereview.chromium.org/2795173002
Cr-Commit-Position: refs/heads/master@{#482122}
Committed: https://chromium.googlesource.com/chromium/src/+/830854ccc657258329381910e6b1fa679a058ffe
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added layout test #Patch Set 3 : Added layout test expected result #
Total comments: 10
Patch Set 4 : Addressed comments #
Total comments: 8
Patch Set 5 : Addressed kouhei comments #
Total comments: 8
Patch Set 6 : rebased #Patch Set 7 : Addressed comments #
Total comments: 4
Patch Set 8 : rebased and addressed comments #
Total comments: 10
Patch Set 9 : Addressed comments #
Total comments: 3
Patch Set 10 : fixed nits #Patch Set 11 : tryjob #Patch Set 12 : remove a test #Messages
Total messages: 42 (18 generated)
rajendrant@chromium.org changed reviewers: + suzyh@chromium.org
ptal https://codereview.chromium.org/2795173002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:122: // Simple hueristic to guess if CSS background image is used to create CSS This heuristic is similar to: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Sma...
It's not clear to me why we want to do this. Is there a relevant blink-dev thread or something that you could link to from the CL description? Also, please add tests.
Patchset #2 (id:20001) has been deleted
rajendrant@chromium.org changed reviewers: + sclittle@chromium.org
Adding sclittle@ as well. Following thread summarizes the image replacement feature to for saving data. https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/pOTcSsMsAuo/YF... Since CSS sprites are often used for interactive icons and such, this CL shows the original image, instead of placeholder image for sprites.
On 2017/04/10 20:42:46, Raj wrote: > Adding sclittle@ as well. > > Following thread summarizes the image replacement feature to for saving data. > https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/pOTcSsMsAuo/YF... > > Since CSS sprites are often used for interactive icons and such, this CL shows > the original image, instead of placeholder image for sprites. suzyh: I have added layout tests. Any suggestions to add unittests. It was tough since there is no direct unittest file for the files changed in this CL.
kouhei@chromium.org changed reviewers: + hiroshige@chromium.org, kouhei@chromium.org
On 2017/04/10 at 20:46:00, rajendrant wrote: > On 2017/04/10 20:42:46, Raj wrote: > > Adding sclittle@ as well. > > > > Following thread summarizes the image replacement feature to for saving data. > > https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/pOTcSsMsAuo/YF... > > > > Since CSS sprites are often used for interactive icons and such, this CL shows > > the original image, instead of placeholder image for sprites. > > suzyh: > I have added layout tests. > Any suggestions to add unittests. It was tough since there is no direct unittest file for the files changed in this CL. Layout tests are fine. This seems fine to me. +kouhei,hiroshige for a loading perspective.
https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSImageSetValue.h (right): https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSImageSetValue.h:49: bool allowImagePlaceholder = false); Can we take ImagePlaceHolderRequestType? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/loade... https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:121: static bool isSprite(const ComputedStyle& style) { I'd prefer to have a more descriptive name. ComputedStyleMayBeCSSSpriteBackgroundImage? https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127: (!style.logicalHeight().isAuto() || !style.logicalWidth().isAuto()); Would you provide more info about this heuristic? Should we also check background-clip, background-position? Also, checking on style here would mean we will disallow imagePlaceHolders on non-bg css images (see L189) like WebkitBoxReflect if CSS sprite bg image is maybe set. Is this intended? https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:133: document.settings()->getFetchImagePlaceholders() && !isSprite(style); It looks like we are introducing another check so that we check document.settings() twice. Would you explain why?
Patchset #4 (id:80001) has been deleted
kouhei: ptal https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSImageSetValue.h (right): https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSImageSetValue.h:49: bool allowImagePlaceholder = false); On 2017/04/11 07:45:59, kouhei wrote: > Can we take ImagePlaceHolderRequestType? > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/loade... Done. https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:121: static bool isSprite(const ComputedStyle& style) { On 2017/04/11 07:45:59, kouhei wrote: > I'd prefer to have a more descriptive name. > ComputedStyleMayBeCSSSpriteBackgroundImage? Done. https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127: (!style.logicalHeight().isAuto() || !style.logicalWidth().isAuto()); On 2017/04/11 07:45:59, kouhei wrote: > Would you provide more info about this heuristic? > Should we also check background-clip, background-position? > Also, checking on style here would mean we will disallow imagePlaceHolders on > non-bg css images (see L189) like WebkitBoxReflect if CSS sprite bg image is > maybe set. Is this intended? The heuristic is to catch only the following case: background image is set, height or width is specified. It captures all the examples from https://www.w3schools.com/css/css_image_sprites.asp I have added check to enable imagePlaceholder for WebkitBoxReflect case https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:133: document.settings()->getFetchImagePlaceholders() && !isSprite(style); On 2017/04/11 07:45:59, kouhei wrote: > It looks like we are introducing another check so that we check > document.settings() twice. Would you explain why? Check not needed here. Removed.
https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127: (!style.logicalHeight().isAuto() || !style.logicalWidth().isAuto()); On 2017/04/20 19:01:17, Raj wrote: > On 2017/04/11 07:45:59, kouhei wrote: > > Would you provide more info about this heuristic? > > Should we also check background-clip, background-position? > > Also, checking on style here would mean we will disallow imagePlaceHolders on > > non-bg css images (see L189) like WebkitBoxReflect if CSS sprite bg image is > > maybe set. Is this intended? > > The heuristic is to catch only the following case: > background image is set, height or width is specified. > It captures all the examples from > https://www.w3schools.com/css/css_image_sprites.asp > > I have added check to enable imagePlaceholder for WebkitBoxReflect case Would you add test case for this? https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:130: GetPlaceholderImageRequestType(const ComputedStyle& style) { I think we should simply inline the logic here. I found the name "GetPlaceholderImageRequestType" too generic and hard to guess what the function is doing without reading it. https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:170: GetPlaceholderImageRequestType(*style); IIUC, this decides place_holder_image_request_type_ to be applied on all image-holding CSS value on the "style", which will disable place_holder on non-bg image if the style has css bg sprite. I think what we want to achieve here is disable place_holder on bg image only. https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:199: placeholder_image_request_type)); Can we fill in place_holder_image_request_type here only https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:213: placeholder_image_request_type)); and use FetchParameters::kAllowPlaceholder here and below?
suzyh@chromium.org changed reviewers: - suzyh@chromium.org
kouhei: ptal https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127: (!style.logicalHeight().isAuto() || !style.logicalWidth().isAuto()); On 2017/04/24 00:02:19, kouhei wrote: > On 2017/04/20 19:01:17, Raj wrote: > > On 2017/04/11 07:45:59, kouhei wrote: > > > Would you provide more info about this heuristic? > > > Should we also check background-clip, background-position? > > > Also, checking on style here would mean we will disallow imagePlaceHolders > on > > > non-bg css images (see L189) like WebkitBoxReflect if CSS sprite bg image is > > > maybe set. Is this intended? > > > > The heuristic is to catch only the following case: > > background image is set, height or width is specified. > > It captures all the examples from > > https://www.w3schools.com/css/css_image_sprites.asp > > > > I have added check to enable imagePlaceholder for WebkitBoxReflect case > > Would you add test case for this? Added test. https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:130: GetPlaceholderImageRequestType(const ComputedStyle& style) { On 2017/04/24 00:02:19, kouhei wrote: > I think we should simply inline the logic here. I found the name > "GetPlaceholderImageRequestType" too generic and hard to guess what the function > is doing without reading it. Done. https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:170: GetPlaceholderImageRequestType(*style); On 2017/04/24 00:02:19, kouhei wrote: > IIUC, this decides place_holder_image_request_type_ to be applied on all > image-holding CSS value on the "style", which will disable place_holder on > non-bg image if the style has css bg sprite. > > I think what we want to achieve here is disable place_holder on bg image only. Done. https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:199: placeholder_image_request_type)); On 2017/04/24 00:02:19, kouhei wrote: > Can we fill in place_holder_image_request_type here only Done. https://codereview.chromium.org/2795173002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:213: placeholder_image_request_type)); On 2017/04/24 00:02:19, kouhei wrote: > and use FetchParameters::kAllowPlaceholder here and below? Done.
https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/previews/client-lofi.html (right): https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/previews/client-lofi.html:1: <!DOCTYPE html> Please modify this test so that it will fit in expected.png w/o scrollbars. https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/previews/client-lofi.html:10: //window.internals.settings.setFetchImagePlaceholders(true); this needs to be un-comment out? https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageSetValue.h (right): https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageSetValue.h:52: FetchParameters::kDisallowPlaceholder); Option 1. [Preferred] Can we make this non-optional param? Option 2. Should the default be kAllowPlaceholder now that most of the callers are specifying kAllowPlaceholder? https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageValue.h (right): https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageValue.h:69: FetchParameters::kDisallowPlaceholder); same here.
kouhei: ptal https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/previews/client-lofi.html (right): https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/previews/client-lofi.html:1: <!DOCTYPE html> On 2017/04/28 03:50:50, kouhei wrote: > Please modify this test so that it will fit in expected.png w/o scrollbars. Done. https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/previews/client-lofi.html:10: //window.internals.settings.setFetchImagePlaceholders(true); On 2017/04/28 03:50:50, kouhei wrote: > this needs to be un-comment out? Yes. That was a mistake. https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageSetValue.h (right): https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageSetValue.h:52: FetchParameters::kDisallowPlaceholder); On 2017/04/28 03:50:50, kouhei wrote: > Option 1. [Preferred] Can we make this non-optional param? > Option 2. Should the default be kAllowPlaceholder now that most of the callers > are specifying kAllowPlaceholder? Made it non-optional. https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageValue.h (right): https://codereview.chromium.org/2795173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageValue.h:69: FetchParameters::kDisallowPlaceholder); On 2017/04/28 03:50:50, kouhei wrote: > same here. Done.
https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:126: (!style.LogicalHeight().IsAuto() || !style.LogicalWidth().IsAuto()); These LogicalWidth/Height().IsAuto() checks seem odd - have you tested these rules on a lot of sites to see how well these rules target all sprites and only sprites?
https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:126: (!style.LogicalHeight().IsAuto() || !style.LogicalWidth().IsAuto()); On 2017/04/28 23:55:54, sclittle wrote: > These LogicalWidth/Height().IsAuto() checks seem odd - have you tested these > rules on a lot of sites to see how well these rules target all sprites and only > sprites? I tested all examples from below link and some other demo sites. https://www.w3schools.com/css/css_image_sprites.asp I will test with few more. This heuristic is similar to: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Sma... The problem with similar check from below CL is that xPosition().isZero() will fail for the sprite image with background_x=0 https://codereview.chromium.org/2710113002/diff/1/third_party/WebKit/Source/c... For example, first image will not be detected as sprite. https://www.w3schools.com/css/tryit.asp?filename=trycss_sprites_img background.xPosition().isFixed() || background.yPosition().isFixed() check may be sufficient enough too.
kouhei@chromium.org changed required reviewers: + sclittle@chromium.org
Thanks for addressing the comments! lgtm from myside re: core/ changes. Please wait for sclittle@ lgtm
https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:126: (!style.LogicalHeight().IsAuto() || !style.LogicalWidth().IsAuto()); On 2017/04/30 18:35:28, Raj wrote: > On 2017/04/28 23:55:54, sclittle wrote: > > These LogicalWidth/Height().IsAuto() checks seem odd - have you tested these > > rules on a lot of sites to see how well these rules target all sprites and > only > > sprites? > > I tested all examples from below link and some other demo sites. > https://www.w3schools.com/css/css_image_sprites.asp > I will test with few more. > > This heuristic is similar to: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Sma... > > The problem with similar check from below CL is that xPosition().isZero() will > fail for the sprite image with background_x=0 > https://codereview.chromium.org/2710113002/diff/1/third_party/WebKit/Source/c... > For example, first image will not be detected as sprite. > https://www.w3schools.com/css/tryit.asp?filename=trycss_sprites_img > > background.xPosition().isFixed() || background.yPosition().isFixed() check may > be sufficient enough too. Any updates from testing these rules out on a few more sprites? I'm just worried that width and height could be set on a lot of objects that aren't sprites.
rajendrant@google.com changed reviewers: + rajendrant@google.com
sclittle: ptal client-lofi-expected.png is the updated placeholder image with icons. https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:126: (!style.LogicalHeight().IsAuto() || !style.LogicalWidth().IsAuto()); On 2017/05/08 20:34:44, sclittle wrote: > On 2017/04/30 18:35:28, Raj wrote: > > On 2017/04/28 23:55:54, sclittle wrote: > > > These LogicalWidth/Height().IsAuto() checks seem odd - have you tested these > > > rules on a lot of sites to see how well these rules target all sprites and > > only > > > sprites? > > > > I tested all examples from below link and some other demo sites. > > https://www.w3schools.com/css/css_image_sprites.asp > > I will test with few more. > > > > This heuristic is similar to: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Sma... > > > > The problem with similar check from below CL is that xPosition().isZero() will > > fail for the sprite image with background_x=0 > > > https://codereview.chromium.org/2710113002/diff/1/third_party/WebKit/Source/c... > > For example, first image will not be detected as sprite. > > https://www.w3schools.com/css/tryit.asp?filename=trycss_sprites_img > > > > background.xPosition().isFixed() || background.yPosition().isFixed() check may > > be sufficient enough too. > > Any updates from testing these rules out on a few more sprites? I'm just worried > that width and height could be set on a lot of objects that aren't sprites. As you suggested, I changed the check to (background.xPosition().isFixed() || background.yPosition().isFixed()), since that is more meaningful for sprite images.
https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:128: document.GetSettings()->GetFetchImagePlaceholders() && Could you remove the Settings checks here? That logic (plus logic to allow placeholders when Client LoFi is on) is handled by document.GetFrame()->MaybeAllowImagePlaceholder(params). https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageValue.cpp:79: document.GetSettings()->GetFetchImagePlaceholders() && Could you remove the Settings checks here? That logic (plus logic to allow placeholders when Client LoFi is on) is handled by document.GetFrame()->MaybeAllowImagePlaceholder(params). https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:123: // width will be set to auto. For CSS sprite image, height or width will nit: Could you update this comment to reflect the fact that Position.IsFixed() is being checked instead? https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:207: FetchParameters::kAllowPlaceholder)); Could you change this and all the below CSSProperty types to use kDisallowPlaceholder? The current policy is that the only CSS images that we want to replace are CSS background images for now. Looking at the code around here more closely, it looks like other images like cursor or border images could have been replaced with placeholders even before this CL. I realize that your CL just maintains this behavior, and this bug was already there in the old code, but do you mind fixing it? Thanks! https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:195: frame_->GetSettings()->GetFetchImagePlaceholders()) { Could you remove the Settings checks here? They're already handled by frame_->MaybeAllowImagePlaceholder.
sclittle: ptal https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:128: document.GetSettings()->GetFetchImagePlaceholders() && On 2017/06/13 00:28:27, sclittle wrote: > Could you remove the Settings checks here? That logic (plus logic to allow > placeholders when Client LoFi is on) is handled by > document.GetFrame()->MaybeAllowImagePlaceholder(params). Done. https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageValue.cpp:79: document.GetSettings()->GetFetchImagePlaceholders() && On 2017/06/13 00:28:27, sclittle wrote: > Could you remove the Settings checks here? That logic (plus logic to allow > placeholders when Client LoFi is on) is handled by > document.GetFrame()->MaybeAllowImagePlaceholder(params). Done. https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:123: // width will be set to auto. For CSS sprite image, height or width will On 2017/06/13 00:28:27, sclittle wrote: > nit: Could you update this comment to reflect the fact that Position.IsFixed() > is being checked instead? Done. https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:207: FetchParameters::kAllowPlaceholder)); On 2017/06/13 00:28:27, sclittle wrote: > Could you change this and all the below CSSProperty types to use > kDisallowPlaceholder? The current policy is that the only CSS images that we > want to replace are CSS background images for now. > > Looking at the code around here more closely, it looks like other images like > cursor or border images could have been replaced with placeholders even before > this CL. I realize that your CL just maintains this behavior, and this bug was > already there in the old code, but do you mind fixing it? > > Thanks! Done. https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2795173002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:195: frame_->GetSettings()->GetFetchImagePlaceholders()) { On 2017/06/13 00:28:27, sclittle wrote: > Could you remove the Settings checks here? They're already handled by > frame_->MaybeAllowImagePlaceholder. Done.
LGTM % nits https://codereview.chromium.org/2795173002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right): https://codereview.chromium.org/2795173002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:33: #include "core/frame/Settings.h" nit: Remove unused include https://codereview.chromium.org/2795173002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right): https://codereview.chromium.org/2795173002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSImageValue.cpp:26: #include "core/frame/Settings.h" nit: remove unused include https://codereview.chromium.org/2795173002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2795173002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:221: FetchParameters::kDisallowPlaceholder)); nit: could you add a comment stating that cursor images shouldn't be replaced with placeholders? Same with the other cases where placeholders aren't allowed below.
The CQ bit was checked by rajendrant@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 checked by rajendrant@google.com
The CQ bit was unchecked by rajendrant@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2795173002/#ps220001 (title: "fixed nits")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rajendrant@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2795173002/#ps260001 (title: "remove a test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498263325526410,
"parent_rev": "6af669e60ec74609162210d06b017e93e3b5fc39", "commit_rev":
"830854ccc657258329381910e6b1fa679a058ffe"}
Message was sent while issue was closed.
Description was changed from ========== Do not show image placeholders for CSS sprites This CL recognizes if CSS background image is used for CSS sprites, and shows the full image instead of image placeholders. sprite BUG=678792 ========== to ========== Do not show image placeholders for CSS sprites This CL recognizes if CSS background image is used for CSS sprites, and shows the full image instead of image placeholders. sprite BUG=678792 Review-Url: https://codereview.chromium.org/2795173002 Cr-Commit-Position: refs/heads/master@{#482122} Committed: https://chromium.googlesource.com/chromium/src/+/830854ccc657258329381910e6b1... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as https://chromium.googlesource.com/chromium/src/+/830854ccc657258329381910e6b1... |
