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

Issue 170015: This change enables Chrome to load locale information for the extension. It d... (Closed)

Created:
11 years, 4 months ago by Nebojša Ćirić
Modified:
9 years, 7 months ago
Reviewers:
Aaron Boodman
CC:
chromium-reviews_googlegroups.com, Erik does not do reviews, Ben Goodger (Google)
Visibility:
Public.

Description

This change enables Chrome to load locale information for the extension. It detects default locale, and filters out all locales not supported by Chrome or with invalid names/missing messages. It also checks for folders that start with _ and are not in the reserved list. We don't validate messages file with this CL. Added support for loading supplied locale information to the extension_file_util, and detecting default locale. Added new constants to extension class (_locales directory name, messages filename). Added new error messages to _constants. Added new unittests. BUG=12131 TEST=There should be no visible changes, except in case of error when loading extension (e.g. create empty _locales folder and try loading).

Patch Set 1 #

Total comments: 6

Patch Set 2 : '' #

Total comments: 4

Patch Set 3 : '' #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+507 lines, -2 lines) Patch
M chrome/browser/extensions/extension_file_util.h View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_file_util.cc View 1 2 3 chunks +57 lines, -0 lines 1 comment Download
M chrome/browser/extensions/extension_file_util_unittest.cc View 1 2 2 chunks +111 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_l10n_util.h View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_l10n_util.cc View 1 2 1 chunk +90 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_l10n_util_unittest.cc View 1 2 1 chunk +134 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/common/extensions/extension.h View 4 chunks +27 lines, -1 line 0 comments Download
M chrome/common/extensions/extension.cc View 1 2 2 chunks +14 lines, -0 lines 1 comment Download
M chrome/common/extensions/extension_constants.h View 1 2 3 chunks +5 lines, -1 line 0 comments Download
M chrome/common/extensions/extension_constants.cc View 1 2 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/common/extensions/extension_unittest.cc View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Nebojša Ćirić
First of many i18n changes. I tried to keep it as small as possible, but ...
11 years, 4 months ago (2009-08-14 22:22:04 UTC) #1
Aaron Boodman
http://codereview.chromium.org/170015/diff/1/7 File chrome/browser/extensions/extension_file_util.cc (right): http://codereview.chromium.org/170015/diff/1/7#newcode289 Line 289: Since you are adding the first case of ...
11 years, 4 months ago (2009-08-17 18:48:43 UTC) #2
Aaron Boodman
Also, sorry for taking so long to review this. Please poke me next time I ...
11 years, 4 months ago (2009-08-17 18:50:15 UTC) #3
Nebojša Ćirić
:), I don't expect people to work on weekends, and this was sent Friday afternoon, ...
11 years, 4 months ago (2009-08-17 20:24:52 UTC) #4
Aaron Boodman
On 2009/08/17 20:24:52, Nebojša Ćirić wrote: > :), I don't expect people to work on ...
11 years, 4 months ago (2009-08-18 00:59:36 UTC) #5
Nebojša Ćirić
Ready for the next round. I haven't implemented loading/parsing of message catalogs in this CL ...
11 years, 4 months ago (2009-08-18 17:13:41 UTC) #6
Aaron Boodman
http://codereview.chromium.org/170015/diff/4001/5004 File chrome/browser/extensions/extension_file_util.cc (right): http://codereview.chromium.org/170015/diff/4001/5004#newcode278 Line 278: if (!extension_l10n_util::SetDefaultLocale(manifest, extension.get())) { I think it would ...
11 years, 4 months ago (2009-08-18 23:47:12 UTC) #7
Nebojša Ćirić
I think everything is covered now. Updated some tests (and added few lines to extension_unittest). ...
11 years, 4 months ago (2009-08-19 02:28:24 UTC) #8
Nebojša Ćirić
Btw. why are all try servers constantly chocking on this CL? Log tells me that ...
11 years, 4 months ago (2009-08-19 02:29:49 UTC) #9
Aaron Boodman
11 years, 4 months ago (2009-08-19 03:00:49 UTC) #10
LGTM with one more nit and a question.

http://codereview.chromium.org/170015/diff/5016/5020
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/170015/diff/5016/5020#newcode389
Line 389: *error = StringPrintf("Illegal file/directory name %s", filename);
Can you make this error a bit more developer friendly, like:

Cannot load extension with file or directory name "/_blah.png". Filenames
starting with "_" are reserved for use by the system.

http://codereview.chromium.org/170015/diff/5016/5024
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/170015/diff/5016/5024#newcode71
Line 71: const char Extension::kMessagesFilename[] = "messages";
I asked this in a previous review, but it looks to me like presently, the file
heirarchy is:

/_locales
  /en_us
    messages

Is that right? Out of curiosity, I'm wondering what other files you might think
would go in each locale folder.

Powered by Google App Engine
This is Rietveld 408576698