Issue 13473013: Move ThemeHandler from c/c/e/api; move GetBrowserImages() out of Extension class

Issue 13473013: Move ThemeHandler from c/c/e/api; move GetBrowserImages() out of Extension class (Closed)

Created:
7 years, 8 months ago by Devlin

Modified:
7 years, 8 months ago

Reviewers:
pkotwicz, Yoyo Zhou, viettrungluu, Ben Goodger (Google)

CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Move ThemeHandler from c/c/e/api; move GetBrowserImages() out of Extension class Move the ThemeHandler class to c/c/e/manifest_handlers/, so that the c/c/e/api directory remains used for true APIs. Change names of ThemeInfo::GetTheme* to ThemeInfo::Get* (the Theme should be obvious). Change ThemeInfo::Get* to return const DictionaryValues. Also move GetBrowserImages from Extension class into extension_file_util; remove dependency of Extension on ThemeHandler/Info and ExtensionIconSet. TBR=ben@chromium.org, pkotwicz@chromium.org (ben - gypis, utility_client; pkotwicz - themes; both mechanical changes) BUG=159265 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195138

Patch Set 1 : Latest master (+blink) #

Total comments: 7

Patch Set 2 : Yoyo's requests #

Patch Set 3 : #

Patch Set 4 : Latest master #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+120 lines, -384 lines)			Patch
M	chrome/browser/extensions/sandboxed_unpacker.cc	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/themes/browser_theme_pack.h	View		1 chunk	+6 lines, -5 lines	0 comments	Download
M	chrome/browser/themes/browser_theme_pack.cc	View		7 chunks	+12 lines, -10 lines	0 comments	Download
M	chrome/browser/themes/browser_theme_pack_unittest.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/themes/theme_service.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/chrome_common.gypi	View	1 2	3 chunks	+2 lines, -4 lines	0 comments	Download
D	chrome/common/extensions/api/themes/theme_handler.h	View		1 chunk	+0 lines, -61 lines	0 comments	Download
D	chrome/common/extensions/api/themes/theme_handler.cc	View		1 chunk	+0 lines, -205 lines	0 comments	Download
M	chrome/common/extensions/extension.h	View	1 2	1 chunk	+0 lines, -4 lines	0 comments	Download
M	chrome/common/extensions/extension.cc	View	1 2	2 chunks	+0 lines, -51 lines	0 comments	Download
M	chrome/common/extensions/extension_file_util.h	View	1	1 chunk	+9 lines, -1 line	0 comments	Download
M	chrome/common/extensions/extension_file_util.cc	View	1 2	4 chunks	+44 lines, -0 lines	0 comments	Download
A +	chrome/common/extensions/manifest_handlers/theme_handler.h	View		3 chunks	+7 lines, -7 lines	0 comments	Download
A +	chrome/common/extensions/manifest_handlers/theme_handler.cc	View	1	10 chunks	+30 lines, -28 lines	0 comments	Download
M	chrome/common/extensions/unpacker.cc	View	1	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/common/extensions/unpacker_unittest.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/utility/chrome_content_utility_client.cc	View	1 2 3	2 chunks	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Yoyo Zhou

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/extension_file_util.cc File chrome/common/extensions/extension_file_util.cc (right): https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/extension_file_util.cc#newcode59 chrome/common/extensions/extension_file_util.cc:59: base::FilePath::FromWStringHack(UTF8ToWide(iter->second))); Seems like we should be using FromUTF8Unsafe instead. ...

7 years, 8 months ago (2013-04-10 21:44:58 UTC) #2

Devlin

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/extension_file_util.cc File chrome/common/extensions/extension_file_util.cc (right): https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/extension_file_util.cc#newcode59 chrome/common/extensions/extension_file_util.cc:59: base::FilePath::FromWStringHack(UTF8ToWide(iter->second))); On 2013/04/10 21:44:58, Yoyo Zhou wrote: > Seems ...

7 years, 8 months ago (2013-04-12 02:17:05 UTC) #3

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.cc (right):

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.cc:59:
base::FilePath::FromWStringHack(UTF8ToWide(iter->second)));
On 2013/04/10 21:44:58, Yoyo Zhou wrote:
> Seems like we should be using FromUTF8Unsafe instead. viettrungluu, do you
> agree?

Done (will change if viettrungluu would prefer something else).

I was wondering if we wanted to change ExtensionIconSet::IconMap to use either
FilePaths or FilePath::StringTypes.  In either case, if we do this, when we load
from the dictionary (LoadIconsFromDictionary()), we can do:

FilePath::StringType icon_path;
icons_dict->GetString(key, &icon_path);
icon_map->Add(icon_size, [icon_path | FilePath(icon_path)]);

If this sounds good, I'll handle it in a subsequent CL.

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.h (right):

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.h:88: std::set<base::FilePath>
GetExtensionBrowserImages(
On 2013/04/10 21:44:58, Yoyo Zhou wrote:
> While we're at it, can we give this a better name? How about
> GetBrowserImagePaths
> (Extension is implied since it's extension_file_util.)

Done.

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handlers/theme_handler.cc (right):

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/m...
chrome/common/extensions/manifest_handlers/theme_handler.cc:26: const
base::DictionaryValue* images_value = NULL;
On 2013/04/10 21:44:58, Yoyo Zhou wrote:
> It's ok to use using base::DictionaryValue; in a .cc file.

Yeah; I just wanted to not be using base::DictionaryValue through values.h -
between a using statement in the .cc and prefixes, it's six to the one and a
half dozen the other for me. =)

Done.

viettrungluu

https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/extension_file_util.cc File chrome/common/extensions/extension_file_util.cc (right): https://codereview.chromium.org/13473013/diff/9001/chrome/common/extensions/extension_file_util.cc#newcode59 chrome/common/extensions/extension_file_util.cc:59: base::FilePath::FromWStringHack(UTF8ToWide(iter->second))); On 2013/04/10 21:44:58, Yoyo Zhou wrote: > Seems ...

7 years, 8 months ago (2013-04-12 22:33:15 UTC) #4

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/13473013/41001

7 years, 8 months ago (2013-04-19 01:24:51 UTC) #6

Message was sent while issue was closed.

Change committed as 195138

Expand Messages | Collapse Messages