Remove PrerenderTracker::TryCancel* methods and associated ones, since destruction of prerender onl…

Remove PrerenderTracker::TryCancel* methods and associated ones, since destruction of prerender only happens on the UI thread through PrerenderContents now.

BUG=304341
R=mmenke@chromium.org, tburkard@chromium.org

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

[jam, 2014-01-09 22:48:08]

+site-isolation-reviews@chromium.org

[jam, 2014-01-09 22:50:10]

btw I will try to get rid of PrerenderTracker completely in a followup. For now
I'm just removing the thread-safe map which isn't needed anymore.

[tburkard, 2014-01-10 06:14:03]

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
Why is this check necessary?

I assume because you deleted TryUse above?

But since everything has been reworked now to only cancel on the UI thread, and
since Destroy() on a webcontents removes the item from active_prerenders_, the
prerender should always be unused if we get here.

[jam, 2014-01-10 07:44:59]

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
On 2014/01/10 06:14:03, tburkard wrote:
> Why is this check necessary?
> 
> I assume because you deleted TryUse above?

Yep

> 
> But since everything has been reworked now to only cancel on the UI thread,
and
> since Destroy() on a webcontents removes the item from active_prerenders_, the
> prerender should always be unused if we get here.

My reading of the code, and please correct me if I'm wrong, is that
OnMergeCompleted is called asynchronously. So I was trying to guard for the
prerender being cancelled while the merge is happening. Is this not possible?
The PrerenderContents is still in active_prerenders_ map while this async
operation is happening, so it could be destroyed?

[tburkard, 2014-01-10 09:57:12]

lgtm

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
So, when PrerenderData is destroyed, PendingSwaped along with it is destroyed,
too, cancelling the async callback from the merge.

When a PrerenderContents is Destroy'ed, it is removed from active_entries_, and
when I originally wrote this code, being removed from active_entries_ would
imply cancelling the callbacks.

However, I just noticed
https://codereview.chromium.org/98373010/diff/120001/chrome/browser/prerender...
which removed line 845, ClearPendingSwap.

So I guess it could happen now that the callback still happens, and the item is
no longer in active_prerenders_, and has a final status, so I think the check
you added is fine now.

However, I think the way it was before, ensuring that if something is no longer
available (i.e. part of active_prerenders_), no more callbacks will be called,
was cleaner.

Your choice, you can either leave it the way it is, or remove it and add back
the stuff David removed.

Thanks.

[tburkard, 2014-01-10 10:06:14]

still ltgtm

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
sorry, in the comment above, "line 845" should be "line 854"

[jam, 2014-01-10 15:56:08]

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
On 2014/01/10 09:57:12, tburkard wrote:
> So, when PrerenderData is destroyed, PendingSwaped along with it is destroyed,
> too, cancelling the async callback from the merge.
> 
> When a PrerenderContents is Destroy'ed, it is removed from active_entries_,
and
> when I originally wrote this code, being removed from active_entries_ would
> imply cancelling the callbacks.
> 
> However, I just noticed
>
https://codereview.chromium.org/98373010/diff/120001/chrome/browser/prerender...
> which removed line 845, ClearPendingSwap.
> 
> So I guess it could happen now that the callback still happens, and the item
is
> no longer in active_prerenders_, and has a final status, so I think the check
> you added is fine now.
> 
> However, I think the way it was before, ensuring that if something is no
longer
> available (i.e. part of active_prerenders_), no more callbacks will be called,
> was cleaner.
> 
> Your choice, you can either leave it the way it is, or remove it and add back
> the stuff David removed.
> 
> Thanks.

I agree that the way you describe is easier to read/follow. I was a little
surprised that the callback runs even if the prerender is destroyed. Done.

[davidben, 2014-01-10 16:03:09]

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
On 2014/01/10 15:56:08, jam wrote:
> On 2014/01/10 09:57:12, tburkard wrote:
> > So, when PrerenderData is destroyed, PendingSwaped along with it is
destroyed,
> > too, cancelling the async callback from the merge.
> > 
> > When a PrerenderContents is Destroy'ed, it is removed from active_entries_,
> and
> > when I originally wrote this code, being removed from active_entries_ would
> > imply cancelling the callbacks.
> > 
> > However, I just noticed
> >
>
https://codereview.chromium.org/98373010/diff/120001/chrome/browser/prerender...
> > which removed line 845, ClearPendingSwap.
> > 
> > So I guess it could happen now that the callback still happens, and the item
> is
> > no longer in active_prerenders_, and has a final status, so I think the
check
> > you added is fine now.
> > 
> > However, I think the way it was before, ensuring that if something is no
> longer
> > available (i.e. part of active_prerenders_), no more callbacks will be
called,
> > was cleaner.
> > 
> > Your choice, you can either leave it the way it is, or remove it and add
back
> > the stuff David removed.
> > 
> > Thanks.
> 
> I agree that the way you describe is easier to read/follow. I was a little
> surprised that the callback runs even if the prerender is destroyed. Done.

I think I had to remove it there because SwapInternal will call Destroy in some
cases, which meant that |this| may or may not have been deleted by the time
SwapInternal returns. (This logic is screwy... having |this| deleted is already
awkward, but when it only happens some of the time...) Maybe if SwapInternal
took care of the ClearPendingSwap() call in all cases? Slight complication in
that, in the success case, the PendingSwap can't be deleted until it knows the
swap succeeded, so the destructor can cancel the throttles instead of resume
them.

[jam, 2014-01-10 16:24:08]

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/132613002/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1272: if
(prerender_data_->contents()->final_status() == FINAL_STATUS_MAX) {
On 2014/01/10 16:03:09, David Benjamin wrote:
> On 2014/01/10 15:56:08, jam wrote:
> > On 2014/01/10 09:57:12, tburkard wrote:
> > > So, when PrerenderData is destroyed, PendingSwaped along with it is
> destroyed,
> > > too, cancelling the async callback from the merge.
> > > 
> > > When a PrerenderContents is Destroy'ed, it is removed from
active_entries_,
> > and
> > > when I originally wrote this code, being removed from active_entries_
would
> > > imply cancelling the callbacks.
> > > 
> > > However, I just noticed
> > >
> >
>
https://codereview.chromium.org/98373010/diff/120001/chrome/browser/prerender...
> > > which removed line 845, ClearPendingSwap.
> > > 
> > > So I guess it could happen now that the callback still happens, and the
item
> > is
> > > no longer in active_prerenders_, and has a final status, so I think the
> check
> > > you added is fine now.
> > > 
> > > However, I think the way it was before, ensuring that if something is no
> > longer
> > > available (i.e. part of active_prerenders_), no more callbacks will be
> called,
> > > was cleaner.
> > > 
> > > Your choice, you can either leave it the way it is, or remove it and add
> back
> > > the stuff David removed.
> > > 
> > > Thanks.
> > 
> > I agree that the way you describe is easier to read/follow. I was a little
> > surprised that the callback runs even if the prerender is destroyed. Done.
> 
> I think I had to remove it there because SwapInternal will call Destroy in
some
> cases, which meant that |this| may or may not have been deleted by the time
> SwapInternal returns. (This logic is screwy... having |this| deleted is
already
> awkward, but when it only happens some of the time...) Maybe if SwapInternal
> took care of the ClearPendingSwap() call in all cases? Slight complication in
> that, in the success case, the PendingSwap can't be deleted until it knows the
> swap succeeded, so the destructor can cancel the throttles instead of resume
> them.

hmm, since I'm not familiar with this code, I will leave as it was before my
change. undid the change I did per Timo's comment.

[mmenke, 2014-01-10 16:29:18]

https://codereview.chromium.org/132613002/diff/720001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/132613002/diff/720001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker_unittest.cc:215: 
We should still have a test that makes sure IsPrerenderingOnIOThread returns
false before we start prendering, and false after we're done prerendering.  My
suggestion:

TEST_F(PrerenderTrackerTest, IsPrerenderingOnIOThread) {
  EXPECT_FALSE(prerender_tracker()->IsPrerenderingOnIOThread(
               kDefaultChildId, kDefaultRouteId));

  test_contents()->Start();
  // This calls AddPrerenderOnIOThreadTask().	
  RunEvents();
  EXPECT_TRUE(prerender_tracker()->IsPrerenderingOnIOThread(
              kDefaultChildId, kDefaultRouteId));

  test_contents()->Stop();
  // This calls AddPrerenderOnIOThreadTask().	
  RunEvents();
  EXPECT_FALSE(prerender_tracker()->IsPrerenderingOnIOThread(
              kDefaultChildId, kDefaultRouteId));
}

[mmenke, 2014-01-10 16:32:19]

https://codereview.chromium.org/132613002/diff/720001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/132613002/diff/720001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker_unittest.cc:215: 
Err...The second comment should be RemovePrerenderOnIOThreadTask().

On 2014/01/10 16:29:19, mmenke wrote:
> We should still have a test that makes sure IsPrerenderingOnIOThread returns
> false before we start prendering, and false after we're done prerendering.  My
> suggestion:
> 
> TEST_F(PrerenderTrackerTest, IsPrerenderingOnIOThread) {
>   EXPECT_FALSE(prerender_tracker()->IsPrerenderingOnIOThread(
>                kDefaultChildId, kDefaultRouteId));
> 
>   test_contents()->Start();
>   // This calls AddPrerenderOnIOThreadTask().	
>   RunEvents();
>   EXPECT_TRUE(prerender_tracker()->IsPrerenderingOnIOThread(
>               kDefaultChildId, kDefaultRouteId));
> 
>   test_contents()->Stop();
>   // This calls AddPrerenderOnIOThreadTask().	
>   RunEvents();
>   EXPECT_FALSE(prerender_tracker()->IsPrerenderingOnIOThread(
>               kDefaultChildId, kDefaultRouteId));
> }

[jam, 2014-01-10 16:51:39]

https://codereview.chromium.org/132613002/diff/720001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/132613002/diff/720001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker_unittest.cc:215: 
On 2014/01/10 16:29:19, mmenke wrote:
> We should still have a test that makes sure IsPrerenderingOnIOThread returns
> false before we start prendering, and false after we're done prerendering.  My
> suggestion:
> 
> TEST_F(PrerenderTrackerTest, IsPrerenderingOnIOThread) {
>   EXPECT_FALSE(prerender_tracker()->IsPrerenderingOnIOThread(
>                kDefaultChildId, kDefaultRouteId));
> 
>   test_contents()->Start();
>   // This calls AddPrerenderOnIOThreadTask().	
>   RunEvents();
>   EXPECT_TRUE(prerender_tracker()->IsPrerenderingOnIOThread(
>               kDefaultChildId, kDefaultRouteId));
> 
>   test_contents()->Stop();
>   // This calls AddPrerenderOnIOThreadTask().	
>   RunEvents();
>   EXPECT_FALSE(prerender_tracker()->IsPrerenderingOnIOThread(
>               kDefaultChildId, kDefaultRouteId));
> }

Done.

[mmenke, 2014-01-10 17:02:38]

Thanks!  LGTM!