Move CrxFile, FileReader, ExtensionResource to src/extensions.

Move CrxFile, FileReader, ExtensionResource to src/extensions.

Also move ExtensionResource to extensions namespace.
Also clean up the GenerateId and ShouldLocalizeManfest interfaces.

BUG=159265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190078

[Yoyo Zhou, 2013-03-22 19:30:30]

jyasskin: please review. Sorry it's a lot of files, but the changes are mostly
trivial.
ben: please review gyp and DEPS changes.

[Ben Goodger (Google), 2013-03-22 19:32:48]

rubber stamp lgtm for the move once you have a closer review.

[Yoyo Zhou, 2013-03-22 19:35:04]

On 2013/03/22 19:32:48, Ben Goodger (Google) wrote:
> rubber stamp lgtm for the move once you have a closer review.

(If you'd like to only review one file, extensions/DEPS would be a good choice.)

[Ben Goodger (Google), 2013-03-22 19:35:39]

yep saw that one... makes sense.


On Fri, Mar 22, 2013 at 12:35 PM, <yoz@chromium.org> wrote:

> On 2013/03/22 19:32:48, Ben Goodger (Google) wrote:
>
>> rubber stamp lgtm for the move once you have a closer review.
>>
>
> (If you'd like to only review one file, extensions/DEPS would be a good
> choice.)
>
>
https://codereview.chromium.**org/12578008/<https://codereview.chromium.org/1...
>

[Jeffrey Yasskin, 2013-03-22 20:26:50]

https://codereview.chromium.org/12578008/diff/5005/chrome/browser/sync/test/i...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/12578008/diff/5005/chrome/browser/sync/test/i...
chrome/browser/sync/test/integration/sync_extension_helper.cc:50: std::string
SyncExtensionHelper::NameToId(const std::string& name) {
Inline this now?

https://codereview.chromium.org/12578008/diff/5005/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/12578008/diff/5005/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:58:
'../extensions/browser/file_reader.cc',
Why are these still here, but crx_file isn't?

https://codereview.chromium.org/12578008/diff/5005/chrome/common/extensions/e...
File chrome/common/extensions/extension_l10n_util.cc (right):

https://codereview.chromium.org/12578008/diff/5005/chrome/common/extensions/e...
chrome/common/extensions/extension_l10n_util.cc:51: bool
ShouldRelocalizeManifest(DictionaryValue* const manifest) {
I think you want const DictionaryValue* here, since you're not modifying the
manifest.

https://codereview.chromium.org/12578008/diff/5005/chrome/common/extensions/e...
File chrome/common/extensions/extension_l10n_util.h (right):

https://codereview.chromium.org/12578008/diff/5005/chrome/common/extensions/e...
chrome/common/extensions/extension_l10n_util.h:38: bool
ShouldRelocalizeManifest(base::DictionaryValue* const manifest);
"const" after the * has no effect in a declaration. You can include it in the
definition if you want, but you should omit it from the declaration.

https://codereview.chromium.org/12578008/diff/5005/chrome/common/extensions/e...
File chrome/common/extensions/extension_l10n_util_unittest.cc (right):

https://codereview.chromium.org/12578008/diff/5005/chrome/common/extensions/e...
chrome/common/extensions/extension_l10n_util_unittest.cc:31: base::FilePath
src_path = temp.path().Append(extensions::kLocaleFolder);
Maybe add "using extensions::kLocaleFolder;"

https://codereview.chromium.org/12578008/diff/5005/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/12578008/diff/5005/extensions/DEPS#newcode13
extensions/DEPS:13: ".*test\.cc": [
Shouldn't content/public/test live down here?

[Yoyo Zhou, 2013-03-22 21:33:31]

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/browser/sync...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/browser/sync...
chrome/browser/sync/test/integration/sync_extension_helper.cc:50: std::string
SyncExtensionHelper::NameToId(const std::string& name) {
On 2013/03/22 20:26:50, Jeffrey Yasskin wrote:
> Inline this now?

Done.

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/chrome_brows...
File chrome/chrome_browser_extensions.gypi (right):

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/chrome_brows...
chrome/chrome_browser_extensions.gypi:58:
'../extensions/browser/file_reader.cc',
On 2013/03/22 20:26:50, Jeffrey Yasskin wrote:
> Why are these still here, but crx_file isn't?

crx_file moved to common.

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/common/exten...
File chrome/common/extensions/extension_l10n_util.cc (right):

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/common/exten...
chrome/common/extensions/extension_l10n_util.cc:51: bool
ShouldRelocalizeManifest(DictionaryValue* const manifest) {
On 2013/03/22 20:26:50, Jeffrey Yasskin wrote:
> I think you want const DictionaryValue* here, since you're not modifying the
> manifest.

Done.

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/common/exten...
File chrome/common/extensions/extension_l10n_util.h (right):

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/common/exten...
chrome/common/extensions/extension_l10n_util.h:38: bool
ShouldRelocalizeManifest(base::DictionaryValue* const manifest);
On 2013/03/22 20:26:50, Jeffrey Yasskin wrote:
> "const" after the * has no effect in a declaration. You can include it in the
> definition if you want, but you should omit it from the declaration.

Changed to const base::DictionaryValue*

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/common/exten...
File chrome/common/extensions/extension_l10n_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/12578008/diff/5005/chrome/common/exten...
chrome/common/extensions/extension_l10n_util_unittest.cc:31: base::FilePath
src_path = temp.path().Append(extensions::kLocaleFolder);
On 2013/03/22 20:26:50, Jeffrey Yasskin wrote:
> Maybe add "using extensions::kLocaleFolder;"

Done.

https://chromiumcodereview.appspot.com/12578008/diff/5005/extensions/DEPS
File extensions/DEPS (right):

https://chromiumcodereview.appspot.com/12578008/diff/5005/extensions/DEPS#new...
extensions/DEPS:13: ".*test\.cc": [
On 2013/03/22 20:26:50, Jeffrey Yasskin wrote:
> Shouldn't content/public/test live down here?

I suppose it could, but I'll strike Temporary from the comment.

[Jeffrey Yasskin, 2013-03-22 21:37:17]

lgtm

[commit-bot: I haz the power, 2013-03-22 22:30:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12578008/43001

[commit-bot: I haz the power, 2013-03-23 15:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12578008/43001

[commit-bot: I haz the power, 2013-03-23 21:25:21]

Change committed as 190078