New CPP docs for mouse lock and fullscreen. The docs are identical to those found in the C IDL wi...

New CPP docs for mouse lock and fullscreen. The docs are identical to those found in the C IDL with the exception of class descriptions and a CPP-specific types

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

[dmichael (off chromium), 2011-12-21 22:41:41]

lgtm

[yzshen1, 2011-12-21 23:04:14]

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h
File ppapi/cpp/fullscreen.h (right):

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h#newcode26
ppapi/cpp/fullscreen.h:26: /// Destructor.
Comment like this (as well as the one for constructor) is of little use. I would
suggest to remove them.

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h#newcode61
ppapi/cpp/fullscreen.h:61: /// @return <code>false</code> on success or
<code>false</code> on
*true* on success

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/mouse_lock.h
File ppapi/cpp/mouse_lock.h (right):

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/mouse_lock.h#newcode24
ppapi/cpp/mouse_lock.h:24: /// You would typically implement this class using
inheritance on your instance
nit: I think we usually use 'implement interface' instead of 'implement class'.
MouseLock is not a pure interface in this case,  so I prefer something like 'You
would typically use this class by inheritance on your instance or by
composition'.

(But I am not a native speaker. :))

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/mouse_lock.h#newcode57
ppapi/cpp/mouse_lock.h:57: /// Destructor.
Comment like this (as well as the one for constructor) is of little use.

[jond, 2012-01-03 17:48:28]

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h
File ppapi/cpp/fullscreen.h (right):

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h#newcode26
ppapi/cpp/fullscreen.h:26: /// Destructor.
Yeah, I realize that, but I think for sake of having some documentation for
everything, it looks better to have a string than not. I can change these to
"empty destructor" or something else if you have a suggestion.

On 2011/12/21 23:04:14, yzshen1 wrote:
> Comment like this (as well as the one for constructor) is of little use. I
would
> suggest to remove them.

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h#newcode61
ppapi/cpp/fullscreen.h:61: /// @return <code>false</code> on success or
<code>false</code> on
On 2011/12/21 23:04:14, yzshen1 wrote:
> *true* on success

Done.

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/mouse_lock.h
File ppapi/cpp/mouse_lock.h (right):

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/mouse_lock.h#newcode24
ppapi/cpp/mouse_lock.h:24: /// You would typically implement this class using
inheritance on your instance
On 2011/12/21 23:04:14, yzshen1 wrote:
> nit: I think we usually use 'implement interface' instead of 'implement
class'.
> MouseLock is not a pure interface in this case,  so I prefer something like
'You
> would typically use this class by inheritance on your instance or by
> composition'.
> 
> (But I am not a native speaker. :))

Done.

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/mouse_lock.h#newcode57
ppapi/cpp/mouse_lock.h:57: /// Destructor.
On 2011/12/21 23:04:14, yzshen1 wrote:
> Comment like this (as well as the one for constructor) is of little use.
> 

Done.

[yzshen1, 2012-01-03 18:06:25]

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h
File ppapi/cpp/fullscreen.h (right):

http://codereview.chromium.org/9018032/diff/1/ppapi/cpp/fullscreen.h#newcode26
ppapi/cpp/fullscreen.h:26: /// Destructor.
IMHO, I think it is the better to remove it.
The reason to just have some string for everything doesn't seem compelling.
On 2012/01/03 17:48:28, jond wrote:
> Yeah, I realize that, but I think for sake of having some documentation for
> everything, it looks better to have a string than not. I can change these to
> "empty destructor" or something else if you have a suggestion.
> 
> On 2011/12/21 23:04:14, yzshen1 wrote:
> > Comment like this (as well as the one for constructor) is of little use. I
> would
> > suggest to remove them.
>

[commit-bot: I haz the power, 2012-01-04 16:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jond@google.com/9018032/2001

[dmichael (off chromium), 2012-01-04 17:03:35]

Just noticed you have Lint errors on a couple of lines for missing a space after
"//". Please fix those.

Also, you should not commit when a reviewer still has concerns. Try to find
somebody to break the tie if you disagree.

In this case, I think "Destructor" is okay if only because we already have it in
a number of other classes in ppapi/cpp. You'll have to show me the difference in
the Doxygen output some time to see how much it matters. I tend to agree with
yzshen (and brettw) here, that it's generally a useless comment. We can look at
it and maybe remove them all at the same time if they don't add anything to the
doxygen output.

[yzshen1, 2012-01-04 18:03:05]

On 2012/01/04 17:03:35, dmichael wrote:
> Just noticed you have Lint errors on a couple of lines for missing a space
after
> "//". Please fix those.
> 
> Also, you should not commit when a reviewer still has concerns. Try to find
> somebody to break the tie if you disagree.
> 
> In this case, I think "Destructor" is okay if only because we already have it
in
> a number of other classes in ppapi/cpp. You'll have to show me the difference
in
> the Doxygen output some time to see how much it matters. I tend to agree with
> yzshen (and brettw) here, that it's generally a useless comment. We can look
at
> it and maybe remove them all at the same time if they don't add anything to
the
> doxygen output.

Thanks for your input, Dave!
As long as you think it is okay, I am fine to leave it as it is.

[yzshen1, 2012-01-04 18:03:25]

lgtm

[commit-bot: I haz the power, 2012-01-04 18:12:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jond@google.com/9018032/2001

[commit-bot: I haz the power, 2012-01-04 19:41:12]

Change committed as 116358

[dmichael (off chromium), 2012-01-05 20:53:57]

This one's already committed. Please do the lint fix in a new CL.