Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7792011:
Run LocalizeStrings() on plugin thread. (Closed)
|
Created:
9 years, 3 months ago by Lambros Modified:
9 years, 3 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRun LocalizeStrings() on plugin thread.
BUG=94135
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98831
Patch Set 1 #Patch Set 2 : Add DCHECK for running LocalizeStrings() on correct thread. #
Total comments: 12
Patch Set 3 : Fixes from review. #Patch Set 4 : Remove attribute and add localize() method. #
Total comments: 4
Patch Set 5 : Pass localize_func as parameter. #
Total comments: 10
Patch Set 6 : Fix various nits. #
Total comments: 3
Messages
Total messages: 23 (0 generated)
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:652: DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread()); Other similar checks in the same file use CHECK_EQ, so is DCHECK right here, or would we be better off with CHECK? http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:654: base::AutoLock auto_lock(ui_strings_lock_); I worry slightly about holding this lock while we're calling into JS. Might it be cleaner to keep the temporary ui_strings local and assign it to the member variable after localizing? That way, only the assignment needs to be mutexed.
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:652: DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread()); On 2011/08/29 20:59:22, Jamie wrote: > Other similar checks in the same file use CHECK_EQ, so is DCHECK right here, or > would we be better off with CHECK? Removed. http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:654: base::AutoLock auto_lock(ui_strings_lock_); On 2011/08/29 20:59:22, Jamie wrote: > I worry slightly about holding this lock while we're calling into JS. Might it > be cleaner to keep the temporary ui_strings local and assign it to the member > variable after localizing? That way, only the assignment needs to be mutexed. Done.
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:279: LocalizeStrings(); Can we call this from Connect()? JavaScript may not expect localization callback to be called when it sets this property. http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:652: DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread()); On 2011/08/29 20:59:22, Jamie wrote: > Other similar checks in the same file use CHECK_EQ, so is DCHECK right here, or > would we be better off with CHECK? IMO DCHECK is fine here. We should change this code to use DCHECK when checking thread in other places too. http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:654: base::AutoLock auto_lock(ui_strings_lock_); On 2011/08/29 20:59:22, Jamie wrote: > I worry slightly about holding this lock while we're calling into JS. Might it > be cleaner to keep the temporary ui_strings local and assign it to the member > variable after localizing? That way, only the assignment needs to be mutexed. +1. If javascript sets the localization callback it would deadlock this code.
LGTM
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:279: LocalizeStrings(); On 2011/08/29 21:22:05, sergeyu wrote: > Can we call this from Connect()? JavaScript may not expect localization callback > to be called when it sets this property. I don't think that buys us anything, and I prefer to have things localized as soon as they possibly can be so that it's out of the way and we don't need to think about it any more.
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:279: LocalizeStrings(); On 2011/08/29 21:50:46, Jamie wrote: > On 2011/08/29 21:22:05, sergeyu wrote: > > Can we call this from Connect()? JavaScript may not expect localization > callback > > to be called when it sets this property. > > I don't think that buys us anything, and I prefer to have things localized as > soon as they possibly can be so that it's out of the way and we don't need to > think about it any more. What bothers me is that javascript interface looks really weird with this change: JS code just sets property localizeString to a method, and in result this method is called before property is set. Maybe we should replace this property with localize() method, and pass the pointer to the localization function to that method?
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:652: DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread()); On 2011/08/29 21:21:42, Lambros wrote: > On 2011/08/29 20:59:22, Jamie wrote: > > Other similar checks in the same file use CHECK_EQ, so is DCHECK right here, > or > > would we be better off with CHECK? > Removed. Please add it back. We do need this DCHECK to ensure that we localize on the right thread (we wouldn't have this bug if we had a DCHECK here in the first place).
http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:279: LocalizeStrings(); On 2011/08/29 22:28:07, sergeyu wrote: > On 2011/08/29 21:50:46, Jamie wrote: > > On 2011/08/29 21:22:05, sergeyu wrote: > > > Can we call this from Connect()? JavaScript may not expect localization > > callback > > > to be called when it sets this property. > > > > I don't think that buys us anything, and I prefer to have things localized as > > soon as they possibly can be so that it's out of the way and we don't need to > > think about it any more. > > What bothers me is that javascript interface looks really weird with this > change: > JS code just sets property localizeString to a method, and in result this method > is called before property is set. > Maybe we should replace this property with localize() method, and pass the > pointer to the localization function to that method? Done. http://codereview.chromium.org/7792011/diff/2001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:652: DCHECK(plugin_message_loop_proxy_->BelongsToCurrentThread()); On 2011/08/29 22:30:27, sergeyu wrote: > On 2011/08/29 21:21:42, Lambros wrote: > > On 2011/08/29 20:59:22, Jamie wrote: > > > Other similar checks in the same file use CHECK_EQ, so is DCHECK right here, > > or > > > would we be better off with CHECK? > > Removed. > > Please add it back. We do need this DCHECK to ensure that we localize on the > right thread (we wouldn't have this bug if we had a DCHECK here in the first > place). Done.
LGTM when the comments are addressed. http://codereview.chromium.org/7792011/diff/7001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/7001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:44: // attribute Function string localizeString(string,...); Please update this comment http://codereview.chromium.org/7792011/diff/7001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:509: localize_func_ = NPVARIANT_TO_OBJECT(args[0]); localize_func_ doesn't need to be member anymore. You can pass it as a parameter to LocalizeString().
PTAL - I've added another constructor to ScopedRefNPObject. http://codereview.chromium.org/7792011/diff/7001/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/7001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:44: // attribute Function string localizeString(string,...); On 2011/08/29 23:43:32, sergeyu wrote: > Please update this comment Done. http://codereview.chromium.org/7792011/diff/7001/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:509: localize_func_ = NPVARIANT_TO_OBJECT(args[0]); On 2011/08/29 23:43:32, sergeyu wrote: > localize_func_ doesn't need to be member anymore. You can pass it as a parameter > to LocalizeString(). Done.
http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... File remoting/host/plugin/host_plugin_utils.cc (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.cc:50: When I write objects where the ctor and operator= semantics are basically the same, I like to write one using the other, ie: ScopedRefNPObject(NPObject* object) : object_(NULL) { *this = object; } It keeps them in sync and is generally less typing. http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.h (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.h:141: void LocalizeStrings(ScopedRefNPObject *localize_func); Not a big deal, but why not a const reference?
http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... File remoting/host/plugin/host_plugin_utils.cc (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.cc:45: ScopedRefNPObject::ScopedRefNPObject(NPObject* object) : object_(object) { nit: move member initialization to the next line. http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.cc:50: On 2011/08/30 00:43:34, Jamie wrote: > When I write objects where the ctor and operator= semantics are basically the > same, I like to write one using the other, ie: > > ScopedRefNPObject(NPObject* object) : object_(NULL) { > *this = object; > } > > It keeps them in sync and is generally less typing. I suppose this will not work because of DISALLOW_COPY_AND_ASSIGN(). http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.h (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.h:141: void LocalizeStrings(ScopedRefNPObject *localize_func); Here and in other places: star is in a wrong place. Does it need to be ScopedRefNPObject? Why can't you just pass NPObject*?
http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... File remoting/host/plugin/host_plugin_utils.cc (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.cc:50: On 2011/08/30 00:53:08, sergeyu wrote: > On 2011/08/30 00:43:34, Jamie wrote: > > When I write objects where the ctor and operator= semantics are basically the > > same, I like to write one using the other, ie: > > > > ScopedRefNPObject(NPObject* object) : object_(NULL) { > > *this = object; > > } > > > > It keeps them in sync and is generally less typing. > > I suppose this will not work because of DISALLOW_COPY_AND_ASSIGN(). It should work because the parameter is an NPObject*, not a const ScopedRefNPObject& http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.h (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.h:141: void LocalizeStrings(ScopedRefNPObject *localize_func); On 2011/08/30 00:53:08, sergeyu wrote: > Here and in other places: star is in a wrong place. > Does it need to be ScopedRefNPObject? Why can't you just pass NPObject*? That's a good point. With hindsight, NPObject* is the obvious choice.
http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... File remoting/host/plugin/host_plugin_utils.cc (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.cc:45: ScopedRefNPObject::ScopedRefNPObject(NPObject* object) : object_(object) { On 2011/08/30 00:53:08, sergeyu wrote: > nit: move member initialization to the next line. Done. http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.cc:50: On 2011/08/30 00:57:55, Jamie wrote: > On 2011/08/30 00:53:08, sergeyu wrote: > > On 2011/08/30 00:43:34, Jamie wrote: > > > When I write objects where the ctor and operator= semantics are basically > the > > > same, I like to write one using the other, ie: > > > > > > ScopedRefNPObject(NPObject* object) : object_(NULL) { > > > *this = object; > > > } > > > > > > It keeps them in sync and is generally less typing. > > > > I suppose this will not work because of DISALLOW_COPY_AND_ASSIGN(). > > It should work because the parameter is an NPObject*, not a const > ScopedRefNPObject& Done. http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.h (right): http://codereview.chromium.org/7792011/diff/7004/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.h:141: void LocalizeStrings(ScopedRefNPObject *localize_func); On 2011/08/30 00:57:55, Jamie wrote: > On 2011/08/30 00:53:08, sergeyu wrote: > > Here and in other places: star is in a wrong place. > > Does it need to be ScopedRefNPObject? Why can't you just pass NPObject*? > > That's a good point. With hindsight, NPObject* is the obvious choice. Done.
LGTM http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_pl... File remoting/host/plugin/host_plugin_utils.h (right): http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_pl... remoting/host/plugin/host_plugin_utils.h:38: explicit ScopedRefNPObject(NPObject* object); Do we still need this constructor?
On 2011/08/30 02:30:16, sergeyu wrote: > LGTM > > http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_pl... > File remoting/host/plugin/host_plugin_utils.h (right): > > http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_pl... > remoting/host/plugin/host_plugin_utils.h:38: explicit > ScopedRefNPObject(NPObject* object); > Do we still need this constructor? I think so - see line 509 of host_script_object.cc.
http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:509: ScopedRefNPObject localize_func(NPVARIANT_TO_OBJECT(args[0])); Why does this need to be ScopedRefNPObject? I think the caller owns the arguments, we shouldn't free them here, no?
On 2011/08/30 16:39:26, sergeyu wrote: > http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_sc... > File remoting/host/plugin/host_script_object.cc (right): > > http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_sc... > remoting/host/plugin/host_script_object.cc:509: ScopedRefNPObject > localize_func(NPVARIANT_TO_OBJECT(args[0])); > Why does this need to be ScopedRefNPObject? I think the caller owns the > arguments, we shouldn't free them here, no? Are you saying we could just use the NPVARIANT_TO_OBJECT(args[0]) directly, without bothering to call g_npnetscape_funcs->retainobject/releaseobject() ? So there'd be no need at all for the ScopedRefNPObject class?
Change committed as 98831
http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_sc... File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_sc... remoting/host/plugin/host_script_object.cc:509: ScopedRefNPObject localize_func(NPVARIANT_TO_OBJECT(args[0])); On 2011/08/30 16:39:26, sergeyu wrote: > Why does this need to be ScopedRefNPObject? I think the caller owns the > arguments, we shouldn't free them here, no? Yes, this looks like it will cause a double-free when control returns to the caller.
On Tue, Aug 30, 2011 at 10:05 AM, <wez@chromium.org> wrote: > > http://codereview.chromium.**org/7792011/diff/5005/** > remoting/host/plugin/host_**script_object.cc<http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_script_object.cc> > File remoting/host/plugin/host_**script_object.cc (right): > > http://codereview.chromium.**org/7792011/diff/5005/** > remoting/host/plugin/host_**script_object.cc#newcode509<http://codereview.chromium.org/7792011/diff/5005/remoting/host/plugin/host_script_object.cc#newcode509> > remoting/host/plugin/host_**script_object.cc:509: ScopedRefNPObject > localize_func(NPVARIANT_TO_**OBJECT(args[0])); > On 2011/08/30 16:39:26, sergeyu wrote: > >> Why does this need to be ScopedRefNPObject? I think the caller owns >> > the > >> arguments, we shouldn't free them here, no? >> > > Yes, this looks like it will cause a double-free when control returns to > the caller. > > > http://codereview.chromium.**org/7792011/<http://codereview.chromium.org/7792... > I don't think so. The ctor addRefs, and the dtor releases. So, using ScopedRefNPObject is unnecessary at worst. Coming to a CL near you :)
On 2011/08/30 17:08:16, Lambros wrote: > On Tue, Aug 30, 2011 at 10:05 AM, <mailto:wez@chromium.org> wrote: > > Yes, this looks like it will cause a double-free when control returns to > > the caller. > > I don't think so. The ctor addRefs, and the dtor releases. So, using > ScopedRefNPObject is unnecessary at worst. Coming to a CL near you :) You're quite right. Panic over. ;) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
