[test fixlet] Add tests for the zoom icon in the location bar on GTK.

[test fixlet] Add tests for the zoom icon in the location bar on GTK.

BUG=None
TEST=LocationBarGtkZoomTest.* and ViewIDTest.* pass on bots.


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

[Dan Beam, 2012-09-28 00:48:45]

+thestig@ for chrome/chrome_test.gypi, +vandebo as FYI

[Lei Zhang, 2012-09-28 00:52:27]

.gypi change lgtm

erg: perhaps we should add a new rule in build/filename_rules.gypi?

[Elliot Glaysher, 2012-09-28 16:41:20]

On 2012/09/28 00:52:27, Lei Zhang wrote:
> .gypi change lgtm
> 
> erg: perhaps we should add a new rule in build/filename_rules.gypi?

That would be preferable, I think. Dan, instead of changing chrome_tests.gypi,
could you modify the rule on line 71 of build/filename_rules.gypi that checks
for gtk(_unittest)?

[Dan Beam, 2012-09-28 19:09:50]

On 2012/09/28 16:41:20, Elliot Glaysher wrote:
> On 2012/09/28 00:52:27, Lei Zhang wrote:
> > .gypi change lgtm
> > 
> > erg: perhaps we should add a new rule in build/filename_rules.gypi?
> 
> That would be preferable, I think. Dan, instead of changing chrome_tests.gypi,
> could you modify the rule on line 71 of build/filename_rules.gypi that checks
> for gtk(_unittest)?

Let me know if this is what you wanted.

[Lei Zhang, 2012-09-28 21:01:21]

lgtm

yep, thanks!

[Lei Zhang, 2012-10-01 23:35:54]

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.h (right):

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.h:425:
FRIEND_TEST_ALL_PREFIXES(ZoomBubbleGtkTest, BubbleSanityTest);
IWYU: base/gtest_prod_util.h

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:24: #define
ASSERT_ZOOM_EQ(contents, percent) \
Any particular reason you prefer this over CheckZoomEquals() ?

[Dan Beam, 2012-10-02 00:03:05]

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.h (right):

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.h:425:
FRIEND_TEST_ALL_PREFIXES(ZoomBubbleGtkTest, BubbleSanityTest);
On 2012/10/01 23:35:54, Lei Zhang wrote:
> IWYU: base/gtest_prod_util.h

Done.

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:24: #define
ASSERT_ZOOM_EQ(contents, percent) \
On 2012/10/01 23:35:54, Lei Zhang wrote:
> Any particular reason you prefer this over CheckZoomEquals() ?

Not really, just seemed to be more familiar to the ASSERT_EQ gmock macro.  I
don't really care either way if this generates a lot of bad code after
prepossessing or something...

[Lei Zhang, 2012-10-02 00:09:52]

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:24: #define
ASSERT_ZOOM_EQ(contents, percent) \
On 2012/10/02 00:03:06, Dan Beam wrote:
> On 2012/10/01 23:35:54, Lei Zhang wrote:
> > Any particular reason you prefer this over CheckZoomEquals() ?
> 
> Not really, just seemed to be more familiar to the ASSERT_EQ gmock macro.  I
> don't really care either way if this generates a lot of bad code after
> prepossessing or something...

The C++ style guide prefers not to use them.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preprocessor_M...

[Dan Beam, 2012-10-02 01:42:49]

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:24: #define
ASSERT_ZOOM_EQ(contents, percent) \
On 2012/10/02 00:09:52, Lei Zhang wrote:
> On 2012/10/02 00:03:06, Dan Beam wrote:
> > On 2012/10/01 23:35:54, Lei Zhang wrote:
> > > Any particular reason you prefer this over CheckZoomEquals() ?
> > 
> > Not really, just seemed to be more familiar to the ASSERT_EQ gmock macro.  I
> > don't really care either way if this generates a lot of bad code after
> > prepossessing or something...
> 
> The C++ style guide prefers not to use them.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Preprocessor_M...

Done.

[Lei Zhang, 2012-10-02 02:09:15]

lgtm++

[Dan Beam, 2012-10-02 02:37:15]

erg: ping as you're a local owner in chrome/browser/ui/gtk, let me know if/when
you can review this

[Evan Stade, 2012-10-02 08:05:58]

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
File chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:43: std::string
label(text);
combine L42-43

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:44:
EXPECT_TRUE(label.find(base::IntToString(percent)) != std::string::npos);
nit: slight preference for EXPECT_FALSE ==

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:49: bool dummy;  // Knowing
whether we can zoom in or out more isn't required.
remove comment

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:90: ZoomIn();  // There are
assumptions the bubble isn't showing at default zoom.
I do not understand this comment.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:97:
GetToolbar()->GetLocationBarView()->zoom_.get();
there's a less fragile way to do this: see ViewID in ui/view_ids.h

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:103:
ZoomBubbleGtk::Show(zoom_icon, tab_contents, true);  // Force show a bubble.
you seem to love these same-line comments but the style in chrome is to put
comments above the code.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:112:
AssertZoomEquals(contents, 110);
instead of testing for an exact value, you should just test that it is greater
than it was before, and that the label text contains that value.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:117:
IN_PROC_BROWSER_TEST_F(ZoomBubbleGtkTest, DefaultToZoomedInTwice) {
subset of ZoomInTwiceAndReset

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:154:
IN_PROC_BROWSER_TEST_F(ZoomBubbleGtkTest, DefaultToZoomedOut) {
this test is a subset of DefaultToZoomedOutAndBack

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:163:
IN_PROC_BROWSER_TEST_F(ZoomBubbleGtkTest, DefaultToZoomedOutTwice) {
this test is a subset of ZoomOutTwiceAndReset

[Evan Stade, 2012-10-02 08:12:00]

and the same line of de-dupe reasoning applies to the LocationBarView tests

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:80:
GetLocationBarView()->zoom_.get();
another place to use ViewIDs

[Dan Beam, 2012-10-02 15:53:23]

still need to use view IDs and change location bar test

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
File chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:43: std::string
label(text);
On 2012/10/02 08:05:58, Evan Stade wrote:
> combine L42-43

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:44:
EXPECT_TRUE(label.find(base::IntToString(percent)) != std::string::npos);
On 2012/10/02 08:05:58, Evan Stade wrote:
> nit: slight preference for EXPECT_FALSE ==

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:49: bool dummy;  // Knowing
whether we can zoom in or out more isn't required.
On 2012/10/02 08:05:58, Evan Stade wrote:
> remove comment

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:90: ZoomIn();  // There are
assumptions the bubble isn't showing at default zoom.
On 2012/10/02 08:05:58, Evan Stade wrote:
> I do not understand this comment.

Hopefully clarified.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:103:
ZoomBubbleGtk::Show(zoom_icon, tab_contents, true);  // Force show a bubble.
On 2012/10/02 08:05:58, Evan Stade wrote:
> you seem to love these same-line comments but the style in chrome is to put
> comments above the code.

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:112:
AssertZoomEquals(contents, 110);
On 2012/10/02 08:05:58, Evan Stade wrote:
> instead of testing for an exact value, you should just test that it is greater
> than it was before, and that the label text contains that value.

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:117:
IN_PROC_BROWSER_TEST_F(ZoomBubbleGtkTest, DefaultToZoomedInTwice) {
On 2012/10/02 08:05:58, Evan Stade wrote:
> subset of ZoomInTwiceAndReset

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:154:
IN_PROC_BROWSER_TEST_F(ZoomBubbleGtkTest, DefaultToZoomedOut) {
On 2012/10/02 08:05:58, Evan Stade wrote:
> this test is a subset of DefaultToZoomedOutAndBack

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:163:
IN_PROC_BROWSER_TEST_F(ZoomBubbleGtkTest, DefaultToZoomedOutTwice) {
On 2012/10/02 08:05:58, Evan Stade wrote:
> this test is a subset of ZoomOutTwiceAndReset

Done.

[Elliot Glaysher, 2012-10-02 16:31:28]

estade is also an owner, but lgtm

[Dan Beam, 2012-10-02 16:41:42]

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:80:
GetLocationBarView()->zoom_.get();
On 2012/10/02 08:12:00, Evan Stade wrote:
> another place to use ViewIDs

Done.

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
File chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/17003/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:97:
GetToolbar()->GetLocationBarView()->zoom_.get();
On 2012/10/02 08:05:58, Evan Stade wrote:
> there's a less fragile way to do this: see ViewID in ui/view_ids.h

Done.

[Evan Stade, 2012-10-02 18:35:59]

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:26: // TODO(dbeam):
share some testing code with ZoomBubbleGtkTest.
is this TODO ever going to be accomplished? if not I'd just as soon you remove
it. I don't mind if some code is not shared, it's just tests. But if you do plan
to fix it, by all means leave it here.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:52: }
no line return before }

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:55: }
ditto

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
File chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:166:
ExpectLabelTextContains(90);
I suggested you get rid of these hard-coded values, but you didn't.

What I meant was that after zooming out, you could determine the zoom level Z
(from the zoom controller). Make sure this is less than 100. Then make sure that
the label contains the text Z. That way if we change how much each click zooms,
we don't have to change this test.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
File chrome/browser/ui/view_ids.h (right):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/view_ids.h:82: VIEW_ID_ACTION_BOX_BUTTON,
why is this below PREDEFINED_COUNT >:(

anyway, don't add this comma because it will mess with the svn history.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/chrome_test...
File chrome/chrome_tests.gypi (left):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/chrome_test...
chrome/chrome_tests.gypi:3367:
'browser/ui/gtk/confirm_bubble_gtk_browsertest.cc',
shouldn't the /gtk/ directory be enough to make these compile on gtk only?

[Dan Beam, 2012-10-02 19:34:13]

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:26: // TODO(dbeam):
share some testing code with ZoomBubbleGtkTest.
On 2012/10/02 18:35:59, Evan Stade wrote:
> is this TODO ever going to be accomplished? if not I'd just as soon you remove
> it. I don't mind if some code is not shared, it's just tests. But if you do
plan
> to fix it, by all means leave it here.

yes, it's going to be fixed

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:52: }
On 2012/10/02 18:35:59, Evan Stade wrote:
> no line return before }

Done.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:55: }
On 2012/10/02 18:35:59, Evan Stade wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
File chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/gtk/zoom_bubble_gtk_browsertest.cc:166:
ExpectLabelTextContains(90);
On 2012/10/02 18:35:59, Evan Stade wrote:
> I suggested you get rid of these hard-coded values, but you didn't.
> 
> What I meant was that after zooming out, you could determine the zoom level Z
> (from the zoom controller). Make sure this is less than 100. Then make sure
that
> the label contains the text Z. That way if we change how much each click
zooms,
> we don't have to change this test.

Ah, OK. Done.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
File chrome/browser/ui/view_ids.h (right):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/browser/ui/...
chrome/browser/ui/view_ids.h:82: VIEW_ID_ACTION_BOX_BUTTON,
On 2012/10/02 18:35:59, Evan Stade wrote:
> why is this below PREDEFINED_COUNT >:(
> 
> anyway, don't add this comma because it will mess with the svn history.

Done.

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/chrome_test...
File chrome/chrome_tests.gypi (left):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/chrome_test...
chrome/chrome_tests.gypi:3367:
'browser/ui/gtk/confirm_bubble_gtk_browsertest.cc',
On 2012/10/02 18:35:59, Evan Stade wrote:
> shouldn't the /gtk/ directory be enough to make these compile on gtk only?

you'd think so. so that means this is OK to remove, then, right?

[Lei Zhang, 2012-10-02 19:56:25]

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/chrome_test...
File chrome/chrome_tests.gypi (left):

https://chromiumcodereview.appspot.com/10985069/diff/27020/chrome/chrome_test...
chrome/chrome_tests.gypi:3367:
'browser/ui/gtk/confirm_bubble_gtk_browsertest.cc',
On 2012/10/02 19:34:14, Dan Beam wrote:
> On 2012/10/02 18:35:59, Evan Stade wrote:
> > shouldn't the /gtk/ directory be enough to make these compile on gtk only?
> 
> you'd think so. so that means this is OK to remove, then, right?

Ya. I'd still leave the filename_rules.gypi change as is, in case there are gtk
tests outside of a gtk directory.

[Dan Beam, 2012-10-03 09:09:07]

estade@: I'll address any follow up comments in another CL, let's see if this
even lands first (my guess is it wont) when CQ'd.

[commit-bot: I haz the power, 2012-10-03 09:09:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10985069/41001

[commit-bot: I haz the power, 2012-10-03 11:02:06]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-04 01:33:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10985069/45001

[commit-bot: I haz the power, 2012-10-04 03:39:50]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-04 07:41:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10985069/21007

[commit-bot: I haz the power, 2012-10-04 08:21:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Evan Stade, 2012-10-04 11:51:21]

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:118:
ExpectTooltipContainsZoom();
...

you didn't check the percent was >100

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:129: ZoomIn();
and here you should be checking that zoom after two zoom ins is greater than
after 1

[Dan Beam, 2012-10-04 18:14:41]

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:118:
ExpectTooltipContainsZoom();
On 2012/10/04 11:51:22, Evan Stade wrote:
> ...
> 
> you didn't check the percent was >100

AssertZoomedIn(contents); does this

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:129: ZoomIn();
On 2012/10/04 11:51:22, Evan Stade wrote:
> and here you should be checking that zoom after two zoom ins is greater than
> after 1

Done.

[Evan Stade, 2012-10-04 19:07:43]

lgtm

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/21007/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:118:
ExpectTooltipContainsZoom();
On 2012/10/04 18:14:41, Dan Beam wrote:
> On 2012/10/04 11:51:22, Evan Stade wrote:
> > ...
> > 
> > you didn't check the percent was >100
> 
> AssertZoomedIn(contents); does this

oh, so it does.

[Evan Stade, 2012-10-04 19:08:30]

https://chromiumcodereview.appspot.com/10985069/diff/30020/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/30020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:36:
ASSERT_GT(GetZoomPercent(contents), 100);
actually, shouldn't these be EXPECT? I don't think ASSERT works within a helper
function anyway

[Dan Beam, 2012-10-04 21:24:02]

https://chromiumcodereview.appspot.com/10985069/diff/30020/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10985069/diff/30020/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk_browsertest.cc:36:
ASSERT_GT(GetZoomPercent(contents), 100);
On 2012/10/04 19:08:30, Evan Stade wrote:
> actually, shouldn't these be EXPECT? I don't think ASSERT works within a
helper
> function anyway

Done.

[commit-bot: I haz the power, 2012-10-04 21:30:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10985069/23017

[commit-bot: I haz the power, 2012-10-05 01:47:15]

Change committed as 160294