Close the screenshot flash effect before the screenshot task is actually executed.

Close the screenshot flash effect before the screenshot task is actually executed.

The screenshot task takes time so the flash could be noticeably long.

BUG=120092
TEST=manually

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

Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=129442

[Daniel Erat, 2012-03-26 13:15:32]

I forget already, what was the reason for making the flash happen before the
screenshot is taken instead of the other way around?

https://chromiumcodereview.appspot.com/9854012/diff/1/chrome/browser/ui/views...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

https://chromiumcodereview.appspot.com/9854012/diff/1/chrome/browser/ui/views...
chrome/browser/ui/views/ash/screenshot_taker.cc:122:
MessageLoopForUI::current()->PostTask(FROM_HERE, task);
I don't know if this will be guaranteed to work in the future.  I'd double-check
with someone working on the compositor.

[Jun Mukai, 2012-03-26 14:27:12]

See crbug.com/119492 for the reason of order change: visual feedback -> actual
screenshot task.

The motivation of this CL is actually same as the bug.  The root cause is
GrabWindowSnapshot() takes very long time; we found sometimes it could be
~600msec for the whole screenshot on lumpy (noticed this in chrome://tracing). 
Cc-ed mazda who knows this much more.

If we make the visual feedback after the screenshot, the effect could have
noticeable delay, so I changed the order.  And because the current code runs
GrabWindowSnapshot() within CloseVisualFeedbackLayer(), the visual effect will
remain by end of the task.

http://codereview.chromium.org/9854012/diff/1/chrome/browser/ui/views/ash/scr...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/9854012/diff/1/chrome/browser/ui/views/ash/scr...
chrome/browser/ui/views/ash/screenshot_taker.cc:122:
MessageLoopForUI::current()->PostTask(FROM_HERE, task);
On 2012/03/26 13:15:32, Daniel Erat wrote:
> I don't know if this will be guaranteed to work in the future.  I'd
double-check
> with someone working on the compositor.

Who should we ask for the double-check?  oshima?
The comment would be misleading by the way.  Here it's not necessary to
guarantee the task runs before the view visually disappears.  Here I just want
the visual_feedback_layer_ to disappear without waiting the end of task running.
Updated the comment.  Sorry for the confusing comment.

[Daniel Erat, 2012-03-26 14:49:14]

LGTM

Hmm, so this means that we can sometimes block the UI thread for 600 ms?

http://codereview.chromium.org/9854012/diff/1/chrome/browser/ui/views/ash/scr...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/9854012/diff/1/chrome/browser/ui/views/ash/scr...
chrome/browser/ui/views/ash/screenshot_taker.cc:122:
MessageLoopForUI::current()->PostTask(FROM_HERE, task);
On 2012/03/26 14:27:12, Jun Mukai wrote:
> On 2012/03/26 13:15:32, Daniel Erat wrote:
> > I don't know if this will be guaranteed to work in the future.  I'd
> double-check
> > with someone working on the compositor.
> 
> Who should we ask for the double-check?  oshima?
> The comment would be misleading by the way.  Here it's not necessary to
> guarantee the task runs before the view visually disappears.  Here I just want
> the visual_feedback_layer_ to disappear without waiting the end of task
running.
> Updated the comment.  Sorry for the confusing comment.

piman@ would know what the plans (if any) are for moving compositing off of the
UI thread.

http://codereview.chromium.org/9854012/diff/5001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/9854012/diff/5001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/screenshot_taker.cc:120: // We want to make the
close of visual effect without waiting for the end of
nit: "Hide the visual feedback immediately because |task| may take a long time
to finish."

[Jun Mukai, 2012-03-26 15:09:30]

Added piman to Cc just in case.

> Hmm, so this means that we can sometimes block the UI thread for 600 ms?
I'm afraid so, but I guess it highly depends on the device/GPU.
As far as I tested locally, the block is not noticeable if visual effect closes
immediately, but things may change if I'm viewing highly dynamic pages?

http://codereview.chromium.org/9854012/diff/5001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/screenshot_taker.cc (right):

http://codereview.chromium.org/9854012/diff/5001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/screenshot_taker.cc:120: // We want to make the
close of visual effect without waiting for the end of
On 2012/03/26 14:49:15, Daniel Erat wrote:
> nit: "Hide the visual feedback immediately because |task| may take a long time
> to finish."

Done.

[piman, 2012-03-26 16:49:56]

I sincerely lack the context.
At some point we may indeed want to move the compositor off the main thread.
What is the concern about this particular patch?

[Daniel Erat, 2012-03-26 16:53:19]

On 2012/03/26 16:49:56, piman wrote:
> I sincerely lack the context.
> At some point we may indeed want to move the compositor off the main thread.
> What is the concern about this particular patch?

That with the approach used here (to display visual feedback in response to a
screenshot being taken, create a layer, hide it slightly later, and then post a
task to actually capture the screen), the layer could end up obscuring the
screenshot.  I think that this change is dependent on the compositor drawing a
frame between the layer being destroyed and the screenshot-taking task being
run.

[piman, 2012-03-26 16:54:29]

On 2012/03/26 16:53:19, Daniel Erat wrote:
> On 2012/03/26 16:49:56, piman wrote:
> > I sincerely lack the context.
> > At some point we may indeed want to move the compositor off the main thread.
> > What is the concern about this particular patch?
> 
> That with the approach used here (to display visual feedback in response to a
> screenshot being taken, create a layer, hide it slightly later, and then post
a
> task to actually capture the screen), the layer could end up obscuring the
> screenshot.  I think that this change is dependent on the compositor drawing a
> frame between the layer being destroyed and the screenshot-taking task being
> run.

Well that could happen with or without the compositor thread.