Modifications to skpicture_printer

Modifications to skpicture_printer

* Changed the output directory option to '-s' and '--skp-outdir' because this was broken, running run_multipage_benchmarks with skpicture_printer resulted in the following error: 
optparse.OptionConflictError: option -o/--outdir: conflicting option string(s): -o 

* Added additional browser options due on recent changes in Skia/Chromium code base.

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

[nduca, 2013-05-28 18:50:24]

https://codereview.chromium.org/15665003/diff/1/perf_tools/skpicture_printer.py
File perf_tools/skpicture_printer.py (left):

https://codereview.chromium.org/15665003/diff/1/perf_tools/skpicture_printer....
perf_tools/skpicture_printer.py:19: if self.options.outdir is not None:
why'd you remove the is not none handling?

[rmistry, 2013-05-28 18:55:11]

https://codereview.chromium.org/15665003/diff/1/perf_tools/skpicture_printer.py
File perf_tools/skpicture_printer.py (left):

https://codereview.chromium.org/15665003/diff/1/perf_tools/skpicture_printer....
perf_tools/skpicture_printer.py:19: if self.options.outdir is not None:
On 2013/05/28 18:50:25, nduca wrote:
> why'd you remove the is not none handling?

If outdir was not specified (None), the next line was failing because it was
referencing outpath before it was assigned:
outpath = os.path.abspath(outpath)

Maybe I should throw an Exception if skp_outdir is not specified, though I
expected line 12 to do that for me.

[nduca, 2013-05-28 19:10:20]

Make this recover gracefully when its not provided. Right now, it'll explode,
I'm pretty sure. look at optparse reference to learn more --- optopins are
optional in optparse.

[rmistry, 2013-05-28 19:47:23]

On 2013/05/28 19:10:20, nduca wrote:
> Make this recover gracefully when its not provided. Right now, it'll explode,
> I'm pretty sure. look at optparse reference to learn more --- optopins are
> optional in optparse.

Using tempfile.gettempdir() if skp_outdir is not specified. What do you think?

[dtu, 2013-05-28 20:06:09]

On 2013/05/28 19:47:23, rmistry wrote:
> On 2013/05/28 19:10:20, nduca wrote:
> > Make this recover gracefully when its not provided. Right now, it'll
explode,
> > I'm pretty sure. look at optparse reference to learn more --- optopins are
> > optional in optparse.
> 
> Using tempfile.gettempdir() if skp_outdir is not specified. What do you think?

That's fine, but log the directory to let the user know where it is.

[dtu, 2013-05-28 20:06:16]

https://codereview.chromium.org/15665003/diff/6001/perf_tools/skpicture_print...
File perf_tools/skpicture_printer.py (right):

https://codereview.chromium.org/15665003/diff/6001/perf_tools/skpicture_print...
perf_tools/skpicture_printer.py:14: parser.add_option('-s', '--skp_outdir',
--skip-outdir, with a hypen

[rmistry, 2013-05-28 21:00:36]

On 2013/05/28 20:06:09, Dave Tu wrote:
> On 2013/05/28 19:47:23, rmistry wrote:
> > On 2013/05/28 19:10:20, nduca wrote:
> > > Make this recover gracefully when its not provided. Right now, it'll
> explode,
> > > I'm pretty sure. look at optparse reference to learn more --- optopins are
> > > optional in optparse.
> > 
> > Using tempfile.gettempdir() if skp_outdir is not specified. What do you
think?
> 
> That's fine, but log the directory to let the user know where it is.

Done.

[rmistry, 2013-05-28 21:00:42]

https://codereview.chromium.org/15665003/diff/6001/perf_tools/skpicture_print...
File perf_tools/skpicture_printer.py (right):

https://codereview.chromium.org/15665003/diff/6001/perf_tools/skpicture_print...
perf_tools/skpicture_printer.py:14: parser.add_option('-s', '--skp_outdir',
On 2013/05/28 20:06:16, Dave Tu wrote:
> --skip-outdir, with a hypen

Done.

[dtu, 2013-05-28 21:35:54]

Other than the comment, lgtm

https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
File perf_tools/skpicture_printer.py (right):

https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
perf_tools/skpicture_printer.py:24: skp_outdir = self.options.skp-outdir
Sorry, the variable name is still skp_outdir, the parser does some name mangling
magic.

[rmistry, 2013-05-28 21:41:32]

https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
File perf_tools/skpicture_printer.py (right):

https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
perf_tools/skpicture_printer.py:24: skp_outdir = self.options.skp-outdir
On 2013/05/28 21:35:55, Dave Tu wrote:
> Sorry, the variable name is still skp_outdir, the parser does some name
mangling
> magic.

I should change it back to skp_outdir then?

[dtu, 2013-05-28 21:43:25]

On 2013/05/28 21:41:32, rmistry wrote:
>
https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
> File perf_tools/skpicture_printer.py (right):
> 
>
https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
> perf_tools/skpicture_printer.py:24: skp_outdir = self.options.skp-outdir
> On 2013/05/28 21:35:55, Dave Tu wrote:
> > Sorry, the variable name is still skp_outdir, the parser does some name
> mangling
> > magic.
> 
> I should change it back to skp_outdir then?

No, the option name is '--skp-outdir', but the variable name is 'skp_outdir'.
The optparse parser converts it automatically, since options usually use
hyphens, but Python variable names disallow hyphens.

[rmistry, 2013-05-28 21:50:53]

On 2013/05/28 21:43:25, Dave Tu wrote:
> On 2013/05/28 21:41:32, rmistry wrote:
> >
>
https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
> > File perf_tools/skpicture_printer.py (right):
> > 
> >
>
https://codereview.chromium.org/15665003/diff/10002/perf_tools/skpicture_prin...
> > perf_tools/skpicture_printer.py:24: skp_outdir = self.options.skp-outdir
> > On 2013/05/28 21:35:55, Dave Tu wrote:
> > > Sorry, the variable name is still skp_outdir, the parser does some name
> > mangling
> > > magic.
> > 
> > I should change it back to skp_outdir then?
> 
> No, the option name is '--skp-outdir', but the variable name is 'skp_outdir'.
> The optparse parser converts it automatically, since options usually use
> hyphens, but Python variable names disallow hyphens.

Ah right, sorry for the confusion. Done.

[commit-bot: I haz the power, 2013-05-29 11:57:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmistry@google.com/15665003/19001

[commit-bot: I haz the power, 2013-05-29 12:21:08]

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

[rmistry, 2013-05-29 12:55:03]

The CQ seems to be failing while applying the patch, Nat or Dave could one of
you please submit this for me?

[nduca, 2013-05-29 13:36:43]

just re-cq.

i'd prefer that it fail if no outdir is provided.

[rmistry, 2013-05-29 13:51:46]

On 2013/05/29 13:36:43, nduca wrote:
> just re-cq.
> 
> i'd prefer that it fail if no outdir is provided.

Changed it to fail if no outdir is provided.

I spoke to M-A about CQ failing, it is because my base url in this CL is not
http://src.chromium.org/svn/trunk/src/, I created a new checkout, and it should
hopefully work now, but I will wait for your LGTM first.

[nduca, 2013-05-29 13:58:56]

lgtm definitely

[commit-bot: I haz the power, 2013-05-29 14:05:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmistry@google.com/15665003/27001

[commit-bot: I haz the power, 2013-05-29 14:05:43]

Failed to apply patch for tools/perf/tools/perf/perf_tools/skpicture_printer.py:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  A         tools/perf/tools
  Created missing directory tools/perf/tools.
  A         tools/perf/tools/perf
  Created missing directory tools/perf/tools/perf.
  A         tools/perf/tools/perf/perf_tools
  Created missing directory tools/perf/tools/perf/perf_tools.
  can't find file to patch at input line 5
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: tools/perf/perf_tools/skpicture_printer.py
  |===================================================================
  |--- tools/perf/tools/perf/perf_tools/skpicture_printer.py	(revision 199247)
  |+++ tools/perf/tools/perf/perf_tools/skpicture_printer.py	(working copy)
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       tools/perf/tools/perf/perf_tools/skpicture_printer.py
Index: tools/perf/perf_tools/skpicture_printer.py
===================================================================
--- tools/perf/tools/perf/perf_tools/skpicture_printer.py	(revision 199247)
+++ tools/perf/tools/perf/perf_tools/skpicture_printer.py	(working copy)
@@ -9,16 +9,22 @@
 
 class SkPicturePrinter(page_measurement.PageMeasurement):
   def AddCommandLineOptions(self, parser):
-    parser.add_option('-o', '--outdir', help='Output directory')
+    parser.add_option('-s', '--skp-outdir',
+                      help='Output directory for the SKP files')
 
   def CustomizeBrowserOptions(self, options):
     options.extra_browser_args.extend(['--enable-gpu-benchmarking',
-                                       '--no-sandbox'])
+                                       '--no-sandbox',
+                                       '--enable-deferred-image-decoding',
+                                       '--force-compositing-mode'])
 
   def MeasurePage(self, page, tab, results):
-    if self.options.outdir is not None:
-      outpath = os.path.join(self.options.outdir, page.url_as_file_safe_name)
-    outpath = os.path.abspath(outpath)
+    skp_outdir = self.options.skp_outdir
+    if not skp_outdir:
+      raise Exception('Please specify --skp-outdir')
+    outpath = os.path.abspath(
+        os.path.join(skp_outdir,
+                     page.url_as_file_safe_name))
     # Replace win32 path separator char '\' with '\\'.
     js = _JS.format(outpath.replace('\\', '\\\\'))
     tab.EvaluateJavaScript(js)

[commit-bot: I haz the power, 2013-05-29 14:09:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmistry@google.com/15665003/27001

[commit-bot: I haz the power, 2013-05-29 17:10:04]

Change committed as 202895