Converts usage of SetProp/GetProp to a map. Even after making sure we

Converts usage of SetProp/GetProp to a map. Even after making sure we
clean up props we still leak in a handful of cases that are causing
test grief. By and large our usage of properties is for inside the
application, so that a map works fine.

BUG=61528
44991

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

[sky, 2010-11-18 02:14:51]

Ping

On Tue, Nov 16, 2010 at 3:57 PM,  <sky@chromium.org> wrote:
> Reviewers: cpu,
>
> Description:
> Converts usage of SetProp/GetProp to a map. Even after making sure we
> clean up props we still leak in a handful of cases that are causing
> test grief. By and large our usage of properties is for inside the
> application, so that a map works fine.
>
> BUG=61528 44991
>
> Please review this at http://codereview.chromium.org/5075003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M app/app.gyp
>  M app/app_base.gypi
>  M app/win/scoped_prop.h
>  A app/win/view_prop.h
>  A app/win/view_prop.cc
>  M chrome/browser/external_tab_container_win.h
>  M chrome/browser/external_tab_container_win.cc
>  M chrome/browser/renderer_host/render_widget_host_view_win.h
>  M chrome/browser/renderer_host/render_widget_host_view_win.cc
>  M chrome/browser/ui/views/frame/browser_view.cc
>  M views/accessibility/view_accessibility.h
>  M views/accessibility/view_accessibility.cc
>  M views/controls/native_control.cc
>  M views/controls/native_control_win.h
>  M views/controls/native_control_win.cc
>  M views/focus/focus_util_win.h
>  M views/focus/focus_util_win.cc
>  M views/widget/child_window_message_processor.h
>  M views/widget/child_window_message_processor.cc
>  M views/widget/widget.h
>  M views/widget/widget_gtk.h
>  M views/widget/widget_gtk.cc
>  M views/widget/widget_win.h
>  M views/widget/widget_win.cc
>
>
>

[Ben Goodger (Google), 2010-11-18 20:31:15]

The only real question I have is since this is not windows-specific anymore can
you just put it in app/ :-)

[cpu_(ooo_6.6-7.5), 2010-11-18 22:49:14]

looking good, just two questions.

http://codereview.chromium.org/5075003/diff/11001/app/view_prop.cc
File app/view_prop.cc (right):

http://codereview.chromium.org/5075003/diff/11001/app/view_prop.cc#newcode36
app/view_prop.cc:36: void set_data(void* data) { data_ = data; }
shouldn't set to NULL remove the item? if so above Get would need to deal with
that.

http://codereview.chromium.org/5075003/diff/11001/app/view_prop_unittest.cc
File app/view_prop_unittest.cc (right):

http://codereview.chromium.org/5075003/diff/11001/app/view_prop_unittest.cc#n...
app/view_prop_unittest.cc:30: ViewProp prop(nv1, kKey1, data1);
so being refcounted does not cause a problem when instanciated in the stack?

[sky, 2010-11-18 23:16:31]

I've also moved ViewProp to app as Ben suggested.

  -Scott

http://codereview.chromium.org/5075003/diff/11001/app/view_prop.cc
File app/view_prop.cc (right):

http://codereview.chromium.org/5075003/diff/11001/app/view_prop.cc#newcode36
app/view_prop.cc:36: void set_data(void* data) { data_ = data; }
On 2010/11/18 22:49:15, cpu wrote:
> shouldn't set to NULL remove the item? if so above Get would need to deal with
> that.

data_ is just the arbitrary value and it's used by these classes (only by the
users of ViewProp). The Data instance is removed from the map when there are no
more references. But maybe I'm misunderstanding your question?

http://codereview.chromium.org/5075003/diff/11001/app/view_prop_unittest.cc
File app/view_prop_unittest.cc (right):

http://codereview.chromium.org/5075003/diff/11001/app/view_prop_unittest.cc#n...
app/view_prop_unittest.cc:30: ViewProp prop(nv1, kKey1, data1);
On 2010/11/18 22:49:15, cpu wrote:
> so being refcounted does not cause a problem when instanciated in the stack?

ViewProp isn't ref-counted. ViewProp::Data is though.

[cpu_(ooo_6.6-7.5), 2010-11-19 18:22:17]

lgtm


On 2010/11/18 23:16:31, sky wrote:
> I've also moved ViewProp to app as Ben suggested.
> 
>   -Scott
> 
> http://codereview.chromium.org/5075003/diff/11001/app/view_prop.cc
> File app/view_prop.cc (right):
> 
> http://codereview.chromium.org/5075003/diff/11001/app/view_prop.cc#newcode36
> app/view_prop.cc:36: void set_data(void* data) { data_ = data; }
> On 2010/11/18 22:49:15, cpu wrote:
> > shouldn't set to NULL remove the item? if so above Get would need to deal
with
> > that.
> 
> data_ is just the arbitrary value and it's used by these classes (only by the
> users of ViewProp). The Data instance is removed from the map when there are
no
> more references. But maybe I'm misunderstanding your question?
> 
> http://codereview.chromium.org/5075003/diff/11001/app/view_prop_unittest.cc
> File app/view_prop_unittest.cc (right):
> 
>
http://codereview.chromium.org/5075003/diff/11001/app/view_prop_unittest.cc#n...
> app/view_prop_unittest.cc:30: ViewProp prop(nv1, kKey1, data1);
> On 2010/11/18 22:49:15, cpu wrote:
> > so being refcounted does not cause a problem when instanciated in the stack?
> 
> ViewProp isn't ref-counted. ViewProp::Data is though.