Host process i18n and Linux implementation.

Host process i18n and Linux implementation.

BUG=None
TEST=Manual

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

[Jamie, 2011-08-12 01:19:45]

This CL allows the host dialogs to be localized, and implements this for Linux.

Wez, please take a look at the NPAPI changes.
Lambros, please take a look at the rest of it.

[Lambros, 2011-08-12 17:20:35]

Everything else looks OK, but I'd prefer to see a more robust scheme for dealing
with multiple connected users.

http://codereview.chromium.org/7635012/diff/1/remoting/host/ui_strings.h
File remoting/host/ui_strings.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/ui_strings.h#newc...
remoting/host/ui_strings.h:31: extern std::string disconnectMessage;
This won't work for multiple plugin instances (when multiple users are connected
simultaneously).  The username somehow needs to be stored per instance.

[Jamie, 2011-08-12 23:09:53]

http://codereview.chromium.org/7635012/diff/1/remoting/host/ui_strings.h
File remoting/host/ui_strings.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/ui_strings.h#newc...
remoting/host/ui_strings.h:31: extern std::string disconnectMessage;
On 2011/08/12 17:20:36, Lambros wrote:
> This won't work for multiple plugin instances (when multiple users are
connected
> simultaneously).  The username somehow needs to be stored per instance.

Good point. Please take a look at the new approach.

[Lambros, 2011-08-13 00:32:13]

Non-plugin parts LGTM, provided you undo the timeout change :)

http://codereview.chromium.org/7635012/diff/8001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7635012/diff/8001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:18: static const int
kContinueWindowTimeoutMs = 10 * 1 * 1000;
Don't forget to undo this :)

http://codereview.chromium.org/7635012/diff/8001/remoting/host/ui_strings.h
File remoting/host/ui_strings.h (right):

http://codereview.chromium.org/7635012/diff/8001/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:21: struct UIStrings {
Optional nit: I wonder if this should be named "UiStrings"?  Google C++ style
guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...)
gives "UrlTable" as an example class name, which suggests that acronyms should
only have first letter upper-case.

[Jamie, 2011-08-13 00:56:19]

http://codereview.chromium.org/7635012/diff/8001/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7635012/diff/8001/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:18: static const int
kContinueWindowTimeoutMs = 10 * 1 * 1000;
On 2011/08/13 00:32:13, Lambros wrote:
> Don't forget to undo this :)

Oh, if you insist...

http://codereview.chromium.org/7635012/diff/8001/remoting/host/ui_strings.h
File remoting/host/ui_strings.h (right):

http://codereview.chromium.org/7635012/diff/8001/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:21: struct UIStrings {
On 2011/08/13 00:32:13, Lambros wrote:
> Optional nit: I wonder if this should be named "UiStrings"?  Google C++ style
> guide
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...)
> gives "UrlTable" as an example class name, which suggests that acronyms should
> only have first letter upper-case.

Done.

[Sergey Ulanov, 2011-08-13 01:11:36]

drive-by nits

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
File remoting/host/disconnect_window_linux.cc (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
remoting/host/disconnect_window_linux.cc:59: std::string buttonLabel =
ui_strings->disconnectButtonText + " (" +
Whole string must be localizible, and not some parts of it. In some languages it
may be necessary to use some symbols different from parentheses, etc. A better
solution would be to have disconnectButtonText set to something like "Disconnect
(%s)" and then substitute the shortcut here. Or just localize the whole string

http://codereview.chromium.org/7635012/diff/9019/remoting/host/plugin/host_pl...
File remoting/host/plugin/host_plugin_utils.h (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_utils.h:35: class scoped_NPObject {
ScopedNPObject?

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h
File remoting/host/ui_strings.h (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:5: #ifndef REMOTING_UI_STRINGS_H_
REMOTING_HOST_UI_STRINGS_H_

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:26: enum Direction { RTL, LTR };
nit: each value on a separate line.

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:30: std::string productName;
product_name? same for the other methods.

[Jamie, 2011-08-13 01:34:03]

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
File remoting/host/disconnect_window_linux.cc (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
remoting/host/disconnect_window_linux.cc:59: std::string buttonLabel =
ui_strings->disconnectButtonText + " (" +
On 2011/08/13 01:11:36, sergeyu wrote:
> Whole string must be localizible, and not some parts of it. In some languages
it
> may be necessary to use some symbols different from parentheses, etc. A better
> solution would be to have disconnectButtonText set to something like
"Disconnect
> (%s)" and then substitute the shortcut here. Or just localize the whole string

Unfortunately on Windows we need to remove the shortcut if we fail to register
it, so it has to be separate. Would it be better to have the parentheses
included in the localizable string?

http://codereview.chromium.org/7635012/diff/9019/remoting/host/plugin/host_pl...
File remoting/host/plugin/host_plugin_utils.h (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/plugin/host_pl...
remoting/host/plugin/host_plugin_utils.h:35: class scoped_NPObject {
On 2011/08/13 01:11:36, sergeyu wrote:
> ScopedNPObject?

Done.

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h
File remoting/host/ui_strings.h (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:5: #ifndef REMOTING_UI_STRINGS_H_
On 2011/08/13 01:11:36, sergeyu wrote:
> REMOTING_HOST_UI_STRINGS_H_

Done.

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:26: enum Direction { RTL, LTR };
On 2011/08/13 01:11:36, sergeyu wrote:
> nit: each value on a separate line.

I don't see anything about this in the style guide, and this is such a simple
enum (I almost used a bool) that I'd rather not waste vertical space.

http://codereview.chromium.org/7635012/diff/9019/remoting/host/ui_strings.h#n...
remoting/host/ui_strings.h:30: std::string productName;
On 2011/08/13 01:11:36, sergeyu wrote:
> product_name? same for the other methods.

Done.

[Wez, 2011-08-13 01:35:31]

Apologies for the delay in getting to this.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
File remoting/host/plugin/host_plugin_utils.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
remoting/host/plugin/host_plugin_utils.h:45: };
This should really be scoped_refNPObject, I think, since NPObjects are
reference-counted, i.e. more like scoped_refptr (see base/memory/ref_counted.h).

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:41: // attribute Function string
localize(string,...);
nit: Call this "localizeString"?

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:281: NULL,
&remoting::ui_strings::stopSharingButtonText);
Move these into a LocalizeStrings() function, rather than doing it in-line here.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:603: bool
HostNPScriptObject::Localize(const char* tag, const char* parameter,
nit: |parameter| isn't very descriptive; is there a better name?

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:612: plugin_, localize_func_.get(),
&args[0], parameter ? 2 : 1, &np_result);
nit: Maybe pre-calculate arg_count rather than use ? here?

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:614: LOG(ERROR) << "Localization
failed for " << tag;
nit: localize() failed?  This really means that the JS failed in some way,
right, rather than a localization error, which is handled below?

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.h:123: // unchanged and returns false on
failure.
Would it make more sense to do the substitution even on failure, so that the
result indicates whether the string was localized, but it'll at least display
the correct information, albeit in English(ish) if not?

[Sergey Ulanov, 2011-08-13 01:37:26]

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
File remoting/host/disconnect_window_linux.cc (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
remoting/host/disconnect_window_linux.cc:59: std::string buttonLabel =
ui_strings->disconnectButtonText + " (" +
On 2011/08/13 01:34:03, Jamie wrote:
> On 2011/08/13 01:11:36, sergeyu wrote:
> > Whole string must be localizible, and not some parts of it. In some
languages
> it
> > may be necessary to use some symbols different from parentheses, etc. A
better
> > solution would be to have disconnectButtonText set to something like
> "Disconnect
> > (%s)" and then substitute the shortcut here. Or just localize the whole
string
> 
> Unfortunately on Windows we need to remove the shortcut if we fail to register
> it, so it has to be separate. Would it be better to have the parentheses
> included in the localizable string?

Then I think the right solution in this case is to have two strings, one with
shortcut and one without.

[Jamie, 2011-08-13 02:20:15]

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
File remoting/host/plugin/host_plugin_utils.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
remoting/host/plugin/host_plugin_utils.h:45: };
On 2011/08/13 01:35:32, Wez wrote:
> This should really be scoped_refNPObject, I think, since NPObjects are
> reference-counted, i.e. more like scoped_refptr (see
base/memory/ref_counted.h).

I've taken Sergey's suggestion of calling it ScopedNPObject. The fact that
that's its reference-counted doesn't affect how this class is used, so there's
no reason to include it in the name.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:41: // attribute Function string
localize(string,...);
On 2011/08/13 01:35:32, Wez wrote:
> nit: Call this "localizeString"?

Done.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:281: NULL,
&remoting::ui_strings::stopSharingButtonText);
On 2011/08/13 01:35:32, Wez wrote:
> Move these into a LocalizeStrings() function, rather than doing it in-line
here.

Done.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:603: bool
HostNPScriptObject::Localize(const char* tag, const char* parameter,
On 2011/08/13 01:35:32, Wez wrote:
> nit: |parameter| isn't very descriptive; is there a better name?

Hmmm... Now that you mention it, that's probably the worst variable name I've
ever come up with :) I'll change it to |substitution|.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:612: plugin_, localize_func_.get(),
&args[0], parameter ? 2 : 1, &np_result);
On 2011/08/13 01:35:32, Wez wrote:
> nit: Maybe pre-calculate arg_count rather than use ? here?

Done.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.cc:614: LOG(ERROR) << "Localization
failed for " << tag;
On 2011/08/13 01:35:32, Wez wrote:
> nit: localize() failed?  This really means that the JS failed in some way,
> right, rather than a localization error, which is handled below?

I don't really know in what circumstances this might fail. As long as the error
logged is different to the one below, I think it's fine.

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_scrip...
remoting/host/plugin/host_script_object.h:123: // unchanged and returns false on
failure.
On 2011/08/13 01:35:32, Wez wrote:
> Would it make more sense to do the substitution even on failure, so that the
> result indicates whether the string was localized, but it'll at least display
> the correct information, albeit in English(ish) if not?

The strings have their English translations hard-coded so that
remoting_simple_host still works, so if the translations fails, that's what
you'll get.

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
File remoting/host/disconnect_window_linux.cc (right):

http://codereview.chromium.org/7635012/diff/9019/remoting/host/disconnect_win...
remoting/host/disconnect_window_linux.cc:59: std::string buttonLabel =
ui_strings->disconnectButtonText + " (" +
On 2011/08/13 01:37:26, sergeyu wrote:
> On 2011/08/13 01:34:03, Jamie wrote:
> > On 2011/08/13 01:11:36, sergeyu wrote:
> > > Whole string must be localizible, and not some parts of it. In some
> languages
> > it
> > > may be necessary to use some symbols different from parentheses, etc. A
> better
> > > solution would be to have disconnectButtonText set to something like
> > "Disconnect
> > > (%s)" and then substitute the shortcut here. Or just localize the whole
> string
> > 
> > Unfortunately on Windows we need to remove the shortcut if we fail to
register
> > it, so it has to be separate. Would it be better to have the parentheses
> > included in the localizable string?
> 
> Then I think the right solution in this case is to have two strings, one with
> shortcut and one without.

Done.

[Jamie, 2011-08-15 17:21:02]

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
File remoting/host/plugin/host_plugin_utils.h (right):

http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
remoting/host/plugin/host_plugin_utils.h:45: };
On 2011/08/13 02:20:15, Jamie wrote:
> On 2011/08/13 01:35:32, Wez wrote:
> > This should really be scoped_refNPObject, I think, since NPObjects are
> > reference-counted, i.e. more like scoped_refptr (see
> base/memory/ref_counted.h).
> 
> I've taken Sergey's suggestion of calling it ScopedNPObject. The fact that
> that's its reference-counted doesn't affect how this class is used, so there's
> no reason to include it in the name.

Actually, now I think about it, it does affect the semantics. Done.

[Wez, 2011-08-15 17:27:33]

On 2011/08/15 17:21:02, Jamie wrote:
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> File remoting/host/plugin/host_plugin_utils.h (right):
> 
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> remoting/host/plugin/host_plugin_utils.h:45: };
> On 2011/08/13 02:20:15, Jamie wrote:
> > On 2011/08/13 01:35:32, Wez wrote:
> > > This should really be scoped_refNPObject, I think, since NPObjects are
> > > reference-counted, i.e. more like scoped_refptr (see
> > base/memory/ref_counted.h).
> > 
> > I've taken Sergey's suggestion of calling it ScopedNPObject. The fact that
> > that's its reference-counted doesn't affect how this class is used, so
there's
> > no reason to include it in the name.
> 
> Actually, now I think about it, it does affect the semantics. Done.

It still has a different interface from scoped_refptr<>, and no comments
clarifying if/when references are taken/released.  You're also neither disabling
copy ctor & assignment operators, nor providing ones that take a reference to
the object.

[Jamie, 2011-08-15 17:39:21]

On 2011/08/15 17:27:33, Wez wrote:
> On 2011/08/15 17:21:02, Jamie wrote:
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > File remoting/host/plugin/host_plugin_utils.h (right):
> > 
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > remoting/host/plugin/host_plugin_utils.h:45: };
> > On 2011/08/13 02:20:15, Jamie wrote:
> > > On 2011/08/13 01:35:32, Wez wrote:
> > > > This should really be scoped_refNPObject, I think, since NPObjects are
> > > > reference-counted, i.e. more like scoped_refptr (see
> > > base/memory/ref_counted.h).
> > > 
> > > I've taken Sergey's suggestion of calling it ScopedNPObject. The fact that
> > > that's its reference-counted doesn't affect how this class is used, so
> there's
> > > no reason to include it in the name.
> > 
> > Actually, now I think about it, it does affect the semantics. Done.
> 
> It still has a different interface from scoped_refptr<>, and no comments
> clarifying if/when references are taken/released.  You're also neither
disabling
> copy ctor & assignment operators, nor providing ones that take a reference to
> the object.

There's no need for the interface to be the same as scoped_refptr. In fact,
since there's an implicit dependency on |g_npnetscape_funcs|, the semantics are
different anyway. Fair point about the copy and assign and lack of comments
though. I've added both.

[Wez, 2011-08-15 17:44:10]

On 2011/08/15 17:39:21, Jamie wrote:
> On 2011/08/15 17:27:33, Wez wrote:
> > On 2011/08/15 17:21:02, Jamie wrote:
> > >
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > > File remoting/host/plugin/host_plugin_utils.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > > remoting/host/plugin/host_plugin_utils.h:45: };
> > > On 2011/08/13 02:20:15, Jamie wrote:
> > > > On 2011/08/13 01:35:32, Wez wrote:
> > > > > This should really be scoped_refNPObject, I think, since NPObjects are
> > > > > reference-counted, i.e. more like scoped_refptr (see
> > > > base/memory/ref_counted.h).
> > > > 
> > > > I've taken Sergey's suggestion of calling it ScopedNPObject. The fact
that
> > > > that's its reference-counted doesn't affect how this class is used, so
> > there's
> > > > no reason to include it in the name.
> > > 
> > > Actually, now I think about it, it does affect the semantics. Done.
> > 
> > It still has a different interface from scoped_refptr<>, and no comments
> > clarifying if/when references are taken/released.  You're also neither
> disabling
> > copy ctor & assignment operators, nor providing ones that take a reference
to
> > the object.
> 
> There's no need for the interface to be the same as scoped_refptr. In fact,
> since there's an implicit dependency on |g_npnetscape_funcs|, the semantics
are
> different anyway. Fair point about the copy and assign and lack of comments
> though. I've added both.

No, there's no need, but it's not unreasonable for someone encountering this
class to expect it to behave similarly to scoped_refptr<>.

[Jamie, 2011-08-15 18:36:46]

On 2011/08/15 17:44:10, Wez wrote:
> On 2011/08/15 17:39:21, Jamie wrote:
> > On 2011/08/15 17:27:33, Wez wrote:
> > > On 2011/08/15 17:21:02, Jamie wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > > > File remoting/host/plugin/host_plugin_utils.h (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > > > remoting/host/plugin/host_plugin_utils.h:45: };
> > > > On 2011/08/13 02:20:15, Jamie wrote:
> > > > > On 2011/08/13 01:35:32, Wez wrote:
> > > > > > This should really be scoped_refNPObject, I think, since NPObjects
are
> > > > > > reference-counted, i.e. more like scoped_refptr (see
> > > > > base/memory/ref_counted.h).
> > > > > 
> > > > > I've taken Sergey's suggestion of calling it ScopedNPObject. The fact
> that
> > > > > that's its reference-counted doesn't affect how this class is used, so
> > > there's
> > > > > no reason to include it in the name.
> > > > 
> > > > Actually, now I think about it, it does affect the semantics. Done.
> > > 
> > > It still has a different interface from scoped_refptr<>, and no comments
> > > clarifying if/when references are taken/released.  You're also neither
> > disabling
> > > copy ctor & assignment operators, nor providing ones that take a reference
> to
> > > the object.
> > 
> > There's no need for the interface to be the same as scoped_refptr. In fact,
> > since there's an implicit dependency on |g_npnetscape_funcs|, the semantics
> are
> > different anyway. Fair point about the copy and assign and lack of comments
> > though. I've added both.
> 
> No, there's no need, but it's not unreasonable for someone encountering this
> class to expect it to behave similarly to scoped_refptr<>.

Done.

[Wez, 2011-08-15 19:17:53]

On 2011/08/15 18:36:46, Jamie wrote:
> On 2011/08/15 17:44:10, Wez wrote:
> > On 2011/08/15 17:39:21, Jamie wrote:
> > > On 2011/08/15 17:27:33, Wez wrote:
> > > > On 2011/08/15 17:21:02, Jamie wrote:
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > > > > File remoting/host/plugin/host_plugin_utils.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/7635012/diff/1/remoting/host/plugin/host_plugi...
> > > > > remoting/host/plugin/host_plugin_utils.h:45: };
> > > > > On 2011/08/13 02:20:15, Jamie wrote:
> > > > > > On 2011/08/13 01:35:32, Wez wrote:
> > > > > > > This should really be scoped_refNPObject, I think, since NPObjects
> are
> > > > > > > reference-counted, i.e. more like scoped_refptr (see
> > > > > > base/memory/ref_counted.h).
> > > > > > 
> > > > > > I've taken Sergey's suggestion of calling it ScopedNPObject. The
fact
> > that
> > > > > > that's its reference-counted doesn't affect how this class is used,
so
> > > > there's
> > > > > > no reason to include it in the name.
> > > > > 
> > > > > Actually, now I think about it, it does affect the semantics. Done.
> > > > 
> > > > It still has a different interface from scoped_refptr<>, and no comments
> > > > clarifying if/when references are taken/released.  You're also neither
> > > disabling
> > > > copy ctor & assignment operators, nor providing ones that take a
reference
> > to
> > > > the object.
> > > 
> > > There's no need for the interface to be the same as scoped_refptr. In
fact,
> > > since there's an implicit dependency on |g_npnetscape_funcs|, the
semantics
> > are
> > > different anyway. Fair point about the copy and assign and lack of
comments
> > > though. I've added both.
> > 
> > No, there's no need, but it's not unreasonable for someone encountering this
> > class to expect it to behave similarly to scoped_refptr<>.
> 
> Done.

LGTM

[commit-bot: I haz the power, 2011-08-16 05:34:34]

Can't apply patch for file remoting/host/plugin/host_script_object.cc.
While running patch -p1 --forward --force;
patching file remoting/host/plugin/host_script_object.cc
Hunk #1 FAILED at 16.
Hunk #2 succeeded at 40 (offset 1 line).
Hunk #3 succeeded at 53 (offset 1 line).
Hunk #4 succeeded at 82 with fuzz 1 (offset 1 line).
Hunk #5 succeeded at 135 (offset 11 lines).
Hunk #6 succeeded at 188 (offset 16 lines).
Hunk #7 succeeded at 209 (offset 16 lines).
Hunk #8 succeeded at 260 (offset 16 lines).
Hunk #9 succeeded at 282 (offset 16 lines).
Hunk #10 succeeded at 306 (offset 16 lines).
Hunk #11 succeeded at 347 (offset 16 lines).
Hunk #12 succeeded at 587 (offset 65 lines).
Hunk #13 succeeded at 625 (offset 65 lines).
Hunk #14 succeeded at 640 (offset 65 lines).
1 out of 14 hunks FAILED -- saving rejects to file
remoting/host/plugin/host_script_object.cc.rej

[Sergey Ulanov, 2011-08-30 17:36:51]

http://codereview.chromium.org/7635012/diff/16001/remoting/host/plugin/host_p...
File remoting/host/plugin/host_plugin_utils.h (right):

http://codereview.chromium.org/7635012/diff/16001/remoting/host/plugin/host_p...
remoting/host/plugin/host_plugin_utils.h:42: ScopedRefNPObject&
operator=(NPObject* object);
FYI: Operator overloading is prohibited by code style:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl...

"The assignment operator (operator=), in particular, is insidious and should be
avoided."

[Wez, 2011-08-30 17:47:09]

On 2011/08/30 17:36:51, sergeyu wrote:
>
http://codereview.chromium.org/7635012/diff/16001/remoting/host/plugin/host_p...
> File remoting/host/plugin/host_plugin_utils.h (right):
> 
>
http://codereview.chromium.org/7635012/diff/16001/remoting/host/plugin/host_p...
> remoting/host/plugin/host_plugin_utils.h:42: ScopedRefNPObject&
> operator=(NPObject* object);
> FYI: Operator overloading is prohibited by code style:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl...
> 
> "The assignment operator (operator=), in particular, is insidious and should
be
> avoided."

This was my suggestion; I wanted ScopedRefNPObject to present what little
functionality it has in the same way as scoped_refptr<> does.  Having a "scoped
ref" named class that doesn't behave like other "scoped ref" classes seems
likely to impact code readability.

[Sergey Ulanov, 2011-08-30 19:28:09]

On 2011/08/30 17:47:09, Wez wrote:
> This was my suggestion; I wanted ScopedRefNPObject to present what little
> functionality it has in the same way as scoped_refptr<> does.  Having a
"scoped
> ref" named class that doesn't behave like other "scoped ref" classes seems
> likely to impact code readability.

Hm, ok, makes sense.