Move Blink images to use ByteSize rather than ByteCount [chromium/src : main]
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
I think this is a minimum set to cover ownership here; PTAL.
Related details
- Corentin Wallez
- Francois Pierre Doray
- Jean-Philippe Gravel
- Kenneth Russell
- Mike West
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")
base::ByteSizeDelta(4 * size_.width() * size_.height());nit: Consistency: Use 'base::ByteSize' instead of 'base::ByteSizeDelta' to represent the size of the depth/stencil buffer. This maintains consistency with other buffer size calculations in this method (e.g., line 1233) and semantically represents a size rather than a difference.
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
- Corentin Wallez
- Francois Pierre Doray
- Jean-Philippe Gravel
- Kenneth Russell
- Mike West
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. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
base::ByteSizeDelta(4 * size_.width() * size_.height());Switch to `base::ByteSize(base::checked_cast<uint64_t>(4 * size_.width() * size_.height()))`.
Reason: This represents an absolute quantity of bytes, not a delta. It just happens to be computed from signed numbers that we expect to be non-negative. If we wanted this to be less verbose, we could introduce `base::ByteSize::FromNonNegativeSigned`, which would CHECK that its signed argument isn't negative.
Related details
- Avi Drissman
- Corentin Wallez
Corentin Wallez (Gerrit)
Corentin Wallez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Corentin Wallez
| Code-Review | +1 |
1 comment
- Avi Drissman
- Jean-Philippe Gravel
- Kenneth Russell
- Mike West
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. |
Avi Drissman (Gerrit)
Avi Drissman added 2 comments![]( "Open in Gerrit")
base::ByteSizeDelta(4 * size_.width() * size_.height());nit: Consistency: Use 'base::ByteSize' instead of 'base::ByteSizeDelta' to represent the size of the depth/stencil buffer. This maintains consistency with other buffer size calculations in this method (e.g., line 1233) and semantically represents a size rather than a difference.
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
base::ByteSizeDelta(4 * size_.width() * size_.height());Switch to `base::ByteSize(base::checked_cast<uint64_t>(4 * size_.width() * size_.height()))`.
Reason: This represents an absolute quantity of bytes, not a delta. It just happens to be computed from signed numbers that we expect to be non-negative. If we wanted this to be less verbose, we could introduce `base::ByteSize::FromNonNegativeSigned`, which would CHECK that its signed argument isn't negative.
Done. We probably should change rect to have unsigned size.
Related details
- Francois Pierre Doray
- Jean-Philippe Gravel
- Kenneth Russell
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 3 comments![]( "Open in Gerrit")
#include "base/byte_size.h"IWYU nit: We're still using `ByteCount` below, so we should still include the `byte_count.h` header. I'm not actually sure `base/byte_size.h` is necessary in this file?
base::ByteCount memory_used(entry.key->GetMemoryUsage().InBytes());I'm not sure I understand this change. Why convert the `base::ByteCount` to an `int64_t` and then use that to construct a `base::ByteCount`?
Related details
- Francois Pierre Doray
- Jean-Philippe Gravel
- Kenneth Russell
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. |
Avi Drissman (Gerrit)
Avi Drissman added 2 comments![]( "Open in Gerrit")
IWYU nit: We're still using `ByteCount` below, so we should still include the `byte_count.h` header. I'm not actually sure `base/byte_size.h` is necessary in this file?
Marked as resolved.
base::ByteCount memory_used(entry.key->GetMemoryUsage().InBytes());I'm not sure I understand this change. Why convert the `base::ByteCount` to an `int64_t` and then use that to construct a `base::ByteCount`?
We’re switching from the new type to the old type. I missed that there’s a conversion function; using that instead.
Related details
- Francois Pierre Doray
- Jean-Philippe Gravel
- Kenneth Russell
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted Code-Review+1![]( "Open in Gerrit")
- Avi Drissman
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Avi Drissman
- Kenneth Russell
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
I think kbr is redundant as an owner so removing.
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
2 comments
LGTM % nit. Thanks!
#include "base/byte_size.h"Avi DrissmanIWYU nit: We're still using `ByteCount` below, so we should still include the `byte_count.h` header. I'm not actually sure `base/byte_size.h` is necessary in this file?
Marked as resolved.
Unfortunately, you're now using `base::ByteSize` below, so I think we need the new header... :)
Related details
- Avi Drissman
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. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Commit-Queue | +2 |
1 comment
#include "base/byte_size.h"Avi DrissmanIWYU nit: We're still using `ByteCount` below, so we should still include the `byte_count.h` header. I'm not actually sure `base/byte_size.h` is necessary in this file?
Mike WestMarked as resolved.
Unfortunately, you're now using `base::ByteSize` below, so I think we need the new header... :)
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/controller/performance_manager/v8_detailed_memory_reporter_impl.cc
Insertions: 1, Deletions: 1.
@@ -9,7 +9,7 @@
#include <utility>
#include <vector>
-#include "base/byte_count.h"
+#include "base/byte_size.h"
#include "base/check.h"
#include "base/functional/callback.h"
#include "base/memory/raw_ptr.h"
```
Change information
Move Blink images to use ByteSize rather than ByteCount
- M gpu/command_buffer/client/client_shared_image.h
- M third_party/blink/DEPS
- M third_party/blink/renderer/controller/performance_manager/v8_detailed_memory_reporter_impl.cc
- M third_party/blink/renderer/core/html/canvas/canvas_rendering_context.cc
- M third_party/blink/renderer/core/html/canvas/canvas_rendering_context.h
- M third_party/blink/renderer/core/html/canvas/canvas_rendering_context_host.cc
- M third_party/blink/renderer/core/html/canvas/canvas_rendering_context_host.h
- M third_party/blink/renderer/core/html/canvas/html_canvas_element_test.cc
- M third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.h
- M third_party/blink/renderer/modules/canvas/imagebitmap/image_bitmap_rendering_context.cc
- M third_party/blink/renderer/modules/canvas/imagebitmap/image_bitmap_rendering_context.h
- M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc
- M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.h
- M third_party/blink/renderer/platform/graphics/canvas_resource.h
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider.h
- M third_party/blink/renderer/platform/graphics/gpu/drawing_buffer.cc
- M third_party/blink/renderer/platform/graphics/gpu/drawing_buffer.h
- M third_party/blink/renderer/platform/graphics/static_bitmap_image.h
Code-Review: +1 by Francois Pierre Doray, +1 by Mike West, +1 by Corentin Wallez, +1 by Jean-Philippe Gravel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |