Localize CSS files in content scripts (but don't localize JS files)....

Localize CSS files in content scripts (but don't localize JS files).
Add UserScriptSlave unittest.

BUG=39899
TEST=List css file in content_scripts section of the manifest. Refer to an image using url(chrome-extension://__MSG_@@extension_id_/image.png); within that css. @@extension_id message should be replaced with actual id of the extension.


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

[Nebojša Ćirić, 2010-04-02 23:44:45]

This CL is fixing my oversight with content scripts and i18n (I didn't notice
they are loaded in a different manner compared to other resources - so they are
not being localized).

It replaces i18n messages only in CSS files. JS files can use
chrome.i18n.getMessage anyways.

Added Matt because of the incognito stuff I touched.

I would like to get this in some time next week so it goes into official 5.0.

Thanks.

[Matt Perry, 2010-04-05 19:13:07]

LGTM with a couple small comments.

I'm not familiar with the i18n stuff, but here's a vague concern: is it possible
for the i18n message map to become stale? Say I have a tab open with some
content scripts injected, then I install a new extension which has some
localized CSS. Does that tab get the new extension's messages, or will only new
processes get them?

Also, any reason not to do string replacement on JS?

http://codereview.chromium.org/1585013/diff/24001/25008
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/1585013/diff/24001/25008#newcode82
chrome/renderer/render_thread.h:82: virtual bool is_incognito_process() const =
0;
virtual methods should be CamelCase style, since they are no longer cheap to
call.

Alternatively, you can just pass is_incognito_process to UserScriptSlave so he
doesn't have to call back to RenderThread.

http://codereview.chromium.org/1585013/diff/24001/25006
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/1585013/diff/24001/25006#newcode136
chrome/renderer/user_script_slave.cc:136: L10nMessagesMap* l10n_messages =
Maybe we should only bother doing this if the extension has css_scripts. That
would save a browser round-trip.

http://codereview.chromium.org/1585013/diff/24001/25005
File chrome/renderer/user_script_slave.h (right):

http://codereview.chromium.org/1585013/diff/24001/25005#newcode59
chrome/renderer/user_script_slave.h:59: RenderThreadBase* parent_;
nit: parent_ is a vague name. how about just render_thread_

[Nebojša Ćirić, 2010-04-05 22:11:52]

We don't replace __MSG_name__ placeholders in JS scripts since developers can
use chrome.i18n.getMessage instead (and we don't do that for i.e. popup.js so it
may be confusing to enable this for content scripts).

As for staleness, you are correct. I've changed the code to fetch on every
UpdateScript. Do you think that would help?

http://codereview.chromium.org/1585013/diff/24001/25008
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/1585013/diff/24001/25008#newcode82
chrome/renderer/render_thread.h:82: virtual bool is_incognito_process() const =
0;
I've decided to pass is_incognito as a parameter to UserScriptSlave.

On 2010/04/05 19:13:07, Matt Perry wrote:
> virtual methods should be CamelCase style, since they are no longer cheap to
> call.
> 
> Alternatively, you can just pass is_incognito_process to UserScriptSlave so he
> doesn't have to call back to RenderThread.

http://codereview.chromium.org/1585013/diff/24001/25006
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/1585013/diff/24001/25006#newcode136
chrome/renderer/user_script_slave.cc:136: L10nMessagesMap* l10n_messages =
We fetch now only if there is extension_id and there is at least one css script.

On 2010/04/05 19:13:07, Matt Perry wrote:
> Maybe we should only bother doing this if the extension has css_scripts. That
> would save a browser round-trip.

http://codereview.chromium.org/1585013/diff/24001/25005
File chrome/renderer/user_script_slave.h (right):

http://codereview.chromium.org/1585013/diff/24001/25005#newcode59
chrome/renderer/user_script_slave.h:59: RenderThreadBase* parent_;
On 2010/04/05 19:13:07, Matt Perry wrote:
> nit: parent_ is a vague name. how about just render_thread_

Done.

[Matt Perry, 2010-04-05 22:45:37]

LGTM

http://codereview.chromium.org/1585013/diff/11002/40006
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/1585013/diff/11002/40006#newcode108
chrome/renderer/user_script_slave.cc:108: std::string error;
nit: can move this inside the if statement

[rafaelw, 2010-04-06 23:20:34]

sorry for being late to this review. i see that you already committed it.

i'd like to understand why the approach here isn't localizing the script in the
browser process before notifying the renderers to update their scripts.

if they always have to turn around and do a sync ipc to the browser, it seems
like unnecessary complexity. additionally, i'm concerned it means that the
browser process is responding to multiple requests to localize the same content
and hitting the file each time.

[Nebojša Ćirić, 2010-04-07 00:04:06]

Sorry for committing it - it had to go in this week before beta cut on Friday.

Lets see if I can get your LGTM:

1. We already had the plumbing to get messages from browser to the renderer.
Reuse looked better then reorganizing UserScriptMaster.

2. I did this with assumption that script update notifications are not frequent
(i.e. on new extension install, reload), so it wouldn't impact browser too much.

3. I guess we would need to load those catalogs in the browser process either
way (maybe less if we were to localize before sending).

WDYT?

On 2010/04/06 23:20:34, rafaelw wrote:
> sorry for being late to this review. i see that you already committed it.
> 
> i'd like to understand why the approach here isn't localizing the script in
the
> browser process before notifying the renderers to update their scripts.
> 
> if they always have to turn around and do a sync ipc to the browser, it seems
> like unnecessary complexity. additionally, i'm concerned it means that the
> browser process is responding to multiple requests to localize the same
content
> and hitting the file each time.

[Matt Perry, 2010-04-07 00:11:06]

On 2010/04/07 00:04:06, Nebojša Ćirić wrote:
> 
> 2. I did this with assumption that script update notifications are not
frequent
> (i.e. on new extension install, reload), so it wouldn't impact browser too
much.

This part is not quite true. The script updates are sent to new renderers as
well, which means each new tab would be delayed by the extra N roundtrips to the
browser. I agree with Raf that it makes more sense to do the localization in the
browser before sending the update scripts message (though of course it is more
work).

> WDYT?
> 
> On 2010/04/06 23:20:34, rafaelw wrote:
> > sorry for being late to this review. i see that you already committed it.
> > 
> > i'd like to understand why the approach here isn't localizing the script in
> the
> > browser process before notifying the renderers to update their scripts.
> > 
> > if they always have to turn around and do a sync ipc to the browser, it
seems
> > like unnecessary complexity. additionally, i'm concerned it means that the
> > browser process is responding to multiple requests to localize the same
> content
> > and hitting the file each time.

[rafaelw1, 2010-04-07 00:12:51]

I'm less worried about actually performance impact and more about code
complexity. Both in terms of future generations understanding what's
going on, and also about the possibility that weird timing events
(which we have lots of) end up creating bugs.

Sorry, but I'd really prefer that we do the localization before the
new scripts get broadcast and avoid round trips back to the browser
for all listening renderers.

R

On Tue, Apr 6, 2010 at 5:04 PM,  <cira@chromium.org> wrote:
> Sorry for committing it - it had to go in this week before beta cut on
> Friday.
>
> Lets see if I can get your LGTM:
>
> 1. We already had the plumbing to get messages from browser to the renderer.
> Reuse looked better then reorganizing UserScriptMaster.
>
> 2. I did this with assumption that script update notifications are not
> frequent
> (i.e. on new extension install, reload), so it wouldn't impact browser too
> much.
>
> 3. I guess we would need to load those catalogs in the browser process
> either
> way (maybe less if we were to localize before sending).
>
> WDYT?
>
> On 2010/04/06 23:20:34, rafaelw wrote:
>>
>> sorry for being late to this review. i see that you already committed it.
>
>> i'd like to understand why the approach here isn't localizing the script
>> in
>
> the
>>
>> browser process before notifying the renderers to update their scripts.
>
>> if they always have to turn around and do a sync ipc to the browser, it
>> seems
>> like unnecessary complexity. additionally, i'm concerned it means that the
>> browser process is responding to multiple requests to localize the same
>
> content
>>
>> and hitting the file each time.
>
>
>
> http://codereview.chromium.org/1585013
>

[Nebojša Ćirić, 2010-04-07 00:13:01]

Ok, I'll revert then. :(

On 2010/04/07 00:11:06, Matt Perry wrote:
> On 2010/04/07 00:04:06, Nebojša Ćirić wrote:
> > 
> > 2. I did this with assumption that script update notifications are not
> frequent
> > (i.e. on new extension install, reload), so it wouldn't impact browser too
> much.
> 
> This part is not quite true. The script updates are sent to new renderers as
> well, which means each new tab would be delayed by the extra N roundtrips to
the
> browser. I agree with Raf that it makes more sense to do the localization in
the
> browser before sending the update scripts message (though of course it is more
> work).
> 
> > WDYT?
> > 
> > On 2010/04/06 23:20:34, rafaelw wrote:
> > > sorry for being late to this review. i see that you already committed it.
> > > 
> > > i'd like to understand why the approach here isn't localizing the script
in
> > the
> > > browser process before notifying the renderers to update their scripts.
> > > 
> > > if they always have to turn around and do a sync ipc to the browser, it
> seems
> > > like unnecessary complexity. additionally, i'm concerned it means that the
> > > browser process is responding to multiple requests to localize the same
> > content
> > > and hitting the file each time.

[Aaron Boodman, 2010-04-07 00:13:32]

I thought the plan of record was to do localization at unpack time and to save
the localized versions.

[Nebojša Ćirić, 2010-04-07 00:21:52]

I think we said that couldn't be done because of the unpacked extensions (not
installed). Where would we save originals?

On 2010/04/07 00:13:32, Aaron Boodman wrote:
> I thought the plan of record was to do localization at unpack time and to save
> the localized versions.