gfx::Image is not const-correct |
||
Issue descriptiongfx::Image isn't logically "const-correct". As explained here: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-const-correctly This is making it hard for me to reason about the thread safety in Issue 742145 . Problems: - AddRepresentation and GetRepresentation take a const Image and return non-const pointers to ImageReps contained therein. This allows clients to modify a const object. - A const ImageRep isn't usable because none of the methods on ImageRep are const, even those that make no changes. - The internal access to Image's storage_: it's treated as a non-const object even by const methods. - Again, a const ImageStorage isn't usable because none of the methods are const. - By making ImageStorage const (when accessed through const Image methods), we can't modify the |representations_| map through const AddRepresentation(). That's OK because |representations_| is a cache (it doesn't visibly alter an Image to write to it); it's exactly why the "mutable" keyword exists, so it should be marked "mutable" and then modified through a const method.
,
Jul 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f44860c11fe5a64fdb31d34d7595c1fbe0fcbf3e commit f44860c11fe5a64fdb31d34d7595c1fbe0fcbf3e Author: Matt Giuca <mgiuca@chromium.org> Date: Tue Jul 25 03:59:26 2017 Make gfx::Image logically const correct. Makes a number of changes to the internal methods of gfx::Image (does not affect its public interface). - ImageRep and ImageStorage methods are now const where appropriate. - AddRepresentation and GetRepresentation return const ImageRep. - Image has a private storage() method to force its const methods to access |storage_| as a const object. - Moved some Image methods into ImageStorage, and removed direct access to ImageStorage's representation map. - Made ImageStorage's representation map "mutable" so it can be accessed from a const method. Bug: 745355 Change-Id: I0a26c1a79fa7a12ed5b1485395f796f1352a88dc Reviewed-on: https://chromium-review.googlesource.com/575114 Commit-Queue: Matt Giuca <mgiuca@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#489224} [modify] https://crrev.com/f44860c11fe5a64fdb31d34d7595c1fbe0fcbf3e/ui/gfx/image/image.cc [modify] https://crrev.com/f44860c11fe5a64fdb31d34d7595c1fbe0fcbf3e/ui/gfx/image/image.h
,
Jul 25 2017
,
Aug 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e51004f83bece58455811a9195a21001c937019a commit e51004f83bece58455811a9195a21001c937019a Author: Matt Giuca <mgiuca@chromium.org> Date: Thu Aug 03 03:45:38 2017 Moved platform-specific functions from image.cc to new module. Adds image_platform.h which declares these methods. The existing platform-specific implementations for iOS and macOS remain in image_ios.mm and image_mac.mm, respectively. The generic implementations are moved to image_generic.cc. This change makes it more obvious what's going on and moves some code into anonymous namespace, and out of ifdefs. Bug: 745355 Change-Id: Id40677dc1e11a0d8b6fc4db5cd22565fd7533660 Reviewed-on: https://chromium-review.googlesource.com/574994 Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#491637} [modify] https://crrev.com/e51004f83bece58455811a9195a21001c937019a/ui/gfx/BUILD.gn [modify] https://crrev.com/e51004f83bece58455811a9195a21001c937019a/ui/gfx/image/image.cc [add] https://crrev.com/e51004f83bece58455811a9195a21001c937019a/ui/gfx/image/image_generic.cc [modify] https://crrev.com/e51004f83bece58455811a9195a21001c937019a/ui/gfx/image/image_ios.mm [modify] https://crrev.com/e51004f83bece58455811a9195a21001c937019a/ui/gfx/image/image_mac.mm [add] https://crrev.com/e51004f83bece58455811a9195a21001c937019a/ui/gfx/image/image_platform.h |
||
►
Sign in to add a comment |
||
Comment 1 by mgiuca@chromium.org
, Jul 18 2017