[Mac]Implement ViewID support. (third approach)

[Mac]Implement ViewID support. (third approach)

This CL adds several extension methods to NSView class for ViewID
support, and uses a map to store ViewIDs of views.
Each view requiring ViewID support can set its ViewID upon
initialization and unset it before destruction.
When looking up a view with a specific ViewID, just search all sub views
recursively from the root view of a window.

BUG=44692
need ViewIds on mac
TEST=none

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

[John Grabowski, 2010-07-20 05:53:17]

LGTM from me.
Adding trung to reviewers list.

http://codereview.chromium.org/2878037/diff/1/4
File chrome/browser/cocoa/bookmark_button.mm (right):

http://codereview.chromium.org/2878037/diff/1/4#newcode29
chrome/browser/cocoa/bookmark_button.mm:29: self = [super
initWithFrame:frameRect];
convention: 

if ((self = [super initBlahBlah])) {
  [self setViewID:  ... ];
}
return self;

http://codereview.chromium.org/2878037/diff/1/20
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/1/20#newcode55
chrome/browser/cocoa/view_id_util.mm:55: 
Are there other safety tests you can add?
E.g. register for windowWillClose notification, then walk the map removing all
view/id pairs where the view is contained in that window?

[Nico, 2010-07-20 06:01:15]

http://codereview.chromium.org/2878037/diff/1/8
File chrome/browser/cocoa/find_bar_view.mm (right):

http://codereview.chromium.org/2878037/diff/1/8#newcode22
chrome/browser/cocoa/find_bar_view.mm:22: [self setViewID:VIEW_ID_FIND_IN_PAGE];
here and in most other places, couldn't you instead say

- (ViewID)viewID {
  return VIEW_ID_FIND_IN_PAGE;
}

? Then the set functions would be required by only very few views, and this
wouldn't even have to be a category (it could be a static function for the few
places that need this)

http://codereview.chromium.org/2878037/diff/1/20
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/1/20#newcode20
chrome/browser/cocoa/view_id_util.mm:20: DLOG(WARNING) << "setViewID:" << viewID
<< " to:" << [[self description] UTF8String];
80 col

[viettrungluu, 2010-07-20 06:22:20]

+pink, since he might have opinions on this

I'll echo what Nico said.

Moreover, I don't like the singleton std::map; it seems largely unnecessary. I
also object to the added need to override so many -dealloc's, especially since
it's fragile (you forget, and you end up with a stale pointer and a leaky map).
(The added things in the -init's/-awakeFromNib's aren't so great either.)

Sorry, I don't really approve of this change as it looks now. I think it adds
too much complexity/fussiness for what it does.

http://codereview.chromium.org/2878037/diff/1/20
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/1/20#newcode62
chrome/browser/cocoa/view_id_util.mm:62: if (viewID >= VIEW_ID_TAB_0 && viewID
<= VIEW_ID_TAB_LAST) {
This seems like the wrong place to do this sort of thing. If you're going to
call -viewWithViewID: (maybe that should be "-subviewWithViewID: or somesuch?),
then you should override that in the TabStripView (or something like that) and
handle that there.

[John Grabowski, 2010-07-20 07:29:08]

We need to stop this game.

This is perhaps the 3rd rev of this CL.
For example, it originally did not have a map; it had a bunch of

   - (ViewID)viewID {
     return VIEW_ID_FIND_IN_PAGE;
   }

Then thakis suggested a map.
Now you (trung) say "I hate this map".

The whole point of us meeting in a room was to circle on a common
suggestion.
Looks like things got worse; not better.
Perhaps this is a case of too many reviewers.


jrg


On Mon, Jul 19, 2010 at 11:22 PM, <viettrungluu@chromium.org> wrote:

> +pink, since he might have opinions on this
>
> I'll echo what Nico said.
>
> Moreover, I don't like the singleton std::map; it seems largely
> unnecessary. I
> also object to the added need to override so many -dealloc's, especially
> since
> it's fragile (you forget, and you end up with a stale pointer and a leaky
> map).
> (The added things in the -init's/-awakeFromNib's aren't so great either.)
>
> Sorry, I don't really approve of this change as it looks now. I think it
> adds
> too much complexity/fussiness for what it does.
>
>
>
> http://codereview.chromium.org/2878037/diff/1/20
> File chrome/browser/cocoa/view_id_util.mm (right):
>
> http://codereview.chromium.org/2878037/diff/1/20#newcode62
> chrome/browser/cocoa/view_id_util.mm:62: if (viewID >= VIEW_ID_TAB_0 &&
> viewID <= VIEW_ID_TAB_LAST) {
> This seems like the wrong place to do this sort of thing. If you're
> going to call -viewWithViewID: (maybe that should be
> "-subviewWithViewID: or somesuch?), then you should override that in the
> TabStripView (or something like that) and handle that there.
>
>
> http://codereview.chromium.org/2878037/show
>

[James Su, 2010-07-20 08:48:53]

Hopefully it's the last version :)

http://codereview.chromium.org/2878037/diff/1/4
File chrome/browser/cocoa/bookmark_button.mm (right):

http://codereview.chromium.org/2878037/diff/1/4#newcode29
chrome/browser/cocoa/bookmark_button.mm:29: self = [super
initWithFrame:frameRect];
On 2010/07/20 05:53:18, John Grabowski wrote:
> convention: 
> 
> if ((self = [super initBlahBlah])) {
>   [self setViewID:  ... ];
> }
> return self;
> 

Done.

http://codereview.chromium.org/2878037/diff/1/8
File chrome/browser/cocoa/find_bar_view.mm (right):

http://codereview.chromium.org/2878037/diff/1/8#newcode22
chrome/browser/cocoa/find_bar_view.mm:22: [self setViewID:VIEW_ID_FIND_IN_PAGE];
On 2010/07/20 06:01:16, Nico wrote:
> here and in most other places, couldn't you instead say
> 
> - (ViewID)viewID {
>   return VIEW_ID_FIND_IN_PAGE;
> }
> 
> ? Then the set functions would be required by only very few views, and this
> wouldn't even have to be a category (it could be a static function for the few
> places that need this)

Done.

http://codereview.chromium.org/2878037/diff/1/20
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/1/20#newcode20
chrome/browser/cocoa/view_id_util.mm:20: DLOG(WARNING) << "setViewID:" << viewID
<< " to:" << [[self description] UTF8String];
On 2010/07/20 06:01:16, Nico wrote:
> 80 col
This line should be removed.

http://codereview.chromium.org/2878037/diff/1/20#newcode55
chrome/browser/cocoa/view_id_util.mm:55: 
On 2010/07/20 05:53:18, John Grabowski wrote:
> Are there other safety tests you can add?
> E.g. register for windowWillClose notification, then walk the map removing all
> view/id pairs where the view is contained in that window?
> 
Now we don't keep window in the map, I'm not sure if it's feasible and 
necessary to watch for the window's destruction.

http://codereview.chromium.org/2878037/diff/1/20#newcode62
chrome/browser/cocoa/view_id_util.mm:62: if (viewID >= VIEW_ID_TAB_0 && viewID
<= VIEW_ID_TAB_LAST) {
On 2010/07/20 06:22:21, viettrungluu wrote:
> This seems like the wrong place to do this sort of thing. If you're going to
> call -viewWithViewID: (maybe that should be "-subviewWithViewID: or
somesuch?),
> then you should override that in the TabStripView (or something like that) and
> handle that there.

It's now a standalone function rather than a category method of NSView.

[pink (ping after 24hrs), 2010-07-20 13:23:58]

Can someone explain why we need this mechanism?

On Tue, Jul 20, 2010 at 4:48 AM,  <suzhe@chromium.org> wrote:
> Hopefully it's the last version :)
>
>
> http://codereview.chromium.org/2878037/diff/1/4
> File chrome/browser/cocoa/bookmark_button.mm (right):
>
> http://codereview.chromium.org/2878037/diff/1/4#newcode29
> chrome/browser/cocoa/bookmark_button.mm:29: self = [super
> initWithFrame:frameRect];
> On 2010/07/20 05:53:18, John Grabowski wrote:
>>
>> convention:
>
>> if ((self = [super initBlahBlah])) {
>>   [self setViewID:  ... ];
>> }
>> return self;
>
>
> Done.
>
> http://codereview.chromium.org/2878037/diff/1/8
> File chrome/browser/cocoa/find_bar_view.mm (right):
>
> http://codereview.chromium.org/2878037/diff/1/8#newcode22
> chrome/browser/cocoa/find_bar_view.mm:22: [self
> setViewID:VIEW_ID_FIND_IN_PAGE];
> On 2010/07/20 06:01:16, Nico wrote:
>>
>> here and in most other places, couldn't you instead say
>
>> - (ViewID)viewID {
>>   return VIEW_ID_FIND_IN_PAGE;
>> }
>
>> ? Then the set functions would be required by only very few views, and
>
> this
>>
>> wouldn't even have to be a category (it could be a static function for
>
> the few
>>
>> places that need this)
>
> Done.
>
> http://codereview.chromium.org/2878037/diff/1/20
> File chrome/browser/cocoa/view_id_util.mm (right):
>
> http://codereview.chromium.org/2878037/diff/1/20#newcode20
> chrome/browser/cocoa/view_id_util.mm:20: DLOG(WARNING) << "setViewID:"
> << viewID << " to:" << [[self description] UTF8String];
> On 2010/07/20 06:01:16, Nico wrote:
>>
>> 80 col
>
> This line should be removed.
>
> http://codereview.chromium.org/2878037/diff/1/20#newcode55
> chrome/browser/cocoa/view_id_util.mm:55:
> On 2010/07/20 05:53:18, John Grabowski wrote:
>>
>> Are there other safety tests you can add?
>> E.g. register for windowWillClose notification, then walk the map
>
> removing all
>>
>> view/id pairs where the view is contained in that window?
>
> Now we don't keep window in the map, I'm not sure if it's feasible and
> necessary to watch for the window's destruction.
>
> http://codereview.chromium.org/2878037/diff/1/20#newcode62
> chrome/browser/cocoa/view_id_util.mm:62: if (viewID >= VIEW_ID_TAB_0 &&
> viewID <= VIEW_ID_TAB_LAST) {
> On 2010/07/20 06:22:21, viettrungluu wrote:
>>
>> This seems like the wrong place to do this sort of thing. If you're
>
> going to
>>
>> call -viewWithViewID: (maybe that should be "-subviewWithViewID: or
>
> somesuch?),
>>
>> then you should override that in the TabStripView (or something like
>
> that) and
>>
>> handle that there.
>
> It's now a standalone function rather than a category method of NSView.
>
> http://codereview.chromium.org/2878037/show
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[Nico, 2010-07-20 14:54:55]

LG

I like the newest revision very much.

I don't think the dealloc issue is a huge problem, since it's needed only in a
handful cases (and they have it with this CL), and even if the dealloc override
is forgotten you won't really get stale points, as the method that uses this
always walks live subviews.

http://codereview.chromium.org/2878037/diff/10001/11019
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11019#newcode22
chrome/browser/cocoa/view_id_util.mm:22: if ([view viewID] == viewID)
DCHECK that [view respondsToSelector:@selector(viewID)] (if we expect this to be
true of all views. Else, add if ([view respon…)

[Nico, 2010-07-20 14:56:45]

On 2010/07/20 07:29:08, John Grabowski wrote:
> We need to stop this game.
> 
> This is perhaps the 3rd rev of this CL.
> For example, it originally did not have a map; it had a bunch of
> 
>    - (ViewID)viewID {
>      return VIEW_ID_FIND_IN_PAGE;
>    }
> 
> Then thakis suggested a map.

I objected to introducing a superclass, and suggested that that could be done
via a hash map (which is how it is now in some cases").

> Now you (trung) say "I hate this map".

He said "The code could be simpler in some of the cases; you don't need the hash
map everywhere", which is true.

> The whole point of us meeting in a room was to circle on a common
> suggestion.
> Looks like things got worse; not better.

I feel like this converged nicely.

*shrug*

> Perhaps this is a case of too many reviewers.
> 
> 
> jrg
> 
> 
> On Mon, Jul 19, 2010 at 11:22 PM, <mailto:viettrungluu@chromium.org> wrote:
> 
> > +pink, since he might have opinions on this
> >
> > I'll echo what Nico said.
> >
> > Moreover, I don't like the singleton std::map; it seems largely
> > unnecessary. I
> > also object to the added need to override so many -dealloc's, especially
> > since
> > it's fragile (you forget, and you end up with a stale pointer and a leaky
> > map).
> > (The added things in the -init's/-awakeFromNib's aren't so great either.)
> >
> > Sorry, I don't really approve of this change as it looks now. I think it
> > adds
> > too much complexity/fussiness for what it does.
> >
> >
> >
> > http://codereview.chromium.org/2878037/diff/1/20
> > File chrome/browser/cocoa/view_id_util.mm (right):
> >
> > http://codereview.chromium.org/2878037/diff/1/20#newcode62
> > chrome/browser/cocoa/view_id_util.mm:62: if (viewID >= VIEW_ID_TAB_0 &&
> > viewID <= VIEW_ID_TAB_LAST) {
> > This seems like the wrong place to do this sort of thing. If you're
> > going to call -viewWithViewID: (maybe that should be
> > "-subviewWithViewID: or somesuch?), then you should override that in the
> > TabStripView (or something like that) and handle that there.
> >
> >
> > http://codereview.chromium.org/2878037/show
> >
>

[viettrungluu, 2010-07-20 15:19:48]

Overall, this is looking much nicer. Could you replace the map with associative
references? It shouldn't be too much code, and then you wouldn't need to do
anything on -dealloc.

Thanks,

- Trung

[James Su, 2010-07-20 16:59:31]

I'm afraid that it may not feasible for us, because associative references are
only available in Mac OS X v10.6 and later.

Regards
James Su

On 2010/07/20 15:19:48, viettrungluu wrote:
> Overall, this is looking much nicer. Could you replace the map with
associative
> references? It shouldn't be too much code, and then you wouldn't need to do
> anything on -dealloc.
> 
> Thanks,
> 
> - Trung

[vtl, 2010-07-20 17:20:15]

You're right about associative refs and 10.6, sorry about that. I can't think of
a nicer solution then, but could you add a note about why you're using a map?
(When we move to 10.6+, maybe we can revisit this.) Thanks for bearing with me.

http://codereview.chromium.org/2878037/diff/10001/11001
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11001#newcode1090
chrome/browser/cocoa/bookmark_bar_controller.mm:1090: // Overwrite the ViewID of
the button.
Nit: useless comment; delete.

http://codereview.chromium.org/2878037/diff/10001/11008
File chrome/browser/cocoa/infobar_container_controller.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11008#newcode101
chrome/browser/cocoa/infobar_container_controller.mm:101: // Called after the
view is done loading and the outlets have been hooked up.
Nit: You can omit this comment.

http://codereview.chromium.org/2878037/diff/10001/11012
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/2878037/diff/10001/11012#newcode160
chrome/browser/cocoa/tab_strip_controller.h:160: // Return the number of tab
views in the tab strip. It's same as number of tabs
Are you sure you want to give access to tab views which are closing and no
longer in the model? (What happens on other platforms?)

http://codereview.chromium.org/2878037/diff/10001/11019
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11019#newcode9
chrome/browser/cocoa/view_id_util.mm:9: #include <utility>
Nit: newline after <utility>. (Possibly one before #include <map> too.)

[James Su, 2010-07-20 18:05:30]

May I commit it?

http://codereview.chromium.org/2878037/diff/10001/11001
File chrome/browser/cocoa/bookmark_bar_controller.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11001#newcode1090
chrome/browser/cocoa/bookmark_bar_controller.mm:1090: // Overwrite the ViewID of
the button.
On 2010/07/20 17:20:15, vtl wrote:
> Nit: useless comment; delete.

Done.

http://codereview.chromium.org/2878037/diff/10001/11008
File chrome/browser/cocoa/infobar_container_controller.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11008#newcode101
chrome/browser/cocoa/infobar_container_controller.mm:101: // Called after the
view is done loading and the outlets have been hooked up.
On 2010/07/20 17:20:15, vtl wrote:
> Nit: You can omit this comment.

Done.

http://codereview.chromium.org/2878037/diff/10001/11012
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/2878037/diff/10001/11012#newcode160
chrome/browser/cocoa/tab_strip_controller.h:160: // Return the number of tab
views in the tab strip. It's same as number of tabs
On 2010/07/20 17:20:15, vtl wrote:
> Are you sure you want to give access to tab views which are closing and no
> longer in the model? (What happens on other platforms?)

I believe both win and gtk versions have the same behavior.

http://codereview.chromium.org/2878037/diff/10001/11019
File chrome/browser/cocoa/view_id_util.mm (right):

http://codereview.chromium.org/2878037/diff/10001/11019#newcode9
chrome/browser/cocoa/view_id_util.mm:9: #include <utility>
On 2010/07/20 17:20:15, vtl wrote:
> Nit: newline after <utility>. (Possibly one before #include <map> too.)

Done.

[viettrungluu, 2010-07-20 18:11:14]

Thanks, I just wanted to make sure about the question below. LGTM.

On 2010/07/20 18:05:30, James Su wrote:
> May I commit it?

> On 2010/07/20 17:20:15, vtl wrote:
> > Are you sure you want to give access to tab views which are closing and no
> > longer in the model? (What happens on other platforms?)
> 
> I believe both win and gtk versions have the same behavior.

Okay.

[James Su, 2010-07-20 18:18:28]

CL committed. Thanks a lot for your valuable feedback.