Alternate NTP: Show detached bookmark bar

Alternate NTP: Show detached bookmark bar

With the new full page NTP the detached bookmark bar was no longer visible.

The problem was that we relied on an instance of NewTabUI to control the bookmark bar visibility. The new full page NTP no longer has an instance of NewTabUI so the bookmark bar was never visible.

Fix was to move the relevant code out of NewTabUI and into BookmarkTabHelper.

BUG=175146
TEST=Ran with instant extended enabled. Verified that the detached bookmark bar was visible.
Added a new test BookmarkInstantExtendedBrowsertest.DetachedBookmarkBar. Verified that the test fails without my patch and passes with it.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182165

[sail, 2013-02-12 02:00:02]

Note, I'm still working on tests for this. I wanted to send it out early to
check if I'm on the right track with this fix.

[sail, 2013-02-12 03:24:39]

estade: webui/ntp/* OWNERS review
sky: ui/bookmarks/* OWNERS review

Added tests. Please take a look.

[sky, 2013-02-12 03:27:49]

I don't see any problem other than tests.

https://codereview.chromium.org/12217127/diff/3001/chrome/browser/ui/bookmark...
File chrome/browser/ui/bookmarks/bookmark_browsertest.cc (right):

https://codereview.chromium.org/12217127/diff/3001/chrome/browser/ui/bookmark...
chrome/browser/ui/bookmarks/bookmark_browsertest.cc:70: };
private:DISALLOW... and move this above tests that subclass it. I also encourage
to investigate making this a unit test.

[Evan Stade, 2013-02-12 03:32:03]

ntp lgtm

[sail, 2013-02-12 03:34:04]

https://codereview.chromium.org/12217127/diff/3001/chrome/browser/ui/bookmark...
File chrome/browser/ui/bookmarks/bookmark_browsertest.cc (right):

https://codereview.chromium.org/12217127/diff/3001/chrome/browser/ui/bookmark...
chrome/browser/ui/bookmarks/bookmark_browsertest.cc:70: };
On 2013/02/12 03:27:49, sky wrote:
> private:DISALLOW... and move this above tests that subclass it. I also
encourage
> to investigate making this a unit test.

Done.

One advantage of making this a browser test is that it properly tests the
behavior of Browser::UpdateBookmarkBarState().

[sky, 2013-02-12 16:49:23]

Why can't you do that in a unit test?

  -Scott

On Mon, Feb 11, 2013 at 7:34 PM,  <sail@chromium.org> wrote:
>
>
https://codereview.chromium.org/12217127/diff/3001/chrome/browser/ui/bookmark...
> File chrome/browser/ui/bookmarks/bookmark_browsertest.cc (right):
>
>
https://codereview.chromium.org/12217127/diff/3001/chrome/browser/ui/bookmark...
> chrome/browser/ui/bookmarks/bookmark_browsertest.cc:70: };
> On 2013/02/12 03:27:49, sky wrote:
>>
>> private:DISALLOW... and move this above tests that subclass it. I also
>
> encourage
>>
>> to investigate making this a unit test.
>
>
> Done.
>
> One advantage of making this a browser test is that it properly tests
> the behavior of Browser::UpdateBookmarkBarState().
>
> https://codereview.chromium.org/12217127/

[sail, 2013-02-12 17:46:44]

On 2013/02/12 16:49:23, sky wrote:
> Why can't you do that in a unit test?

Do you mean create a browser instance? I haven't seen that done in a unit test
before.

Also, the rest of the test requires things like profiles, IO thread, etc... that
browser test already has setup.

[sky, 2013-02-12 18:52:48]

On Tue, Feb 12, 2013 at 9:46 AM,  <sail@chromium.org> wrote:
> On 2013/02/12 16:49:23, sky wrote:
>>
>> Why can't you do that in a unit test?
>
>
> Do you mean create a browser instance? I haven't seen that done in a unit
> test
> before.
>
> Also, the rest of the test requires things like profiles, IO thread, etc...
> that
> browser test already has setup.

See
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/base/browser_with...
. And yes, you can do all this stuff in a unit test. Remember that
unit tests are typically an order of magnitude faster than a browser
test and generally less flaky.

  -Scott
>
>
> https://codereview.chromium.org/12217127/

[sail, 2013-02-12 20:26:58]

Changed from browser_test to a unit_test. Please take another look.

[sky, 2013-02-12 22:29:11]

Nice, LGTM

https://codereview.chromium.org/12217127/diff/10004/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_factory.cc (right):

https://codereview.chromium.org/12217127/diff/10004/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_factory.cc:96: return false;
I would be surprised if this doesn't effect a bunch of other things.

https://codereview.chromium.org/12217127/diff/10004/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_unittest.cc (right):

https://codereview.chromium.org/12217127/diff/10004/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_unittest.cc:22: };
private:DISALLOW...

[sky, 2013-02-12 22:29:47]

Actually, one question. Is there a way to force the factory to be created just
for your test?

[sail, 2013-02-12 22:52:03]

> Actually, one question. Is there a way to force the factory to be created just
for your test?

Ahh, good idea. ProfileKeyedServiceFactory allows tests to specify their own
factory functions. I added a custom factory function to create a
TemplateURLService just for this test.

https://codereview.chromium.org/12217127/diff/10004/chrome/browser/ui/bookmar...
File chrome/browser/ui/bookmarks/bookmark_unittest.cc (right):

https://codereview.chromium.org/12217127/diff/10004/chrome/browser/ui/bookmar...
chrome/browser/ui/bookmarks/bookmark_unittest.cc:22: };
On 2013/02/12 22:29:11, sky wrote:
> private:DISALLOW...

Done.

[sky, 2013-02-13 00:32:50]

LGTM

[commit-bot: I haz the power, 2013-02-13 00:45:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12217127/2033

[commit-bot: I haz the power, 2013-02-13 08:31:49]

Change committed as 182165