Use Custome ListContainer to Allocate SharedQuadState

Use Custome ListContainer to Allocate SharedQuadState

In RenderPass use ListContainer for generating SharedQuadState and act
as SharedQuadStateList.

This CL follows 448303002 which use ListContainer for
DrawQuad.

BUG=344962
Committed: https://crrev.com/808f70fa59631bb0fc9d0e0fd7e5956d0f1c052a
Cr-Commit-Position: refs/heads/master@{#298107}

[weiliangc, 2014-09-12 22:18:19]

weiliangc@chromium.org changed reviewers:
+ danakj@chromium.org, jamesr@chromium.org, kenrb@chromium.org

[weiliangc, 2014-09-12 22:18:51]

danakj: cc
kenrb: cc_message
jamesr: mojo

[weiliangc, 2014-09-23 16:40:45]

Rebase to latest. This follows the DrawQuad CL.

[weiliangc, 2014-09-26 16:58:32]

Patchset #5 (id:80001) has been deleted

[weiliangc, 2014-09-26 17:06:04]

Patchset #5 (id:100001) has been deleted

[weiliangc, 2014-09-26 17:38:32]

Rebased to latest and ready to be reviewed. :)

[danakj, 2014-09-26 20:40:56]

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:69: num_layers ? num_layers :
kDefaultNumSharedQuadStatesToReserve) {
can num_layers be 0? Can the ListContainer not handle that as an input? maybe
you can fix that instead of doing this check here?

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:121: copy_shared_quad_state->CopyFrom(&(*iter));
you didn't use &(*a) just &*a in other patches, what changed?

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:130: while (iter->shared_quad_state != &(*sqs_iter)) {
&*?

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:135: DCHECK(iter->shared_quad_state == &(*sqs_iter));
&*?

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:137: DrawQuad* quad = &*iter;
can you put the copy SQS* into a variable here too, to make below more clear?

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.h#...
cc/quads/render_pass.h:122: RenderPass(size_t shared_quad_state_list, size_t
quad_list_size);
sqs_list_size <- size missing

https://codereview.chromium.org/551013002/diff/140001/cc/quads/shared_quad_st...
File cc/quads/shared_quad_state.h (left):

https://codereview.chromium.org/551013002/diff/140001/cc/quads/shared_quad_st...
cc/quads/shared_quad_state.h:22: 
don't

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:295: while (quad->shared_quad_state !=
&(*sqs_iter)) {
&*?

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:299: DCHECK_EQ(quad->shared_quad_state,
&(*sqs_iter));
&*?

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:853: ++sqs_iter;
ElementAt

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:858: EXPECT_EQ(gfx::Rect(0, 10, 5,
5).ToString(), sqs_iter->clip_rect.ToString());
ElementAt

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:299: cc::SharedQuadStateList::ConstIterator
shared_quad_state_index =
this isn't an index, it's an iterator, so let's name it as such

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:301: cc::SharedQuadStateList::ConstIterator
last_shared_quad_state_index =
same

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:359: quad->shared_quad_state !=
&(*shared_quad_state_index))
&*?

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:379: // The shared_quad_state_index for each quad.
update this

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:512: for (cc::SharedQuadStateList::ConstIterator
iter =
Can you use auto and c++11 for loops in this CL? They are allowed now and would
make things like this much easier to read

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:518: LogParam(*(&(*iter)), l);
wat. you mean *iter?

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:704: to_reserve += sizeof(size_t);
*2?

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:735: if (!ReadParam(m, iter, &quad_list_size) ||
can you range check these things? like kMaxRenderPasses, but larger. Maybe
100000?

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages_unittest.cc:481: Compare(&*cmp_iterator,
&*in_iterator);
wdyt of changing all the Compare() methods to take const& instead of const* in a
followup CL?

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:364:
shared_quad_state[sqs_i] = SharedQuadState::From(*sqs_iter);
what if the list is empty? this seems less safe than the old code. maybe..

const cc::SQS* last_sqs = nullptr;
auto next_sqs_iter = input.sqs_list.begin();

and then the rest of the code looks mostly the same?

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:372: sqs_i++;
nit: ++sqs_i; to match

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:405: ++sqs_i;
if you added an index() to the iterator, would it let you avoid this double
incrementing in a bunch of places?

[weiliangc, 2014-09-26 21:48:02]

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:69: num_layers ? num_layers :
kDefaultNumSharedQuadStatesToReserve) {
On 2014/09/26 20:40:55, danakj wrote:
> can num_layers be 0? Can the ListContainer not handle that as an input? maybe
> you can fix that instead of doing this check here?

num_layer can be 0 in unittests.

Yeah you are right. The size would be "minimum" size of list to reserve for,
thus a 0 input would make sense.

I thought of this input as "exact" size of list so it's user's responsibility to
not reserve for size 0.

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:121: copy_shared_quad_state->CopyFrom(&(*iter));
On 2014/09/26 20:40:55, danakj wrote:
> you didn't use &(*a) just &*a in other patches, what changed?

...I used &*a in ListContainer and its unittest.

For iterators, operator* return T&, and to get T*, it has to be &*iter

Plus the use of const T* for input field in some code.

Clean up time for const T* related to SQS and DQ?

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages_unittest.cc:481: Compare(&*cmp_iterator,
&*in_iterator);
On 2014/09/26 20:40:55, danakj wrote:
> wdyt of changing all the Compare() methods to take const& instead of const* in
a
> followup CL?

^ per previous comment, clean up time for all the const SQS*? (more than just
Compare)

[weiliangc, 2014-09-26 21:57:39]

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:405: ++sqs_i;
On 2014/09/26 20:40:55, danakj wrote:
> if you added an index() to the iterator, would it let you avoid this double
> incrementing in a bunch of places?

This implementation would be kind of awkward. What would the index be for
rend()?

[danakj, 2014-09-26 22:46:40]

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages_unittest.cc:481: Compare(&*cmp_iterator,
&*in_iterator);
On 2014/09/26 21:48:02, weiliangc wrote:
> On 2014/09/26 20:40:55, danakj wrote:
> > wdyt of changing all the Compare() methods to take const& instead of const*
in
> a
> > followup CL?
> 
> ^ per previous comment, clean up time for all the const SQS*? (more than just
> Compare)

SGTM

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:405: ++sqs_i;
On 2014/09/26 21:57:39, weiliangc wrote:
> On 2014/09/26 20:40:55, danakj wrote:
> > if you added an index() to the iterator, would it let you avoid this double
> > incrementing in a bunch of places?
> 
> This implementation would be kind of awkward. What would the index be for
> rend()?

Either 0 or size is fine as long as we define it as such I think.

[weiliangc, 2014-10-01 23:02:01]

Address most review comments (other than C++11 loop)

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:130: while (iter->shared_quad_state != &(*sqs_iter)) {
On 2014/09/26 20:40:55, danakj wrote:
> &*?

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:135: DCHECK(iter->shared_quad_state == &(*sqs_iter));
On 2014/09/26 20:40:55, danakj wrote:
> &*?

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:137: DrawQuad* quad = &*iter;
On 2014/09/26 20:40:55, danakj wrote:
> can you put the copy SQS* into a variable here too, to make below more clear?

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/551013002/diff/140001/cc/quads/render_pass.h#...
cc/quads/render_pass.h:122: RenderPass(size_t shared_quad_state_list, size_t
quad_list_size);
On 2014/09/26 20:40:55, danakj wrote:
> sqs_list_size <- size missing

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:295: while (quad->shared_quad_state !=
&(*sqs_iter)) {
On 2014/09/26 20:40:55, danakj wrote:
> &*?

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:299: DCHECK_EQ(quad->shared_quad_state,
&(*sqs_iter));
On 2014/09/26 20:40:55, danakj wrote:
> &*?

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:853: ++sqs_iter;
On 2014/09/26 20:40:55, danakj wrote:
> ElementAt

Done.

https://codereview.chromium.org/551013002/diff/140001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_unittest.cc:858: EXPECT_EQ(gfx::Rect(0, 10, 5,
5).ToString(), sqs_iter->clip_rect.ToString());
On 2014/09/26 20:40:55, danakj wrote:
> ElementAt

Done.

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:299: cc::SharedQuadStateList::ConstIterator
shared_quad_state_index =
On 2014/09/26 20:40:55, danakj wrote:
> this isn't an index, it's an iterator, so let's name it as such

Done.

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:301: cc::SharedQuadStateList::ConstIterator
last_shared_quad_state_index =
On 2014/09/26 20:40:55, danakj wrote:
> same

Done.

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:359: quad->shared_quad_state !=
&(*shared_quad_state_index))
On 2014/09/26 20:40:55, danakj wrote:
> &*?

Done.

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:518: LogParam(*(&(*iter)), l);
On 2014/09/26 20:40:55, danakj wrote:
> wat. you mean *iter?

Done.

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:704: to_reserve += sizeof(size_t);
On 2014/09/26 20:40:55, danakj wrote:
> *2?

Done.

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:735: if (!ReadParam(m, iter, &quad_list_size) ||
On 2014/09/26 20:40:55, danakj wrote:
> can you range check these things? like kMaxRenderPasses, but larger. Maybe
> 100000?

Done.

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/140001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:364:
shared_quad_state[sqs_i] = SharedQuadState::From(*sqs_iter);
On 2014/09/26 20:40:56, danakj wrote:
> what if the list is empty? this seems less safe than the old code. maybe..
> 
> const cc::SQS* last_sqs = nullptr;
> auto next_sqs_iter = input.sqs_list.begin();
> 
> and then the rest of the code looks mostly the same?

Done.

[weiliangc, 2014-10-01 23:02:50]

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/551013002/diff/140001/content/common/cc_messa...
content/common/cc_messages.cc:379: // The shared_quad_state_index for each quad.
On 2014/09/26 20:40:55, danakj wrote:
> update this

I think the logic is correct, but I add more comments hopefully more clear.

[weiliangc, 2014-10-01 23:33:06]

Use range based loop other than one case where iterator.index() is useful.

Follow up CL:
- Clean up QuadList with range based loop
- Clean up const DQ* and const SQS* to const DQ/SQS&

[danakj, 2014-10-02 15:32:49]

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container.cc
File cc/quads/list_container.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.cc:380: size_t cache_index = index;
s/cache/original/

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.cc:397: size_t cache_index = index;
same

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:89: // is
fix formatting

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:115: // is
formatting

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:144: // rend()
formatting

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:172: // rend()
formattting

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
File cc/quads/list_container_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container_unittest.cc:539:
std::vector<SharedQuadState*>::const_iterator sqs_iter = sqs_list.begin();
drop teh vector iterator

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container_unittest.cc:543: EXPECT_EQ(*sqs_iter, &*iter);
you already test this in other tests, just test the new bits here

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:124: for (auto& quad : source->quad_list) {
can this be const&?

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:130: DCHECK(quad.shared_quad_state == &*sqs_iter);
drop the sqs_iter and just use ElementAt since it's only for the DCHECK

https://codereview.chromium.org/551013002/diff/260001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:294: DCHECK(sqs_iter !=
source_shared_quad_state_list.end());
sqs_iter is only used for dchecks, can you use ElementAt instead? i think using
the old size_t is a bit easier to read, or at least makes the diff easier. or
maybe use the iterator index (which i guess would mean no fancy for loop,
though)

https://codereview.chromium.org/551013002/diff/260001/content/common/cc_messa...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/551013002/diff/260001/content/common/cc_messa...
content/common/cc_messages.cc:719: const static size_t kMaxQuadListSize =
100000;
actually i wonder if this is too few for webview, can you *10 this one?

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:377:
DCHECK_EQ(static_cast<size_t>(next_sqs_iter.index()),
static cast needed?

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/in...
File mojo/services/public/interfaces/surfaces/quads.mojom (right):

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/in...
mojo/services/public/interfaces/surfaces/quads.mojom:126: uint32
shared_quad_state_index;
did you run bots for this? you're using a size_t elsewhere which isn't 32bits
always. either this needs to be size_t or you need to case, I think (probly the
latter)

[weiliangc, 2014-10-02 22:01:41]

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container.cc
File cc/quads/list_container.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.cc:380: size_t cache_index = index;
On 2014/10/02 15:32:47, danakj wrote:
> s/cache/original/

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.cc:397: size_t cache_index = index;
On 2014/10/02 15:32:47, danakj wrote:
> same

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:89: // is
On 2014/10/02 15:32:48, danakj wrote:
> fix formatting

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:115: // is
On 2014/10/02 15:32:48, danakj wrote:
> formatting

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:144: // rend()
On 2014/10/02 15:32:48, danakj wrote:
> formatting

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container.h:172: // rend()
On 2014/10/02 15:32:48, danakj wrote:
> formattting

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
File cc/quads/list_container_unittest.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container_unittest.cc:539:
std::vector<SharedQuadState*>::const_iterator sqs_iter = sqs_list.begin();
On 2014/10/02 15:32:48, danakj wrote:
> drop teh vector iterator

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/list_container...
cc/quads/list_container_unittest.cc:543: EXPECT_EQ(*sqs_iter, &*iter);
On 2014/10/02 15:32:48, danakj wrote:
> you already test this in other tests, just test the new bits here

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:124: for (auto& quad : source->quad_list) {
On 2014/10/02 15:32:48, danakj wrote:
> can this be const&?

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:124: for (auto& quad : source->quad_list) {
On 2014/10/02 15:32:48, danakj wrote:
> can this be const&?

Done.

https://codereview.chromium.org/551013002/diff/260001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:130: DCHECK(quad.shared_quad_state == &*sqs_iter);
On 2014/10/02 15:32:48, danakj wrote:
> drop the sqs_iter and just use ElementAt since it's only for the DCHECK

Actually it is used to keep track of the original SQS, changing to use index
will introduce ElementAt at line 125 too.

https://codereview.chromium.org/551013002/diff/260001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/551013002/diff/260001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:294: DCHECK(sqs_iter !=
source_shared_quad_state_list.end());
On 2014/10/02 15:32:48, danakj wrote:
> sqs_iter is only used for dchecks, can you use ElementAt instead? i think
using
> the old size_t is a bit easier to read, or at least makes the diff easier. or
> maybe use the iterator index (which i guess would mean no fancy for loop,
> though)

It is used in the while loop check at line 292.

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:377:
DCHECK_EQ(static_cast<size_t>(next_sqs_iter.index()),
On 2014/10/02 15:32:48, danakj wrote:
> static cast needed?

Done.

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:377:
DCHECK_EQ(static_cast<size_t>(next_sqs_iter.index()),
On 2014/10/02 15:32:48, danakj wrote:
> static cast needed?

No longer needed.

[weiliangc, 2014-10-02 22:07:27]

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/in...
File mojo/services/public/interfaces/surfaces/quads.mojom (right):

https://codereview.chromium.org/551013002/diff/260001/mojo/services/public/in...
mojo/services/public/interfaces/surfaces/quads.mojom:126: uint32
shared_quad_state_index;
On 2014/10/02 15:32:49, danakj wrote:
> did you run bots for this? you're using a size_t elsewhere which isn't 32bits
> always. either this needs to be size_t or you need to case, I think (probly
the
> latter)

I don't think size_t is defined in mojom files, and not too sure how to approach
this.

Another alternative would be not use size_t for index()? (unsigned maybe?)

Let me ask jamesr@.

[jamesr, 2014-10-02 22:15:48]

size_t doesn't exist in mojom (or in any IPC definition type) since the type can
be different for processes on different sides of the channel.  If a process
running code compiled for 64 bit is talking to a process running code compiled
for 32 bit, they might disagree about how many bits a size_t contains.  mojom
has explicitly-sized types only.

[jamesr, 2014-10-02 22:17:41]

I think saying that this index is 32 bits is fine, since mojo does not support
sending arrays with more than 4b elements.  sizeof(size_t) >= sizeof(uint32_t)
so you should be fine converting.  To be safe, you can explicitly reject index
values >= UINT32_MAX when serializing a value. You should probably do this for
the IPC version as well.

[weiliangc, 2014-10-02 22:21:53]

On 2014/10/02 22:17:41, jamesr wrote:
> I think saying that this index is 32 bits is fine, since mojo does not support
> sending arrays with more than 4b elements.  sizeof(size_t) >= sizeof(uint32_t)
> so you should be fine converting.  To be safe, you can explicitly reject index
> values >= UINT32_MAX when serializing a value. You should probably do this for
> the IPC version as well.

What about compiler warning about possible loss of data when assigning size_t
index to uint32_t index?
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...

[weiliangc, 2014-10-02 22:23:16]

https://codereview.chromium.org/551013002/diff/280001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/280001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:374:
quads[iter.index()]->shared_quad_state_index = next_sqs_iter.index() - 1;
Compiler warning here for possible loss of data.

[jamesr, 2014-10-02 23:01:50]

On 2014/10/02 22:21:53, weiliangc wrote:
> On 2014/10/02 22:17:41, jamesr wrote:
> > I think saying that this index is 32 bits is fine, since mojo does not
support
> > sending arrays with more than 4b elements.  sizeof(size_t) >=
sizeof(uint32_t)
> > so you should be fine converting.  To be safe, you can explicitly reject
index
> > values >= UINT32_MAX when serializing a value. You should probably do this
for
> > the IPC version as well.
> 
> What about compiler warning about possible loss of data when assigning size_t
> index to uint32_t index?
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...

(1) check that the range is valid
(2) once you know the size_t will fit, static_cast<> it

[weiliangc, 2014-10-02 23:25:38]

All review comments are addressed. :D

[danakj, 2014-10-03 15:39:49]

LGTM % jamesr

https://codereview.chromium.org/551013002/diff/260001/content/common/cc_messa...
File content/common/cc_messages.cc (right):

https://codereview.chromium.org/551013002/diff/260001/content/common/cc_messa...
content/common/cc_messages.cc:719: const static size_t kMaxQuadListSize =
100000;
On 2014/10/02 15:32:48, danakj wrote:
> actually i wonder if this is too few for webview, can you *10 this one?

Missed this one?

https://codereview.chromium.org/551013002/diff/300001/cc/quads/list_container.h
File cc/quads/list_container.h (right):

https://codereview.chromium.org/551013002/diff/300001/cc/quads/list_container...
cc/quads/list_container.h:89: // is used to avoid doulble increment at places an
index reference is
"double"

https://codereview.chromium.org/551013002/diff/300001/cc/quads/list_container...
cc/quads/list_container.h:114: // is used to avoid doulble increment at places
an index reference is
"double"

https://codereview.chromium.org/551013002/diff/300001/cc/quads/list_container...
cc/quads/list_container.h:139: // is used to avoid doulble increment at places
an index reference is
"double"

https://codereview.chromium.org/551013002/diff/300001/cc/quads/list_container...
cc/quads/list_container.h:165: // is used to avoid doulble increment at places
an index reference is
"double"

https://codereview.chromium.org/551013002/diff/300001/mojo/services/public/cp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/551013002/diff/300001/mojo/services/public/cp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:374: if
(next_sqs_iter.index() - 1 <= UINT32_MAX) {
DCHECK this instead

[jamesr, 2014-10-03 17:35:50]

lgtm

[kenrb, 2014-10-03 19:58:50]

ipc lgtm

[weiliangc, 2014-10-03 21:07:16]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-10-03 21:08:59]

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

[commit-bot: I haz the power, 2014-10-03 22:53:24]

Committed patchset #16 (id:340001) as 195a8d6886226a66e2fa7e8fb1d1cd0ba5b93ab9

[commit-bot: I haz the power, 2014-10-03 22:54:04]

Patchset 16 (id:??) landed as
https://crrev.com/808f70fa59631bb0fc9d0e0fd7e5956d0f1c052a
Cr-Commit-Position: refs/heads/master@{#298107}