Add option for SKP capture without reencoding images

Add option for SKP capture without reencoding images

Generating SKPs with this new flag will allow us to obtain a
large set of images that is representative of what is common
on the web.

BUG=

Committed: https://crrev.com/397e7278792bc30195afebaf23afd6304cd772ff
Cr-Commit-Position: refs/heads/master@{#377026}

[msarett, 2016-02-17 20:47:30]

msarett@google.com changed reviewers:
+ scroggo@google.com

[msarett, 2016-02-17 20:47:31]

Let me know if you have any thoughts about the "illegal include" that I've
commented on.  Otherwise, there is no need to review, I'll need to try a new
approach in Patch Set 2.

https://codereview.chromium.org/1710553002/diff/1/content/renderer/gpu/gpu_be...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1710553002/diff/1/content/renderer/gpu/gpu_be...
content/renderer/gpu/gpu_benchmarking_extension.cc:18: // #include
"chrome/common/chrome_switches.h"
This seems to work (it builds), but the presubmit checks tell me that this is an
"illegal include".

[msarett, 2016-02-17 21:46:28]

msarett@google.com changed reviewers:
+ jbauman@chromium.org

[msarett, 2016-02-17 21:46:29]

Hi John,

I'm looking to modify how SKPs are captured using command line flags.  However,
it looks like uses of "chrome/common/chrome_switches.h" are illegal in this
section of the codebase.

Is there another utility used in src/content/renderer for command line switches?
 Or should I consider trying to solve the problem in another way?

Also please feel free to let me know if you're not the right person to ask about
this.

Thanks!
Matt

[jbauman, 2016-02-17 21:58:13]

I'd recommend putting the switch in content/public/common/content_switches.h
instead.

[msarett, 2016-02-17 22:21:03]

msarett@google.com changed reviewers:
+ rmistry@google.com

[msarett, 2016-02-17 22:21:03]

Thanks John!  That's exactly what I needed.

This is ready for review.  Adding Ravi for thoughts on generating a batch of
SKPs with this flag turned on.

[scroggo, 2016-02-17 23:13:30]

lgtm

https://codereview.chromium.org/1710553002/diff/20001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1710553002/diff/20001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:85: uint32_t width =
bm.width();
I think you can get all this info from the pixmap without ever calling
installPixels.

[rmistry, 2016-02-18 12:17:00]

On 2016/02/17 22:21:03, msarett wrote:
> Thanks John!  That's exactly what I needed.
> 
> This is ready for review.  Adding Ravi for thoughts on generating a batch of
> SKPs with this flag turned on.

Should be fine. It is easy for me to specify any flags to builds for SKP
captures.

[msarett, 2016-02-18 16:44:39]

https://codereview.chromium.org/1710553002/diff/20001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1710553002/diff/20001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:85: uint32_t width =
bm.width();
On 2016/02/17 23:13:30, scroggo wrote:
> I think you can get all this info from the pixmap without ever calling
> installPixels.

Restructured the code to check the flag before installing pixels.

[scroggo, 2016-02-18 17:31:51]

lgtm patch set 3

[msarett, 2016-02-18 17:33:02]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-02-18 17:37:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710553002/40001

[commit-bot: I haz the power, 2016-02-18 17:37:06]

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

[commit-bot: I haz the power, 2016-02-18 17:37:07]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rmistry1, 2016-02-18 17:38:12]

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

[rmistry1, 2016-02-18 17:38:13]

lgtm

[msarett, 2016-02-18 17:40:34]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-02-18 17:43:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710553002/40001

[commit-bot: I haz the power, 2016-02-18 17:56:02]

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

[commit-bot: I haz the power, 2016-02-18 17:56:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[msarett, 2016-02-18 18:00:19]

msarett@google.com changed reviewers:
+ nick@semenkovich.com

[msarett, 2016-02-18 18:00:19]

Adding OWNERs

[msarett, 2016-02-19 21:41:57]

msarett@google.com changed reviewers:
+ palmer@chromium.org
- nick@semenkovich.com

[msarett, 2016-02-19 21:41:58]

PTAL

[msarett, 2016-02-22 21:00:23]

John and Chris,

Can you please let me know if you are the right owners to review this change? 
Thanks a lot!

[palmer, 2016-02-22 23:12:24]

lgtm

https://codereview.chromium.org/1710553002/diff/40001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1710553002/diff/40001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:64: std::vector<unsigned
char> vector;
Nit: uint8_t more clearly means "byte" than does unsigned char.

[jbauman, 2016-02-22 23:14:58]

content/renderer/gpu OWNERS lgtm

[msarett, 2016-02-22 23:19:10]

https://codereview.chromium.org/1710553002/diff/40001/content/renderer/gpu/gp...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/1710553002/diff/40001/content/renderer/gpu/gp...
content/renderer/gpu/gpu_benchmarking_extension.cc:64: std::vector<unsigned
char> vector;
On 2016/02/22 23:12:24, palmer wrote:
> Nit: uint8_t more clearly means "byte" than does unsigned char.

Done :)

[msarett, 2016-02-23 13:37:32]

The CQ bit was checked by msarett@google.com

[msarett, 2016-02-23 13:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
scroggo@google.com, jbauman@chromium.org, rmistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1710553002/#ps60001
(title: "Use uint8_t")

[commit-bot: I haz the power, 2016-02-23 13:37:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710553002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710553002/60001

[commit-bot: I haz the power, 2016-02-23 13:46:53]

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

[commit-bot: I haz the power, 2016-02-23 13:46:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[msarett, 2016-02-23 14:01:41]

msarett@google.com changed reviewers:
+ davidben@chromium.org

[msarett, 2016-02-23 14:01:42]

Hi David,

Can you please take a look?  I believe I need an OWNERS review for
content/public/common/content_switches.*

[davidben, 2016-02-23 17:34:40]

content/public lgtm. This does violate the "content/public shouldn't contain
stuff not used by embedders" rule, but I see we've got plenty other of these for
switches and there isn't another file in content with switches definitions, so I
think this is right?

[msarett, 2016-02-23 17:37:58]

On 2016/02/23 17:34:40, davidben wrote:
> content/public lgtm. This does violate the "content/public shouldn't contain
> stuff not used by embedders" rule, but I see we've got plenty other of these
for
> switches and there isn't another file in content with switches definitions, so
I
> think this is right?

SGTM thanks!  I would defer to your opinion on this.

[msarett, 2016-02-23 17:38:06]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-02-23 17:38:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710553002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710553002/60001

[commit-bot: I haz the power, 2016-02-23 18:49:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-23 18:51:07]

Description was changed from

==========
Add option for SKP capture without reencoding images

Generating SKPs with this new flag will allow us to obtain a
large set of images that is representative of what is common
on the web.

BUG=
==========

to

==========
Add option for SKP capture without reencoding images

Generating SKPs with this new flag will allow us to obtain a
large set of images that is representative of what is common
on the web.

BUG=

Committed: https://crrev.com/397e7278792bc30195afebaf23afd6304cd772ff
Cr-Commit-Position: refs/heads/master@{#377026}
==========

[commit-bot: I haz the power, 2016-02-23 18:51:08]

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