While screencasting a tab, do not disable rendering updates while hidden.

While screencasting a tab, do not disable rendering updates while hidden.

Approach: There is already a mechanism in WebContentsImpl to prevent WasHidden() from bubbling down to RWHV during tab dragging.  This mechanism is extended to also include the span of tab mirroring sessions.  A counter is used to account for both dragging a tab and screencasting it at the same time.

Fixed a bug in tab_drag_controller.cc where SetCapturingContents(false) was not being called after a drag in some circumstances.

Note on GTK-specific drag controller change: WebContents::SetCapturingContents(true) is never called, making the Set(false) call a no-op.  Removed the Set(false) call and confirmed this did not affect tab dragging behavior on Linux.

BUG=154184
TEST=Win/Mac/Linux: Tested that hidden tabs continue to render with both GPU and software compositing paths.  Also, ran through several tab dragging scenarios with and without screencasting engaged.


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

[miu, 2013-02-02 23:30:55]

Sky: Please review my changes to the tab drag controllers and WebContentsImpl.

Nick: Please focus on web_contents_video_capture_device.cc, but also
sanity-check the changes in the other files.

Thanks,
Yuri

[sky, 2013-02-04 15:02:25]

I'm not familiar with web_contents_video_capture_device.cc. Make sure you get an
OWNER to review it.

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:494:
source_dragged_contents()->SetCapturingContents(true);
At this point the tab is very likely still in the tabstrip. You should only need
to set this once the tab has been detached (where this call was before). What
made you need to move it?

https://codereview.chromium.org/12179007/diff/2001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/12179007/diff/2001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:1011: void
WebContentsImpl::SetCapturingContents(bool cap) {
Update the description of this method to indicate it's a ref count.

[miu, 2013-02-04 21:36:52]

Thanks, sky.  Explanation below:

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:494:
source_dragged_contents()->SetCapturingContents(true);
On 2013/02/04 15:02:25, sky wrote:
> At this point the tab is very likely still in the tabstrip. You should only
need
> to set this once the tab has been detached (where this call was before). What
> made you need to move it?

Short story: It's the simplest solution that fixes the bug (mentioned in the
descr.) and avoids adding a lot of extra complicated logic to TabDragController.

In more detail, the bug is this: After a tab is detached during a drag, there
are multiple scenarios where Attach() will not be called after the drag has
ended.  This is bad because SetCapturingContents(false) won't be called. 
Example scenarios:

1. Whenever you detach a tab from the tab strip, and then release it such that
it creates a new browser window for itself.

2. Whenever you detach a tab from the tab strip, and then hit the Escape key.

So, I spent a lot of time looking for a way to correct these problems, but I was
adding a *lot* of complicated logic to TabDragController to handle all the edge
cases.  Furthermore, I felt it would be way too easy for a bug regression to
occur when future logic changes are made by others.

Therefore, I turned to looking for the single code path that is always executed
on drag start (and on stop).  While you're correct in that we only *need* to
call SetCapturingContents(true) once the tab has been detached, it's perfectly
harmless to call it right at the start of the drag.  IMHO, the code is so much
cleaner this way.

https://codereview.chromium.org/12179007/diff/2001/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/12179007/diff/2001/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:1011: void
WebContentsImpl::SetCapturingContents(bool cap) {
On 2013/02/04 15:02:25, sky wrote:
> Update the description of this method to indicate it's a ref count.

Done.  (in content/public/browser/web_contents.h)

[ncarter (slow), 2013-02-04 22:13:43]

WCVCD lgtm. One suggestion for web_contents_impl.

https://codereview.chromium.org/12179007/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/12179007/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1011: void
WebContentsImpl::SetCapturingContents(bool cap) {
It seems a little risky to have a method named SetSomething(bool) that is not
idempotent. Would any of the following be an improvement:

AddCaptureSession() / RemoveCaptureSession()
IncrementCapturerCount() / DecrementCapturerCount()
RegisterCapturer() / UnregisterCapturer()

?

[justinlin, 2013-02-04 22:16:03]

https://codereview.chromium.org/12179007/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/12179007/diff/11001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:884:
base::Bind(&CaptureMachine::DoShutdownTasksOnUIThread, this));
Drive-by: Hmm, couldn't this class get deleted before the callback is run. Maybe
it should call a static method like in my cl.

[miu, 2013-02-04 22:23:46]

joi: Please review changes to content/public/browser/web_contents.h.

All: After chatting with Nick, we decided to split the SetCapturingContents()
method into two: Increment/Decrement methods.  PTAL.

Thanks,
Yuri

[miu, 2013-02-04 22:26:25]

https://codereview.chromium.org/12179007/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/12179007/diff/11001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:884:
base::Bind(&CaptureMachine::DoShutdownTasksOnUIThread, this));
On 2013/02/04 22:16:03, justinlin wrote:
> Drive-by: Hmm, couldn't this class get deleted before the callback is run.
Maybe
> it should call a static method like in my cl.

Nope.  CaptureMachine is ref-counted, so the Bind(...) safely holds a ref until
the task is run.

[sky, 2013-02-04 22:48:50]

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:494:
source_dragged_contents()->SetCapturingContents(true);
On 2013/02/04 21:36:52, Yuri wrote:
> On 2013/02/04 15:02:25, sky wrote:
> > At this point the tab is very likely still in the tabstrip. You should only
> need
> > to set this once the tab has been detached (where this call was before).
What
> > made you need to move it?
> 
> Short story: It's the simplest solution that fixes the bug (mentioned in the
> descr.) and avoids adding a lot of extra complicated logic to
TabDragController.

None-the-less it's the *wrong* state. The tab is not yet detached. Additionally
if you have live tab dragging on (as aura does), the method shouldn't be
invoked.

> In more detail, the bug is this: After a tab is detached during a drag, there
> are multiple scenarios where Attach() will not be called after the drag has
> ended.  This is bad because SetCapturingContents(false) won't be called. 
> Example scenarios:
> 
> 1. Whenever you detach a tab from the tab strip, and then release it such that
> it creates a new browser window for itself.
> 
> 2. Whenever you detach a tab from the tab strip, and then hit the Escape key.
> 
> So, I spent a lot of time looking for a way to correct these problems, but I
was
> adding a *lot* of complicated logic to TabDragController to handle all the
edge
> cases.  Furthermore, I felt it would be way too easy for a bug regression to
> occur when future logic changes are made by others.
> 
> Therefore, I turned to looking for the single code path that is always
executed
> on drag start (and on stop).  While you're correct in that we only *need* to
> call SetCapturingContents(true) once the tab has been detached, it's perfectly
> harmless to call it right at the start of the drag.  IMHO, the code is so much
> cleaner this way.

TabDragController should have code paths the deal with all the case you care
about. Those cases should be fixed to do the right thing.

[miu, 2013-02-05 01:06:25]

Thanks, sky.  All changes as you requested.  PTAL.

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

https://codereview.chromium.org/12179007/diff/2001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:494:
source_dragged_contents()->SetCapturingContents(true);
On 2013/02/04 22:48:50, sky wrote:
> None-the-less it's the *wrong* state. The tab is not yet detached.
Additionally
> if you have live tab dragging on (as aura does), the method shouldn't be
> invoked.

Fixed.  This was all a bit confusing to me.  Would have been nice if the Aura
behavior was a separate TabDragController implementation entirely, since a lot
of concepts/assumptions are different.

> TabDragController should have code paths the deal with all the case you care
> about. Those cases should be fixed to do the right thing.

Done.  My changes to TabDragController are only to fix exactly the cases that
were broken.  And, it goes without saying that I've extensively re-tested all of
this to make sure.  ;-)

[sky, 2013-02-05 01:43:19]

https://codereview.chromium.org/12179007/diff/10014/chrome/browser/ui/gtk/tab...
File chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc (left):

https://codereview.chromium.org/12179007/diff/10014/chrome/browser/ui/gtk/tab...
chrome/browser/ui/gtk/tabs/dragged_tab_controller_gtk.cc:439:
drag_data_->GetSourceWebContents()->SetCapturingContents(false);
How come you don't need calls to increment/decrement int gtk?

[miu, 2013-02-05 06:20:29]

On 2013/02/05 01:43:19, sky wrote:
> How come you don't need calls to increment/decrement int gtk?

Apparently, these calls have been "passed along" in the code base over the last
5 years, and are now mostly vestigal.  To prove the point, I removed them
completely from both controllers, and tab drag screenshotting works perfectly
fine on Linux and Windows 7.

However, I'm *still* reluctant to remove them.  A comment hints at "Prevent the
WebContents HWND from being hidden," and then code in
native_view_photobooth_win.cc:26 (same dir) specifically mentions a Vista bug
where the screenshotting will fail unless at least a 1x1 pixel is on-screen. 
Sigh.

My proposition: I'll create and own a crbug to remove the use of
NativeViewPhotobooth in favor of using RenderWidgetHost::CopyFromBackingStore()
(and remove the inc/dec calls).  This is the approach the GTK controller uses,
and would remove the "code bloat" of having two separate screenshotting
implementations.

Does all this sound good to you?

-Yuri


P.S. If you're interested:

I traced back through the source code history.  SetCapturingContents() is a
rename from set_capturing_contents(), which itself was a merging of two methods
called DidCaptureContents() and WillCaptureContents().  I had to go all the way
back into the old internal source code base to find a check-in by beng@ back in
June 2008 which first introduced the code.  Based on the code description, and
beng's other check-ins at the time, I'm seems that the purpose of the code was
to make sure a HWND would be screenshotted correctly when starting a tab drag. 
There's evidence of a "HWNDPhotobooth" introduced just before this code (now
called NativeViewPhotobooth).

[Jói, 2013-02-05 12:49:47]

content/public API change LGTM, but I have a question about the implementation.

Cheers,
Jói

https://codereview.chromium.org/12179007/diff/10014/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/12179007/diff/10014/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1100: rwhv->WasHidden();
Should this also fire while the RWHVP is hidden, when capturer_count_
transitions from 1 to 0?

[sky, 2013-02-05 16:28:56]

Nuking NativeViewPhotobooth  SGTM.

  -Scott

On Mon, Feb 4, 2013 at 10:20 PM,  <miu@chromium.org> wrote:
> On 2013/02/05 01:43:19, sky wrote:
>>
>> How come you don't need calls to increment/decrement int gtk?
>
>
> Apparently, these calls have been "passed along" in the code base over the
> last
> 5 years, and are now mostly vestigal.  To prove the point, I removed them
> completely from both controllers, and tab drag screenshotting works
> perfectly
> fine on Linux and Windows 7.
>
> However, I'm *still* reluctant to remove them.  A comment hints at "Prevent
> the
> WebContents HWND from being hidden," and then code in
> native_view_photobooth_win.cc:26 (same dir) specifically mentions a Vista
> bug
> where the screenshotting will fail unless at least a 1x1 pixel is on-screen.
> Sigh.
>
> My proposition: I'll create and own a crbug to remove the use of
> NativeViewPhotobooth in favor of using
> RenderWidgetHost::CopyFromBackingStore()
> (and remove the inc/dec calls).  This is the approach the GTK controller
> uses,
> and would remove the "code bloat" of having two separate screenshotting
> implementations.
>
> Does all this sound good to you?
>
> -Yuri
>
>
> P.S. If you're interested:
>
> I traced back through the source code history.  SetCapturingContents() is a
> rename from set_capturing_contents(), which itself was a merging of two
> methods
> called DidCaptureContents() and WillCaptureContents().  I had to go all the
> way
> back into the old internal source code base to find a check-in by beng@ back
> in
> June 2008 which first introduced the code.  Based on the code description,
> and
> beng's other check-ins at the time, I'm seems that the purpose of the code
> was
> to make sure a HWND would be screenshotted correctly when starting a tab
> drag.
> There's evidence of a "HWNDPhotobooth" introduced just before this code (now
> called NativeViewPhotobooth).
>
> https://codereview.chromium.org/12179007/

[miu, 2013-02-05 20:58:26]

https://codereview.chromium.org/12179007/diff/10014/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/12179007/diff/10014/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1100: rwhv->WasHidden();
On 2013/02/05 12:49:47, Jói wrote:
> Should this also fire while the RWHVP is hidden, when capturer_count_
> transitions from 1 to 0?

Good call.  Done and tested.

[sky, 2013-02-05 21:35:57]

LGTM

[Jói, 2013-02-05 21:51:35]

LGTM, thanks.

[miu, 2013-02-05 22:51:12]

On 2013/02/05 16:28:56, sky wrote:
> Nuking NativeViewPhotobooth  SGTM.

FYI--Filed bug http://crbug.com/174474 to follow-up on this.

[commit-bot: I haz the power, 2013-02-05 22:53:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/12179007/27001

[commit-bot: I haz the power, 2013-02-06 01:59:57]

Change committed as 180857