Update Sad Tab help text and link.

Change chrome://crash (sad tab page) "Learn more" link to:
"If you're seeing this frequently, try these suggestions."
Link "these suggestions" to the "Learn more" help article.

Update Views, GTK, and Cocoa implementations.
chrome://kill (unused) changes on Mac, but not on Views nor GTK.
Not wrapping this line on overflow for Views and GTK seems acceptable.

Strings: Add sad tab help message and embedded link resources.
Views: Rewrite the SadTabView to use ImageView, GridLayout, etc.
GTK: Add new prefix and suffix Label Widgets to an hbox with the link. 
Cocoa Support: Create HyperlinkTextView for non-selectable text & link.
Cocoa IB: Replace SadTab.xib NSButton with a NSTextfield placeholder.
Cocoa: Replace the placeholder with a customized HyperlinkTextView.

BUG=80428
TEST=chrome://crash and chrome://kill on all platforms.

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

[msw, 2011-08-12 01:01:32]

kuan: Please review all changes.
pkasting: Views & generated_resources.
thakis: Cocoa.
estade: GTK.

[Avi (use Gerrit), 2011-08-12 01:07:48]

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h:39: // Called by
SadTabController to provide a weak refernce to the view's parent.
drive-by: typo

[Nico, 2011-08-12 02:29:56]

Great job figuring out all the mac stuff!

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h:40: -
(void)setController:(SadTabController*)controller;
You can make this an IBOutlet in interface builder as well and ctrl-drag it
together there. Then you don't need this function and the call to it.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:97: 
Instead, you would then DCHECK(controller_) here;

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:162: if (help_.get()) {
The .get() shouldn't be necessary I think.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:188: -
(void)initializeHelpText {
For stuff like this, it's very useful to say "Taken from -[InfoBarController
initializeLabel}", that makes the review much easier.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:219:
forKey:NSForegroundColorAttributeName];
This is very similiar to setLabelToMessage:withLink:atOffset:. Can you think of
a good way to factor this common code out of here and infobar_controller to make
changes like this easier for the "next you" who gets to do a simple string
change?

[Peter Kasting, 2011-08-12 16:53:40]

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/views/sad_...
File chrome/browser/ui/views/sad_tab_view.h (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/views/sad_...
chrome/browser/ui/views/sad_tab_view.h:60: string16 help_prefix_;
Instead of doing all the work to size and draw strings ourselves, the help
prefix and suffix, as well as the title and message, should be using Label
objects.  The icon can be an ImageView.

This should hopefully simplify the .cc file noticeably.

[Evan Stade, 2011-08-12 19:21:37]

gtk lgtm

[msw, 2011-08-30 04:09:06]

I made significant Views&Mac updates as requested; PTAL.
"git pull" is broken, so my try jobs are on old rev 98585.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h:39: // Called by
SadTabController to provide a weak refernce to the view's parent.
On 2011/08/12 01:07:48, Avi wrote:
> drive-by: typo

Removed.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.h:40: -
(void)setController:(SadTabController*)controller;
On 2011/08/12 02:29:56, Nico wrote:
> You can make this an IBOutlet in interface builder as well and ctrl-drag it
> together there. Then you don't need this function and the call to it.

Done.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:97: 
On 2011/08/12 02:29:56, Nico wrote:
> Instead, you would then DCHECK(controller_) here;

Done.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:162: if (help_.get()) {
On 2011/08/12 02:29:56, Nico wrote:
> The .get() shouldn't be necessary I think.

Done.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:188: -
(void)initializeHelpText {
On 2011/08/12 02:29:56, Nico wrote:
> For stuff like this, it's very useful to say "Taken from -[InfoBarController
> initializeLabel}", that makes the review much easier.

Done.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/cocoa/tab_...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:219:
forKey:NSForegroundColorAttributeName];
On 2011/08/12 02:29:56, Nico wrote:
> This is very similiar to setLabelToMessage:withLink:atOffset:. Can you think
of
> a good way to factor this common code out of here and infobar_controller to
make
> changes like this easier for the "next you" who gets to do a simple string
> change?

Done.

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/views/sad_...
File chrome/browser/ui/views/sad_tab_view.h (right):

http://codereview.chromium.org/7610011/diff/3001/chrome/browser/ui/views/sad_...
chrome/browser/ui/views/sad_tab_view.h:60: string16 help_prefix_;
On 2011/08/12 16:53:40, Peter Kasting wrote:
> Instead of doing all the work to size and draw strings ourselves, the help
> prefix and suffix, as well as the title and message, should be using Label
> objects.  The icon can be an ImageView.
> 
> This should hopefully simplify the .cc file noticeably.

Done.

[Peter Kasting, 2011-08-30 19:58:12]

We discussed in person using raw pointers in place of scoped_ptrs and
constructing/adding children in ViewHierarchyChanged instead of the constructor.

sky might have an opinion on CenterLayout and the rest of the change, he's done
more Views layout work than I have.

http://codereview.chromium.org/7610011/diff/25001/views/layout/center_layout.cc
File views/layout/center_layout.cc (right):

http://codereview.chromium.org/7610011/diff/25001/views/layout/center_layout....
views/layout/center_layout.cc:32: LOG(INFO) << "Child Area: " << child_area;
Nit: Are these LOGs still necessary?  If so, use VLOG(1) instead of LOG(INFO).

[msw, 2011-08-31 00:54:22]

Comments addressed, PTAL; thanks!

http://codereview.chromium.org/7610011/diff/25001/views/layout/center_layout.cc
File views/layout/center_layout.cc (right):

http://codereview.chromium.org/7610011/diff/25001/views/layout/center_layout....
views/layout/center_layout.cc:32: LOG(INFO) << "Child Area: " << child_area;
On 2011/08/30 19:58:12, Peter Kasting wrote:
> Nit: Are these LOGs still necessary?  If so, use VLOG(1) instead of LOG(INFO).

Removed.

[Peter Kasting, 2011-08-31 17:28:34]

As I said before, sky might know how to deal with all the centering and boxing
and stuff in an easier way.

LGTM

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:30: static const SkColor kCrashColor1 =
SkColorSetRGB(35, 48, 64);
Seems like we can do away with one of the copies of these two color constants
and set the background to a solid color rather than a vertical gradient.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:86: message_view_->Layout();
Why is this necessary?  Shouldn't the Layout() call above trigger this?

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:102: ResourceBundle& rb =
ResourceBundle::GetSharedInstance();
Nit: Push each of these down to just above their first uses

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:105: int image = (kind_ == CRASHED) ?
IDR_SAD_TAB : IDR_KILLED_TAB;
Nit: Just roll this into the call below and avoid the temp (5 places)

[msw, 2011-08-31 17:48:38]

sky: Can you review CenterLayout? My approach may not be correct.
thakis: ping!

[Peter Kasting, 2011-08-31 17:49:57]

On 2011/08/31 17:48:38, msw wrote:
> sky: Can you review CenterLayout? My approach may not be correct.

Well, CenterLayout itself is dead simple, I meant the general assortment of
containing views and layout managers used to construct the overall sad tab
appearance.  Maybe sky knows a better way of laying all that out that does not
involve so many pieces.

[sky, 2011-08-31 20:28:42]

I find that any time you start nesting boxes the code gets unreadable. For this
reason I generally favor GridLayout.

[Nico, 2011-08-31 22:35:18]

LGTM with a few minor tweaks. Thanks!

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
File chrome/browser/ui/cocoa/static_text_view.h (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
chrome/browser/ui/cocoa/static_text_view.h:10: @interface StaticTextView :
NSTextView
I feel this should say "hyperlink" somewhere in the name…maybe
HyperlinkTextView? (tools/git/mass_rename.sh makes this a 15 second project)

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
File chrome/browser/ui/cocoa/static_text_view.mm (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
chrome/browser/ui/cocoa/static_text_view.mm:11: const float kTextBaselineShift =
-1.0;
"const" has implicit internal linkage, no need for the anonymous namespace

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:150: [help_.get()
setAlignment:NSCenterTextAlignment];
can you move the four :NO lines into the StaticTextView init method? They're the
identical at both calling sites

[msw, 2011-09-01 03:30:49]

sky: The views impl with GridView is way nicer, PTAL; thanks!
thakis: Comments addressed, test added, PTAL; thanks!

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
File chrome/browser/ui/cocoa/static_text_view.h (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
chrome/browser/ui/cocoa/static_text_view.h:10: @interface StaticTextView :
NSTextView
On 2011/08/31 22:35:19, Nico wrote:
> I feel this should say "hyperlink" somewhere in the name…maybe
> HyperlinkTextView? (tools/git/mass_rename.sh makes this a 15 second project)

Done.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
File chrome/browser/ui/cocoa/static_text_view.mm (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/sta...
chrome/browser/ui/cocoa/static_text_view.mm:11: const float kTextBaselineShift =
-1.0;
On 2011/08/31 22:35:19, Nico wrote:
> "const" has implicit internal linkage, no need for the anonymous namespace

Done.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tab_contents/sad_tab_view.mm:150: [help_.get()
setAlignment:NSCenterTextAlignment];
On 2011/08/31 22:35:19, Nico wrote:
> can you move the four :NO lines into the StaticTextView init method? They're
the
> identical at both calling sites

Done.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:30: static const SkColor kCrashColor1 =
SkColorSetRGB(35, 48, 64);
On 2011/08/31 17:28:34, Peter Kasting wrote:
> Seems like we can do away with one of the copies of these two color constants
> and set the background to a solid color rather than a vertical gradient.

Done.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:86: message_view_->Layout();
On 2011/08/31 17:28:34, Peter Kasting wrote:
> Why is this necessary?  Shouldn't the Layout() call above trigger this?

I'm glad sky joined the review... The new GridLayout implementation is a better
change.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:102: ResourceBundle& rb =
ResourceBundle::GetSharedInstance();
On 2011/08/31 17:28:34, Peter Kasting wrote:
> Nit: Push each of these down to just above their first uses

Done.

http://codereview.chromium.org/7610011/diff/27004/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:105: int image = (kind_ == CRASHED) ?
IDR_SAD_TAB : IDR_KILLED_TAB;
On 2011/08/31 17:28:34, Peter Kasting wrote:
> Nit: Just roll this into the call below and avoid the temp (5 places)

Done.

[Nico, 2011-09-01 03:36:03]

SGTM

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:14: - (id)init {
Do you use this? Views normally don't have a bare init function.

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm:21: HyperlinkTextView*
view_;
This won't work. view is only alive in the constructor and is autoreleased right
after, so view_ refers to a dead object. Instead, make view_ a scoped_nsobject

http://codereview.chromium.org/7610011/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2634: 'browser/ui/cocoa/hyperlink_text_view.h',
alphabetize

[sky, 2011-09-01 03:38:55]

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:43: painted_(false) {
Initialize all the new fields you're adding. In fact I suspect you don't need
that many of them as fields as they are only used in ViewHierarchyChanged

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:74:
message_->SizeToFit(static_cast<int>(width() * kMessageSize));
Is this needed? I suspect you don't need it or to even override Layout.

[Nico, 2011-09-01 04:12:37]

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm:18: [[test_window()
contentView] addSubview:view_];
Nevermind, your window keeps a ref to the the view.

[msw, 2011-09-01 16:27:42]

PTAL, thanks!

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:14: - (id)init {
On 2011/09/01 03:36:03, Nico wrote:
> Do you use this? Views normally don't have a bare init function.

Removed, wasn't sure if it was used or not.

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm:18: [[test_window()
contentView] addSubview:view_];
On 2011/09/01 04:12:37, Nico wrote:
> Nevermind, your window keeps a ref to the the view.

Undone.

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view_unittest.mm:21: HyperlinkTextView*
view_;
On 2011/09/01 03:36:03, Nico wrote:
> This won't work. view is only alive in the constructor and is autoreleased
right
> after, so view_ refers to a dead object. Instead, make view_ a scoped_nsobject

Done.

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:43: painted_(false) {
On 2011/09/01 03:38:55, sky wrote:
> Initialize all the new fields you're adding. In fact I suspect you don't need
> that many of them as fields as they are only used in ViewHierarchyChanged

Done.

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:74:
message_->SizeToFit(static_cast<int>(width() * kMessageSize));
On 2011/09/01 03:38:55, sky wrote:
> Is this needed? I suspect you don't need it or to even override Layout.

I wasn't able to get the text to SizeToFit expanded widths without this explicit
call. I tried adjusting the ColumnSet's resizing weights and/or setting an
initial size for the multi-line Label, but it only ever resizes to accomodate
thinner widths. I suspect the Label is being left at its preferred size (its
current size) rather than GridView checking SizeToFit/GetHeightForWidth with
available expanded widths. Do you have any suggestions?

http://codereview.chromium.org/7610011/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2634: 'browser/ui/cocoa/hyperlink_text_view.h',
On 2011/09/01 03:36:03, Nico wrote:
> alphabetize

Done.

[sky, 2011-09-01 17:27:47]

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
File chrome/browser/ui/views/sad_tab_view.cc (right):

http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
chrome/browser/ui/views/sad_tab_view.cc:74:
message_->SizeToFit(static_cast<int>(width() * kMessageSize));
On 2011/09/01 16:27:42, msw wrote:
> On 2011/09/01 03:38:55, sky wrote:
> > Is this needed? I suspect you don't need it or to even override Layout.
> 
> I wasn't able to get the text to SizeToFit expanded widths without this
explicit
> call. I tried adjusting the ColumnSet's resizing weights and/or setting an
> initial size for the multi-line Label, but it only ever resizes to accomodate
> thinner widths. I suspect the Label is being left at its preferred size (its
> current size) rather than GridView checking SizeToFit/GetHeightForWidth with
> available expanded widths. Do you have any suggestions?

I believe that's because the column message_ ends up in is not resizable. If you
put it in a resizable column you should get the behavior you're after.

[msw, 2011-09-01 19:54:08]

On 2011/09/01 17:27:47, sky wrote:
>
http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
> File chrome/browser/ui/views/sad_tab_view.cc (right):
> 
>
http://codereview.chromium.org/7610011/diff/40001/chrome/browser/ui/views/sad...
> chrome/browser/ui/views/sad_tab_view.cc:74:
> message_->SizeToFit(static_cast<int>(width() * kMessageSize));
> On 2011/09/01 16:27:42, msw wrote:
> > On 2011/09/01 03:38:55, sky wrote:
> > > Is this needed? I suspect you don't need it or to even override Layout.
> > 
> > I wasn't able to get the text to SizeToFit expanded widths without this
> explicit
> > call. I tried adjusting the ColumnSet's resizing weights and/or setting an
> > initial size for the multi-line Label, but it only ever resizes to
accomodate
> > thinner widths. I suspect the Label is being left at its preferred size (its
> > current size) rather than GridView checking SizeToFit/GetHeightForWidth with
> > available expanded widths. Do you have any suggestions?
> 
> I believe that's because the column message_ ends up in is not resizable. If
you
> put it in a resizable column you should get the behavior you're after.

I again tried locally adjusting the resize weights, but the message only ever
shrinks in width (as I mentioned above) and it has other ill effects. I'm not
sure I could get this to work without adjusting the multi-line
Label::GetPreferredSize to return the full text width for a single line (which
may have other ramifications) or adjusting the GridView layout logic to support
View::GetHeightForWidth for expanded widths. But I'm open to suggestions or a
second set of eyes on my local changes.

[sky, 2011-09-01 19:57:31]

Stick with what you have. I'll take a look when I have some free time.

LGTM

[commit-bot: I haz the power, 2011-09-02 16:01:40]

Can't process patch for file chrome/chrome_tests.gypi.
File's status is None, patchset upload is incomplete.