Injected CSS localization fix (see bug no.)

Added code for localizing scripts CSS before injection takes place.

BUG=39899
TEST=browser_tests:ExtensionApiTest.ContentScriptCSSLocalization


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97365

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97927

[Nebojša Ćirić, 2011-08-10 20:38:57]

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:166: std::string
code_string_to_execute = code_string;
Why change name here? You could just rename the function parameter. Am I missing
something?

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:176:
localization_messages, &code_string_to_execute, &error);
If this code snippet shows up in other places you could create a helper method
in extension_i18n_utils - like LocalizeCSS.

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.cc:156: this,
&UserScriptMaster::ScriptReloader::RunLoad, user_scripts));
Do you wanna pass your map here to file thread?

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.cc:185: LOG(INFO) <<
"$$$_BEFORE_LOCALIZATION " << content;
Remove LOG(INFO).

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.cc:213:
extensions_info_[script.extension_id()].second);
Refactor this into a method call.

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_master.h (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.h:85: typedef std::pair<FilePath,
std::string> ExtensionPathAndDefaultLocale;
This could go into ExtensionSet class, as a public typedef.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:426: return returnValue;
I am torn between ext_i18n_utils, message_bundle or here for this code.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.h:70: // the entry for the
extension ID if extension is valid.
Contains at least extension_id item if extension is valid.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:61: void
GetExtensionsPathAndDefaultLocale(
Do you need this method? Couldn't you just:

make_pair(extension->extension_path(), extension->default_locale())?

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:62: std::map<std::string,
std::pair<FilePath, std::string> >& info) const;
You could typedef it here as ExtensionPathAndDefaultLocale and then use it in
the .cc file and in user_script file.

[adriansc, 2011-08-10 23:20:53]

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:166: std::string
code_string_to_execute = code_string;
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> Why change name here? You could just rename the function parameter. Am I
missing
> something?

The code_string gets passed in the binding as a const std::string reference. In
order to do message replacement, it has to be mutable. I added a comment.

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:176:
localization_messages, &code_string_to_execute, &error);
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> If this code snippet shows up in other places you could create a helper method
> in extension_i18n_utils - like LocalizeCSS.

Moved to extension_l10n_utils

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.cc:156: this,
&UserScriptMaster::ScriptReloader::RunLoad, user_scripts));
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> Do you wanna pass your map here to file thread?

I could, but the methods executing on BrowserThread::FILE belong to
ScriptReloader, so while loading takes place, the object cannot die (enforced by
AddRef() above). Since I put the map inside the ScriptReloader object, it will
be freely accessible by the FILE thread.

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.cc:185: LOG(INFO) <<
"$$$_BEFORE_LOCALIZATION " << content;
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> Remove LOG(INFO).

Done.

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.cc:213:
extensions_info_[script.extension_id()].second);
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> Refactor this into a method call.

Done.

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_master.h (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master.h:85: typedef std::pair<FilePath,
std::string> ExtensionPathAndDefaultLocale;
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> This could go into ExtensionSet class, as a public typedef.

Done.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:426: return returnValue;
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> I am torn between ext_i18n_utils, message_bundle or here for this code.

Perhaps we should get rid of the static functions in MessageBundle (not quite
sure how straightforward that might be) and only allow message replacement
through MessageBundle objects. This way, we could merge this code with the
MessageBundle internal code.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.h:70: // the entry for the
extension ID if extension is valid.
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> Contains at least extension_id item if extension is valid.

Done.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:61: void
GetExtensionsPathAndDefaultLocale(
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> Do you need this method? Couldn't you just:
> 
> make_pair(extension->extension_path(), extension->default_locale())?

This method fills in a map by repeatedly calling make_pair for all extension IDs
in the set. The underlying map that I use for collecting the data is private, so
in order to iterate through all IDs, I added a function.

http://codereview.chromium.org/7552028/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:62: std::map<std::string,
std::pair<FilePath, std::string> >& info) const;
On 2011/08/10 20:38:57, Nebojša Ćirić wrote:
> You could typedef it here as ExtensionPathAndDefaultLocale and then use it in
> the .cc file and in user_script file.

Done.

[Mihai Parparita -not on Chrome, 2011-08-11 01:58:34]

It would be good to have tests for this too.

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:172:
extension_l10n_util::LocalizeCSSScript(extension, code_string_to_execute);
I don't think you want to do this when you're passed in a code string (callsite
at line 104), only when you loaded a file (callsite at line 131).

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:172:
extension_l10n_util::LocalizeCSSScript(extension, code_string_to_execute);
LocalizeCSSScript ends up doing file IO (to load the messages), and extension
functions are handled on the UI thread, so I think you'll need to execute this
on the file thread.

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_master_unittest.cc (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master_unittest.cc:225:
script_reloader->AddRef();
Instead of having to manually call AddRef/Release, can you just make the
ScriptReloader instance be on the stack?

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:414:
extension_file_util::LoadExtensionMessageBundle(
extension_file_util:: namespace prefix is unnecessary.

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.h:71: std::map<std::string,
std::string>* LoadExtensionMessageBundleSubstitutionMap(
Shouldn't the return type be SubstitutionMap*?

[adriansc, 2011-08-11 21:25:07]

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:172:
extension_l10n_util::LocalizeCSSScript(extension, code_string_to_execute);
On 2011/08/11 01:58:34, Mihai Parparita wrote:
> I don't think you want to do this when you're passed in a code string
(callsite
> at line 104), only when you loaded a file (callsite at line 131).

I moved the localization logic to the file thread in DidLoadFile before calling
Execute.

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_code_in_tab_function.cc:172:
extension_l10n_util::LocalizeCSSScript(extension, code_string_to_execute);
On 2011/08/11 01:58:34, Mihai Parparita wrote:
> I don't think you want to do this when you're passed in a code string
(callsite
> at line 104), only when you loaded a file (callsite at line 131).

Done. Localization is only called in DidLoadFile now.

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/us...
File chrome/browser/extensions/user_script_master_unittest.cc (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/browser/extensions/us...
chrome/browser/extensions/user_script_master_unittest.cc:225:
script_reloader->AddRef();
On 2011/08/11 01:58:34, Mihai Parparita wrote:
> Instead of having to manually call AddRef/Release, can you just make the
> ScriptReloader instance be on the stack?

ScriptReloader is derived from RefCountedThreadSafe which in turn derived from
RefCountedThreadSafeBase. The destructor of this class exits with error unless
reference count has at least once been positive during the lifetime of the
object (base/memory/ref_counted.h). When the reference count falls back to 0,
the memory is released. This is why I allocated on the heap.

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:414:
extension_file_util::LoadExtensionMessageBundle(
On 2011/08/11 01:58:34, Mihai Parparita wrote:
> extension_file_util:: namespace prefix is unnecessary.

Done.

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/7552028/diff/8004/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.h:71: std::map<std::string,
std::string>* LoadExtensionMessageBundleSubstitutionMap(
On 2011/08/11 01:58:34, Mihai Parparita wrote:
> Shouldn't the return type be SubstitutionMap*?

Done.

[Mihai Parparita -not on Chrome, 2011-08-12 23:15:20]

Almos there, but still needs a test.

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:170:
origin_loop->PostTask(
The pattern in other extension APIs to to use
BrowserThread::PostTask(BrowserThread::UI, ...) instead of holding on to the
original MessageLoop.

[adriansc, 2011-08-16 07:09:19]

Added browser_tests

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:170:
origin_loop->PostTask(
On 2011/08/12 23:15:20, Mihai Parparita wrote:
> The pattern in other extension APIs to to use
> BrowserThread::PostTask(BrowserThread::UI, ...) instead of holding on to the
> original MessageLoop.

Done.

[Nebojša Ćirić, 2011-08-16 15:04:54]

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:7: #include
"base/message_loop.h"
Do you need this now?

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:145:
MessageLoop::current()));
Do you need MessageLoop::current()?

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:156: MessageLoop*
origin_loop) {
Remove origin_loop?

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/background.html (right):

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/background.html:20: function
extensionIDMessageGetsReplacedInContentScriptCSS() {
Comment about 2 paths you have to cover would be nice (how you did it), also
comment both test functions - at a glance they look the same/very similar, so
somebody may miss the difference.

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js (right):

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js:1: var
gotReplaced = true;
Maybe put a comment in each .js as to what they do (one for
__MSG_@extension_id__ the other for __MSG_@bidi_...

[adriansc, 2011-08-16 17:07:03]

Updated

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:7: #include
"base/message_loop.h"
On 2011/08/16 15:04:54, Nebojša Ćirić wrote:
> Do you need this now?

Done.

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:145:
MessageLoop::current()));
On 2011/08/16 15:04:54, Nebojša Ćirić wrote:
> Do you need MessageLoop::current()?

Done.

http://codereview.chromium.org/7552028/diff/18012/chrome/browser/extensions/e...
chrome/browser/extensions/execute_code_in_tab_function.cc:156: MessageLoop*
origin_loop) {
On 2011/08/16 15:04:54, Nebojša Ćirić wrote:
> Remove origin_loop?

Done.

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/background.html (right):

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/background.html:20: function
extensionIDMessageGetsReplacedInContentScriptCSS() {
On 2011/08/16 15:04:54, Nebojša Ćirić wrote:
> Comment about 2 paths you have to cover would be nice (how you did it), also
> comment both test functions - at a glance they look the same/very similar, so
> somebody may miss the difference.

Done.

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js (right):

http://codereview.chromium.org/7552028/diff/23002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js:1: var
gotReplaced = true;
On 2011/08/16 15:04:54, Nebojša Ćirić wrote:
> Maybe put a comment in each .js as to what they do (one for
> __MSG_@extension_id__ the other for __MSG_@bidi_...

Done.

[Nebojša Ćirić, 2011-08-16 17:40:23]

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
File chrome/browser/extensions/content_script_apitest.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
chrome/browser/extensions/content_script_apitest.cc:98:
ASSERT_TRUE(RunExtensionTest("css_l10n")) << message_;
Do you want to move css_l10n under content_scripts/css_l10n - we are testing
content scripts here. Not a big deal.

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/u...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/u...
chrome/browser/extensions/user_script_master.cc:186: *localization_messages,
&content, &error);
Do we care about the error here?

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:324:
scoped_ptr<std::map<std::string, std::string> > dictionary_map(
Is it possible to use ExtensionSet::ExtensionPathAndID typedef here?

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/_locales/en/messages.json
(right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/_locales/en/messages.json:3:
"message": "ltr"
bidi_dir is already defined in
http://code.google.com/chrome/extensions/i18n.html#overview-predefined.

You should use something else, say
"text_color": { "red" }

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/background.html (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/background.html:38: // checks that
it has had __MSG_@@bidi_dir__ replaced with the correct
make sure you update the comment when you change the message catalog.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/manifest.json (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/manifest.json:3: "description":
"Checks that messages in CSS scripts get localized.",
...CSS content scripts...

CSS scripts that belong to the extension (say for options page) are already
localized.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js:21:
document.title = 'passed';
Why not send request to the background page from here (same for the other
script)?

[Mihai Parparita -not on Chrome, 2011-08-16 17:42:41]

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
File chrome/browser/extensions/content_script_apitest.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
chrome/browser/extensions/content_script_apitest.cc:98:
ASSERT_TRUE(RunExtensionTest("css_l10n")) << message_;
All the other API tests in this file are in the content_scripts subdirectory,
can you put this one there too?

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/background.html (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/background.html:14: if
((!firstEnter))
One too many parentheses here.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js:11:
document.title = 'no CSS rules injected';
This title will never be displayed, since you still set the title to passed/fail
below.

[adriansc, 2011-08-16 22:13:51]

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
File chrome/browser/extensions/content_script_apitest.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
chrome/browser/extensions/content_script_apitest.cc:98:
ASSERT_TRUE(RunExtensionTest("css_l10n")) << message_;
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> Do you want to move css_l10n under content_scripts/css_l10n - we are testing
> content scripts here. Not a big deal.

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/c...
chrome/browser/extensions/content_script_apitest.cc:98:
ASSERT_TRUE(RunExtensionTest("css_l10n")) << message_;
On 2011/08/16 17:42:41, Mihai Parparita wrote:
> All the other API tests in this file are in the content_scripts subdirectory,
> can you put this one there too?

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/u...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/extensions/u...
chrome/browser/extensions/user_script_master.cc:186: *localization_messages,
&content, &error);
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> Do we care about the error here?

Errors in this case are of the type "message not found in message bundle". As it
is now, they left unreplaced. As far as the implementation goes, I have to pass
in a valid pointer, because it gets referenced without a test in
extension_message_bundle.cc.

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_message_filter.cc:324:
scoped_ptr<std::map<std::string, std::string> > dictionary_map(
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> Is it possible to use ExtensionSet::ExtensionPathAndID typedef here?

Changed return type to ExtensionMessageBundle::SubstitutionMap scoped pointer.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/_locales/en/messages.json
(right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/_locales/en/messages.json:3:
"message": "ltr"
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> bidi_dir is already defined in
> http://code.google.com/chrome/extensions/i18n.html#overview-predefined.
> 
> You should use something else, say
> "text_color": { "red" }

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/background.html (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/background.html:14: if
((!firstEnter))
On 2011/08/16 17:42:41, Mihai Parparita wrote:
> One too many parentheses here.

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/background.html:38: // checks that
it has had __MSG_@@bidi_dir__ replaced with the correct
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> make sure you update the comment when you change the message catalog.

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/manifest.json (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/manifest.json:3: "description":
"Checks that messages in CSS scripts get localized.",
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> ...CSS content scripts...
> 
> CSS scripts that belong to the extension (say for options page) are already
> localized.

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js (right):

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js:11:
document.title = 'no CSS rules injected';
On 2011/08/16 17:42:41, Mihai Parparita wrote:
> This title will never be displayed, since you still set the title to
passed/fail
> below.

Done.

http://codereview.chromium.org/7552028/diff/22047/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/css_l10n/test_extension_id.js:21:
document.title = 'passed';
On 2011/08/16 17:40:24, Nebojša Ćirić wrote:
> Why not send request to the background page from here (same for the other
> script)?

Done.

[Nebojša Ćirić, 2011-08-16 22:23:35]

LGTM

[Mihai Parparita -not on Chrome, 2011-08-17 22:31:24]

LGTM

http://codereview.chromium.org/7552028/diff/34004/chrome/browser/extensions/c...
File chrome/browser/extensions/content_script_apitest.cc (right):

http://codereview.chromium.org/7552028/diff/34004/chrome/browser/extensions/c...
chrome/browser/extensions/content_script_apitest.cc:96:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, CSSLocalization) {
Call this ContentScriptCCSSLocalization (all the other test cases in this file
being with ContentScript).

[adriansc, 2011-08-18 00:49:25]

http://codereview.chromium.org/7552028/diff/34004/chrome/browser/extensions/c...
File chrome/browser/extensions/content_script_apitest.cc (right):

http://codereview.chromium.org/7552028/diff/34004/chrome/browser/extensions/c...
chrome/browser/extensions/content_script_apitest.cc:96:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, CSSLocalization) {
On 2011/08/17 22:31:24, Mihai Parparita wrote:
> Call this ContentScriptCCSSLocalization (all the other test cases in this file
> being with ContentScript).

Done.

[commit-bot: I haz the power, 2011-08-18 21:46:20]

Change committed as 97365

[Nebojša Ćirić, 2011-08-19 21:19:15]

http://codereview.chromium.org/7552028/diff/40001/chrome/browser/extensions/u...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/40001/chrome/browser/extensions/u...
chrome/browser/extensions/user_script_master.cc:284: const UserScriptList&
user_scripts) {
//... called on the IO thread

Can this be const all the way (if extensions() is mutable then nothing)?
const ExtensionInfoMap* const info_map

http://codereview.chromium.org/7552028/diff/40001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_set.cc (right):

http://codereview.chromium.org/7552028/diff/40001/chrome/common/extensions/ex...
chrome/common/extensions/extension_set.cc:89: i->second->path(),
i->second->default_locale())));
I don't like the way it's wrapped - it's hard to track who belongs where...

[adriansc, 2011-08-19 21:48:08]

http://codereview.chromium.org/7552028/diff/40001/chrome/browser/extensions/u...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/40001/chrome/browser/extensions/u...
chrome/browser/extensions/user_script_master.cc:284: const UserScriptList&
user_scripts) {
On 2011/08/19 21:19:15, Nebojša Ćirić wrote:
> //... called on the IO thread
> 
> Can this be const all the way (if extensions() is mutable then nothing)?
> const ExtensionInfoMap* const info_map

Done.

http://codereview.chromium.org/7552028/diff/40001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_set.cc (right):

http://codereview.chromium.org/7552028/diff/40001/chrome/common/extensions/ex...
chrome/common/extensions/extension_set.cc:89: i->second->path(),
i->second->default_locale())));
On 2011/08/19 21:19:15, Nebojša Ćirić wrote:
> I don't like the way it's wrapped - it's hard to track who belongs where...

I changed the nested instantiations into separate statements with results in
local variables.

[Nebojša Ćirić, 2011-08-19 22:02:25]

LGTM

But ping Mihai to take another look.

http://codereview.chromium.org/7552028/diff/47001/chrome/browser/extensions/u...
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/7552028/diff/47001/chrome/browser/extensions/u...
chrome/browser/extensions/user_script_master.cc:282: void
UserScriptMaster::ScriptReloader::GatherExtensionsInfo(
Maybe:

CollectExtensionsInfo, GetExtensionsInfo?

[Mihai Parparita -not on Chrome, 2011-08-19 22:10:26]

LGTM

[Mihai Parparita -not on Chrome, 2011-08-22 18:02:06]

LGTM for the latest version (which populates ExtensionsInfo in UserScriptMaster
when extensions are loaded, and passes a copy to ScriptReloader).

[Nebojša Ćirić, 2011-08-22 18:06:12]

LGTM

On 2011/08/22 18:02:06, Mihai Parparita wrote:
> LGTM for the latest version (which populates ExtensionsInfo in
UserScriptMaster
> when extensions are loaded, and passes a copy to ScriptReloader).

[commit-bot: I haz the power, 2011-08-23 21:51:06]

Change committed as 97927