ImageReplacement: Prevent reset after sizing related source changes [chromium/src : main]
Adithya Srinivasan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Jeremy Roman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
void SelectSourceURL(ImageLoader::UpdateFromElementBehavior,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve type safety and readability without relying on inline comments at the call sites, consider using a base::StrongAlias<class ShouldResetImageReplacementTag, bool> or an enum for the should_reset_image_replacement parameter.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Jeremy Roman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 5 comments![]( "Open in Gerrit")
- viewport size change
- image resize
- sizes attribute change
- srcset attribute change
- enclosing <picture> changeat a high level, I understand why we might not want to reset it when we change which variant of an image source is selected (assuming that they vary only in resolution etc but not semantically), but not all of these obviously match that criterion, so I feel like some detail is missing here
const bool should_reset_image_replacement = name == html_names::kSrcAttr;
SelectSourceURL(ImageLoader::kUpdateIgnorePreviousError,
should_reset_image_replacement);why not if srcset or sizes is changed?
static const unsigned char kGifBytes[] = {
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80,
0x00, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x21, 0xf9, 0x04,
0x01, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01,
0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 0x44, 0x01, 0x00, 0x3b};pull this constant out into a namespace-scoped variable or maybe a function returning Vector<char>, instead of repeating it?
source->setAttribute(html_names::kSrcsetAttr, AtomicString("bar.png"));I don't quite follow here. We're talking about srcset itself being updated, rather than just selecting a different member of the srcset. Why is this a "sizing-related" change and not a potentially semantic change to the rendered image?
source->remove();adding/removing <source>s is maybe a bit more justifiable as "it's still the same one" as long as some <source> (presumably the one that was originally selected) remains, but I don't quite follow the logic here
Related details
- Adithya Srinivasan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
Adithya Srinivasan (Gerrit)
Adithya Srinivasan added 6 comments![]( "Open in Gerrit")
- viewport size change
- image resize
- sizes attribute change
- srcset attribute change
- enclosing <picture> changeat a high level, I understand why we might not want to reset it when we change which variant of an image source is selected (assuming that they vary only in resolution etc but not semantically), but not all of these obviously match that criterion, so I feel like some detail is missing here
Yeah I think I was being a little over aggressive with this.
My main rationale here was that the image src attribute changing is probably a stronger semantic signal here that just the srcset attribute changing (they would presumably change both), partly because I read in a web.dev article that setting a src on an img was mandatory (https://web.dev/learn/design/responsive-images#:~:text=You%20still%20need%20to%20have%20a%20valid%20src%20attribute%2C%20but%20the%20browser%20can%20replace%20its%20value%20with%20one%20of%20the%20options%20listed%20in%20the%20srcset).
But apparently that is not true (https://github.com/whatwg/html/issues/9181), so some of these changes are like you say, potential semantic changes. I've updated the patch to be more conservative and allow replacement reset in more scenarios.
void SelectSourceURL(ImageLoader::UpdateFromElementBehavior,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve type safety and readability without relying on inline comments at the call sites, consider using a base::StrongAlias<class ShouldResetImageReplacementTag, bool> or an enum for the should_reset_image_replacement parameter.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
const bool should_reset_image_replacement = name == html_names::kSrcAttr;
SelectSourceURL(ImageLoader::kUpdateIgnorePreviousError,
should_reset_image_replacement);why not if srcset or sizes is changed?
just `sizes` now, but `sizes` helps determine which src in the srcset gets used but doesn't really change the srcset itself, so it changing on its own probably means a resize of some kind happened (and some sites change `sizes` on a page resize)
static const unsigned char kGifBytes[] = {
0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x01, 0x00, 0x01, 0x00, 0x80,
0x00, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x21, 0xf9, 0x04,
0x01, 0x00, 0x00, 0x00, 0x00, 0x2c, 0x00, 0x00, 0x00, 0x00, 0x01,
0x00, 0x01, 0x00, 0x00, 0x02, 0x02, 0x44, 0x01, 0x00, 0x3b};pull this constant out into a namespace-scoped variable or maybe a function returning Vector<char>, instead of repeating it?
Done
source->setAttribute(html_names::kSrcsetAttr, AtomicString("bar.png"));I don't quite follow here. We're talking about srcset itself being updated, rather than just selecting a different member of the srcset. Why is this a "sizing-related" change and not a potentially semantic change to the rendered image?
removed
adding/removing <source>s is maybe a bit more justifiable as "it's still the same one" as long as some <source> (presumably the one that was originally selected) remains, but I don't quite follow the logic here
- Jeremy Roman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
1 comment
class CORE_EXPORT HTMLSourceElement final : public HTMLElement {nit: I think this might not be needed anymore
Related details
- Adithya Srinivasan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adithya Srinivasan (Gerrit)
Adithya Srinivasan added 1 comment![]( "Open in Gerrit")
class CORE_EXPORT HTMLSourceElement final : public HTMLElement {nit: I think this might not be needed anymore
- Jeremy Roman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adithya Srinivasan (Gerrit)
Adithya Srinivasan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Jeremy Roman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman voted Code-Review+1![]( "Open in Gerrit")
- Adithya Srinivasan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adithya Srinivasan (Gerrit)
Adithya Srinivasan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adithya Srinivasan
| Commit-Queue | +2 |
1 comment
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
ImageReplacement: Prevent reset after sizing related source changes
Adds a param to HTMLImageElement::SelectSourceURL that dictates whether
an associated active image replacement should get discarded if the URL
changes. This boolean is set to false when a source change is triggered
by:
- viewport size changes
- image resize
- sizes attribute change (both on <source> and <img> elements)
- M third_party/blink/renderer/core/html/html_image_element.cc
- M third_party/blink/renderer/core/html/html_image_element.h
- M third_party/blink/renderer/core/html/html_picture_element.cc
- M third_party/blink/renderer/core/html/html_picture_element.h
- M third_party/blink/renderer/core/html/html_source_element.cc
- M third_party/blink/renderer/core/image_replacement/image_replacement.cc
- M third_party/blink/renderer/core/image_replacement/image_replacement.h
- M third_party/blink/renderer/core/image_replacement/image_replacement_test.cc
Code-Review: +1 by Jeremy Roman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |