Try backing SkPicture with SkRecord in Chromium.

Try backing SkPicture with SkRecord in Chromium.

This is our first attempt, mostly to feel this out.  Performance and
correctness problems are both possible.  Please revert this if a benchmark even
smells funny.

That said, we've been working on this a while, are pretty confident it's good
stuff, and have had Skia's internal testing entirely switched over for a week.
Parts of Chromium that serialize and deserialize SkPictures are already
switched over.  This CL finishes things off by switching the default
recorded-from-scratch SkPicture to use SkRecord too.

BUG=408985, 409110, 409138
Committed: https://chromium.googlesource.com/chromium/src/+/f7450daaf38f4951b23fabaaaf659c6af33a705c

Committed: https://chromium.googlesource.com/chromium/src/+/5f2a6ab31313eb2fc6e456f65c3863a77d2d0c30

CQ_EXTRA_TRYBOTS=tryserver.blink:linux_blink_rel,linux_blink_dbg;tryserver.chromium.linux:linux_browser_asan

Committed: https://chromium.googlesource.com/chromium/src/+/130033d6c732af90ed117f05517e4efffd23ae58

Committed: https://crrev.com/ba819aea3f302d89100dc89632fd225a40e65411
Cr-Commit-Position: refs/heads/master@{#293503}

Committed: https://crrev.com/c3d2efb33238a3ee19cc8e21f4d91ef8c55f23c4
Cr-Commit-Position: refs/heads/master@{#294423}

[mtklein, 2014-08-25 19:07:37]

mtklein@google.com changed reviewers:
+ mtklein@google.com, reed@google.com

[Stephen White, 2014-08-25 19:15:23]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2014-08-25 19:15:23]

https://codereview.chromium.org/504823003/diff/1/skia/config/SkUserConfig.h
File skia/config/SkUserConfig.h (right):

https://codereview.chromium.org/504823003/diff/1/skia/config/SkUserConfig.h#n...
skia/config/SkUserConfig.h:25: #define SK_PICTURE_USE_SK_RECORD 1
Please put this below the "// ===== Begin Chrome-specific definitions ====="
line below, or in a .gypi instead.

[mtklein, 2014-08-25 19:18:50]

https://codereview.chromium.org/504823003/diff/1/skia/config/SkUserConfig.h
File skia/config/SkUserConfig.h (right):

https://codereview.chromium.org/504823003/diff/1/skia/config/SkUserConfig.h#n...
skia/config/SkUserConfig.h:25: #define SK_PICTURE_USE_SK_RECORD 1
On 2014/08/25 19:15:23, Stephen White wrote:
> Please put this below the "// ===== Begin Chrome-specific definitions ====="
> line below, or in a .gypi instead.

Done.

[Stephen White, 2014-08-25 19:23:43]

LGTM

[reed1, 2014-08-25 20:04:43]

The CQ bit was checked by reed@google.com

[reed1, 2014-08-25 20:04:44]

lgtm

[commit-bot: I haz the power, 2014-08-25 20:05:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/20001

[commit-bot: I haz the power, 2014-08-25 21:10:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-25 22:34:48]

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

[commit-bot: I haz the power, 2014-08-25 22:34:49]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Stephen White, 2014-08-26 04:33:44]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-08-26 04:35:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/20001

[commit-bot: I haz the power, 2014-08-26 05:32:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-26 06:26:01]

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

[commit-bot: I haz the power, 2014-08-26 06:26:02]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[mtklein, 2014-08-26 13:12:50]

+Tom

[mtklein, 2014-08-27 14:28:46]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-27 14:30:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/20001

[commit-bot: I haz the power, 2014-08-27 14:36:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-27 15:36:01]

Committed patchset #2 (id:20001) as f7450daaf38f4951b23fabaaaf659c6af33a705c

[eroman, 2014-08-27 18:19:57]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2014-08-27 18:19:57]

Could this be responsible for memory leaks?

http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...

[mtklein, 2014-08-27 18:23:11]

On 2014/08/27 18:19:57, eroman wrote:
> Could this be responsible for memory leaks?
> 
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...

Hmm.  Yes, definitely looks like it.  Will revert this and see if I can
reproduce and fix locally.

Is there a trybot that will reproduce this test for the next time I try to
submit?

[mtklein, 2014-08-27 18:23:46]

A revert of this CL (patchset #2) has been created in
https://codereview.chromium.org/513793003/ by mtklein@google.com.

The reason for reverting is: Appears to leak bitmaps..

[eroman, 2014-08-27 18:38:21]

On Wed, Aug 27, 2014 at 11:23 AM, <mtklein@google.com> wrote:

> On 2014/08/27 18:19:57, eroman wrote:
>
>> Could this be responsible for memory leaks?
>>
>
>
> http://build.chromium.org/p/chromium.memory/builders/
> Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%282%29/builds/2509
>
> Hmm.  Yes, definitely looks like it.  Will revert this and see if I can
> reproduce and fix locally.
>
> Is there a trybot that will reproduce this test for the next time I try to
> submit?
>

Regarding trybot, I don't know. I am told lsan tests only run on Linux, so
perhaps one of the linux_asan bots run them


>
> https://codereview.chromium.org/504823003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mtklein, 2014-08-27 21:54:23]

Looks like the revert fixed things.  I'm debugging locally now.  Thanks for the
heads up!

[mtklein, 2014-08-29 02:11:28]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-29 02:12:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/20001

[commit-bot: I haz the power, 2014-08-29 02:14:04]

Committed patchset #2 (id:20001) as 5f2a6ab31313eb2fc6e456f65c3863a77d2d0c30

[mlamouri (slow - plz ping), 2014-08-29 09:30:06]

A revert of this CL (patchset #2) has been created in
https://codereview.chromium.org/522623002/ by mlamouri@chromium.org.

The reason for reverting is: This is breaking the following Blink test:
inspector/layers/layer-canvas-log.html.

[mtklein, 2014-08-29 14:08:18]

mtklein@google.com changed reviewers:
+ fmalita@chromium.org

[mtklein, 2014-08-29 14:08:18]

Florin, can you see if I've got new the test suppression correct?

[f(malita), 2014-08-29 14:10:27]

lgtm

[mtklein, 2014-09-03 19:39:11]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-03 19:40:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/40001

[mtklein, 2014-09-03 19:47:59]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-03 19:49:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/60001

[mtklein, 2014-09-03 22:24:13]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2014-09-03 22:24:20]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-03 23:23:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/60001

[commit-bot: I haz the power, 2014-09-04 04:38:31]

Committed patchset #4 (id:60001) as 130033d6c732af90ed117f05517e4efffd23ae58

[lushnikov, 2014-09-04 14:27:38]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/544643002/ by lushnikov@chromium.org.

The reason for reverting is: This patch broke icons in Developer Tools:
crbug.com/410847.

[mtklein, 2014-09-05 12:03:53]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-05 12:04:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtklein@chromium.org/504823003/60001

[commit-bot: I haz the power, 2014-09-05 12:06:40]

Committed patchset #4 (id:60001) as 96730ee0419e528d8d3eac360e9319a871d20e43

[mtklein_C, 2014-09-05 16:58:27]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/544263002/ by mtklein@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=411330.

[commit-bot: I haz the power, 2014-09-10 02:51:51]

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

[commit-bot: I haz the power, 2014-09-10 03:05:23]

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

[commit-bot: I haz the power, 2014-09-10 03:29:47]

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

[commit-bot: I haz the power, 2014-09-10 03:38:55]

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

[mtklein, 2014-09-11 15:43:16]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-11 15:45:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/504823003/80001

[commit-bot: I haz the power, 2014-09-11 18:13:22]

Committed patchset #5 (id:80001) as 906798a8761ad6864e43105341745994acc4d38b

[commit-bot: I haz the power, 2014-09-11 18:43:14]

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