Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(29)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1869693002: Theme suppliers: Avoid all Image copying. (Closed)

Created:
4 years, 8 months ago by Matt Giuca
Modified:
3 years, 6 months ago
Reviewers:
CC:
chromium-reviews, tfarina, pam+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Theme suppliers: Avoid all Image copying. DO NOT SUBMIT: This is a case study / proof of concept for https://crbug.com/600237 (making gfx::Image non-copyable). It makes it much easier to reason about the ownership of Images. CustomThemeSupplier::GetImageNamed returns a const Image&, not an Image. The caller no longer takes ownership of a new Image object; it just temporarily holds a reference to an image owned by either the theme supplier or the resource bundle. BrowserThemePack: Avoid copying Image objects internally by using std::move. Some minor refactoring of relevant code. BUG=600237

Patch Set 1 #

Patch Set 2 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -68 lines) Patch
M chrome/browser/supervised_user/supervised_user_theme.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/supervised_user/supervised_user_theme.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/themes/browser_theme_pack.h View 1 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/themes/browser_theme_pack.cc View 1 8 chunks +37 lines, -40 lines 0 comments Download
M chrome/browser/themes/custom_theme_supplier.h View 4 chunks +6 lines, -1 line 0 comments Download
M chrome/browser/themes/custom_theme_supplier.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/themes/theme_service.h View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/themes/theme_service.cc View 1 3 chunks +8 lines, -9 lines 0 comments Download
M chrome/browser/themes/theme_service_aurax11.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/libgtk2ui/gtk2_ui.h View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/libgtk2ui/gtk2_ui.cc View 1 2 chunks +3 lines, -3 lines 0 comments Download
M ui/views/linux_ui/linux_ui.h View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 1 (1 generated)
Matt Giuca
3 years, 6 months ago (2017-06-07 03:02:51 UTC) #1 
Description was changed from

==========
Theme suppliers: Avoid all Image copying.

DO NOT SUBMIT: This is a case study / proof of concept for
https://crbug.com/600237 (making gfx::Image non-copyable). It makes it
much easier to reason about the ownership of Images.

CustomThemeSupplier::GetImageNamed returns a const Image&, not an Image.
The caller no longer takes ownership of a new Image object; it just
temporarily holds a reference to an image owned by either the theme
supplier or the resource bundle.

BrowserThemePack: Avoid copying Image objects internally by using
std::move. Some minor refactoring of relevant code.

BUG=600237
==========

to

==========
Theme suppliers: Avoid all Image copying.

DO NOT SUBMIT: This is a case study / proof of concept for
https://crbug.com/600237 (making gfx::Image non-copyable). It makes it
much easier to reason about the ownership of Images.

CustomThemeSupplier::GetImageNamed returns a const Image&, not an Image.
The caller no longer takes ownership of a new Image object; it just
temporarily holds a reference to an image owned by either the theme
supplier or the resource bundle.

BrowserThemePack: Avoid copying Image objects internally by using
std::move. Some minor refactoring of relevant code.

BUG=600237
==========

Powered by Google App Engine
This is Rietveld 408576698