Mojo C++ bindings: better log message for serialization warnings.

Mojo C++ bindings: better log message for serialization warnings.

BUG=324170
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291058

[yzshen1, 2014-08-20 23:44:05]

Hi, Hans.

Would you please take a look? Thanks!

[hansmuller, 2014-08-21 00:26:40]

LGTM

I had a few suggestions about how the error messages could be beefed up a
little.

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
mojo/public/cpp/bindings/lib/array_serialization.h:140: "null pointer in array
of non-nullable pointers");
You could specify the element index in the error message. It would help the
reader if it were possible to name the field or parameter that the array came
from, but that's going to be relatively difficult.

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
File mojo/public/cpp/bindings/lib/validation_errors.h (right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
mojo/public/cpp/bindings/lib/validation_errors.h:112: <<
ValidationErrorToString(error) << " at the receiving side. (" \
I'm sure this is fine as is, but maybe you should move the period to after the
parenthesized expression.

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:66:
"null {{param.name}} argument");
It might be useful to include the name of the interface in the error message.
Likewise for the handle case below.

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:29:
"null {{pf.field.name}} field");
It might be useful include the name of the struct here as well. Likewise for the
handle case below.

[yzshen1, 2014-08-21 00:32:31]

These are all great ideas. I will upload a new patch this evening. (I need
to catch a carpool now.) Thanks!


On Wed, Aug 20, 2014 at 5:26 PM, <hansmuller@chromium.org> wrote:

> LGTM
>
> I had a few suggestions about how the error messages could be beefed up a
> little.
>
>
>
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/cpp/bindings/lib/array_serialization.h
> File mojo/public/cpp/bindings/lib/array_serialization.h (right):
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/cpp/bindings/lib/array_serialization.h#newcode140
> mojo/public/cpp/bindings/lib/array_serialization.h:140: "null pointer in
>
> array of non-nullable pointers");
> You could specify the element index in the error message. It would help
> the reader if it were possible to name the field or parameter that the
> array came from, but that's going to be relatively difficult.
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/cpp/bindings/lib/validation_errors.h
> File mojo/public/cpp/bindings/lib/validation_errors.h (right):
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/cpp/bindings/lib/validation_errors.h#newcode112
> mojo/public/cpp/bindings/lib/validation_errors.h:112: <<
>
> ValidationErrorToString(error) << " at the receiving side. (" \
> I'm sure this is fine as is, but maybe you should move the period to
> after the parenthesized expression.
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
> File
> mojo/public/tools/bindings/generators/cpp_templates/
> interface_definition.tmpl
> (right):
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/tools/bindings/generators/cpp_templates/interface_definition.tmpl#
> newcode66
> mojo/public/tools/bindings/generators/cpp_templates/
> interface_definition.tmpl:66:
> "null {{param.name}} argument");
> It might be useful to include the name of the interface in the error
> message. Likewise for the handle case below.
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl
> File
> mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl
> (right):
>
> https://codereview.chromium.org/473793004/diff/1/mojo/
> public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl#newcode29
> mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_
> definition.tmpl:29:
> "null {{pf.field.name}} field");
> It might be useful include the name of the struct here as well. Likewise
> for the handle case below.
>
> https://codereview.chromium.org/473793004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-08-21 05:57:49]

Thanks, Hans!

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
mojo/public/cpp/bindings/lib/array_serialization.h:140: "null pointer in array
of non-nullable pointers");
On 2014/08/21 00:26:40, hansmuller wrote:
> You could specify the element index in the error message. It would help the
> reader if it were possible to name the field or parameter that the array came
> from, but that's going to be relatively difficult.

Done.

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
File mojo/public/cpp/bindings/lib/validation_errors.h (right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/cpp/bindings/lib...
mojo/public/cpp/bindings/lib/validation_errors.h:112: <<
ValidationErrorToString(error) << " at the receiving side. (" \
On 2014/08/21 00:26:40, hansmuller wrote:
> I'm sure this is fine as is, but maybe you should move the period to after the
> parenthesized expression.

Done.

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:66:
"null {{param.name}} argument");
On 2014/08/21 00:26:40, hansmuller wrote:
> It might be useful to include the name of the interface in the error message.
> Likewise for the handle case below.

Done.

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl
(right):

https://codereview.chromium.org/473793004/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/cpp_templates/struct_serialization_definition.tmpl:29:
"null {{pf.field.name}} field");
On 2014/08/21 00:26:40, hansmuller wrote:
> It might be useful include the name of the struct here as well. Likewise for
the
> handle case below.

Done.

[yzshen1, 2014-08-21 05:57:56]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-08-21 05:58:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/473793004/40001

[yzshen1, 2014-08-21 07:20:54]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-08-21 07:21:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/473793004/60001

[commit-bot: I haz the power, 2014-08-21 13:51:37]

Committed patchset #4 (60001) as 291058