cc: handling debug settings in new LayerTreeDebugState structure

This change puts all settings for debugging/HudLayer in a new LayerTreeDebugState structure. This allows for easier toggling of debug settings. Commandline switches get OR'ed in. 

BUG=


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

[egraether, 2012-11-15 23:37:07]

When working on a new toggle for the showPaintRects flag in the HUDLayer, I
realized that having all these flags in the LayerTreeSettings is not good. They
should belong to the HUDLayer, it should be easy to change them and easy to add
new ones.

https://codereview.chromium.org/11414017/diff/1/cc/debug_rect_history.cc
File cc/debug_rect_history.cc (right):

https://codereview.chromium.org/11414017/diff/1/cc/debug_rect_history.cc#newc...
cc/debug_rect_history.cc:33: if (flags.showPaintRects)
One of the main reasons for this whole change was my goal to make showPaintRects
toggle-able. It would have been a mess to plumb it in here, this way it's much
cleaner and easier to change the others to toggles as well.

https://codereview.chromium.org/11414017/diff/1/cc/heads_up_display_layer.h
File cc/heads_up_display_layer.h (right):

https://codereview.chromium.org/11414017/diff/1/cc/heads_up_display_layer.h#n...
cc/heads_up_display_layer.h:14: struct HeadsUpDisplayLayerFlags {
The new structure for storing all the flags that concern the HUDLayer. I
couldn't come up with a better name, any ideas?

Is the macro CC_EXPORT necessary? I saw it used on structures like this.

https://codereview.chromium.org/11414017/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/1/cc/layer_tree_host.cc#newcode121
cc/layer_tree_host.cc:121: flags.showPropertyChangedRects =
CommandLine::ForCurrentProcess()->HasSwitch(cc::switches::kShowPropertyChangedRects);
I moved the commandline reading for the rectangle flags temporarily here.

https://codereview.chromium.org/11414017/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/11414017/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_layer_tree_view_impl.cc:196:
m_layerTreeHost->requestHUDLayer()->setShowFPSCounter(show);
I tried to improve the pipeline here, by leaving out the methods in
LayerTreeHost and directly calling the HUDLayer.

[danakj, 2012-11-16 00:17:14]

This actually won't work well unfortunately.

While we are reading the command line flags in LayerTreeHost right now, we are
going to be moving that out to chromium (see TODO there). In order for chromium
to set these flags, then we would have to add setters for every flag, or plumb
around a second structure with LayerTreeSettings, which basically does the same
thing.

So, I would much rather keep these inside the LayerTreeSettings. We could group
them in a sub-struct there, but I'm not sure how much value that would add.

[egraether, 2012-11-16 01:13:02]

On 2012/11/16 00:17:14, danakj wrote:
> This actually won't work well unfortunately.
> 
> While we are reading the command line flags in LayerTreeHost right now, we are
> going to be moving that out to chromium (see TODO there). In order for
chromium
> to set these flags, then we would have to add setters for every flag, or plumb
> around a second structure with LayerTreeSettings, which basically does the
same
> thing.
> 
> So, I would much rather keep these inside the LayerTreeSettings. We could
group
> them in a sub-struct there, but I'm not sure how much value that would add.

I can wait till you are done with your todo and figure out something that works
then. The values need to get in somehow and putting them in this new structure
makes everything else easier.

Besides the commandline reading, what do you think of the changes?
Please let me know when you are done with your todo.
Thanks for having a look.

[egraether, 2012-11-20 21:30:29]

made changes based on 1:1 with nduca:

The commandline flags for the hudLayer are stored now in a substructure in
LayerTreeHostSettings called globalHudLayerSettings. The layerTreeHost also has
localHudLayerSettings that can be manipulated. These two get or'ed together
before passed to the hudLayer.

I could not make the hudLayer releaseable, because in that case the fontAtlas
gets lost. What's a good way to store the fontAtlas in layerTreeHost AND pass it
down to hudLayerImpl?

Right now I only avoid drawing and updating the hudLayer when it's not used.

https://codereview.chromium.org/11414017/diff/6002/cc/heads_up_display_layer.cc
File cc/heads_up_display_layer.cc (right):

https://codereview.chromium.org/11414017/diff/6002/cc/heads_up_display_layer....
cc/heads_up_display_layer.cc:91: return m_settings.showDebugInfo();
prevent drawing when nothing is displayed.

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host.cc#newc...
cc/layer_tree_host.cc:290: HeadsUpDisplayLayerSettings settings =
m_settings.globalHudLayerSettings.merge(m_localHUDLayerSettings);
or'ing global and local settings

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:71: HeadsUpDisplayLayerSettings globalHudLayerSettings;
substructure for the commandline settings

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:717: if (m_hudLayerImpl &&
m_hudLayerImpl->settings().showDebugInfo()) {
prevent updating when nothing is displayed.

[danakj, 2012-11-20 22:12:21]

https://codereview.chromium.org/11414017/diff/6002/cc/heads_up_display_layer.h
File cc/heads_up_display_layer.h (right):

https://codereview.chromium.org/11414017/diff/6002/cc/heads_up_display_layer....
cc/heads_up_display_layer.h:14: struct HeadsUpDisplayLayerSettings {
I'd rather not tie this to HUD layer so strongly, and instead make it
"LayerTreeDebugSettings" or something. And include the layer borders debug
option in it. That way we can potentially toggle layer borders in the future
through inspector, and doesn't tie the design to the HUD layer for future debug
visualizations.

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11414017/diff/6002/cc/layer_tree_host.h#newco...
cc/layer_tree_host.h:71: HeadsUpDisplayLayerSettings globalHudLayerSettings;
rename to initial instead of global?

[egraether, 2012-11-22 00:23:15]

Rewrote to new structure LayerTreeSwitches, which holds values that can be
changed while running. This structure gets stored in LayerTreeHost and is
synchronised with LayerTreeHostImpl.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.cc#newcod...
cc/layer_tree_host.cc:330: hostImpl->setSwitches(m_switches);
The switches get pushed to hostImpl. No more need to plumb through the HudLayer.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.cc#newcod...
cc/layer_tree_host.cc:480: m_switches =
m_settings.initialSwitchSettings.merge(switches);
The switches get or'ed together with the initial ones from the commandline.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode52
cc/layer_tree_host.h:52: struct CC_EXPORT LayerTreeSwitches {
New structure LayerTreeSwitches holds values that can be changed while running.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode58
cc/layer_tree_host.h:58: bool showDebugBorders;
Also holds showDebugBorders

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode92
cc/layer_tree_host.h:92: LayerTreeSwitches initialSwitchSettings;
Initial switch values from the commandline get stored as substructure of
LayerTreeSettings.

What about inheriting LayerTreeSettings from LayerTreeSwitches instead?

[danakj, 2012-11-26 19:57:18]

I like this very very much :)

Some naming nits/thoughts, and a couple comments for you.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.cc#newcod...
cc/layer_tree_host.cc:481: setNeedsCommit();
We should avoid setting needsCommit if the new switch values don't differ from
the old ones. You probably want another static method on the struct called "bool
equal(a, b)".

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode52
cc/layer_tree_host.h:52: struct CC_EXPORT LayerTreeSwitches {
On 2012/11/22 00:23:15, egraether wrote:
> New structure LayerTreeSwitches holds values that can be changed while
running.

This is great. The only concern I have here is that "Switches" is the word we
use for --these-things in the command line as well.

I'm not sure if I have any reasonable name suggestions though. What about
LayerTreeToggles?

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode69
cc/layer_tree_host.h:69: bool showHudRects() const;
I like the name changes here a lot.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode71
cc/layer_tree_host.h:71: LayerTreeSwitches merge(const LayerTreeSwitches& other)
const;
How about a static method that takes two instances as arguments? structs
shouldn't really have member methods.

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode92
cc/layer_tree_host.h:92: LayerTreeSwitches initialSwitchSettings;
initialSwitchValues? (or initialToggleValues?)

"Settings" is a bit overloaded here.

https://codereview.chromium.org/11414017/diff/16/webkit/compositor_bindings/w...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11414017/diff/16/webkit/compositor_bindings/w...
webkit/compositor_bindings/web_layer_tree_view_impl.h:54: virtual void
setShowPaintRects(bool show);
This looks like a separate CL?

[danakj, 2012-11-26 19:58:25]

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h
File cc/layer_tree_host.h (right):

https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode52
cc/layer_tree_host.h:52: struct CC_EXPORT LayerTreeSwitches {
On 2012/11/26 19:57:18, danakj wrote:
> On 2012/11/22 00:23:15, egraether wrote:
> > New structure LayerTreeSwitches holds values that can be changed while
> running.
> 
> This is great. The only concern I have here is that "Switches" is the word we
> use for --these-things in the command line as well.
> 
> I'm not sure if I have any reasonable name suggestions though. What about
> LayerTreeToggles?

Nat suggested LayerTreeDebugState, which I like. Let's do that?

[egraether, 2012-11-27 19:23:38]

On 2012/11/26 19:58:25, danakj wrote:
> https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h
> File cc/layer_tree_host.h (right):
> 
>
https://codereview.chromium.org/11414017/diff/16/cc/layer_tree_host.h#newcode52
> cc/layer_tree_host.h:52: struct CC_EXPORT LayerTreeSwitches {
> On 2012/11/26 19:57:18, danakj wrote:
> > On 2012/11/22 00:23:15, egraether wrote:
> > > New structure LayerTreeSwitches holds values that can be changed while
> > running.
> > 
> > This is great. The only concern I have here is that "Switches" is the word
we
> > use for --these-things in the command line as well.
> > 
> > I'm not sure if I have any reasonable name suggestions though. What about
> > LayerTreeToggles?
> 
> Nat suggested LayerTreeDebugState, which I like. Let's do that?

done.

[danakj, 2012-11-27 20:23:15]

Do we have any tests that verify these debug flags end up being
set/merged/committed correctly? Can we add a test if not?

https://codereview.chromium.org/11414017/diff/13009/cc/heads_up_display_layer.cc
File cc/heads_up_display_layer.cc (right):

https://codereview.chromium.org/11414017/diff/13009/cc/heads_up_display_layer...
cc/heads_up_display_layer.cc:70: if (m_fontAtlas.get()) {
instead of nesting here, just early-out when (!m_fontAtlas)

.get() is redundant also :)

https://codereview.chromium.org/11414017/diff/13009/cc/heads_up_display_layer...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11414017/diff/13009/cc/heads_up_display_layer...
cc/heads_up_display_layer_impl.cc:160: if (debugState.showPlatformLayerTree &&
m_fontAtlas.get()) {
drop the .get() while we're here

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:83: return memcmp(&a, &b, sizeof(LayerTreeDebugState)) ==
0;
this is fine.

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:86: LayerTreeDebugState LayerTreeDebugState::merge(const
LayerTreeDebugState& a, const LayerTreeDebugState& b)
better names than "a" and "b" please. initialState and requestedEnabledState?

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
i liked just doing the | for each flag before better.

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:488: if (!LayerTreeDebugState::equal(m_debugState,
debugState)) {
I think you want to merge, then compare to m_debugState. Then
assign/setNeedsCommit if anything changed.

[nduca, 2012-11-27 20:42:02]

I dont think we need tests

[enne (OOO), 2012-11-27 21:26:21]

On 2012/11/27 20:42:02, nduca wrote:
> I dont think we need tests

I do.  I think that this settings implementation has gotten complicated enough
(e.g. memmrg) that it needs unit tests.  Alternatively, make it less
complicated.

[egraether, 2012-11-27 22:10:47]

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:86: LayerTreeDebugState LayerTreeDebugState::merge(const
LayerTreeDebugState& a, const LayerTreeDebugState& b)
On 2012/11/27 20:23:15, danakj wrote:
> better names than "a" and "b" please. initialState and requestedEnabledState?

this method just ORs the states and returns a new one, so there is no real
meaning to the parameters. I think putting something could be misleading. same
applies for the equal method.

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
On 2012/11/27 20:23:15, danakj wrote:
> i liked just doing the | for each flag before better.

that was just a little experiment, because I don't really like that big block of
| statements. I then realized that this wouldn't work to well as soon as you add
members that are numbers or anything.

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:488: if (!LayerTreeDebugState::equal(m_debugState,
debugState)) {
On 2012/11/27 20:23:15, danakj wrote:
> I think you want to merge, then compare to m_debugState. Then
> assign/setNeedsCommit if anything changed.

Yeah, that sounds more stable. I just tried to be efficient.

[danakj, 2012-11-27 22:15:00]

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
On 2012/11/27 22:10:47, egraether wrote:
> On 2012/11/27 20:23:15, danakj wrote:
> > i liked just doing the | for each flag before better.
> 
> that was just a little experiment, because I don't really like that big block
of
> | statements. I then realized that this wouldn't work to well as soon as you
add
> members that are numbers or anything.

merge(a, b) is super not clear what is happening though.

We should use names for the variables that show their intended use.

equals(a,b) doesn't make me wonder about what the inside of the function is
doing and what the purpose of the two variables is.

[egraether, 2012-11-27 22:16:26]

On 2012/11/27 21:26:21, enne wrote:
> On 2012/11/27 20:42:02, nduca wrote:
> > I dont think we need tests
> 
> I do.  I think that this settings implementation has gotten complicated enough
> (e.g. memmrg) that it needs unit tests.  Alternatively, make it less
> complicated.

Yeah, the memmrg was just a little experiment, it won't get in.

The reason this implementation has gotten complicated is that the commandline
flags get now ORed in, instead of just being the initial values. I understand
what's good about that, but it sure adds complexity.

[egraether, 2012-11-27 23:16:41]

On 2012/11/27 22:15:00, danakj wrote:
> https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
> File cc/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
> cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
> On 2012/11/27 22:10:47, egraether wrote:
> > On 2012/11/27 20:23:15, danakj wrote:
> > > i liked just doing the | for each flag before better.
> > 
> > that was just a little experiment, because I don't really like that big
block
> of
> > | statements. I then realized that this wouldn't work to well as soon as you
> add
> > members that are numbers or anything.
> 
> merge(a, b) is super not clear what is happening though.
> 
> We should use names for the variables that show their intended use.
> 
> equals(a,b) doesn't make me wonder about what the inside of the function is
> doing and what the purpose of the two variables is.

I can't really think of parameter names that make sense.
To me it sounds like we should change the method name:

combine(a, b);
join(a, b);
mix(a, b);
sum(a, b);
total(a, b);
union(a, b);

I like union.

[danakj, 2012-11-27 23:20:41]

On 2012/11/27 23:16:41, egraether wrote:
> On 2012/11/27 22:15:00, danakj wrote:
> > https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
> > File cc/layer_tree_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
> > cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
> > On 2012/11/27 22:10:47, egraether wrote:
> > > On 2012/11/27 20:23:15, danakj wrote:
> > > > i liked just doing the | for each flag before better.
> > > 
> > > that was just a little experiment, because I don't really like that big
> block
> > of
> > > | statements. I then realized that this wouldn't work to well as soon as
you
> > add
> > > members that are numbers or anything.
> > 
> > merge(a, b) is super not clear what is happening though.
> > 
> > We should use names for the variables that show their intended use.
> > 
> > equals(a,b) doesn't make me wonder about what the inside of the function is
> > doing and what the purpose of the two variables is.
> 
> I can't really think of parameter names that make sense.
> To me it sounds like we should change the method name:
> 
> combine(a, b);
> join(a, b);
> mix(a, b);
> sum(a, b);
> total(a, b);
> union(a, b);
> 
> I like union.

I like union as well.

[egraether, 2012-11-27 23:43:58]

On 2012/11/27 23:20:41, danakj wrote:
> On 2012/11/27 23:16:41, egraether wrote:
> > On 2012/11/27 22:15:00, danakj wrote:
> > > https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
> > > File cc/layer_tree_host.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
> > > cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
> > > On 2012/11/27 22:10:47, egraether wrote:
> > > > On 2012/11/27 20:23:15, danakj wrote:
> > > > > i liked just doing the | for each flag before better.
> > > > 
> > > > that was just a little experiment, because I don't really like that big
> > block
> > > of
> > > > | statements. I then realized that this wouldn't work to well as soon as
> you
> > > add
> > > > members that are numbers or anything.
> > > 
> > > merge(a, b) is super not clear what is happening though.
> > > 
> > > We should use names for the variables that show their intended use.
> > > 
> > > equals(a,b) doesn't make me wonder about what the inside of the function
is
> > > doing and what the purpose of the two variables is.
> > 
> > I can't really think of parameter names that make sense.
> > To me it sounds like we should change the method name:
> > 
> > combine(a, b);
> > join(a, b);
> > mix(a, b);
> > sum(a, b);
> > total(a, b);
> > union(a, b);
> > 
> > I like union.
> 
> I like union as well.

union is a keyword in C++ :)

I could also change the implementation to merge(destination, source) or we take
one of the above.

[danakj, 2012-11-27 23:46:30]

On Tue, Nov 27, 2012 at 3:42 PM,  <nduca@chromium.org> wrote:
> I dont think we need tests

How come? It's not like our debugging stuff doesn't regress all the
time. Verifying that setting a debug flag actually ends up on the
layer tree host seems pretty minimal but useful.

[danakj, 2012-11-27 23:49:31]

On Tue, Nov 27, 2012 at 6:43 PM,  <egraether@chromium.org> wrote:
> On 2012/11/27 23:20:41, danakj wrote:
>>
>> On 2012/11/27 23:16:41, egraether wrote:
>> > On 2012/11/27 22:15:00, danakj wrote:
>> > >
>> > >
https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
>> > > File cc/layer_tree_host.cc (right):
>> > >
>> > >
>> >
>
>
>
https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
>>
>> > > cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
>> > > On 2012/11/27 22:10:47, egraether wrote:
>> > > > On 2012/11/27 20:23:15, danakj wrote:
>> > > > > i liked just doing the | for each flag before better.
>> > > >
>> > > > that was just a little experiment, because I don't really like that
>> > > > big
>> > block
>> > > of
>> > > > | statements. I then realized that this wouldn't work to well as
>> > > > soon as
>> you
>> > > add
>> > > > members that are numbers or anything.
>> > >
>> > > merge(a, b) is super not clear what is happening though.
>> > >
>> > > We should use names for the variables that show their intended use.
>> > >
>> > > equals(a,b) doesn't make me wonder about what the inside of the
>> > > function
>
> is
>>
>> > > doing and what the purpose of the two variables is.
>> >
>> > I can't really think of parameter names that make sense.
>> > To me it sounds like we should change the method name:
>> >
>> > combine(a, b);
>> > join(a, b);
>> > mix(a, b);
>> > sum(a, b);
>> > total(a, b);
>> > union(a, b);
>> >
>> > I like union.
>
>
>> I like union as well.
>
>
> union is a keyword in C++ :)
>
> I could also change the implementation to merge(destination, source) or we
> take
> one of the above.

Union() would work if this file was chromiumized. You can use unite(),
and stick a comment on the header saying it's taking the union.

[egraether, 2012-11-28 19:11:20]

On 2012/11/27 23:49:31, danakj wrote:
> On Tue, Nov 27, 2012 at 6:43 PM,  <mailto:egraether@chromium.org> wrote:
> > On 2012/11/27 23:20:41, danakj wrote:
> >>
> >> On 2012/11/27 23:16:41, egraether wrote:
> >> > On 2012/11/27 22:15:00, danakj wrote:
> >> > >
> >> > >
> https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc
> >> > > File cc/layer_tree_host.cc (right):
> >> > >
> >> > >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/11414017/diff/13009/cc/layer_tree_host.cc#new...
> >>
> >> > > cc/layer_tree_host.cc:89: memmrg(&r, &b, sizeof(LayerTreeDebugState));
> >> > > On 2012/11/27 22:10:47, egraether wrote:
> >> > > > On 2012/11/27 20:23:15, danakj wrote:
> >> > > > > i liked just doing the | for each flag before better.
> >> > > >
> >> > > > that was just a little experiment, because I don't really like that
> >> > > > big
> >> > block
> >> > > of
> >> > > > | statements. I then realized that this wouldn't work to well as
> >> > > > soon as
> >> you
> >> > > add
> >> > > > members that are numbers or anything.
> >> > >
> >> > > merge(a, b) is super not clear what is happening though.
> >> > >
> >> > > We should use names for the variables that show their intended use.
> >> > >
> >> > > equals(a,b) doesn't make me wonder about what the inside of the
> >> > > function
> >
> > is
> >>
> >> > > doing and what the purpose of the two variables is.
> >> >
> >> > I can't really think of parameter names that make sense.
> >> > To me it sounds like we should change the method name:
> >> >
> >> > combine(a, b);
> >> > join(a, b);
> >> > mix(a, b);
> >> > sum(a, b);
> >> > total(a, b);
> >> > union(a, b);
> >> >
> >> > I like union.
> >
> >
> >> I like union as well.
> >
> >
> > union is a keyword in C++ :)
> >
> > I could also change the implementation to merge(destination, source) or we
> > take
> > one of the above.
> 
> Union() would work if this file was chromiumized. You can use unite(),
> and stick a comment on the header saying it's taking the union.

I just called it unite.

[danakj, 2012-11-28 19:22:15]

LGTM+nit thanks!

https://codereview.chromium.org/11414017/diff/12008/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11414017/diff/12008/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:489: if (!LayerTreeDebugState::equal(m_debugState,
newDebugState)) {
nit: early-out if they're equal instead

[egraether, 2012-11-28 20:51:10]

nduca, piman can you please have a look as well?

[nduca, 2012-11-28 21:22:30]

bindings lgtm

[piman, 2012-11-28 22:34:55]

lgtm

[commit-bot: I haz the power, 2012-11-28 22:39:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11414017/12012

[commit-bot: I haz the power, 2012-11-28 23:48:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 17:48:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/egraether@chromium.org/11414017/14007

[commit-bot: I haz the power, 2012-11-30 01:55:22]

Change committed as 170345