Refactor renderer LayerTreeSettings generating code to common.

Start to refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985
Committed: https://crrev.com/bf792fc2423d2ab7cd0e2b95f83c4d1489ee8135
Cr-Commit-Position: refs/heads/master@{#415366}

[xingliu, 2016-08-29 19:29:57]

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

[xingliu, 2016-08-29 19:30:41]

xingliu@chromium.org changed reviewers:
+ khushalsagar@chromium.org

[xingliu, 2016-08-29 19:30:42]

Use util class to refactor renderer LTS code.

[xingliu, 2016-08-29 19:34:19]

Description was changed from

==========
Refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp to reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985
==========

to

==========
Start to refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp to reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985
==========

[xingliu, 2016-08-29 19:36:27]

Description was changed from

==========
Start to refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp to reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985
==========

to

==========
Start to refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985
==========

[commit-bot: I haz the power, 2016-08-29 19:51:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Khushal, 2016-08-29 20:21:14]

https://codereview.chromium.org/2285083004/diff/20001/content/common/composit...
File content/common/compositor_util.h (right):

https://codereview.chromium.org/2285083004/diff/20001/content/common/composit...
content/common/compositor_util.h:16: class CONTENT_EXPORT CompositorUtil {
In cases like these I have seen that we just put it in a different namespace
rather than creating a class. I'll let piman@ suggest what's the preferred
style.

https://codereview.chromium.org/2285083004/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2285083004/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:349:
CompositorUtil::GenerateLayerTreeSettings(settings, cmd);
Do you want to just call it SetTopControlsSettings and move the part below as
well, they belong together. Its nice that we decompose this massive function
into these units now that we're moving it.

Later on you can put all these small functions within the compositor_util.cc
file itself and expose just one GenerateRendererLayerTreeSettings that will call
all of these.

[xingliu, 2016-08-29 20:43:35]

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

[xingliu, 2016-08-29 20:44:33]

Follow the idea of smaller functions.

https://codereview.chromium.org/2285083004/diff/20001/content/common/composit...
File content/common/compositor_util.h (right):

https://codereview.chromium.org/2285083004/diff/20001/content/common/composit...
content/common/compositor_util.h:16: class CONTENT_EXPORT CompositorUtil {
On 2016/08/29 20:21:14, Khushal wrote:
> In cases like these I have seen that we just put it in a different namespace
> rather than creating a class. I'll let piman@ suggest what's the preferred
> style.

Ok, I'll just leave it here for now.

https://codereview.chromium.org/2285083004/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2285083004/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:349:
CompositorUtil::GenerateLayerTreeSettings(settings, cmd);
On 2016/08/29 20:21:14, Khushal wrote:
> Do you want to just call it SetTopControlsSettings and move the part below as
> well, they belong together. Its nice that we decompose this massive function
> into these units now that we're moving it.
> 
> Later on you can put all these small functions within the compositor_util.cc
> file itself and expose just one GenerateRendererLayerTreeSettings that will
call
> all of these.

Done.

[Khushal, 2016-08-29 21:08:26]

khushalsagar@chromium.org changed reviewers:
+ piman@chromium.org

[Khushal, 2016-08-29 21:08:27]

looks great to me.
+piman for content review.

[piman, 2016-08-29 21:11:36]

https://codereview.chromium.org/2285083004/diff/40001/content/common/composit...
File content/common/compositor_util.h (right):

https://codereview.chromium.org/2285083004/diff/40001/content/common/composit...
content/common/compositor_util.h:16: class CONTENT_EXPORT CompositorUtil {
nit: CompositorUtil makes it sound like a grab-bag of stuff, which we usually
try to avoid.

How about LayerTreeSettingsFactory to make it clear what this is for?

[commit-bot: I haz the power, 2016-08-29 21:23:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-08-29 21:30:49]

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

[commit-bot: I haz the power, 2016-08-29 21:55:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2016-08-30 00:06:27]

https://codereview.chromium.org/2285083004/diff/40001/content/common/composit...
File content/common/compositor_util.h (right):

https://codereview.chromium.org/2285083004/diff/40001/content/common/composit...
content/common/compositor_util.h:16: class CONTENT_EXPORT CompositorUtil {
On 2016/08/29 21:11:36, piman wrote:
> nit: CompositorUtil makes it sound like a grab-bag of stuff, which we usually
> try to avoid.
> 
> How about LayerTreeSettingsFactory to make it clear what this is for?

Renamed to LayerTreeSettingsFactory, good idea.

[piman, 2016-08-30 00:11:41]

LGTM, thanks!

[xingliu, 2016-08-30 03:02:36]

The CQ bit was unchecked by xingliu@chromium.org

[xingliu, 2016-08-30 03:02:55]

The CQ bit was checked by xingliu@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:41:01]

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

[commit-bot: I haz the power, 2016-08-30 03:41:02]

Dry run: This issue passed the CQ dry run.

[xingliu, 2016-08-30 18:28:57]

The CQ bit was checked by xingliu@chromium.org

[commit-bot: I haz the power, 2016-08-30 18:29:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 18:35:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-30 18:37:17]

Description was changed from

==========
Start to refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985
==========

to

==========
Start to refactor renderer LayerTreeSettings generating code to common.

The goal is to let Blimp reuse this logic on browser process, since
Blimp client doesn't have renderer process.

BUG=577985

Committed: https://crrev.com/bf792fc2423d2ab7cd0e2b95f83c4d1489ee8135
Cr-Commit-Position: refs/heads/master@{#415366}
==========

[commit-bot: I haz the power, 2016-08-30 18:37:18]

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