Separate StatusAreaView from StatusAreaViewChromeos

WIP: Separate StatusAreaView from StatusAreaViewChromeos

This is in preperation for moving StatusAreaView to ui: so that it is available for Aura.
* Remove button specific accessors from StatusAreaView
* Replace StatusAreaHost with StatusAreaButton::Delegate, eliminating unnecessary methods

BUG=97263
TEST=Thoroughly test the status area on ChromeOS (there should be no visible or functional changes)

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

[stevenjb, 2011-11-03 17:07:06]

Dave, can you take a look at this one? This round of changes are ChromeOS only,
but should make it easy to pull StatusAreaView and StatusAreaButton into ui/ for
Aura in a smaller follow up CL.

[tfarina, 2011-11-03 19:45:05]

http://codereview.chromium.org/8438064/diff/3001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8438064/diff/3001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:33: const views::View*
button_view, int command_id) const = 0;
could you add a blank line between each function?

http://codereview.chromium.org/8438064/diff/3001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:41: };
please, add a protected virtual dtor.

[stevenjb, 2011-11-04 00:46:42]

http://codereview.chromium.org/8438064/diff/3001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8438064/diff/3001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:33: const views::View*
button_view, int command_id) const = 0;
On 2011/11/03 19:45:05, tfarina wrote:
> could you add a blank line between each function?

Done.

http://codereview.chromium.org/8438064/diff/3001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:41: };
On 2011/11/03 19:45:05, tfarina wrote:
> please, add a protected virtual dtor.

Done.

[DaveMoore, 2011-11-07 15:36:18]

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/background_view.cc (right):

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/background_view.cc:217: ClockMenuButton*
clock_view = static_cast<ClockMenuButton*>(
I'd rather see GetClockMenuButton() in StatusAreaViewChromeos than casts

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:41: virtual int
GetFontStyle(const gfx::Font& font) const = 0;
This would read better as ModifyFont() and return modified  a potentially
differnt Font, that typically would be the same as what was passed in.

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:49: virtual ~Delegate() {}
The virtual destructor shouldn't be inlined...move the implementation to the .cc
file.

[tfarina, 2011-11-07 15:45:44]

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:31: Delegate() {}
You don't need this.

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:49: virtual ~Delegate() {}
On 2011/11/07 15:36:18, DaveMoore wrote:
> The virtual destructor shouldn't be inlined...move the implementation to the
.cc
> file.

Why?

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:52:
DISALLOW_COPY_AND_ASSIGN(Delegate);
Nor this.

[stevenjb, 2011-11-08 00:24:22]

Addressed feedback and added RemoveButton() to StatusAreaView() to facilitate
dynamically adding / removing status area buttons (required by for a separate
feature / CL).

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/background_view.cc (right):

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/background_view.cc:217: ClockMenuButton*
clock_view = static_cast<ClockMenuButton*>(
On 2011/11/07 15:36:18, DaveMoore wrote:
> I'd rather see GetClockMenuButton() in StatusAreaViewChromeos than casts
I think all of this is better off in StatusAreaViewChromeos().
Done.

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/status_area_button.h (right):

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:31: Delegate() {}
On 2011/11/07 15:45:44, tfarina wrote:
> You don't need this.
Was needed for DISALLOW_COPY_AND_ASSIGN, but see note below.

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:41: virtual int
GetFontStyle(const gfx::Font& font) const = 0;
On 2011/11/07 15:36:18, DaveMoore wrote:
> This would read better as ModifyFont() and return modified  a potentially
> differnt Font, that typically would be the same as what was passed in.

Done, but named this GetStatusAreaFont(), and renamed GetTextStyle() as
GetStatusAreaTextStyle() for consistency and clarity.

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:49: virtual ~Delegate() {}
On 2011/11/07 15:45:44, tfarina wrote:
> On 2011/11/07 15:36:18, DaveMoore wrote:
> > The virtual destructor shouldn't be inlined...move the implementation to the
> .cc
> > file.
> 
> Why?
After some discussion I believe that it is OK (and preferred) to inline the
destructor of a pure interface class, since it has no members and no
implementation.

http://codereview.chromium.org/8438064/diff/7001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/status_area_button.h:52:
DISALLOW_COPY_AND_ASSIGN(Delegate);
On 2011/11/07 15:45:44, tfarina wrote:
> Nor this.
We are inconsistent as to whether we need this in interface classes, and
opinions on the matter vary. I prefer conciseness so will remove both this and
the constructor.

[DaveMoore, 2011-11-08 06:08:27]

lgtm

[commit-bot: I haz the power, 2011-11-08 18:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/8438064/14001

[commit-bot: I haz the power, 2011-11-08 18:16:05]

Can't apply patch for file
chrome/browser/chromeos/status/accessibility_menu_button.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/status/accessibility_menu_button.cc
Hunk #1 FAILED at 7.
Hunk #2 succeeded at 35 (offset 4 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/status/accessibility_menu_button.cc.rej