Use template specialization to generate WindowProperty code

Use template specialization to generate WindowProperty code.

They used to be a inlined function in the header, which led to code generation when method is used. This changes the code generation only in DECLARE_WINDOW_PROPERTY_TYPE().

BUG=None
R=sadrul@chromium.org

Committed: https://crrev.com/f62e152bb7a13135a597d9766eb79551d3a24a73
Cr-Commit-Position: refs/heads/master@{#309252}

Committed: https://crrev.com/3d8840e01a5c8fbe45b492ce29c9eb34d17eeb25
Cr-Commit-Position: refs/heads/master@{#310179}

[sadrul, 2014-12-15 17:13:39]

Does this make it possible for external code to Set/Get this property? If so, I
think we should try to avoid that.

[oshima, 2014-12-15 21:20:11]

On 2014/12/15 17:13:39, sadrul wrote:
> Does this make it possible for external code to Set/Get this property? If so,
I
> think we should try to avoid that.

It's already possible for external code to get the reference to
DesktopNativeWidgetAura
using DesktopNativeWidgetAura::ForWindow(aura::Window*).

[sadrul, 2014-12-15 21:50:46]

On 2014/12/15 21:20:11, oshima wrote:
> On 2014/12/15 17:13:39, sadrul wrote:
> > Does this make it possible for external code to Set/Get this property? If
so,
> I
> > think we should try to avoid that.
> 
> It's already possible for external code to get the reference to
> DesktopNativeWidgetAura
> using DesktopNativeWidgetAura::ForWindow(aura::Window*).

Right. Getting access to DesktopNativeWidgetAura is not a worry. My concern is
that this makes it possible for call-sites to use window->GetProperty() to get
to it, which would make it difficult if, say,
DesktopNativeWidgetAura::ForWindow() wanted to do some sanity-checks on the
returned value. (even worse, use SetProperty() to change it from underneath)
Having the key in the .cc hides it well, and the API gets to choose how/whether
it wants to expose the key/value.

[oshima, 2014-12-15 22:48:09]

Patchset #2 (id:20001) has been deleted

[oshima, 2014-12-15 22:48:42]

ok that makes sense. Updated the CL. PTAL.

[sadrul, 2014-12-15 22:53:09]

https://codereview.chromium.org/801953002/diff/40001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/801953002/diff/40001/ui/views/views.gyp#newco...
ui/views/views.gyp:415: 'widget/desktop_aura/desktop_native_widget_property.cc',
I think you missed this file?

[oshima, 2014-12-15 22:57:47]

https://codereview.chromium.org/801953002/diff/40001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/801953002/diff/40001/ui/views/views.gyp#newco...
ui/views/views.gyp:415: 'widget/desktop_aura/desktop_native_widget_property.cc',
On 2014/12/15 22:53:09, sadrul wrote:
> I think you missed this file?

oops, sorry. done.

[sadrul, 2014-12-16 17:25:17]

Could you explain a little more what problem this change fixes? What/where are
the dup symbols?

https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:38: #include
"ui/views/widget/desktop_aura/desktop_native_widget_constants.h"
Is this file missing too?

[oshima, 2014-12-16 21:09:12]

window_property is used to instantiate template specializationm, so it creates
the symbol for GetProperty(kShowStateKey) as well in the views library.

This becomes issue if the definition is created in
two shared library and used from other library (thus,
complains which one to use).

This CL prevents generating the code in views component library.

https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:38: #include
"ui/views/widget/desktop_aura/desktop_native_widget_constants.h"
On 2014/12/16 17:25:16, sadrul wrote:
> Is this file missing too?
removed. my local build passed because the file was still there, sorry.

[sadrul, 2014-12-16 22:20:11]

On 2014/12/16 21:09:12, oshima wrote:
> window_property is used to instantiate template specializationm, so it creates
> the symbol for GetProperty(kShowStateKey) as well in the views library.
> 
> This becomes issue if the definition is created in
> two shared library and used from other library (thus,
> complains which one to use).
> 
> This CL prevents generating the code in views component library.

This CL is moving some code out of file A into file B, and both A and B are
built into the same component. So I am afraid I still don't understand what this
is changing. Is there a failure somewhere I could look at that could help me
understand?

> 
>
https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> 
>
https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:38: #include
> "ui/views/widget/desktop_aura/desktop_native_widget_constants.h"
> On 2014/12/16 17:25:16, sadrul wrote:
> > Is this file missing too?
> removed. my local build passed because the file was still there, sorry.

[oshima, 2014-12-16 22:36:21]

On 2014/12/16 22:20:11, sadrul wrote:
> On 2014/12/16 21:09:12, oshima wrote:
> > window_property is used to instantiate template specializationm, so it
creates
> > the symbol for GetProperty(kShowStateKey) as well in the views library.
> > 
> > This becomes issue if the definition is created in
> > two shared library and used from other library (thus,
> > complains which one to use).
> > 
> > This CL prevents generating the code in views component library.
> 
> This CL is moving some code out of file A into file B, and both A and B are
> built into the same component. So I am afraid I still don't understand what
this
> is changing. Is there a failure somewhere I could look at that could help me
> understand?

I found this issue in the CL:

https://codereview.chromium.org/778283002/

linux_chromium_rel_ng is green because it's static build. linux_chromium_dbg_gn
fails because
it's component build.


> 
> > 
> >
>
https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
> > File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/801953002/diff/60001/ui/views/widget/desktop_...
> > ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:38: #include
> > "ui/views/widget/desktop_aura/desktop_native_widget_constants.h"
> > On 2014/12/16 17:25:16, sadrul wrote:
> > > Is this file missing too?
> > removed. my local build passed because the file was still there, sorry.

[sadrul, 2014-12-17 19:42:00]

sadrul@chromium.org changed reviewers:
+ sky@chromium.org

[sadrul, 2014-12-17 19:42:00]

+sky@

On 2014/12/16 22:36:21, oshima wrote:
> On 2014/12/16 22:20:11, sadrul wrote:
> > On 2014/12/16 21:09:12, oshima wrote:
> > > window_property is used to instantiate template specializationm, so it
> creates
> > > the symbol for GetProperty(kShowStateKey) as well in the views library.
> > > 
> > > This becomes issue if the definition is created in
> > > two shared library and used from other library (thus,
> > > complains which one to use).
> > > 
> > > This CL prevents generating the code in views component library.
> > 
> > This CL is moving some code out of file A into file B, and both A and B are
> > built into the same component. So I am afraid I still don't understand what
> this
> > is changing. Is there a failure somewhere I could look at that could help me
> > understand?
> 
> I found this issue in the CL:
> 
> https://codereview.chromium.org/778283002/
> 
> linux_chromium_rel_ng is green because it's static build.
linux_chromium_dbg_gn
> fails because
> it's component build.

I see. Thanks. The failure is this:

    ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: warning: hidden
symbol 'ui::WindowShowState
aura::Window::GetProperty<ui::WindowShowState>(aura::WindowProperty<ui::WindowShowState>
const*) const' in
obj/ui/views/widget/desktop_aura/libviews.desktop_native_widget_aura.o is
referenced by DSO ./libwm.so
    ../../third_party/binutils/Linux_x64/Release/bin/ld.gold: error: treating
warnings as errors
    clang: error: linker command failed with exit code 1 (use -v to see
invocation)
    ninja: build stopped: subcommand failed.

I don't understand what is causing this, but it looks like this would somehow be
related to the use of kWindowShowState in desktop_native_widget_aura.cc. So I
still do not understand how moving the code related to a different key
(kDesktopNativeWidgetAuraKey) from that file into a separate file helps
anything. The change itself looks harmless, but if it fixes the build issue,
then that seems a bit magical. We should find a better fix that makes it easier
to understand, or document how these properties are expected to be used to avoid
such failures.

Having said that, if the fix is obvious to both you and sky@, then I won't block
this change. Also, any idea why the build failure only shows up with gn?

[sky, 2014-12-18 00:37:52]

LGTM

I feel this is all crap too. Hopefully we can straighten that out separately
though.

[oshima, 2014-12-18 16:49:10]

Patchset #6 (id:120001) has been deleted

[oshima, 2014-12-18 16:59:29]

Patchset #6 (id:140001) has been deleted

[oshima, 2014-12-18 17:01:58]

Patchset #6 (id:160001) has been deleted

[oshima, 2014-12-18 17:05:41]

I think I took wrong approach. Changed the window_property to generate the code
only when necessary. I hope this looks better and clearer to you. PTAL.

[sky, 2014-12-18 17:46:08]

Yes, much, thanks. LGTM

https://codereview.chromium.org/801953002/diff/180001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/801953002/diff/180001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:55: virtual ~TestProperty() {
nit: no virtual but override.

https://codereview.chromium.org/801953002/diff/180001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:65: TestProperty* TestProperty::last_deleted_ = NULL;
nullptr

[oshima, 2014-12-18 18:14:43]

https://codereview.chromium.org/801953002/diff/180001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/801953002/diff/180001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:55: virtual ~TestProperty() {
On 2014/12/18 17:46:08, sky wrote:
> nit: no virtual but override.

Done.

https://codereview.chromium.org/801953002/diff/180001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:65: TestProperty* TestProperty::last_deleted_ = NULL;
On 2014/12/18 17:46:08, sky wrote:
> nullptr

Done.

[oshima, 2014-12-18 18:14:47]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-12-18 18:16:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801953002/200001

[commit-bot: I haz the power, 2014-12-18 18:34:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-18 18:34:50]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2014-12-18 20:54:12]

Patchset #9 (id:230001) has been deleted

[oshima, 2014-12-18 20:54:23]

Patchset #8 (id:220001) has been deleted

[oshima, 2014-12-19 06:45:21]

Patchset #9 (id:270001) has been deleted

[oshima, 2014-12-19 17:34:37]

Patchset #8 (id:250001) has been deleted

[oshima, 2014-12-19 17:39:18]

Patchset #9 (id:310001) has been deleted

[oshima, 2014-12-19 17:40:34]

Patchset #8 (id:290001) has been deleted

[oshima, 2014-12-19 20:07:55]

I had to update a few more ash / athena files, and
wth files which was working because it was using inlined
template impl. The window_property.h hasn't changed, so
I'll just go ahead and land this. let me know if you have any concern.

[oshima, 2014-12-19 20:08:01]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-12-19 20:08:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801953002/330001

[commit-bot: I haz the power, 2014-12-19 20:51:36]

Committed patchset #8 (id:330001)

[commit-bot: I haz the power, 2014-12-19 20:53:08]

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

[Chirantan Ekbote, 2014-12-19 22:23:42]

A revert of this CL (patchset #8 id:330001) has been created in
https://codereview.chromium.org/814313002/ by chirantan@chromium.org.

The reason for reverting is: This is breaking chrome on chrome os builds:

In file included from ../../ui/aura/client/screen_position_client.cc:7:0:
../../ui/aura/window_property.h:115:58: error: specialization of 'template<class
T> void aura::Window::SetProperty(const aura::WindowProperty<T>*, T)' in
different namespace [-fpermissive]
         const aura::WindowProperty<T >* property, T value) {  \
                                                          ^
../../ui/aura/window_property.h:127:5: note: in expansion of macro
'DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE'
     DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE(, T)
     ^
../../ui/aura/client/screen_position_client.cc:9:1: note: in expansion of macro
'DECLARE_WINDOW_PROPERTY_TYPE'
 DECLARE_WINDOW_PROPERTY_TYPE(aura::client::ScreenPositionClient*)
 ^
In file included from ../../ui/aura/client/screen_position_client.h:9:0,
                 from ../../ui/aura/client/screen_position_client.cc:5:
../../ui/aura/window.h:297:8: error:   from definition of 'template<class T>
void aura::Window::SetProperty(const aura::WindowProperty<T>*, T)'
[-fpermissive]
   void SetProperty(const WindowProperty<T>* property, T value);
        ^
.

[oshima, 2014-12-20 18:02:14]

Thanks, looks like the waterfall has different compiler options than CQ/try
bots :(



On Fri, Dec 19, 2014 at 2:23 PM, <chirantan@chromium.org> wrote:

> A revert of this CL (patchset #8 id:330001) has been created in
> https://codereview.chromium.org/814313002/ by chirantan@chromium.org.
>
> The reason for reverting is: This is breaking chrome on chrome os builds:
>
> In file included from ../../ui/aura/client/screen_position_client.cc:7:0:
> ../../ui/aura/window_property.h:115:58: error: specialization of
> 'template<class
> T> void aura::Window::SetProperty(const aura::WindowProperty<T>*, T)' in
> different namespace [-fpermissive]
>          const aura::WindowProperty<T >* property, T value) {  \
>                                                           ^
> ../../ui/aura/window_property.h:127:5: note: in expansion of macro
> 'DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE'
>      DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE(, T)
>      ^
> ../../ui/aura/client/screen_position_client.cc:9:1: note: in expansion of
> macro
> 'DECLARE_WINDOW_PROPERTY_TYPE'
>  DECLARE_WINDOW_PROPERTY_TYPE(aura::client::ScreenPositionClient*)
>  ^
> In file included from ../../ui/aura/client/screen_position_client.h:9:0,
>                  from ../../ui/aura/client/screen_position_client.cc:5:
> ../../ui/aura/window.h:297:8: error:   from definition of 'template<class
> T>
> void aura::Window::SetProperty(const aura::WindowProperty<T>*, T)'
> [-fpermissive]
>    void SetProperty(const WindowProperty<T>* property, T value);
>         ^
> .
>
> https://codereview.chromium.org/801953002/
>

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

[tfarina, 2014-12-20 18:49:42]

On 2014/12/20 18:02:14, oshima (OOO Dec 18 - Jan 4) wrote:
> Thanks, looks like the waterfall has different compiler options than CQ/try
> bots :(
> 
> 
> 
> On Fri, Dec 19, 2014 at 2:23 PM, <mailto:chirantan@chromium.org> wrote:
> 
> > A revert of this CL (patchset #8 id:330001) has been created in
> > https://codereview.chromium.org/814313002/ by mailto:chirantan@chromium.org.
> >
> > The reason for reverting is: This is breaking chrome on chrome os builds:
> >
> > In file included from ../../ui/aura/client/screen_position_client.cc:7:0:
> > ../../ui/aura/window_property.h:115:58: error: specialization of
> > 'template<class
> > T> void aura::Window::SetProperty(const aura::WindowProperty<T>*, T)' in
> > different namespace [-fpermissive]
> >          const aura::WindowProperty<T >* property, T value) {  \
> >                                                           ^
> > ../../ui/aura/window_property.h:127:5: note: in expansion of macro
> > 'DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE'
> >      DECLARE_EXPORTED_WINDOW_PROPERTY_TYPE(, T)
> >      ^
> > ../../ui/aura/client/screen_position_client.cc:9:1: note: in expansion of
> > macro
> > 'DECLARE_WINDOW_PROPERTY_TYPE'
> >  DECLARE_WINDOW_PROPERTY_TYPE(aura::client::ScreenPositionClient*)
> >  ^
> > In file included from ../../ui/aura/client/screen_position_client.h:9:0,
> >                  from ../../ui/aura/client/screen_position_client.cc:5:
> > ../../ui/aura/window.h:297:8: error:   from definition of 'template<class
> > T>
> > void aura::Window::SetProperty(const aura::WindowProperty<T>*, T)'
> > [-fpermissive]
> >    void SetProperty(const WindowProperty<T>* property, T value);
> >         ^
> > .
> >
> > https://codereview.chromium.org/801953002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2014-12-20 18:50:56]

On 2014/12/20 18:02:14, oshima (OOO Dec 18 - Jan 4) wrote:
> Thanks, looks like the waterfall has different compiler options than CQ/try
> bots :(
>
Is there a bug filed for this? I think it would be good if we could track this
down and make them match.

[oshima, 2014-12-20 20:04:40]

Patchset #9 (id:350001) has been deleted

[oshima, 2014-12-20 20:18:54]

On 2014/12/20 18:50:56, tfarina wrote:
> On 2014/12/20 18:02:14, oshima (OOO Dec 18 - Jan 4) wrote:
> > Thanks, looks like the waterfall has different compiler options than CQ/try
> > bots :(
> >
> Is there a bug filed for this? I think it would be good if we could track this
> down and make them match.

I couldn't find the failure on main waterfall. chirantan@, do you have a link to
the failure?

[oshima, 2015-01-06 23:20:59]

sky@, can you take another look? Here is the difference from original CL.

https://codereview.chromium.org/801953002/diff2/330001:370001/ui/aura/window_...

[sky, 2015-01-06 23:24:10]

SLGTM

[oshima, 2015-01-06 23:25:49]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2015-01-06 23:27:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801953002/370001

[commit-bot: I haz the power, 2015-01-07 00:18:52]

Committed patchset #9 (id:370001)

[commit-bot: I haz the power, 2015-01-07 00:20:05]

Patchset 9 (id:??) landed as
https://crrev.com/3d8840e01a5c8fbe45b492ce29c9eb34d17eeb25
Cr-Commit-Position: refs/heads/master@{#310179}