ImageReplacement: Introduce blink mojom interfaces for image replacement. [chromium/src : main]
Adithya Srinivasan (Gerrit)
Adithya Srinivasan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adithya Srinivasan
| Commit-Queue | +1 |
1 comment
+jbroman if you have any early thoughts
I want to flesh out the follow up (browser-side) before added a mojo reviewer, because I think they'll ask to see how the frame token is being used.
Related details
- Adithya Srinivasan
- 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 added 3 comments![]( "Open in Gerrit")
mostly makes sense, just trying to keep us out of the way of core/ development as much as possible
interface ImageReplacement {Maybe suggest explicitly that features using image replacement will need to send the browser a pending_remote<ImageReplacement> from the renderer process.
HeapMojoReceiver<mojom::blink::ImageReplacement, HTMLImageElement>
image_replacement_receiver_;
HeapMojoRemote<mojom::blink::ImageReplacementHost> image_replacement_host_;I'm a bit worried about paying this size for every HTMLImageElement. Both for size and clearer code separation, mind seeing if we can split this into a separate class (ImageReplacement or something like that) and look it up in a (per-doc?) HTMLImageElement->ImageReplacement map?
Some things will need to remain here of course, but hoping we can keep the <img> element itself from gaining too much of the complexity directly.
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 2 comments![]( "Open in Gerrit")
Maybe suggest explicitly that features using image replacement will need to send the browser a pending_remote<ImageReplacement> from the renderer process.
Done
HeapMojoReceiver<mojom::blink::ImageReplacement, HTMLImageElement>
image_replacement_receiver_;
HeapMojoRemote<mojom::blink::ImageReplacementHost> image_replacement_host_;I'm a bit worried about paying this size for every HTMLImageElement. Both for size and clearer code separation, mind seeing if we can split this into a separate class (ImageReplacement or something like that) and look it up in a (per-doc?) HTMLImageElement->ImageReplacement map?
Some things will need to remain here of course, but hoping we can keep the <img> element itself from gaining too much of the complexity directly.
- 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 10 comments![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
10 comments
#include "third_party/blink/renderer/platform/mojo/heap_mojo_receiver.h"
#include "third_party/blink/renderer/platform/mojo/heap_mojo_remote.h"unused
#include "third_party/blink/public/mojom/image_replacement/image_replacement.mojom-blink.h"I think mojom-blink-forward will suffice here and avoid bringing too much more stuff in for other translation units of this header. If you take my other suggestion and move CreateImageReplacement elsewhere, it will be entirely unnecessary in this file.
if (!base::FeatureList::IsEnabled(features::kImageReplacement)) {
return base::unexpected("ImageReplacement feature is not enabled.");
}
if (!isConnected()) {
return base::unexpected("HTMLImageElement is not connected to a document.");
}
DocumentImageReplacements* replacements =
DocumentImageReplacements::GetOrCreateFrom(GetDocument());
if (replacements->GetImageReplacement(this)) {
return base::unexpected(
"HTMLImageElement already has a pending/active replacement.");
}
if (complete()) {
ImageResourceContent* image_content = CachedImage();
if (!image_content) {
return base::unexpected("HTMLImageElement has no src");
}
if (image_content->ErrorOccurred()) {
return base::unexpected("HTMLImageElement had a loading error");
}
}
if (!IsPrimaryContent()) {
return base::unexpected(
"HTMLImageElement is not displaying primary content.");
}
ImageReplacement* replacement = MakeGarbageCollected<ImageReplacement>(*this);
replacements->RegisterImageReplacement(this, replacement);
return replacement->BindReceiver();nit: Not a huge deal as this is much smaller now, but maybe this could even be flipped to be something like `static base::expected<...> ImageReplacement::CreateAndBindReceiver(HTMLImageElement*)` and keep even less of it here?
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/bindings/remote.h"almost surely this file can be reverted and needs no new includes, if you haven't made any other changes to it?
static DocumentImageReplacements* From(Document&);
static DocumentImageReplacements* GetOrCreateFrom(Document&);nits: I slightly prefer From/FromIfExists, but I don't feel strongly. Also, the version that always works should probably return a reference (&) nowadays.
auto it = replacements_.find(element);
if (it != replacements_.end()) {
ImageReplacement* replacement = it->value.Get();
replacements_.erase(it);
return replacement;
}
return nullptr;nit: I think this is equivalent to `return replacements_.Take(element);`? Not a huge deal either way.
#include "third_party/blink/renderer/core/image_replacement/document_image_replacements.h"unused right now (might change as a result of other comments)
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +0 |
1 comment
(Obviously you still need an ipc/SECURITY_OWNERS at a minimum. A rendering/layout person wouldn't hurt either.)
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 added 1 comment![]( "Open in Gerrit")
ImageReplacement: Introduce blink mojom interfaces for image replacement.Oh, and this headline is a bit misleading as while it does introduce the mojo interfaces, it does a few other things, too, and the first line should summarize the overall effect
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Jeremy Roman
| Code-Review | +1 |
1 comment
and yet it removed by CR vote, whoops
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. |