Update filter blending to match spec draft.

Update filter blending to match spec draft.

~ Stop blending filter operations which contain a Reference Filter.
~ Blend filter operations of different lengths is they share a common
  prefix.

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

[danakj, 2013-11-11 17:59:56]

driveby style nit

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h
File cc/output/filter_operations.h (right):

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h...
cc/output/filter_operations.h:71: 
remove extra whitespace

[ajuma, 2013-11-11 18:39:40]

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.cc
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.c...
cc/output/filter_operations.cc:177: return BlendRaggedOperations(from,
progress);
Could we merge the logic of blending different sized lists with that for
blending lists of the same size? (Since same-size lists can be viewed as just a
special case of lists whose sizes don't necessarily match.)

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h
File cc/output/filter_operations.h (right):

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h...
cc/output/filter_operations.h:67: // reference filter, are either of different
lengths without a common prefix
The condition is stronger than "common prefix" (since, e.g. {blur, drop shadow},
{blur, sepia} do have a common prefix, but we can't blend them). It might be
easier to re-write the entire comment than just re-writing the last sentence.
Here's an attempt at doing that:
"If |from| is of the same size as this, where in each position, the filter in
|from| is of the same type as the filter in this, and if this doesn't contain
any reference filters, returns a FilterOperations formed by linearly
interpolating at each position a |progress| fraction of the way from the filter
in |from| to the filter in this. If |from| and this are of different lengths,
they are treated as having the same length by padding the shorter sequence with
no-op filters of the same type as the filters in the corresponding positions in
the longer sequence. If either sequence has a reference filter or if there is a
type mismatch at some position, returns a copy of this." WDYT?

[avallee, 2013-11-12 20:18:08]

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.cc
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.c...
cc/output/filter_operations.cc:177: return BlendRaggedOperations(from,
progress);
On 2013/11/11 18:39:41, ajuma wrote:
> Could we merge the logic of blending different sized lists with that for
> blending lists of the same size? (Since same-size lists can be viewed as just
a
> special case of lists whose sizes don't necessarily match.)

Done.

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h
File cc/output/filter_operations.h (right):

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h...
cc/output/filter_operations.h:67: // reference filter, are either of different
lengths without a common prefix
On 2013/11/11 18:39:41, ajuma wrote:
> The condition is stronger than "common prefix" (since, e.g. {blur, drop
shadow},
> {blur, sepia} do have a common prefix, but we can't blend them). It might be
> easier to re-write the entire comment than just re-writing the last sentence.
> Here's an attempt at doing that:
> "If |from| is of the same size as this, where in each position, the filter in
> |from| is of the same type as the filter in this, and if this doesn't contain
> any reference filters, returns a FilterOperations formed by linearly
> interpolating at each position a |progress| fraction of the way from the
filter
> in |from| to the filter in this. If |from| and this are of different lengths,
> they are treated as having the same length by padding the shorter sequence
with
> no-op filters of the same type as the filters in the corresponding positions
in
> the longer sequence. If either sequence has a reference filter or if there is
a
> type mismatch at some position, returns a copy of this." WDYT?

SGTM

https://codereview.chromium.org/64123005/diff/1/cc/output/filter_operations.h...
cc/output/filter_operations.h:71: 
On 2013/11/11 17:59:57, danakj wrote:
> remove extra whitespace

Done.

[ajuma, 2013-11-12 20:30:14]

lgtm with a couple nits

https://codereview.chromium.org/64123005/diff/80001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/64123005/diff/80001/cc/output/filter_operatio...
cc/output/filter_operations.cc:5: #include <algorithm>
I think this can be removed (looks to be leftover from the previous patch set,
which used std::min).

https://codereview.chromium.org/64123005/diff/80001/cc/output/filter_operatio...
cc/output/filter_operations.cc:186: return blended_filters;
We don't really need this, since the for loops below correctly do nothing when
shorter_size == longer_size.

[avallee, 2013-11-12 21:31:24]

https://codereview.chromium.org/64123005/diff/80001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/64123005/diff/80001/cc/output/filter_operatio...
cc/output/filter_operations.cc:186: return blended_filters;
On 2013/11/12 20:30:14, ajuma wrote:
> We don't really need this, since the for loops below correctly do nothing when
> shorter_size == longer_size.

Good point. We'll go down else and then do nothing.

[commit-bot: I haz the power, 2013-11-12 21:37:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/130001

[ajuma, 2013-11-12 21:39:42]

https://codereview.chromium.org/64123005/diff/130001/cc/output/filter_operati...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/64123005/diff/130001/cc/output/filter_operati...
cc/output/filter_operations.cc:5: #include <algorithm>
Can this be removed?

[avallee, 2013-11-12 21:42:16]

https://codereview.chromium.org/64123005/diff/130001/cc/output/filter_operati...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/64123005/diff/130001/cc/output/filter_operati...
cc/output/filter_operations.cc:5: #include <algorithm>
On 2013/11/12 21:39:42, ajuma wrote:
> Can this be removed?

Sorry, missed that before. Done.

[commit-bot: I haz the power, 2013-11-12 21:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/330001

[commit-bot: I haz the power, 2013-11-12 22:44:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-13 16:38:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/550001

[commit-bot: I haz the power, 2013-11-13 17:11:10]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-14 03:39:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/560004

[commit-bot: I haz the power, 2013-11-14 04:29:34]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-15 01:43:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/980001

[commit-bot: I haz the power, 2013-11-15 04:00:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/980001

[commit-bot: I haz the power, 2013-11-15 07:08:28]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 15:31:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/980001

[commit-bot: I haz the power, 2013-11-15 22:01:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/980001

[commit-bot: I haz the power, 2013-11-16 00:29:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avallee@chromium.org/64123005/980001

[commit-bot: I haz the power, 2013-11-17 21:51:50]

Change committed as 235564