Make TaskManagerTabContentsResource explicitly handle prerendering and instant pages.

Make TaskManagerTabContentsResource explicitly handle prerendering and instant pages.

BUG=107201, 133436
TEST=no visible change


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

[Avi (use Gerrit), 2012-12-03 19:22:53]

Gavin: Prerendering files, prerendering part of the task manager
Scott: What remains of the task manager, including the instant code

The task manager is currently broken. It initializes the WebContents list with
items from TabContentsIterator, but updates it with every WebContents that is
wrapped in a TabContents. Thus, it hits prerendering and instant accidentally.

This cleans things up a lot. There was no existing way to get all pages being
prerendered, so I added that. I know that this CL removes background printing
pages from the task manager, but that was broken anyway. I have a CL in progress
to add that back in, but that depends on a few changes in the background
printing code, and _that_ CL is in review and I didn't want to block this.

[sky, 2012-12-03 22:06:19]

LGTM

[Avi (use Gerrit), 2012-12-04 15:58:47]

Any prerender team member for review, plz.

[mmenke, 2012-12-04 16:13:32]

On 2012/12/04 15:58:47, Avi wrote:
> Any prerender team member for review, plz.

I'm not following.  All prerenders use a WebContents wrapped in a TabContents,
so why is the prerendering change needed?

[Avi (use Gerrit), 2012-12-04 16:18:10]

To list all prerenders, we need:

- To iterate through the ones that are going on when the task manager is opened.
- To register for the creation of new ones.

The first was never done. With the current code, if a prerender is ongoing when
the task manager is opened, it will not appear. For correctness, the task
manager needs a way to initialize itself with the current set of prerenders, and
the prerender manager currently has no way to get that info. Thus the change to
the prerender manager.

[gavinp, 2012-12-04 16:38:41]

LGTM.

Thanks for explaining why we need the extra method.

The code is good, and so my comments consist entirely of bike shedding, do what
you will with them.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:695: TabContents*
prerender_tab_contents =
The Style guide
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...
seems to suggest that

if (TabContents* prerender_tab_contents =
(*it)->contents()->prerender_contents()) {
  result.push_back(prerender_tab_contents->web_contents());
}

is slightly preferred. With an eight character shorter name for the
now-only-one-line scoped automatic variable, we could lose the {}. However, lots
of reviewers balk at that and tell me I'm reading the style guide wrong, so I'm
just making this comment here.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:199:
std::vector<content::WebContents*> GetAllPrerenderingContents();
Just checking: this method can't be const, because
TaskManagerTabContentsResourceProvider::Add() can't take a const
content::WebContents*, so making this method const would be bad as it would be
returning an array of non-const pointers.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:443: for (size_t
j = 0; j < contentses.size(); ++j)
Another crazy suggestion presented only as a comment:

std::for_each(contentses.begin(), contentses.end(),
std::bind1st(std::mem_fun(&TaskManagerTabContentsResourceProvider::Add), this);

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:497: iter =
resources_.find(web_contents);
While we're here...

if (resources_.count(web_contents) == 0u)
  return;

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:535: iter =
resources_.find(web_contents);
Could use count() here too.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.h (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.h:108: // Tracks a
single tab content, prerendered page, or instant page.
// Tracks a single tab contents, prerendered page, or instant page.

Well done on the oxford, comma.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.h:114: // Called
when the underlying web_contents has been committed, and is thus no
While we're here, why not lose "thus" from this sentence?

[Avi (use Gerrit), 2012-12-04 18:42:30]

Please take a look at the revised version.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:695: TabContents*
prerender_tab_contents =
I see that as a reasonable reading.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:199:
std::vector<content::WebContents*> GetAllPrerenderingContents();
Actually, it hadn't occurred to me.

First, if I made this a const function, I'd return a const vector, but not const
contents.

Second, there's no good reason that TaskManagerTabContentsResource shouldn't
track a const WebContents, but that's more change than I want to make right now.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:443: for (size_t
j = 0; j < contentses.size(); ++j)
Haha, no.

When the C++11 argument dies down, I'll settle for

std::for_each(contentses.begin(), contentses.end(), [] (auto& contents) {
Add(contents); }

At least I'd be able to make an argument for comprehensibility without laughing
inside.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:497: iter =
resources_.find(web_contents);
You mean > 0? If we already have it, return?

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:535: iter =
resources_.find(web_contents);
Not sure why I'd want to. We need to search for it to use its value so we might
as well just use find.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.h (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.h:108: // Tracks a
single tab content, prerendered page, or instant page.
The next rev of this file will add "background printing job" to that list.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.h:114: // Called
when the underlying web_contents has been committed, and is thus no
On 2012/12/04 16:38:41, gavinp wrote:
> While we're here, why not lose "thus" from this sentence?

Done.

[gavinp, 2012-12-04 19:07:50]

LGTM.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:497: iter =
resources_.find(web_contents);
On 2012/12/04 18:42:31, Avi wrote:
> You mean > 0? If we already have it, return?

Yup. Perfect. When you don't need to refer to the element, std::map::count() is
a lot more succinct.

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:535: iter =
resources_.find(web_contents);
On 2012/12/04 18:42:31, Avi wrote:
> Not sure why I'd want to. We need to search for it to use its value so we
might
> as well just use find.

Yeah, I should really read code before writing review comments on it...

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.h (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.h:108: // Tracks a
single tab content, prerendered page, or instant page.
On 2012/12/04 18:42:31, Avi wrote:
> The next rev of this file will add "background printing job" to that list.

Don't forget the "s" on contents, though!

[Avi (use Gerrit), 2012-12-04 19:11:03]

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.h (right):

https://codereview.chromium.org/11418260/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.h:108: // Tracks a
single tab content, prerendered page, or instant page.
Gah! done

[commit-bot: I haz the power, 2012-12-04 19:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11418260/9003

[commit-bot: I haz the power, 2012-12-04 19:37:51]

Retried try job too often on linux_clang for step(s) compile

[commit-bot: I haz the power, 2012-12-04 19:40:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11418260/3008

[commit-bot: I haz the power, 2012-12-04 20:31:42]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, cacheinvalidation_unittests, check_deps, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2012-12-04 21:01:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11418260/10009

[commit-bot: I haz the power, 2012-12-05 01:15:39]

Change committed as 171110