Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1185183008:
New Task Manager - Phase 1.3.2.B: Implement Tab Contents Task Providing (Prerender) (Closed)
|
Created:
5 years, 6 months ago by afakhry Modified:
4 years, 9 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, davidben+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew Task Manager - Phase 1.3.2.B: Implement Tab Contents Task Providing (Prerender).
Part 2: Implementing a task-manager representation of WebContents owned
by the prerender manager.
R=nick@chromium.org
BUG=470990
TEST=browser_tests --gtest_filter=PrerenderBrowserTest.TaskManagement*
Committed: https://crrev.com/674c7b13184545b77d89afbe1d04cac93d78cda1
Cr-Commit-Position: refs/heads/master@{#335788}
Patch Set 1 #Patch Set 2 : Using task-manager specific code in prerender_contents only if TM is enabled in build. #
Total comments: 33
Patch Set 3 : Handling nick's comments #
Total comments: 16
Patch Set 4 : Addressing gavin's comments and moving web_contents_tags.h/.cc files outside the task manager files. #Patch Set 5 : Try fixing compile #Patch Set 6 : Fix compile 2 #Patch Set 7 : Fix gn compile #
Total comments: 16
Patch Set 8 : gavinp's and thestig's comments #Messages
Total messages: 26 (6 generated)
Hi Nick, Please review. Thanks!
Using task-manager specific code in prerender_contents only if TM is enabled in build.
https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4092: : public PrerenderBrowserTest, I would probably structure this class as a helper utility you create on the stack as needed, rather than extending the test fixture. This class should derive only from TaskProviderObserver, and get a name like a MockTaskManager. Tests can then just instantiate it at the top of their test bodies. Since there's nothing prerender-specific here, you could also put this in a browsertest_util.h file and use it elsewhere. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4102: provided_tasks_.insert(task); FWIW (this is just an idea / food for thought) I've had very good luck with the pattern of having test observer classes like this create a text log of what happens, and writing ASSERTs in terms of those test logs. It's a lame version of gmock; which would work well for this too, though a lot of people find gmock to be a headache. An example of what I'm talking about is here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4141: EXPECT_EQ(1U, tags_manager()->tracked_tags().size()); FYI, this assert may come back to bite you, once you add tags for normal TabContents tabs, since a browsertest, by default, starts with a browser window with a tab. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4174: EXPECT_TRUE(title.find(expected_prefix) == 0); [Option A] EXPECT_TRUE(base::StartsWith(title, expected_prefix, base::CompareCase::INSENSITIVE)); https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... (strings can be implicitly converted to StringPieces) [Option B] ASSERT_TRUE(base::MatchPattern(title, task_manager::browsertest_util::MatchAnyPrerender()); (you'd have to define MatchAnyPrerender(), but it ought to exist) https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:283: #if defined(ENABLE_TASK_MANAGER) This is okay, but I wonder, would it be more elegant if we put these #defines inside of the bodies of the methods of WebContentsTags. Basically, all of WebContentsTags would be a no-op if ENABLE_TASK_MANAGER were false? https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:284: // Tag the prerender contents with the task-manager specific prerender tag, so "task-manager" -> "task manager". Also below. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/prerender_tag.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_tag.cc:19: WebContentsTagsManager::GetInstance()->ClearFromProvider(this); I was thinking that the ClearFromProvider() call would happen not here, but in WebContentsTags::ClearTag, just before the user-data deletion? Is that workable? The idea being, the provider shouldn't see double-deregistration of a tag, in the scenario where the prerender webcontents is being deleted. It looks like the code would DCHECK in that situation? You could test this by installing the real taskprovider during one of the existing prerender tests that winds up with FINAL_STATUS_CANCELLED. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/prerender_task.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_task.cc:17: gfx::ImageSkia* prerender_icon = nullptr; g_prerender_icon (since it's a global) https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_task.cc:39: PrefixTitle(RendererTask::GetTitleFromWebContents(web_contents)), The indentation looks weird here (I expect 4 spaces relative to the R at the start of RendererTask -- did git cl format do this? https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/prerender_task.h (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_task.h:13: // prerendering manager. prerendering manager -> PrerenderManager https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.cc:35: // If this check fails, then it's an indication that the |tag|'s destructor If you move the call out of the dtor (as suggested elsewhere), then make sure you update this comment too. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h:36: // This should be called by the destructor of certain tags whose destruction If you move the call to ClearTag() as I suggest elsewhere, make sure update this comment too. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h:37: // doesn't not correspond to the destruction of their WebContents (like the "doesn't not" https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:55: friend class WebContentsTaskProvider; WebContentsTaskProvider is the only class that can even see the declaration of WebContentsEntry, so if you make it a friend, "private:" seems like it doesn't mean anything here. I think it might be better to just add a public entry point for what you need, like how CreateAllTasks() works, and drop the friend declaration. [You could just call WebContentsDestroyed() here, too, but that feels kind of terrible] https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:295: // Must manually clear the tasks and notifying the observer. notifying->notify https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/web_contents_tags.h (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/web_contents_tags.h:50: // the given |web_contents| if any. Add: "Clearing the tag is necessary only when you need to re-tag an existing WebContents, to indicate a change in ownership."
https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4092: : public PrerenderBrowserTest, On 2015/06/19 19:50:51, ncarter wrote: > I would probably structure this class as a helper utility you create on the > stack as needed, rather than extending the test fixture. This class should > derive only from TaskProviderObserver, and get a name like a MockTaskManager. > Tests can then just instantiate it at the top of their test bodies. > > Since there's nothing prerender-specific here, you could also put this in a > browsertest_util.h file and use it elsewhere. Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4141: EXPECT_EQ(1U, tags_manager()->tracked_tags().size()); On 2015/06/19 19:50:51, ncarter wrote: > FYI, this assert may come back to bite you, once you add tags for normal > TabContents tabs, since a browsertest, by default, starts with a browser window > with a tab. Yes, thanks. I'm aware of that and hence the below (misplaced) TODO. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4174: EXPECT_TRUE(title.find(expected_prefix) == 0); On 2015/06/19 19:50:51, ncarter wrote: > [Option A] EXPECT_TRUE(base::StartsWith(title, expected_prefix, > base::CompareCase::INSENSITIVE)); > > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... > > (strings can be implicitly converted to StringPieces) > > > [Option B] > ASSERT_TRUE(base::MatchPattern(title, > task_manager::browsertest_util::MatchAnyPrerender()); > > (you'd have to define MatchAnyPrerender(), but it ought to exist) Done [Option A]. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:283: #if defined(ENABLE_TASK_MANAGER) On 2015/06/19 19:50:51, ncarter wrote: > This is okay, but I wonder, would it be more elegant if we put these #defines > inside of the bodies of the methods of WebContentsTags. Basically, all of > WebContentsTags would be a no-op if ENABLE_TASK_MANAGER were false? Unfortunately, it has to be done at the call site or otherwise there will be a linking error ... task_manager_source_files are only included if enable_task_manager==1 in the gyp files. enable_task_manager is 0 for ios and android on the gn builds and I think is 0 only for android on the gyp builds. See the build bots that failed the build in the previous patch. See also: https://code.google.com/p/chromium/codesearch#search/&q=%22enable_task_manage... https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:284: // Tag the prerender contents with the task-manager specific prerender tag, so On 2015/06/19 19:50:51, ncarter wrote: > "task-manager" -> "task manager". Also below. Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/prerender_tag.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_tag.cc:19: WebContentsTagsManager::GetInstance()->ClearFromProvider(this); On 2015/06/19 19:50:51, ncarter wrote: > I was thinking that the ClearFromProvider() call would happen not here, but in > WebContentsTags::ClearTag, just before the user-data deletion? Is that workable? > > The idea being, the provider shouldn't see double-deregistration of a tag, in > the scenario where the prerender webcontents is being deleted. It looks like the > code would DCHECK in that situation? You could test this by installing the real > taskprovider during one of the existing prerender tests that winds up with > FINAL_STATUS_CANCELLED. Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/prerender_task.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_task.cc:17: gfx::ImageSkia* prerender_icon = nullptr; On 2015/06/19 19:50:51, ncarter wrote: > g_prerender_icon (since it's a global) Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_task.cc:39: PrefixTitle(RendererTask::GetTitleFromWebContents(web_contents)), On 2015/06/19 19:50:51, ncarter wrote: > The indentation looks weird here (I expect 4 spaces relative to the R at the > start of RendererTask -- did git cl format do this? No that would be me. Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/prerender_task.h (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/prerender_task.h:13: // prerendering manager. On 2015/06/19 19:50:51, ncarter wrote: > prerendering manager -> PrerenderManager Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.cc:35: // If this check fails, then it's an indication that the |tag|'s destructor On 2015/06/19 19:50:51, ncarter wrote: > If you move the call out of the dtor (as suggested elsewhere), then make sure > you update this comment too. Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h:36: // This should be called by the destructor of certain tags whose destruction On 2015/06/19 19:50:51, ncarter wrote: > If you move the call to ClearTag() as I suggest elsewhere, make sure update this > comment too. Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_tags_manager.h:37: // doesn't not correspond to the destruction of their WebContents (like the On 2015/06/19 19:50:51, ncarter wrote: > "doesn't not" Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:55: friend class WebContentsTaskProvider; On 2015/06/19 19:50:51, ncarter wrote: > WebContentsTaskProvider is the only class that can even see the declaration of > WebContentsEntry, so if you make it a friend, "private:" seems like it doesn't > mean anything here. > > I think it might be better to just add a public entry point for what you need, > like how CreateAllTasks() works, and drop the friend declaration. > > [You could just call WebContentsDestroyed() here, too, but that feels kind of > terrible] Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:295: // Must manually clear the tasks and notifying the observer. On 2015/06/19 19:50:51, ncarter wrote: > notifying->notify Done. https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/web_contents_tags.h (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/task_man... chrome/browser/task_management/web_contents_tags.h:50: // the given |web_contents| if any. On 2015/06/19 19:50:51, ncarter wrote: > Add: "Clearing the tag is necessary only when you need to re-tag an existing > WebContents, to indicate a change in ownership." Done.
lgtm! https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:283: #if defined(ENABLE_TASK_MANAGER) On 2015/06/19 21:49:22, afakhry wrote: > On 2015/06/19 19:50:51, ncarter wrote: > > This is okay, but I wonder, would it be more elegant if we put these #defines > > inside of the bodies of the methods of WebContentsTags. Basically, all of > > WebContentsTags would be a no-op if ENABLE_TASK_MANAGER were false? > > Unfortunately, it has to be done at the call site or otherwise there will be a > linking error ... task_manager_source_files are only included if > enable_task_manager==1 in the gyp files. enable_task_manager is 0 for ios and > android on the gn builds and I think is 0 only for android on the gyp builds. > See the build bots that failed the build in the previous patch. > > See also: > https://code.google.com/p/chromium/codesearch#search/&q=%22enable_task_manage... What I was advocating, was to add the web_content_tags.cc (and just that file) back so that it's compiled on the ios/android build. But I am okay with the #defines as they are.
afakhry@chromium.org changed reviewers: + gavinp@chromium.org
gavinp@chromium.org: Could you please review the following files? chrome/browser/prerender/prerender_browsertest.cc chrome/browser/prerender/prerender_contents.cc Some context: For the new task manager we're rewriting here, services that own WebContents must register their WebContents with the task manager by tagging them with a task-manager-specific UserData. Previously, the old task manager used to rely on the contents layer to announce the creation of new WebContents through the notification service, and then the task manager needed to figure out which services WebContents belong to.
Hi, Thanks for this! What are the advantages of this new arrangement for the task manager? I've commented a lot below. Everything with "Nit" is optional. Pay closest attention to the comment in the browsertest about ASSERT_FALSE. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4100: CHECK(task); Remove this check, crashing in unit tests is bad form. Is this check really a part of testing the prerender code, what could cause this to be null? If it's some other system with its own tests that could cause this, just remove the check, and we'll crash later when we dereference it. If it is part of the test for this system, then we have to be a bit silly to avoid the crash, something like: if (!task) { ADD_FAILURE() << "task was nullptr."; return; } Should do the trick? https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4101: ASSERT_FALSE(provided_tasks_.count(task)); ASSERT_FALSE won't work as many expect here; on failure it adds a failure to the test run, which is expected, but then it just returns from this function and continues execution. The test will continue. See https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... in the Google Test manual for details. Any reason this needs to be fatal? I suggest making it EXPECT_FALSE...; of course the world gets confusing fast, but that's OK? https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4142: // TODO(afakhry): Once we start tagging the tab contents the below tests Nit: (optional) Remove the extra indent on the second line, wrap comment at 80 columns. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4155: EXPECT_TRUE(task_manager.provided_tasks().empty()); It seems like a bit of overkill to assert this every time we create a MockTaskManager(). It has some documentary value, but it's not huge. Is there any reason not to just do these EXPECT-it's-empty calls in the constructor of the MockTaskManager? https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:684: // Here we have to clear the task manager tag we added earlier to our Nit: Remove "Here we have to". https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:685: // WebContents since it's no longer a prerender content. Nit: "content" --> "contents" https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:40: void ClearAllTasks(bool notify_observer); Nit: I think the method definition should move to as well to keep in style, see https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... . https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... File chrome/browser/task_management/web_contents_tags.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... chrome/browser/task_management/web_contents_tags.cc:66: DCHECK(web_contents); Nit: I don't think this DCHECK is needed. Won't the next line crash?
https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/1185183008/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:283: #if defined(ENABLE_TASK_MANAGER) On 2015/06/20 04:34:47, ncarter wrote: > On 2015/06/19 21:49:22, afakhry wrote: > > On 2015/06/19 19:50:51, ncarter wrote: > > > This is okay, but I wonder, would it be more elegant if we put these > #defines > > > inside of the bodies of the methods of WebContentsTags. Basically, all of > > > WebContentsTags would be a no-op if ENABLE_TASK_MANAGER were false? > > > > Unfortunately, it has to be done at the call site or otherwise there will be a > > linking error ... task_manager_source_files are only included if > > enable_task_manager==1 in the gyp files. enable_task_manager is 0 for ios and > > android on the gn builds and I think is 0 only for android on the gyp builds. > > See the build bots that failed the build in the previous patch. > > > > See also: > > > https://code.google.com/p/chromium/codesearch#search/&q=%22enable_task_manage... > > What I was advocating, was to add the web_content_tags.cc (and just that file) > back so that it's compiled on the ios/android build. But I am okay with the > #defines as they are. I like Nick's suggestion. That way, simpler callers like prerender_contents.cc won't need to change when we remove the parameter and launch! It removes the conditionals from many call sites that aren't particularly dependent on the new messaging for the task manager.
Thanks for the very helpful comments. To answer you question about the benefits of tagging the WebContents. Please take a look at how the old task manager used to collect the WebContents that belong to a certain service [in the code below, it's collecting the WebContents that belong to TabContents, DevTools, and Prerender]. We used to have something like this for each service type and component owning WebContents we're interested in tracking and showing in the task manager. We used to observe NOTIFICATION_WEB_CONTENTS_CONNECTED and for each WebContents received, each of these tracking objects (called WebContentsInformation) are invoked to check whether any of them is tracking the service owning the WebContents. As you can see that approach wasn't clean nor efficient. Nick and I designed this new approach where each service owning WebContents and desired to be represented in the task manager must explicitly tag their WebContents by their appropriate corresponding Tag. The task manager doesn't need to do any extra work to figure out what type of WebContents it is. We also get the benefit of moving away from the deprecated NotificationService. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4100: CHECK(task); On 2015/06/22 18:38:04, gavinp wrote: > Remove this check, crashing in unit tests is bad form. > > Is this check really a part of testing the prerender code, what could cause this > to be null? If it's some other system with its own tests that could cause this, > just remove the check, and we'll crash later when we dereference it. > > If it is part of the test for this system, then we have to be a bit silly to > avoid the crash, something like: > > if (!task) { > ADD_FAILURE() << "task was nullptr."; > return; > } > > Should do the trick? > That CHECK was actually not needed, so removed. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4101: ASSERT_FALSE(provided_tasks_.count(task)); On 2015/06/22 18:38:03, gavinp wrote: > ASSERT_FALSE won't work as many expect here; on failure it adds a failure to the > test run, which is expected, but then it just returns from this function and > continues execution. The test will continue. > > See > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > in the Google Test manual for details. > > Any reason this needs to be fatal? I suggest making it EXPECT_FALSE...; of > course the world gets confusing fast, but that's OK? Modified to EXPECT_FALSE() https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4142: // TODO(afakhry): Once we start tagging the tab contents the below tests On 2015/06/22 18:38:03, gavinp wrote: > Nit: (optional) Remove the extra indent on the second line, wrap comment at 80 > columns. Done. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:4155: EXPECT_TRUE(task_manager.provided_tasks().empty()); On 2015/06/22 18:38:03, gavinp wrote: > It seems like a bit of overkill to assert this every time we create a > MockTaskManager(). It has some documentary value, but it's not huge. > > Is there any reason not to just do these EXPECT-it's-empty calls in the > constructor of the MockTaskManager? Yes it is actually an overkill, and hence removed. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:684: // Here we have to clear the task manager tag we added earlier to our On 2015/06/22 18:38:04, gavinp wrote: > Nit: Remove "Here we have to". Done. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:685: // WebContents since it's no longer a prerender content. On 2015/06/22 18:38:04, gavinp wrote: > Nit: "content" --> "contents" Done. https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:40: void ClearAllTasks(bool notify_observer); On 2015/06/22 18:38:04, gavinp wrote: > Nit: I think the method definition should move to as well to keep in style, see > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... > . Thanks for catching that! I forgot to do it. Done! https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... File chrome/browser/task_management/web_contents_tags.cc (right): https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... chrome/browser/task_management/web_contents_tags.cc:66: DCHECK(web_contents); On 2015/06/22 18:38:04, gavinp wrote: > Nit: I don't think this DCHECK is needed. Won't the next line crash? Done.
On 2015/06/22 21:56:18, afakhry wrote: > Thanks for the very helpful comments. To answer you question about the benefits > of tagging the WebContents. Please take a look at how the old task manager used > to collect the WebContents that belong to a certain service [in the code below, > it's collecting the WebContents that belong to TabContents, DevTools, and > Prerender]. We used to have something like this for each service type and > component owning WebContents we're interested in tracking and showing in the > task manager. We used to observe NOTIFICATION_WEB_CONTENTS_CONNECTED and for > each WebContents received, each of these tracking objects (called > WebContentsInformation) are invoked to check whether any of them is tracking the > service owning the WebContents. > > As you can see that approach wasn't clean nor efficient. Nick and I designed > this new approach where each service owning WebContents and desired to be > represented in the task manager must explicitly tag their WebContents by their > appropriate corresponding Tag. The task manager doesn't need to do any extra > work to figure out what type of WebContents it is. > We also get the benefit of moving away from the deprecated NotificationService. Just to add to this: NOTIFICATION_WEB_CONTENTS_CONNECTED is a layering violation from the perspective of the content api: content shouldn't have to notify chrome about chrome's own actions -- in this case, calling WebContents::Create(). So we're hoping to ditch it altogether. NOTIFICATION_WEB_CONTENTS_CONNECTED is one of the thorniest thorns preventing the content NotificationService from being deprecated. The alternative design calls for an explicit registration action between feature code like PrerenderContents and the TaskManager. To help make sure the registration always happens, I/we plan to add a CHECK to the chrome browser tests framework, to catch cases where we forget to tag the WebContents. > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_browsertest.cc (right): > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_browsertest.cc:4100: CHECK(task); > On 2015/06/22 18:38:04, gavinp wrote: > > Remove this check, crashing in unit tests is bad form. > > > > Is this check really a part of testing the prerender code, what could cause > this > > to be null? If it's some other system with its own tests that could cause > this, > > just remove the check, and we'll crash later when we dereference it. > > > > If it is part of the test for this system, then we have to be a bit silly to > > avoid the crash, something like: > > > > if (!task) { > > ADD_FAILURE() << "task was nullptr."; > > return; > > } > > > > Should do the trick? > > > > That CHECK was actually not needed, so removed. > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_browsertest.cc:4101: > ASSERT_FALSE(provided_tasks_.count(task)); > On 2015/06/22 18:38:03, gavinp wrote: > > ASSERT_FALSE won't work as many expect here; on failure it adds a failure to > the > > test run, which is expected, but then it just returns from this function and > > continues execution. The test will continue. > > > > See > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > > in the Google Test manual for details. > > > > Any reason this needs to be fatal? I suggest making it EXPECT_FALSE...; of > > course the world gets confusing fast, but that's OK? > > Modified to EXPECT_FALSE() > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_browsertest.cc:4142: // TODO(afakhry): Once > we start tagging the tab contents the below tests > On 2015/06/22 18:38:03, gavinp wrote: > > Nit: (optional) Remove the extra indent on the second line, wrap comment at 80 > > columns. > > Done. > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_browsertest.cc:4155: > EXPECT_TRUE(task_manager.provided_tasks().empty()); > On 2015/06/22 18:38:03, gavinp wrote: > > It seems like a bit of overkill to assert this every time we create a > > MockTaskManager(). It has some documentary value, but it's not huge. > > > > Is there any reason not to just do these EXPECT-it's-empty calls in the > > constructor of the MockTaskManager? > > Yes it is actually an overkill, and hence removed. > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > File chrome/browser/prerender/prerender_contents.cc (right): > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_contents.cc:684: // Here we have to clear the > task manager tag we added earlier to our > On 2015/06/22 18:38:04, gavinp wrote: > > Nit: Remove "Here we have to". > > Done. > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/prerende... > chrome/browser/prerender/prerender_contents.cc:685: // WebContents since it's no > longer a prerender content. > On 2015/06/22 18:38:04, gavinp wrote: > > Nit: "content" --> "contents" > > Done. > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... > File > chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc > (right): > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... > chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc:40: > void ClearAllTasks(bool notify_observer); > On 2015/06/22 18:38:04, gavinp wrote: > > Nit: I think the method definition should move to as well to keep in style, > see > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... > > . > > Thanks for catching that! I forgot to do it. > Done! > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... > File chrome/browser/task_management/web_contents_tags.cc (right): > > https://codereview.chromium.org/1185183008/diff/40001/chrome/browser/task_man... > chrome/browser/task_management/web_contents_tags.cc:66: DCHECK(web_contents); > On 2015/06/22 18:38:04, gavinp wrote: > > Nit: I don't think this DCHECK is needed. Won't the next line crash? > > Done.
Patchset #6 (id:100001) has been deleted
afakhry@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Can you please review BUILD.gn? The idea is that we want to hide the #if defined(ENABLE_TASK_MANAGER) inside the implementation of the WebContentsTags class whose source files we made available for all configuration (regardless of ENABLE_TASK_MANAGER). This should make the call site cleaner.
lgtm, provided you address the g_prerender_icon issue; the others you can take or leave. Thanks! This new architecture does look better. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4090: const char kPrerenderPage[] = "files/prerender/prerender_page.html"; Every other test just repeats this string. I don't see why you shouldn't. Either that, or move this up near the top of the file and replace it everywhere. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4129: } // namespace Why not put the tests in the unnamed namespace too? (Yes, that does work) https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/web_contents/prerender_task.cc (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/prerender_task.cc:17: gfx::ImageSkia* g_prerender_icon = nullptr; I don't think you're ever setting this, instead you're calling ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_...) each time. We should save a value in this variable, or toss this variable. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.h (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.h:34: // Called to manually remove |tag|'s corresponding Task from the provider. Nit: (optional) not sure what "Called to" or "from the provider." adds to this comment. So why not "Manually remove |tag|'s corresponding Task."
https://codereview.chromium.org/1185183008/diff/140001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3203: '<@(chrome_browser_task_manager_unconditional_sources)', Are you sure you need this on all platforms, including iOS? I'm even suspicious that the undo sources do not belong here. Whichever section it ends up in, please keep the list in alphabetical order.
On 2015/06/23 18:10:33, Lei Zhang wrote: > https://codereview.chromium.org/1185183008/diff/140001/chrome/chrome_browser.... > File chrome/chrome_browser.gypi (right): > > https://codereview.chromium.org/1185183008/diff/140001/chrome/chrome_browser.... > chrome/chrome_browser.gypi:3203: > '<@(chrome_browser_task_manager_unconditional_sources)', > Are you sure you need this on all platforms, including iOS? I'm even suspicious > that the undo sources do not belong here. > > Whichever section it ends up in, please keep the list in alphabetical order. Ah, I read the rest of the CL. Of course, less #ifdefs in c/b/prerender makes prerender code simpler, but it's just shifting the #ifdefs elsewhere. In this case, to web_contents_tags.cc, which compiles on all platforms, but it's all skeleton on the platforms where the code isn't being used. What most people do is just not build the file at that point, which is why there's so few source files that are in chrome_browser.gypi that are compiled on all platforms.
nits: https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4100: EXPECT_FALSE(provided_tasks_.count(task)); ContainsKey(provided_tasks_, task) might be slightly more readable. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4167: ASSERT_FALSE(task_manager.provided_tasks().empty()); I don't think you need this since you have a better check on the next line. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/web_contents/prerender_task.cc (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/prerender_task.cc:31: base::string16 PrefixTitle(const base::string16& title) { This might as well take a WebContents and call RendererTask::GetTitleFromWebContents().
https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4090: const char kPrerenderPage[] = "files/prerender/prerender_page.html"; On 2015/06/23 17:09:16, gavinp wrote: > Every other test just repeats this string. I don't see why you shouldn't. Either > that, or move this up near the top of the file and replace it everywhere. Done! Used the literal as the other tests. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4100: EXPECT_FALSE(provided_tasks_.count(task)); On 2015/06/23 18:34:33, Lei Zhang wrote: > ContainsKey(provided_tasks_, task) might be slightly more readable. Done. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4129: } // namespace On 2015/06/23 17:09:16, gavinp wrote: > Why not put the tests in the unnamed namespace too? (Yes, that does work) Done. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/prerend... chrome/browser/prerender/prerender_browsertest.cc:4167: ASSERT_FALSE(task_manager.provided_tasks().empty()); On 2015/06/23 18:34:34, Lei Zhang wrote: > I don't think you need this since you have a better check on the next line. Done. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/web_contents/prerender_task.cc (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/prerender_task.cc:17: gfx::ImageSkia* g_prerender_icon = nullptr; On 2015/06/23 17:09:17, gavinp wrote: > I don't think you're ever setting this, instead you're calling > ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_...) each time. > > We should save a value in this variable, or toss this variable. Thanks for catching this! I don't know how it skipped me. Done. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/prerender_task.cc:31: base::string16 PrefixTitle(const base::string16& title) { On 2015/06/23 18:34:34, Lei Zhang wrote: > This might as well take a WebContents and call > RendererTask::GetTitleFromWebContents(). Yes, but I want to keep RendererTask::GetTitleFromWebContents() protected though. https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... File chrome/browser/task_management/providers/web_contents/web_contents_task_provider.h (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/browser/task_ma... chrome/browser/task_management/providers/web_contents/web_contents_task_provider.h:34: // Called to manually remove |tag|'s corresponding Task from the provider. On 2015/06/23 17:09:17, gavinp wrote: > Nit: (optional) not sure what "Called to" or "from the provider." adds to this > comment. So why not "Manually remove |tag|'s corresponding Task." Done. https://codereview.chromium.org/1185183008/diff/140001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1185183008/diff/140001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3203: '<@(chrome_browser_task_manager_unconditional_sources)', On 2015/06/23 18:10:33, Lei Zhang wrote: > Are you sure you need this on all platforms, including iOS? I'm even suspicious > that the undo sources do not belong here. > > Whichever section it ends up in, please keep the list in alphabetical order. I actually added these files to the list of chrome_browser_non_ios_sources. Thanks for pointing that out.
As I mentioned, I'm kind of meh about the ifdefs in web_contents_tags.cc, but if y'all like it that way, then go for it. lgtm
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, gavinp@chromium.org Link to the patchset: https://codereview.chromium.org/1185183008/#ps160001 (title: "gavinp's and thestig's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1185183008/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/674c7b13184545b77d89afbe1d04cac93d78cda1 Cr-Commit-Position: refs/heads/master@{#335788}
Message was sent while issue was closed.
Description was changed from ========== New Task Manager - Phase 1.3.2: Implement Tab Contents Task Providing Part 2: Implementing a task-manager representation of WebContents owned by the prerender manager. R=nick@chromium.org BUG=470990 TEST=browser_tests --gtest_filter=PrerenderBrowserTest.TaskManagement* Committed: https://crrev.com/674c7b13184545b77d89afbe1d04cac93d78cda1 Cr-Commit-Position: refs/heads/master@{#335788} ========== to ========== New Task Manager - Phase 1.3.2.B: Implement Tab Contents Task Providing (Prerender). Part 2: Implementing a task-manager representation of WebContents owned by the prerender manager. R=nick@chromium.org BUG=470990 TEST=browser_tests --gtest_filter=PrerenderBrowserTest.TaskManagement* Committed: https://crrev.com/674c7b13184545b77d89afbe1d04cac93d78cda1 Cr-Commit-Position: refs/heads/master@{#335788} ========== |
