InstantExtended: remove dead code related to the non-cacheable NTP.

InstantExtended: remove dead code related to the non-cacheable NTP.

BUG=none

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

[samarth, 2014-01-09 22:47:56]

Hi jered, kmadhusu,

Can you take a first pass at this cleanup? I think this code is all safe to go
now that we're definitely sticking with the cacheable NTP.

Please also mark any further areas that can be cleaned up that I forgot.

Thanks,
Samarth

[kmadhusu, 2014-01-09 23:31:43]

Thanks for doing this. Few nits.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:15: #include "base/gtest_prod_util.h"
You can remove this and compiler_specific.h

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:39: class WebContents;
Forward declaration is no longer required.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:105:
FRIEND_TEST_ALL_PREFIXES(InstantExtendedNetworkTest,
No longer required.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:108: MANUAL_ShowsGoogleNTP);
No longer required.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service_unittest.cc:23: #include
"content/public/browser/web_contents.h"
You can remove this and web_contents_observer.h

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:10: #include "base/rand_util.h"
This can be removed.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (left):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:369:
IN_PROC_BROWSER_TEST_F(InstantExtendedNetworkTest, NTPReactsToNetworkChanges) {
Please remove the corresponding friend declaration from instant_controller.h

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_test_utils.cc (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_test_utils.cc:12: #include
"chrome/browser/search/instant_service.h"
You can remove this and instant_service_factory.h

[samarth, 2014-01-10 05:13:32]

Thanks, addressed comments.  Also fixed some failing tests.

Thanks,
Samarth

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:15: #include "base/gtest_prod_util.h"
On 2014/01/09 23:31:43, kmadhusu wrote:
> You can remove this and compiler_specific.h

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:39: class WebContents;
On 2014/01/09 23:31:43, kmadhusu wrote:
> Forward declaration is no longer required.

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:105:
FRIEND_TEST_ALL_PREFIXES(InstantExtendedNetworkTest,
On 2014/01/09 23:31:43, kmadhusu wrote:
> No longer required.

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service.h:108: MANUAL_ShowsGoogleNTP);
On 2014/01/09 23:31:43, kmadhusu wrote:
> No longer required.

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
File chrome/browser/search/instant_service_unittest.cc (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/instan...
chrome/browser/search/instant_service_unittest.cc:23: #include
"content/public/browser/web_contents.h"
On 2014/01/09 23:31:43, kmadhusu wrote:
> You can remove this and web_contents_observer.h

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/search/search...
chrome/browser/search/search.cc:10: #include "base/rand_util.h"
On 2014/01/09 23:31:43, kmadhusu wrote:
> This can be removed.

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (left):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:369:
IN_PROC_BROWSER_TEST_F(InstantExtendedNetworkTest, NTPReactsToNetworkChanges) {
On 2014/01/09 23:31:43, kmadhusu wrote:
> Please remove the corresponding friend declaration from instant_controller.h

Done.

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_test_utils.cc (right):

https://codereview.chromium.org/132733002/diff/1/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_test_utils.cc:12: #include
"chrome/browser/search/instant_service.h"
On 2014/01/09 23:31:43, kmadhusu wrote:
> You can remove this and instant_service_factory.h

Done.

[Jered, 2014-01-10 16:29:40]

Ahhh, sigh of relief. Thanks for doing this. I think instant_controller.cc has
some leftover baggage, too:
InstantPageLoadFailed()
RedirectToLocalNTP()
There are also some stale friend declarations in instant_controller.h

https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
chrome/browser/search/search.cc:355: url.host() ==
chrome::kChromeSearchOnlineNtpHost);
I think we can get rid of this constant now.

https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
chrome/browser/search/search.cc:543: std::string
online_ntp_host(chrome::kChromeSearchOnlineNtpHost);
Here, too.

[samarth, 2014-01-10 20:50:20]

On 2014/01/10 16:29:40, Jered wrote:
> I think instant_controller.cc has
> some leftover baggage, too:
> InstantPageLoadFailed()
> RedirectToLocalNTP()
> There are also some stale friend declarations in instant_controller.h

Cleaned up some more.

Thanks,
Samarth

https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
chrome/browser/search/search.cc:355: url.host() ==
chrome::kChromeSearchOnlineNtpHost);
On 2014/01/10 16:29:41, Jered wrote:
> I think we can get rid of this constant now.

Actually this brings to mind a bug with the cacheable ntp.  See
crbug.com/333397.  Let's clean it up with that.

https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
chrome/browser/search/search.cc:543: std::string
online_ntp_host(chrome::kChromeSearchOnlineNtpHost);
On 2014/01/10 16:29:41, Jered wrote:
> Here, too.

Same.

[kmadhusu, 2014-01-10 21:03:01]

lgtm.

Thanks.

[Jered, 2014-01-10 21:11:10]

On 2014/01/10 20:50:20, samarth wrote:
> On 2014/01/10 16:29:40, Jered wrote:
> > I think instant_controller.cc has
> > some leftover baggage, too:
> > InstantPageLoadFailed()
> > RedirectToLocalNTP()
> > There are also some stale friend declarations in instant_controller.h
> 
> Cleaned up some more.
> 
> Thanks,
> Samarth
> 
>
https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
> File chrome/browser/search/search.cc (right):
> 
>
https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
> chrome/browser/search/search.cc:355: url.host() ==
> chrome::kChromeSearchOnlineNtpHost);
> On 2014/01/10 16:29:41, Jered wrote:
> > I think we can get rid of this constant now.
> 
> Actually this brings to mind a bug with the cacheable ntp.  See
> crbug.com/333397.  Let's clean it up with that.
> 
>
https://codereview.chromium.org/132733002/diff/130001/chrome/browser/search/s...
> chrome/browser/search/search.cc:543: std::string
> online_ntp_host(chrome::kChromeSearchOnlineNtpHost);
> On 2014/01/10 16:29:41, Jered wrote:
> > Here, too.
> 
> Same.

lgtm

[samarth, 2014-01-10 21:22:06]

+sky to review for:
    c/b/task_manager
    c/b/ui/browser_navigator.cc
    c/b/ui/webui

Thanks!
Samarth

[sky, 2014-01-10 22:11:37]

LGTM

[commit-bot: I haz the power, 2014-01-10 23:38:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/132733002/490001

[commit-bot: I haz the power, 2014-01-10 23:39:29]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-01-11 00:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/132733002/620001

[commit-bot: I haz the power, 2014-01-12 02:10:59]

Change committed as 244407