Reland fix for thumbnail generation.

Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.

BUG=530707
Committed: https://crrev.com/7fb8a56234863c20dc9d2bce31f730152d086dfc
Cr-Commit-Position: refs/heads/master@{#370809}

[shrike, 2015-11-18 17:21:51]

Description was changed from

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, with changes to address the
problem in https://crbug.com/555576 .

BUG=530707
==========

to

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes to address
the problem in https://crbug.com/555576 .

BUG=530707
==========

[shrike, 2015-11-18 17:34:28]

Description was changed from

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes to address
the problem in https://crbug.com/555576 .

BUG=530707
==========

to

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.


BUG=530707
==========

[shrike, 2015-11-18 17:34:29]

shrike@chromium.org changed reviewers:
+ ccameron@chromium.org, thestig@chromium.org

[shrike, 2015-11-18 17:36:26]

thestig - the only changes to this cl are in thumbnail_tab_helper.cc (please see
my comments in the cl description).
ccameron - there's nothing new in this cl that you need to review, but I think I
still need a lgtm from you in order to land this cl.

PTAL

[Lei Zhang, 2015-11-18 17:38:48]

On 2015/11/18 17:36:26, shrike wrote:
> thestig - the only changes to this cl are in thumbnail_tab_helper.cc (please
see
> my comments in the cl description).
> ccameron - there's nothing new in this cl that you need to review, but I think
I
> still need a lgtm from you in order to land this cl.
> 
> PTAL

Will do. I forgot to mention it would've been nice to upload the reverted CL as
patch set 1, and the new CL as patch set 2. Then one can more easily see the
diff for the fix.

[shrike, 2015-11-18 17:41:54]

On 2015/11/18 17:38:48, Lei Zhang wrote:
> On 2015/11/18 17:36:26, shrike wrote:
> > thestig - the only changes to this cl are in thumbnail_tab_helper.cc (please
> see
> > my comments in the cl description).
> > ccameron - there's nothing new in this cl that you need to review, but I
think
> I
> > still need a lgtm from you in order to land this cl.
> > 
> > PTAL
> 
> Will do. I forgot to mention it would've been nice to upload the reverted CL
as
> patch set 1, and the new CL as patch set 2. Then one can more easily see the
> diff for the fix.

Ah. Let me try to do that....

[shrike, 2015-11-18 17:44:49]

On 2015/11/18 17:41:54, shrike wrote:
> On 2015/11/18 17:38:48, Lei Zhang wrote:
> > Will do. I forgot to mention it would've been nice to upload the reverted CL
> as
> > patch set 1, and the new CL as patch set 2. Then one can more easily see the
> > diff for the fix.
> 
> Ah. Let me try to do that....

Looks like I can't delete patchset #1. I will talk to you about options when you
get into the office.

[Lei Zhang, 2015-11-18 17:56:04]

On 2015/11/18 17:44:49, shrike wrote:
> On 2015/11/18 17:41:54, shrike wrote:
> > On 2015/11/18 17:38:48, Lei Zhang wrote:
> > > Will do. I forgot to mention it would've been nice to upload the reverted
CL
> > as
> > > patch set 1, and the new CL as patch set 2. Then one can more easily see
the
> > > diff for the fix.
> > 
> > Ah. Let me try to do that....
> 
> Looks like I can't delete patchset #1. I will talk to you about options when
you
> get into the office.

Meh. Don't worry about it. I just did the side-by-side comparison with the old
CL.

You can delete patch set 1 if you upload another patch set.

[Lei Zhang, 2015-11-18 18:06:43]

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:67:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
This generally should be at the top of the function. The one below on like 87 is
a special case since it has to deal with the UI thread "going away" on shutdown.

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:217: registrar_.RemoveAll();
Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing for future
RenderViews / on future hidden events.

[shrike, 2015-11-18 18:20:16]

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:67:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/11/18 18:06:43, Lei Zhang wrote:
> This generally should be at the top of the function. The one below on like 87
is
> a special case since it has to deal with the UI thread "going away" on
shutdown.

I actually wasn't sure if it was needed at all (but I was thinking the
DecrementCapturerCount() needs to run from the UI thread). Should I move it to
the top or remove the check completely?

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:217: registrar_.RemoveAll();
On 2015/11/18 18:06:43, Lei Zhang wrote:
> Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing for
future
> RenderViews / on future hidden events.

It appears to be one per web contents (tab_helpers.cc:205).

[Lei Zhang, 2015-11-18 18:24:00]

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:67:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/11/18 18:20:16, shrike wrote:
> On 2015/11/18 18:06:43, Lei Zhang wrote:
> > This generally should be at the top of the function. The one below on like
87
> is
> > a special case since it has to deal with the UI thread "going away" on
> shutdown.
> 
> I actually wasn't sure if it was needed at all (but I was thinking the
> DecrementCapturerCount() needs to run from the UI thread). Should I move it to
> the top or remove the check completely?

Move it to the top.

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:217: registrar_.RemoveAll();
On 2015/11/18 18:20:16, shrike wrote:
> On 2015/11/18 18:06:43, Lei Zhang wrote:
> > Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing for
> future
> > RenderViews / on future hidden events.
> 
> It appears to be one per web contents (tab_helpers.cc:205).

That I understand. My question is, will a ThumbnailTabHelper make one snapshot
in its lifetime, or more than one? By clearing the notification registrar, a
ThumbnailTabHelper instance will only make one.

[shrike, 2015-11-18 19:18:06]

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:217: registrar_.RemoveAll();
On 2015/11/18 18:24:00, Lei Zhang wrote:
> On 2015/11/18 18:20:16, shrike wrote:
> > On 2015/11/18 18:06:43, Lei Zhang wrote:
> > > Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing for
> > future
> > > RenderViews / on future hidden events.
> > 
> > It appears to be one per web contents (tab_helpers.cc:205).
> 
> That I understand. My question is, will a ThumbnailTabHelper make one snapshot
> in its lifetime, or more than one? By clearing the notification registrar, a
> ThumbnailTabHelper instance will only make one.

Sorry (need more coffee).

My assumption in turning off all notifications at this point was that the user
is switching away from the page, which is when the generator needs to generate a
thumbnail. I was thinking at that point you're done with thumbnail generation,
but I realize that the web contents may not go away completely? and the user
might switch back to its page, which means we'll want another thumbnail
generation attempt if the user switches away again. So I don't think it's a good
idea to disable notifications for good like this.

My goal is to prevent or ignore notifications that result from the call to
DecrementCapturerCount(), so perhaps a better idea is to disable this particular
notification here and to reenable it after the call to IncrementCapturerCount()?

[Lei Zhang, 2015-11-18 23:01:40]

On 2015/11/18 19:18:06, shrike wrote:
>
https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
> File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1461463002/diff/1/chrome/browser/thumbnails/t...
> chrome/browser/thumbnails/thumbnail_tab_helper.cc:217: registrar_.RemoveAll();
> On 2015/11/18 18:24:00, Lei Zhang wrote:
> > On 2015/11/18 18:20:16, shrike wrote:
> > > On 2015/11/18 18:06:43, Lei Zhang wrote:
> > > > Is ThumbnailTabHelper one-shot? If not, then this breaks thumbnailing
for
> > > future
> > > > RenderViews / on future hidden events.
> > > 
> > > It appears to be one per web contents (tab_helpers.cc:205).
> > 
> > That I understand. My question is, will a ThumbnailTabHelper make one
snapshot
> > in its lifetime, or more than one? By clearing the notification registrar, a
> > ThumbnailTabHelper instance will only make one.
> 
> Sorry (need more coffee).
> 
> My assumption in turning off all notifications at this point was that the user
> is switching away from the page, which is when the generator needs to generate
a
> thumbnail. I was thinking at that point you're done with thumbnail generation,
> but I realize that the web contents may not go away completely? and the user
> might switch back to its page, which means we'll want another thumbnail
> generation attempt if the user switches away again. So I don't think it's a
good
> idea to disable notifications for good like this.
> 
> My goal is to prevent or ignore notifications that result from the call to
> DecrementCapturerCount(), so perhaps a better idea is to disable this
particular
> notification here and to reenable it after the call to
IncrementCapturerCount()?

Ya, something like that can work, or maybe you can detect the code stuck in a
(generate thumbnail) <-> (trigger WasHidden()) cycle and break out of it.

[shrike, 2015-11-19 22:41:09]

OK thestig, PTAL.

[Lei Zhang, 2015-11-20 20:01:04]

There's probably a couple ways to better workout the lifetime or
ThumbnailingContext and ThumbnailTabHelper, so we do not need to hand out a
WeakPtr to ThumbnailingContext.

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:60: void
DecrementCapturerCount(const ThumbnailingContext& context) {
If you make these ThumbnailTabHelper methods, and assuming we work out the fact
that ThumbnailingContext can indeed be not refcounted, then ThumbnailTabHelper
can take ownership of the context, and we may not need to hand the WeakPtr to
the context.

Then in ThumbnailTabHelper::AsyncProcessThumbnail(), it can
Bind(&ThumbnailTabHelper::ProcessCapturedBitmap, weak_factory_.GetWeakPtr(),
algorithm);

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:193: void
ThumbnailTabHelper::set_thumbnail_generation_in_progress(bool flag) {
Since we only call this with flag = false, make it
reset_thumbnail_generation_in_progress() ?

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_context.h:42: : public
base::RefCountedThreadSafe<ThumbnailingContext>,
BTW, I don't think ThumbnailingContext needs to be ref counted, and it is never
passed between threads, so it doesn't need to be thread safe either. Likely the
original author did this just to be on the safe side, and to make passing it
around easier?

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_context.h:47:
base::WeakPtr<ThumbnailTabHelper> source_tab_helper,
Isn't the ThumbnailTabHelper also a WebContentsUserData subclass for the same
WebContents?

Doesn't that mean if |web_contents| becomes invalid, we already know
|sorce_tab_helper| is invalid as well?

[Lei Zhang, 2016-01-07 22:15:29]

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:60: void
DecrementCapturerCount(const ThumbnailingContext& context) {
On 2015/11/20 20:01:04, Lei Zhang wrote:
> If you make these ThumbnailTabHelper methods, and assuming we work out the
fact
> that ThumbnailingContext can indeed be not refcounted, then ThumbnailTabHelper
> can take ownership of the context, and we may not need to hand the WeakPtr to
> the context.
> 
> Then in ThumbnailTabHelper::AsyncProcessThumbnail(), it can
> Bind(&ThumbnailTabHelper::ProcessCapturedBitmap, weak_factory_.GetWeakPtr(),
> algorithm);

I just reiterated this idea in person. :)

[shrike, 2016-01-08 18:32:36]

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:60: void
DecrementCapturerCount(const ThumbnailingContext& context) {
On 2015/11/20 20:01:04, Lei Zhang wrote:
> If you make these ThumbnailTabHelper methods, and assuming we work out the
fact
> that ThumbnailingContext can indeed be not refcounted, then ThumbnailTabHelper
> can take ownership of the context, and we may not need to hand the WeakPtr to
> the context.

I think I found a way around having to hand the thumbnail helper to the context,
so that will get rid of the weakptr there.

> Then in ThumbnailTabHelper::AsyncProcessThumbnail(), it can
> Bind(&ThumbnailTabHelper::ProcessCapturedBitmap, weak_factory_.GetWeakPtr(),
> algorithm);

After digging into this, the big complication is threads. WeakPtr says the it
can only be accessed from the thread that created it. AsyncProcessThumbnail()
calls render_widget_host->CopyFromBackingStore() with the Bind() runnable, and
there's a check for the UI thread on the other side in ProcessCapturedBitmap(),
so you could maybe assume everything in between runs in the UI thread (maybe).
However, ProcessCapturedBitmap() passes the context off to the thumbnail
algorithm - I assume the WeakPtr and all it contains must be accessed from the
creation thread, so the context, being tightly owned by the thumbnail helper,
would have to be accessed from the UI thread as well. There are no guarantees
about which thread is doing the work within the thumbnail algorithm.

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:193: void
ThumbnailTabHelper::set_thumbnail_generation_in_progress(bool flag) {
On 2015/11/20 20:01:04, Lei Zhang wrote:
> Since we only call this with flag = false, make it
> reset_thumbnail_generation_in_progress() ?

This method will go away in my revised patchset.

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnailing_context.h (right):

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_context.h:42: : public
base::RefCountedThreadSafe<ThumbnailingContext>,
On 2015/11/20 20:01:04, Lei Zhang wrote:
> BTW, I don't think ThumbnailingContext needs to be ref counted, and it is
never
> passed between threads, so it doesn't need to be thread safe either. Likely
the
> original author did this just to be on the safe side, and to make passing it
> around easier?

I think it does. At the very least the context is passed to the thumbnail
algorithm, and there are no guarantees about the thread doing the work there.

https://codereview.chromium.org/1461463002/diff/20001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnailing_context.h:47:
base::WeakPtr<ThumbnailTabHelper> source_tab_helper,
On 2015/11/20 20:01:04, Lei Zhang wrote:
> Isn't the ThumbnailTabHelper also a WebContentsUserData subclass for the same
> WebContents?
> 
> Doesn't that mean if |web_contents| becomes invalid, we already know
> |sorce_tab_helper| is invalid as well?

Yes and yes. Having a link to the tab helper is important in the case where the
web contents is *not* invalid. The helper sets a flag to know that thumbnail
generation is in progress and the context needs to reset that flag once
generation is complete (otherwise if thumbnail generation fails, say, the tab
helper will never try again to generate a thumbnail as long as it remains
instantiated). But, I have figured out another way to get this information back
to the helper that does not involve the context having a reference to the
helper.

[Lei Zhang, 2016-01-09 02:14:25]

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:58:
thumbnailing_context_(nullptr),
no need to explicitly initialize scoped_refptrs.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:113: // Balance the call to
IncrementCapturerCount() from
Comments go in the header.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:195: WebContents*
web_contents) {
BTW, why bother having this? Just call web_contents() as needed.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:196:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
I think you should add this everywhere.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:224: // It turns out that the
corresponding DecrementCapturerCount() can trigger a
This comment is about IncrementCapturerCount() right? So it should go above the
call, not below.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:229:
scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm(
This can now be done in AsyncProcessThumbnail().

[miu, 2016-01-09 03:32:10]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2016-01-09 03:32:11]

Approach lgtm (as partial owner of CopyFromBackingStore() and
WebContentsImpl::Increment/DecrementCapturerCount()).

[shrike, 2016-01-11 19:07:08]

PTAL

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:58:
thumbnailing_context_(nullptr),
On 2016/01/09 02:14:24, Lei Zhang wrote:
> no need to explicitly initialize scoped_refptrs.

Acknowledged.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:113: // Balance the call to
IncrementCapturerCount() from
On 2016/01/09 02:14:24, Lei Zhang wrote:
> Comments go in the header.

Acknowledged.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:196:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/01/09 02:14:24, Lei Zhang wrote:
> I think you should add this everywhere.

It pretty much already is everywhere.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:224: // It turns out that the
corresponding DecrementCapturerCount() can trigger a
On 2016/01/09 02:14:24, Lei Zhang wrote:
> This comment is about IncrementCapturerCount() right? So it should go above
the
> call, not below.

Yeah. This latest patchset was a checkpoint, not really ready for review. I
uploaded it to get the cl off my machine and saved in the ether.

https://codereview.chromium.org/1461463002/diff/40001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:229:
scoped_refptr<thumbnails::ThumbnailingAlgorithm> algorithm(
On 2016/01/09 02:14:25, Lei Zhang wrote:
> This can now be done in AsyncProcessThumbnail().

Acknowledged.

[Lei Zhang, 2016-01-12 01:54:46]

lgtm

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:140:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Didn't you have problems with this on shutdown, or has the problem gone away?
Either this should be moved, or the comment on line 142 needs to be updated.

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:210: WebContents*
the_web_contents = web_contents();
Just call web_contents() as needed below? At least by convention, web_contents()
is a simple accessor. (vs GetWebContentsRequresLotsOfWork() )

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.h (right):

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.h:43: // Clean up after thumbnail
generation has ended.
Might be good to put this list in chronological order, since these are declared
here. Whereas before, they were in reverse order in the anonymous namespace to
avoid the need to forward declare. The ordering in the .cpp should be flipped as
well.

UpdateThumbnailIfNecessary()
AsyncProcessThumbnail()
ProcessCapturedBitmap()
UpdateThumbnail()
CleanUpFromThumbnailGeneration()

[Lei Zhang, 2016-01-12 02:18:43]

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:129:
context.service->SetPageThumbnail(context, image);
And funny indentation here.

[shrike, 2016-01-12 19:52:39]

PTAL

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.cc (right):

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:129:
context.service->SetPageThumbnail(context, image);
On 2016/01/12 02:18:43, Lei Zhang wrote:
> And funny indentation here.

Thanks for catching that.

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:140:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/01/12 01:54:46, Lei Zhang wrote:
> Didn't you have problems with this on shutdown, or has the problem gone away?
> Either this should be moved, or the comment on line 142 needs to be updated.

This comment is from another engineer - I never encountered a problem. The
DCHECK is in the wrong place (it should be within the if statement).

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.cc:210: WebContents*
the_web_contents = web_contents();
On 2016/01/12 01:54:46, Lei Zhang wrote:
> Just call web_contents() as needed below? At least by convention,
web_contents()
> is a simple accessor. (vs GetWebContentsRequresLotsOfWork() )

OK.

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
File chrome/browser/thumbnails/thumbnail_tab_helper.h (right):

https://codereview.chromium.org/1461463002/diff/60001/chrome/browser/thumbnai...
chrome/browser/thumbnails/thumbnail_tab_helper.h:43: // Clean up after thumbnail
generation has ended.
On 2016/01/12 01:54:46, Lei Zhang wrote:
> Might be good to put this list in chronological order, since these are
declared
> here. Whereas before, they were in reverse order in the anonymous namespace to
> avoid the need to forward declare. The ordering in the .cpp should be flipped
as
> well.
> 
> UpdateThumbnailIfNecessary()
> AsyncProcessThumbnail()
> ProcessCapturedBitmap()
> UpdateThumbnail()
> CleanUpFromThumbnailGeneration()

OK.

[Lei Zhang, 2016-01-12 20:22:28]

++lgtm

[shrike, 2016-01-13 16:40:14]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-01-13 16:40:15]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1461463002/#ps80001
(title: "Rearrange code, fix error handling.")

[commit-bot: I haz the power, 2016-01-13 16:40:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461463002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461463002/80001

[commit-bot: I haz the power, 2016-01-13 16:42:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-13 16:42:40]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[shrike, 2016-01-13 16:48:34]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2016-01-13 16:48:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461463002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461463002/80001

[commit-bot: I haz the power, 2016-01-13 16:50:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-13 16:50:54]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[shrike, 2016-01-13 17:03:30]

ccameron - I think I need your lgtm for your changes in
content/browser/compositor/delegated_frame_host.cc .

[ccameron, 2016-01-13 23:59:00]

DFH still lgtm. I might vaguely prefer just "skip_frame" instead of
"skip_frame_size_mismatch", but it's your call.

[shrike, 2016-01-14 18:38:01]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-01-14 18:38:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, miu@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1461463002/#ps100001
(title: "Change variable name.")

[commit-bot: I haz the power, 2016-01-14 18:39:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461463002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461463002/100001

[commit-bot: I haz the power, 2016-01-14 18:43:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 18:43:20]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shrike, 2016-01-14 19:59:51]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-01-14 19:59:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, miu@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1461463002/#ps120001
(title: "Merged changes from tot.")

[commit-bot: I haz the power, 2016-01-14 20:00:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461463002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461463002/120001

[commit-bot: I haz the power, 2016-01-14 20:56:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 20:56:25]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shrike, 2016-01-14 21:34:23]

Hello miu@,

It looks like everything is good with this cl except for an issue in
fullscreen_controller.cc:99, which says the following:

    DCHECK(web_contents->GetCapturerCount() == 0);

With my change the capturer count can be greater than 0 until the thumbnail has
been generated. I see that you wrote this line of code so can advise how to fix
this issue?

[shrike, 2016-01-15 20:30:42]

Hello miu@,

Regarding the problem at fullscreen_controller.cc:99 where it's checking for
web_contents->GetCapturerCount() == 0, is the issue that the capturer count
*must* be zero, or that someone just needs to make an exception for the fact
that the thumbnail generator might have incremented the capturer count?

[shrike, 2016-01-21 17:22:39]

The CQ bit was checked by shrike@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 17:23:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461463002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461463002/140001

[commit-bot: I haz the power, 2016-01-21 18:25:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 18:25:13]

Dry run: This issue passed the CQ dry run.

[shrike, 2016-01-21 18:45:45]

I spoke with mia@ yesterday and he provided a rewrite of the
IsFullscreenForTabOrPending() method that was expecting a zero capturer count.
This cl had a successful commit dry run so I think it's ready to go.

[shrike, 2016-01-21 22:30:48]

The CQ bit was checked by shrike@chromium.org

[shrike, 2016-01-21 22:30:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, miu@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1461463002/#ps140001
(title: "Rework IsFullscreenForTabOrPending() (per mia@).")

[commit-bot: I haz the power, 2016-01-21 22:33:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1461463002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1461463002/140001

[commit-bot: I haz the power, 2016-01-21 22:39:56]

Description was changed from

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.


BUG=530707
==========

to

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.


BUG=530707
==========

[commit-bot: I haz the power, 2016-01-21 22:39:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-21 22:41:37]

Description was changed from

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.


BUG=530707
==========

to

==========
Reland fix for thumbnail generation.

This is an attempted relanding of
https://codereview.chromium.org/1348833003, which was reverted in
https://codereview.chromium.org/1450083002 . This cl includes changes
to address the problem in https://crbug.com/555576 .

It turns out that DecrementCapturerCount() can trigger a call to
content::WebContentsImpl::WasHidden(), which eventually calls
ThumbnailTabHelper::UpdateThumbnailIfNecessary(), starting the
thumbnail generation process all over again. It also appears that the
bitmap capture operation is not necessarily synchronous, so calling
DecrementCapturerCount() anywhere within ProcessCapturedBitmap()
potentially disposes of the web contents before the thumbnail has been
captured.

This new cl differs from the original (1348833003) only in
thumbnail_tab_helper.cc, where I now unregister the ThumbnailTabHelper
from further notifications within UpdateThumbnailIfNecessary(), and
call DecrementCapturerCount() once the final callback is completely
finished with the web contents.

BUG=530707

Committed: https://crrev.com/7fb8a56234863c20dc9d2bce31f730152d086dfc
Cr-Commit-Position: refs/heads/master@{#370809}
==========

[commit-bot: I haz the power, 2016-01-21 22:41:39]

Patchset 8 (id:??) landed as
https://crrev.com/7fb8a56234863c20dc9d2bce31f730152d086dfc
Cr-Commit-Position: refs/heads/master@{#370809}