[Mac]Implement ViewID support.

[Mac]Implement ViewID support.

This CL implement ViewID support by using NSView's tag property.
ReloadButton is also changed to use a dedicated property to store the
current button command rather than using the tag property.

This CL removes tag values of toolbar buttons set in Toolbar.xib. Because now we use tag property for ViewID support.

BUG=44692
need ViewIds on mac
TEST=none

[Nico, 2010-07-10 06:34:54]

I'm not familiar with what this view id stuff wants to achieve. I'm hoping John
or Stuart know, else I will need to look through the linux code for this.

I'm not sure if storing this in the tag won't cause problems.

http://codereview.chromium.org/2973004/diff/2001/3016
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3016#newcode97
chrome/browser/cocoa/tab_contents_controller.mm:97: BaseView* devtoolsView =
The only subclass of BaseView is SadTabView I believe. s/BaseView/NSView/.

[John Grabowski, 2010-07-10 07:24:10]

I know what this is for; I will review in the morning.
Thanks for hitting this Suzhe.

jrg


On Fri, Jul 9, 2010 at 11:34 PM, <thakis@chromium.org> wrote:

> I'm not familiar with what this view id stuff wants to achieve. I'm hoping
> John
> or Stuart know, else I will need to look through the linux code for this.
>
> I'm not sure if storing this in the tag won't cause problems.
>
>
> http://codereview.chromium.org/2973004/diff/2001/3016
> File chrome/browser/cocoa/tab_contents_controller.mm (right):
>
> http://codereview.chromium.org/2973004/diff/2001/3016#newcode97
> chrome/browser/cocoa/tab_contents_controller.mm:97: BaseView*
> devtoolsView =
> The only subclass of BaseView is SadTabView I believe.
> s/BaseView/NSView/.
>
>
> http://codereview.chromium.org/2973004/show
>

[James Su, 2010-07-10 18:47:26]

ViewID is mainly for ui tests, for example browser_keyevents_browsertest.cc and
browser_focus_uitest.cc.

http://codereview.chromium.org/2973004/diff/2001/3016
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3016#newcode97
chrome/browser/cocoa/tab_contents_controller.mm:97: BaseView* devtoolsView =
On 2010/07/10 06:34:55, Nico wrote:
> The only subclass of BaseView is SadTabView I believe. s/BaseView/NSView/.
TabContentsViewCocoa and RenderWidgetHostViewCocoa are subclasses of BaseView.
This devtoolsView is actually a TabContentsViewCocoa object. Using BaseView here
is necessary for setTag method, which is not supported by NSView.
Maybe I should add a DCHECK here to ensure the object type?

[John Grabowski, 2010-07-12 04:47:57]

Really glad to see ViewIDs come to the Mac.

http://codereview.chromium.org/2973004/diff/2001/3001
File chrome/app/nibs/Toolbar.xib (left):

http://codereview.chromium.org/2973004/diff/2001/3001#oldcode1
chrome/app/nibs/Toolbar.xib:1: <?xml version="1.0" encoding="UTF-8"?>
Always give an english description of the nib change in the CL.
Although this one is real easy to figure out by inspection, most are not.

http://codereview.chromium.org/2973004/diff/2001/3002
File chrome/browser/cocoa/base_view.h (right):

http://codereview.chromium.org/2973004/diff/2001/3002#newcode23
chrome/browser/cocoa/base_view.h:23: NSInteger tag_;
I think it's fine to use tag for ViewID.
However, you should document this convention somewhere on a dev.chromium.org
page.

http://codereview.chromium.org/2973004/diff/2001/3003
File chrome/browser/cocoa/base_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3003#newcode9
chrome/browser/cocoa/base_view.mm:9: @synthesize tag = tag_;
For your use, all tags must be ViewIDs.  
Perhaps you could add a setter which makes sure all setTag: calls pass in a
valid ViewID.  A CHECK like this will help perform "early catch" in case someone
else thinks about using tags for some other purpose.

Also, document here what we're using tags for.

http://codereview.chromium.org/2973004/diff/2001/3004
File chrome/browser/cocoa/base_view_unittest.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3004#newcode48
chrome/browser/cocoa/base_view_unittest.mm:48: TEST_F(BaseViewTest, Tag) {
I like your attitude.

http://codereview.chromium.org/2973004/diff/2001/3006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3006#newcode253
chrome/browser/cocoa/bookmark_bar_view.mm:253: - (NSInteger)tag {
There's a pattern in these changes; I won't add a comment on each one.
Perhaps we can call [self setTag:XXX] in our initializer?  That seems to make
more sense than doing an override each time.

http://codereview.chromium.org/2973004/diff/2001/3016
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3016#newcode97
chrome/browser/cocoa/tab_contents_controller.mm:97: BaseView* devtoolsView =
On 2010/07/10 18:47:26, James Su wrote:
> On 2010/07/10 06:34:55, Nico wrote:
> > The only subclass of BaseView is SadTabView I believe. s/BaseView/NSView/.
> TabContentsViewCocoa and RenderWidgetHostViewCocoa are subclasses of BaseView.
> This devtoolsView is actually a TabContentsViewCocoa object. Using BaseView
here
> is necessary for setTag method, which is not supported by NSView.
> Maybe I should add a DCHECK here to ensure the object type?
> 

Definitely add a DCHECK.

http://codereview.chromium.org/2973004/diff/2001/3017
File chrome/browser/cocoa/tab_controller.h (right):

http://codereview.chromium.org/2973004/diff/2001/3017#newcode106
chrome/browser/cocoa/tab_controller.h:106: // Returns the index of the tab
controller in .
comment looks unfinished.  Also, "index" is rather generic; something more
specific might be warranted.

http://codereview.chromium.org/2973004/diff/2001/3023
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3023#newcode873
chrome/browser/cocoa/tab_view.mm:873: // It's not possible to support
VIEW_ID_TAB_LAST by this way.
Sure it is.  Ask the controller for the # of views; if equal to the index+1,
then return VIEW_ID_TAB_LAST.  Yes?
Perhaps move this logic into the tab controller itself.

http://codereview.chromium.org/2973004/diff/2001/3023#newcode873
chrome/browser/cocoa/tab_view.mm:873: // It's not possible to support
VIEW_ID_TAB_LAST by this way.
DCHECK(index);

http://codereview.chromium.org/2973004/diff/2001/3027
File chrome/browser/cocoa/view_id_util.h (right):

http://codereview.chromium.org/2973004/diff/2001/3027#newcode16
chrome/browser/cocoa/view_id_util.h:16: // However the tag property is also used
elsewhere, such as the window buttons.
Exactly how is the tag property used in window buttons?
Would be nice to make things consistent (e.g. in case someone wants to get the
ViewID of the window buttons).

http://codereview.chromium.org/2973004/diff/2001/3027#newcode17
chrome/browser/cocoa/view_id_util.h:17: // In order to avoid conflict, we use
negative tag value for ViewID.
How can we know "elsewhere" doesn't use negative tags for "other" purposes?

http://codereview.chromium.org/2973004/diff/2001/3027#newcode20
chrome/browser/cocoa/view_id_util.h:20: inline int ViewIDToTag(ViewID viewID) {
This seems a bit awkward.  Is it really necessary?

http://codereview.chromium.org/2973004/diff/2001/3029
File chrome/browser/cocoa/view_id_util_browsertest.cc (right):

http://codereview.chromium.org/2973004/diff/2001/3029#newcode18
chrome/browser/cocoa/view_id_util_browsertest.cc:18: class ViewIDTest : public
InProcessBrowserTest {
Add a comment explaining why we have this.  E.g. "basic sanity check of ViewID
use on the mac".

Also, you use NSWindow, so shouldn't this be named .mm and not .cc?  If so that
automatically makes it Mac-only in gyp-land.

http://codereview.chromium.org/2973004/diff/2001/3030
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3030#newcode214
chrome/browser/renderer_host/render_widget_host_view_mac.mm:214:
setTag:view_id_util::ViewIDToTag(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW)];
Explain why the RenderWidgetHostViewCocoa doesn't sets it's own tag (is there
one?  extension views perhaps?)

http://codereview.chromium.org/2973004/diff/2001/3031
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3031#newcode85
chrome/browser/tab_contents/tab_contents_view_mac.mm:85: [view
setTag:view_id_util::ViewIDToTag(VIEW_ID_TAB_CONTAINER)];
Shouldn't TabContentsViewCocoa set it's own tag?

http://codereview.chromium.org/2973004/diff/2001/3033
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/2973004/diff/2001/3033#oldcode1367
chrome/chrome_tests.gypi:1367:
'browser/chromeos/login/wizard_in_process_browser_test.cc',
As you know, the intent of view IDs is to enable ui_tests.
Did you try enabling them?  Which ones now work?
What follow-up do you intend in this area?

[James Su, 2010-07-13 07:20:50]

http://codereview.chromium.org/2973004/diff/2001/3001
File chrome/app/nibs/Toolbar.xib (left):

http://codereview.chromium.org/2973004/diff/2001/3001#oldcode1
chrome/app/nibs/Toolbar.xib:1: <?xml version="1.0" encoding="UTF-8"?>
On 2010/07/12 04:47:58, John Grabowski wrote:
> Always give an english description of the nib change in the CL.
> Although this one is real easy to figure out by inspection, most are not.
> 

Done.

http://codereview.chromium.org/2973004/diff/2001/3002
File chrome/browser/cocoa/base_view.h (right):

http://codereview.chromium.org/2973004/diff/2001/3002#newcode23
chrome/browser/cocoa/base_view.h:23: NSInteger tag_;
On 2010/07/12 04:47:58, John Grabowski wrote:
> I think it's fine to use tag for ViewID.
> However, you should document this convention somewhere on a
http://dev.chromium.org
> page.
> 

I'll write a document asap.

http://codereview.chromium.org/2973004/diff/2001/3003
File chrome/browser/cocoa/base_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3003#newcode9
chrome/browser/cocoa/base_view.mm:9: @synthesize tag = tag_;
On 2010/07/12 04:47:58, John Grabowski wrote:
> For your use, all tags must be ViewIDs.  
> Perhaps you could add a setter which makes sure all setTag: calls pass in a
> valid ViewID.  A CHECK like this will help perform "early catch" in case
someone
> else thinks about using tags for some other purpose.
> 
> Also, document here what we're using tags for.
> 

I changed it to viewID and setViewID. However we still need to use bare
tag/setTag for system's NSControl subclasses.

http://codereview.chromium.org/2973004/diff/2001/3004
File chrome/browser/cocoa/base_view_unittest.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3004#newcode48
chrome/browser/cocoa/base_view_unittest.mm:48: TEST_F(BaseViewTest, Tag) {
On 2010/07/12 04:47:58, John Grabowski wrote:
> I like your attitude.

Done.

http://codereview.chromium.org/2973004/diff/2001/3006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3006#newcode253
chrome/browser/cocoa/bookmark_bar_view.mm:253: - (NSInteger)tag {
On 2010/07/12 04:47:58, John Grabowski wrote:
> There's a pattern in these changes; I won't add a comment on each one.
> Perhaps we can call [self setTag:XXX] in our initializer?  That seems to make
> more sense than doing an override each time.
> 

I choose to override the tag method for classes unless if:
1. the ViewID may be changed, such as TabContentsView, which may be
VIEW_ID_TAB_CONTAINER or VIEW_ID_DEV_TOOLS_DOCKED.
2. it's a stock NSControl, such as NSButton.

Overriding the tag method has some advantages:
1. Classes inherited from NSView do not have storage for tag property, eg. 
 BookmarkBarView. So we don't need add extra storage for these classes.
2. It prevents setTag from taking effect. So we can catch errors caused by
misuse of setTag easily.

http://codereview.chromium.org/2973004/diff/2001/3016
File chrome/browser/cocoa/tab_contents_controller.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3016#newcode97
chrome/browser/cocoa/tab_contents_controller.mm:97: BaseView* devtoolsView =
On 2010/07/12 04:47:58, John Grabowski wrote:
> On 2010/07/10 18:47:26, James Su wrote:
> > On 2010/07/10 06:34:55, Nico wrote:
> > > The only subclass of BaseView is SadTabView I believe. s/BaseView/NSView/.
> > TabContentsViewCocoa and RenderWidgetHostViewCocoa are subclasses of
BaseView.
> > This devtoolsView is actually a TabContentsViewCocoa object. Using BaseView
> here
> > is necessary for setTag method, which is not supported by NSView.
> > Maybe I should add a DCHECK here to ensure the object type?
> > 
> 
> Definitely add a DCHECK.
> 

Done.

http://codereview.chromium.org/2973004/diff/2001/3017
File chrome/browser/cocoa/tab_controller.h (right):

http://codereview.chromium.org/2973004/diff/2001/3017#newcode106
chrome/browser/cocoa/tab_controller.h:106: // Returns the index of the tab
controller in .
On 2010/07/12 04:47:58, John Grabowski wrote:
> comment looks unfinished.  Also, "index" is rather generic; something more
> specific might be warranted.

Done.

http://codereview.chromium.org/2973004/diff/2001/3023
File chrome/browser/cocoa/tab_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3023#newcode873
chrome/browser/cocoa/tab_view.mm:873: // It's not possible to support
VIEW_ID_TAB_LAST by this way.
On 2010/07/12 04:47:58, John Grabowski wrote:
> Sure it is.  Ask the controller for the # of views; if equal to the index+1,
> then return VIEW_ID_TAB_LAST.  Yes?
> Perhaps move this logic into the tab controller itself.
> 

Done.

http://codereview.chromium.org/2973004/diff/2001/3023#newcode873
chrome/browser/cocoa/tab_view.mm:873: // It's not possible to support
VIEW_ID_TAB_LAST by this way.
On 2010/07/12 04:47:58, John Grabowski wrote:
> DCHECK(index);
> 

Done.

http://codereview.chromium.org/2973004/diff/2001/3027
File chrome/browser/cocoa/view_id_util.h (right):

http://codereview.chromium.org/2973004/diff/2001/3027#newcode16
chrome/browser/cocoa/view_id_util.h:16: // However the tag property is also used
elsewhere, such as the window buttons.
On 2010/07/12 04:47:58, John Grabowski wrote:
> Exactly how is the tag property used in window buttons?
> Would be nice to make things consistent (e.g. in case someone wants to get the
> ViewID of the window buttons).
> 
The "window buttons" means the three colorful buttons on the top left corner of
a window. Seems that the tags of these buttons are set by Cocoa system
automatically.

http://codereview.chromium.org/2973004/diff/2001/3027#newcode17
chrome/browser/cocoa/view_id_util.h:17: // In order to avoid conflict, we use
negative tag value for ViewID.
On 2010/07/12 04:47:58, John Grabowski wrote:
> How can we know "elsewhere" doesn't use negative tags for "other" purposes?
I checked existing code and nowhere uses negative tag. But I don't know how to
guarantee it in the future.

http://codereview.chromium.org/2973004/diff/2001/3027#newcode20
chrome/browser/cocoa/view_id_util.h:20: inline int ViewIDToTag(ViewID viewID) {
On 2010/07/12 04:47:58, John Grabowski wrote:
> This seems a bit awkward.  Is it really necessary?

In case we need to change the mapping algorithm to avoid conflict in the future.

http://codereview.chromium.org/2973004/diff/2001/3029
File chrome/browser/cocoa/view_id_util_browsertest.cc (right):

http://codereview.chromium.org/2973004/diff/2001/3029#newcode18
chrome/browser/cocoa/view_id_util_browsertest.cc:18: class ViewIDTest : public
InProcessBrowserTest {
On 2010/07/12 04:47:58, John Grabowski wrote:
> Add a comment explaining why we have this.  E.g. "basic sanity check of ViewID
> use on the mac".
Done.
> 
> Also, you use NSWindow, so shouldn't this be named .mm and not .cc?  If so
that
> automatically makes it Mac-only in gyp-land.
Using .mm prevents this file from being compiled correctly. A definition in a
test related header file conflict with Cocoa's definition.

http://codereview.chromium.org/2973004/diff/2001/3030
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3030#newcode214
chrome/browser/renderer_host/render_widget_host_view_mac.mm:214:
setTag:view_id_util::ViewIDToTag(VIEW_ID_TAB_CONTAINER_FOCUS_VIEW)];
On 2010/07/12 04:47:58, John Grabowski wrote:
> Explain why the RenderWidgetHostViewCocoa doesn't sets it's own tag (is there
> one?  extension views perhaps?)
> 

Moved this line to RenderWidgetHostViewCocoa's init method.

http://codereview.chromium.org/2973004/diff/2001/3031
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3031#newcode85
chrome/browser/tab_contents/tab_contents_view_mac.mm:85: [view
setTag:view_id_util::ViewIDToTag(VIEW_ID_TAB_CONTAINER)];
On 2010/07/12 04:47:58, John Grabowski wrote:
> Shouldn't TabContentsViewCocoa set it's own tag?

Done.

http://codereview.chromium.org/2973004/diff/2001/3033
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/2973004/diff/2001/3033#oldcode1367
chrome/chrome_tests.gypi:1367:
'browser/chromeos/login/wizard_in_process_browser_test.cc',
On 2010/07/12 04:47:58, John Grabowski wrote:
> As you know, the intent of view IDs is to enable ui_tests.
> Did you try enabling them?  Which ones now work?
> What follow-up do you intend in this area?
I'm trying to port browser_keyevents_browsertest.cc and browser_focus_uitest.cc
to Mac, but I encountered a problem that when I ran the test in command line,
the browser window was always not focused when starting, thus most tests failed.
Do you know how to let the window get focus when starting?

btw, these two tests are actually InProcessBrowserTests. I didn't try any UITest
yet.

[John Grabowski, 2010-07-13 20:02:46]

Looks pretty good; here's a summary of remaining concerns:

1. tag use in stoplight.  Can you clarify more?  It would be best if tags are
only used for ViewIDs.
2. Would like to not have a tag/viewID mapping if possible.  Can we work toward
that?  (Seems like it's a function of the above.)
3. Try a little more to get the test file named .mm and fix the header conflict.

Maybe 1 and 2 can't be resolved but I'd like to know more before blowing it off.

http://codereview.chromium.org/2973004/diff/2001/3003
File chrome/browser/cocoa/base_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3003#newcode9
chrome/browser/cocoa/base_view.mm:9: @synthesize tag = tag_;
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > For your use, all tags must be ViewIDs.  
> > Perhaps you could add a setter which makes sure all setTag: calls pass in a
> > valid ViewID.  A CHECK like this will help perform "early catch" in case
> someone
> > else thinks about using tags for some other purpose.
> > 
> > Also, document here what we're using tags for.
> > 
> 
> I changed it to viewID and setViewID. However we still need to use bare
> tag/setTag for system's NSControl subclasses.

Let me clarify.

- (void)setTag:(NSInteger)tag {
    DCHECK((tag > VIEW_ID_NONE) && (tag < VIEW_ID_PREDEFINED_COUNT)) <<
"NSView's tag is being used for ViewIDs.  You are probably trying to use them
for some other purpose.  See http://dev.chromium.org/DOCPAGE";
}

http://codereview.chromium.org/2973004/diff/2001/3006
File chrome/browser/cocoa/bookmark_bar_view.mm (right):

http://codereview.chromium.org/2973004/diff/2001/3006#newcode253
chrome/browser/cocoa/bookmark_bar_view.mm:253: - (NSInteger)tag {
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > There's a pattern in these changes; I won't add a comment on each one.
> > Perhaps we can call [self setTag:XXX] in our initializer?  That seems to
make
> > more sense than doing an override each time.
> > 
> 
> I choose to override the tag method for classes unless if:
> 1. the ViewID may be changed, such as TabContentsView, which may be
> VIEW_ID_TAB_CONTAINER or VIEW_ID_DEV_TOOLS_DOCKED.
> 2. it's a stock NSControl, such as NSButton.
> 
> Overriding the tag method has some advantages:
> 1. Classes inherited from NSView do not have storage for tag property, eg. 
>  BookmarkBarView. So we don't need add extra storage for these classes.
> 2. It prevents setTag from taking effect. So we can catch errors caused by
> misuse of setTag easily.

OK.  Since the use isn't consistent, please doc in the code for each place where
you do one or the other, since it isn't otherwise obvious.
E.g. "override to prevent changing of the tag".  This will help make sure the
reasons behind your double pattern survive (and continue).

http://codereview.chromium.org/2973004/diff/2001/3027
File chrome/browser/cocoa/view_id_util.h (right):

http://codereview.chromium.org/2973004/diff/2001/3027#newcode16
chrome/browser/cocoa/view_id_util.h:16: // However the tag property is also used
elsewhere, such as the window buttons.
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > Exactly how is the tag property used in window buttons?
> > Would be nice to make things consistent (e.g. in case someone wants to get
the
> > ViewID of the window buttons).
> > 
> The "window buttons" means the three colorful buttons on the top left corner
of
> a window. Seems that the tags of these buttons are set by Cocoa system
> automatically.
> 

Be more specific in the comments.  E.g. say "stoplight".
Exactly how are the tags used in the stoplight?

http://codereview.chromium.org/2973004/diff/2001/3027#newcode17
chrome/browser/cocoa/view_id_util.h:17: // In order to avoid conflict, we use
negative tag value for ViewID.
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > How can we know "elsewhere" doesn't use negative tags for "other" purposes?
> I checked existing code and nowhere uses negative tag. But I don't know how to
> guarantee it in the future.
> 

That's kind of my point; you cannot guarantee it.
Your best bet is to add clarity and checks.
Clarity --> comments, doc, and easily understandable values.
Docs --> right here.

http://codereview.chromium.org/2973004/diff/2001/3027#newcode20
chrome/browser/cocoa/view_id_util.h:20: inline int ViewIDToTag(ViewID viewID) {
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > This seems a bit awkward.  Is it really necessary?
> 
> In case we need to change the mapping algorithm to avoid conflict in the
future.

I worry this obscures the use of ViewID in tag.
E.g. in a debugger you'll see a value but won't understand what it means.

http://codereview.chromium.org/2973004/diff/2001/3029
File chrome/browser/cocoa/view_id_util_browsertest.cc (right):

http://codereview.chromium.org/2973004/diff/2001/3029#newcode18
chrome/browser/cocoa/view_id_util_browsertest.cc:18: class ViewIDTest : public
InProcessBrowserTest {
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > Add a comment explaining why we have this.  E.g. "basic sanity check of
ViewID
> > use on the mac".
> Done.
> > 
> > Also, you use NSWindow, so shouldn't this be named .mm and not .cc?  If so
> that
> > automatically makes it Mac-only in gyp-land.
> Using .mm prevents this file from being compiled correctly. A definition in a
> test related header file conflict with Cocoa's definition.
> 

What definition?  I suspect that definition was intended for files that cannot
be compiled in C++ (e.g. browser_main.cc) which doesn't apply here.  Perhaps
some header trick (e.g. including Cocoa/Cocoa.h first) fixes things.  Can you
look into this a little more?

http://codereview.chromium.org/2973004/diff/2001/3033
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/2973004/diff/2001/3033#oldcode1367
chrome/chrome_tests.gypi:1367:
'browser/chromeos/login/wizard_in_process_browser_test.cc',
On 2010/07/13 07:20:50, James Su wrote:
> On 2010/07/12 04:47:58, John Grabowski wrote:
> > As you know, the intent of view IDs is to enable ui_tests.
> > Did you try enabling them?  Which ones now work?
> > What follow-up do you intend in this area?
> I'm trying to port browser_keyevents_browsertest.cc and
browser_focus_uitest.cc
> to Mac, but I encountered a problem that when I ran the test in command line,
> the browser window was always not focused when starting, thus most tests
failed.
> Do you know how to let the window get focus when starting?
> 
> btw, these two tests are actually InProcessBrowserTests. I didn't try any
UITest
> yet.

OK.  Please add that info to the CL description.
E.g. "on the path to getting blah_test.cc to work"

I added focus comments in the bug you filed.

http://codereview.chromium.org/2973004/diff/20001/21002
File chrome/browser/cocoa/base_view.h (right):

http://codereview.chromium.org/2973004/diff/20001/21002#newcode21
chrome/browser/cocoa/base_view.h:21: // the tag method to return its ViewID
value. So don not use the tag property
s/don not/do not/g

[James Su, 2010-07-15 05:01:48]

Hi,
Just noticed that I made a mistake.
Turns out that the toolbar buttons rely on the tag property to send commands to
the browser window. See commandDispatch and commandDispatchUsingKeyModifiers
methods of BrowserWindowController.
So if we want to use the tag property for ViewID support, we need to find
another approach for this problem.

For now, I'd rather prefer to use another solution for ViewID support and leave
the tag property as it.

What's your opinion?

I'll try to work out a new solution and send it to you for review before the end
of tomorrow.

Regards
James Su

[James Su, 2010-07-15 05:29:37]

Anyway, I just updated this CL to make it workable. Just for your reference.

On 2010/07/15 05:01:48, James Su wrote:
> Hi,
> Just noticed that I made a mistake.
> Turns out that the toolbar buttons rely on the tag property to send commands
to
> the browser window. See commandDispatch and commandDispatchUsingKeyModifiers
> methods of BrowserWindowController.
> So if we want to use the tag property for ViewID support, we need to find
> another approach for this problem.
> 
> For now, I'd rather prefer to use another solution for ViewID support and
leave
> the tag property as it.
> 
> What's your opinion?
> 
> I'll try to work out a new solution and send it to you for review before the
end
> of tomorrow.
> 
> Regards
> James Su

[John Grabowski, 2010-07-15 17:58:52]

There are lots of options; I've got interview in 5min but let's talk after
lunch.

jrg


On Wed, Jul 14, 2010 at 10:01 PM, <suzhe@chromium.org> wrote:

> Hi,
> Just noticed that I made a mistake.
> Turns out that the toolbar buttons rely on the tag property to send
> commands to
> the browser window. See commandDispatch and
> commandDispatchUsingKeyModifiers
> methods of BrowserWindowController.
> So if we want to use the tag property for ViewID support, we need to find
> another approach for this problem.
>
> For now, I'd rather prefer to use another solution for ViewID support and
> leave
> the tag property as it.
>
> What's your opinion?
>
> I'll try to work out a new solution and send it to you for review before
> the end
> of tomorrow.
>
> Regards
> James Su
>
>
> http://codereview.chromium.org/2973004/show
>

[John Grabowski, 2010-07-15 19:40:53]

I think it seems like a good idea to use a new property.
How about the viewID property, and have all relevant NSViews derive from
BaseView (or whatever has the @property defined)?

You could write a +[NSView viewForViewID:] category method which looks for
the viewID; if not found (e.g. applies to an NSView you could not subclass),
use a map.  Or contains a switch to grab it explictly.

jrg


On Wed, Jul 14, 2010 at 10:01 PM, <suzhe@chromium.org> wrote:

> Hi,
> Just noticed that I made a mistake.
> Turns out that the toolbar buttons rely on the tag property to send
> commands to
> the browser window. See commandDispatch and
> commandDispatchUsingKeyModifiers
> methods of BrowserWindowController.
> So if we want to use the tag property for ViewID support, we need to find
> another approach for this problem.
>
> For now, I'd rather prefer to use another solution for ViewID support and
> leave
> the tag property as it.
>
> What's your opinion?
>
> I'll try to work out a new solution and send it to you for review before
> the end
> of tomorrow.
>
> Regards
> James Su
>
>
> http://codereview.chromium.org/2973004/show
>