Mojo C++ bindigns: fix generated namespace when variant is specified.

Mojo C++ bindigns: fix generated namespace when variant is specified.

This fixed the issue that enum generated code didn't work when variant is
specified.
This is a quick fix, we need more fundermental improvement for variant handling
code.

BUG=595791
Committed: https://crrev.com/e56136366dd27d1764c01203565447b53197733f
Cr-Commit-Position: refs/heads/master@{#381760}

[yzshen1, 2016-03-17 17:23:00]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org, sammc@chromium.org

[yzshen1, 2016-03-17 17:23:01]

Hi, Ken and Sam.

Would you please take a look? Thanks!

[Ken Rockot(use gerrit already), 2016-03-17 17:24:33]

Missing the mojom file? :)

[yzshen1, 2016-03-17 17:28:50]

On 2016/03/17 17:24:33, Ken Rockot wrote:
> Missing the mojom file? :)

Just uploaded. Thanks! :)

[Ken Rockot(use gerrit already), 2016-03-17 17:33:23]

lgtm

[yzshen1, 2016-03-17 17:33:54]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-03-17 17:34:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811853004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811853004/20001

[Sam McNally, 2016-03-17 17:37:58]

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:85: if
kind.imported_from:
We'll want to add the variant here too, but it can wait until variant imports
work.

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:88: if
hasattr(kind, "module"):
I don't think it makes sense to add the variant if we aren't adding a namespace
because the kind doesn't have a module.

[yzshen1, 2016-03-17 17:46:36]

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:85: if
kind.imported_from:
On 2016/03/17 17:37:58, Sam McNally wrote:
> We'll want to add the variant here too, but it can wait until variant imports
> work.

Right. I know that part is incorrect currently. To fix that more fundamental
changes are necessary: we either need to assume all imported kinds are using the
same variant; or carry over the variant information if the kind actually is from
a different target which has variant specified.

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:88: if
hasattr(kind, "module"):
On 2016/03/17 17:37:58, Sam McNally wrote:
> I don't think it makes sense to add the variant if we aren't adding a
namespace
> because the kind doesn't have a module.

I am not quite sure I have understood the problem:
Here |add_same_module_namespaces| basically mean whether we want to add the
namespaces beginning from  what is defined by the "module" definition in mojom.
If not, this method just produces <kind name>, or internal::<kind name>.
Otherwise, we will need to add the variant, right?

[yzshen1, 2016-03-17 17:47:00]

The CQ bit was unchecked by yzshen@chromium.org

[Sam McNally, 2016-03-17 17:52:55]

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:88: if
hasattr(kind, "module"):
On 2016/03/17 17:46:36, yzshen1 wrote:
> On 2016/03/17 17:37:58, Sam McNally wrote:
> > I don't think it makes sense to add the variant if we aren't adding a
> namespace
> > because the kind doesn't have a module.
> 
> I am not quite sure I have understood the problem:
> Here |add_same_module_namespaces| basically mean whether we want to add the
> namespaces beginning from  what is defined by the "module" definition in
mojom.
> If not, this method just produces <kind name>, or internal::<kind name>.
> Otherwise, we will need to add the variant, right?

Yes, but if kind.module doesn't exist when |add_same_module_namespaces| and
|add_variant| are true, we'll be returning just <variant>::<kind name> or
<variant>::internal::<kind name>.

[yzshen1, 2016-03-17 17:58:45]

On 2016/03/17 17:52:55, Sam McNally wrote:
>
https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):
> 
>
https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/mojom_cpp_generator.py:88: if
> hasattr(kind, "module"):
> On 2016/03/17 17:46:36, yzshen1 wrote:
> > On 2016/03/17 17:37:58, Sam McNally wrote:
> > > I don't think it makes sense to add the variant if we aren't adding a
> > namespace
> > > because the kind doesn't have a module.
> > 
> > I am not quite sure I have understood the problem:
> > Here |add_same_module_namespaces| basically mean whether we want to add the
> > namespaces beginning from  what is defined by the "module" definition in
> mojom.
> > If not, this method just produces <kind name>, or internal::<kind name>.
> > Otherwise, we will need to add the variant, right?
> 
> Yes, but if kind.module doesn't exist when |add_same_module_namespaces| and
> |add_variant| are true, we'll be returning just <variant>::<kind name> or
> <variant>::internal::<kind name>.

Isn't that expected outcome?

[Sam McNally, 2016-03-17 18:05:15]

On 2016/03/17 17:58:45, yzshen1 wrote:
> On 2016/03/17 17:52:55, Sam McNally wrote:
> >
>
https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
> > File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):
> > 
> >
>
https://codereview.chromium.org/1811853004/diff/20001/mojo/public/tools/bindi...
> > mojo/public/tools/bindings/generators/mojom_cpp_generator.py:88: if
> > hasattr(kind, "module"):
> > On 2016/03/17 17:46:36, yzshen1 wrote:
> > > On 2016/03/17 17:37:58, Sam McNally wrote:
> > > > I don't think it makes sense to add the variant if we aren't adding a
> > > namespace
> > > > because the kind doesn't have a module.
> > > 
> > > I am not quite sure I have understood the problem:
> > > Here |add_same_module_namespaces| basically mean whether we want to add
the
> > > namespaces beginning from  what is defined by the "module" definition in
> > mojom.
> > > If not, this method just produces <kind name>, or internal::<kind name>.
> > > Otherwise, we will need to add the variant, right?
> > 
> > Yes, but if kind.module doesn't exist when |add_same_module_namespaces| and
> > |add_variant| are true, we'll be returning just <variant>::<kind name> or
> > <variant>::internal::<kind name>.
> 
> Isn't that expected outcome?

It isn't what I would have expected, but I don't know when this would situation
would arise. LGTM; figuring this out can wait until the proper cleanup.

[Reilly Grant (use Gerrit), 2016-03-17 18:17:19]

Description was changed from

==========
Mojo C++ bindigns: fix generated namespace when variant is specified.

This fixed the issue that enum generated code didn't work when variant is
specified.
This is a quick fix, we need more fundermental improvement for variant handling
code.

BUG=None
==========

to

==========
Mojo C++ bindigns: fix generated namespace when variant is specified.

This fixed the issue that enum generated code didn't work when variant is
specified.
This is a quick fix, we need more fundermental improvement for variant handling
code.

BUG=595791
==========

[yzshen1, 2016-03-17 18:39:51]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-03-17 18:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811853004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811853004/20001

[commit-bot: I haz the power, 2016-03-17 18:45:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-17 18:48:14]

Description was changed from

==========
Mojo C++ bindigns: fix generated namespace when variant is specified.

This fixed the issue that enum generated code didn't work when variant is
specified.
This is a quick fix, we need more fundermental improvement for variant handling
code.

BUG=595791
==========

to

==========
Mojo C++ bindigns: fix generated namespace when variant is specified.

This fixed the issue that enum generated code didn't work when variant is
specified.
This is a quick fix, we need more fundermental improvement for variant handling
code.

BUG=595791

Committed: https://crrev.com/e56136366dd27d1764c01203565447b53197733f
Cr-Commit-Position: refs/heads/master@{#381760}
==========

[commit-bot: I haz the power, 2016-03-17 18:48:16]

Patchset 2 (id:??) landed as
https://crrev.com/e56136366dd27d1764c01203565447b53197733f
Cr-Commit-Position: refs/heads/master@{#381760}