CompositorOutputSurface should change thread priority on the target thread

CompositorOutputSurface should change thread priority on the target thread

On some platforms, e.g. Linux, it isn't permitted to change other
thread's priority. To avoid confusion, I'm removing the base/threading
interface that changes the other thread's priority.

CompositorOutputSurface is the last use case to change other threads'
priorities. So as a preparation to replace SetThreadPriority() with
SetCurrentThreadPriority(), I remove the case so that the next coming
patch can be trivial.

In this patch, I apply following changes.

- Modifies to change the thread priority on the target thread via PostTask.
  I confirmed that it was called when a user started and stopped page
  scrolling via touch. It does not happen frequently, and changing it on
  the target thread does not affect performance.

- Stop g_prefer_smoothness_count counting. The logic looks wrong, and
  UpdateSmoothnessTakesPriority() isn't called with balanced true and
  false. When scrolling starts, it is called with true, and until the
  scrolling stops, it is called periodically. Then, when the scrolling
  stops, it is called with false finally. So, counting does not make
  sense.

- Removes a wrong thread check in UpdateSmoothnessTakesPriority().
  This could be useful, but I'd remove it for now because the check is
  wrong. It just checked current thread is current thread.

BUG=468793
Committed: https://crrev.com/a281547280197998ca669bbd4a107fca1d6e1ed5
Cr-Commit-Position: refs/heads/master@{#336298}

[Takashi Toyoshima, 2015-06-24 02:13:06]

toyoshim@chromium.org changed reviewers:
+ enne@chromium.org

[Takashi Toyoshima, 2015-06-24 02:24:00]

enne, PTAL.

Here are some additional notes for my changes.

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_output_surface.cc (left):

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.cc:238: DCHECK_EQ(g_last_thread,
base::PlatformThread::CurrentId());
This is DCHECK_EQ(base::PlatformThread::CurrentId(),
base::PlatformThread::CurrentId()).

The right check would be something like:
static base::PlatformThreadId g_last_thread = base::InvalidThreadId;
base::PlatformThreadId current_thread = base::PlatformThread::CurrentId();
if (g_last_thread != base::InvalidThreadId)
  DCHECK_EQ(g_last_thread, current_thread);
g_last_thread = current_thread;

I'm not sure this check is still useful, but to say we do not need lock for
prefers_smoothnes_, it should be called from only one thread.

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.cc:240: if (prefers_smoothness_
== prefers_smoothness)
Because of this shortcut, following g_prefer_smoothness_count does not work
correctly.

[enne (OOO), 2015-06-24 18:21:58]

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_output_surface.h (right):

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.h:57: #if defined(OS_ANDROID)
Style bikeshed: I don't really like #ifdefs on functions, because it tends to
pollute callsites with #ifdefs as well.  Can you hide this in the
implementation, so that UpdateSmoothnessTakesPriority just does nothing early
outs if not on Android?

I'm coming to this patch not knowing that much about this code, but why does the
change description talk about Linux when this codepath only does something for
Android?

[Takashi Toyoshima, 2015-06-25 00:37:16]

Here is a background of this patch to understand the change description.

SetThreadPriority() does not work on some platforms for security restriction.
So I'm going to change the interface to SetCurrentThreadPriority() so that all
platform can work on each sandbox.
This is the last code that changes another thread' priority, and I'm trying to
change it to call SetThreadPriority() on the target thread so that I can change
it to SetCurrentThreadPriority() in the next patch
https://codereview.chromium.org/1193303002/.
Actually this is one of patches that is split from the patch to get deeper
review on each non-trivial change.

[Takashi Toyoshima, 2015-06-25 00:55:52]

I also updated the change description to understand why Android code path needs
this fix.
PTAL.

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_output_surface.h (right):

https://codereview.chromium.org/1205783002/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.h:57: #if defined(OS_ANDROID)
Since this is a virtual function, this #ifdef does not affect callers, but OK, I
will remove the #ifdef.

[enne (OOO), 2015-06-25 17:06:28]

lgtm

[Takashi Toyoshima, 2015-06-25 23:29:46]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-06-25 23:30:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1205783002/20001

[commit-bot: I haz the power, 2015-06-26 00:02:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-26 00:03:18]

Patchset 2 (id:??) landed as
https://crrev.com/a281547280197998ca669bbd4a107fca1d6e1ed5
Cr-Commit-Position: refs/heads/master@{#336298}