Added Tab.SnapshotContent and MapsGL example to telemetry

Added Tab.SnapshotContent and MapsGL example to telemetry

This CL is dependent on an upcoming feature in the gpuBenchmarking extension,
and will not work correctly until that is implemented.

This is a subset of the prior CL 11362023, which has been broken apart to facilitate easier reviews 


BUG=157479
TBR=nduca@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167549

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168518

[nduca, 2012-11-09 19:52:51]

Unit tests

[Ken Russell (switch to Gerrit), 2012-11-09 20:26:27]

LGTM

If this CL depends on another one then clearly that one needs to land first so
non-functional APIs aren't checked in.

Regarding Nat's comment on unit tests; I think it's most important here to add a
snapshot_unittest.py. Later we can add tests of Tab.SnapshotContent which e.g.
navigate to a green page, take a snapshot, and make sure the snapshot was green.

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/examples/gpu_...
File tools/telemetry/examples/gpu_mapsgl_endurance_browsertest.py (right):

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/examples/gpu_...
tools/telemetry/examples/gpu_mapsgl_endurance_browsertest.py:36: #TODO:
BringBrowserWindowToFront?
TODOs require a contact person.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/examples/gpu_...
tools/telemetry/examples/gpu_mapsgl_endurance_browsertest.py:53: # Mock up the
message queue functionality used by the mapping test
Please use periods after comments. It makes them easier to read.

[nduca, 2012-11-09 21:19:00]

Some more details

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
File tools/telemetry/telemetry/snapshot.py (right):

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:9: import png # pylint: disable=F0401
this is a bit surprising, I dont like it. Is there a way to make this append
less global, e.g. third_party/png? you have third_party/png/png.py, right?

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:10: 
please also guard against this running twice, e.g. figure out the dir you plan
to append to sys.path. If its in the path, dont append it.

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:11: class Snapshot(object):
Why call this snapshot and not, say, PngBitmap?

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:11: class Snapshot(object):
This class should be unit tested. Put a png in unittest_data, and have unit
tests that sanity check it

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:33: def GetPixel(self, x, y):
GetPixelColor

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:37: return [row[offset], row[offset + 1],
row[offset + 2]]
make a color class

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:40: """Verifies that the pixel at (x, y)
is the expected color"""
This is a method on the color class.

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/sna...
tools/telemetry/telemetry/snapshot.py:48: f = open(path, "wb")
prefer this style
  with open(..) as f:
     f.write(..)

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/tab.py
File tools/telemetry/telemetry/tab.py (right):

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/tab...
tools/telemetry/telemetry/tab.py:79: 
This should be a feature on inspector_page.py --- the likely path for this in
the future is page.screenshot. So, call it screnshot and put it on
inspector_page.

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/tab...
tools/telemetry/telemetry/tab.py:80: def SnapshotContent(self,
timeout=DEFAULT_TAB_TIMEOUT):
So, what if someone calls this and the gpuBenchmarking extension isn't enabled?

if gpubenchmarkingextnesion not in this.browser.browserOptions:
  raise Exception("Browser was not started with --enable-blahblah")

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/tab...
tools/telemetry/telemetry/tab.py:80: def SnapshotContent(self,
timeout=DEFAULT_TAB_TIMEOUT):
"content"? That would imply we're taking a picture of the invisible parts too.
Screenshot might be good. Or whatever the word is that the inspector bug about
this says.

https://codereview.chromium.org/11360172/diff/1/tools/telemetry/telemetry/tab...
tools/telemetry/telemetry/tab.py:81: self._runtime.Evaluate("""
This function should have """Help text""" that explains what it does and what it
returns.

[nduca, 2012-11-12 19:53:26]

Looks really really good! One final round of nits and I'll rubber stamp you. :)

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/inspector_page.py (right):

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/inspector_page.py:103:
window.chrome.gpuBenchmarking.snapshotData = snapshot;
would probably bebetter if you stash this outside the gpuBenchmarking object,
e.g. window.__telemetry.blahblahblah

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/inspector_page.py:116: if snap:
should you delette the snapshotComplete and snapshotData after you retrieve it?

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/inspector_page_unittest.py (right):

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/inspector_page_unittest.py:28: unittest_data_dir =
os.path.join(os.path.dirname(__file__),
What happens if we run this on an old browser? E.g. ./run_tests --browser=system
...

The test is gonna fail. :(

You'll want to have this test early return if the screenshot feature isn't
supported.

Which might mean you should have a IsScreenshotSupported() method on page.

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/png_bitmap.py (right):

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/png_bitmap.py:33: def CheckRGB(self, r, g, b,
tolerance=0):
These method names really should be named better. IsEqual maybe,
IsApproximatelyEqual.

And no CheckRGB / CheckRGBA ... just make methods that create color objects.

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/tab.py (right):

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/tab.py:26: self._runtime =
inspector_runtime.InspectorRuntime(self._inspector_backend,
line wrap at the ( in these cases...

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/tab_test_case.py (right):

https://codereview.chromium.org/11360172/diff/1007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/tab_test_case.py:14:
options.extra_browser_args.append('--enable-gpu-benchmarking')
dont love this.... can you make the specific screenshot test do this instead of
all tests?

[nduca, 2012-11-13 22:28:36]

lgtm

[commit-bot: I haz the power, 2012-11-13 22:31:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11360172/7002

[commit-bot: I haz the power, 2012-11-14 01:11:18]

Change committed as 167549

[dtu, 2012-11-14 03:04:40]

https://chromiumcodereview.appspot.com/11360172/diff/7002/tools/telemetry/tel...
File tools/telemetry/telemetry/tab_test_case.py (right):

https://chromiumcodereview.appspot.com/11360172/diff/7002/tools/telemetry/tel...
tools/telemetry/telemetry/tab_test_case.py:15: if self._extra_browser_args:
I'm getting run_tests failures in inspector_runtime_unittest:
AttributeError: 'InspectorRuntimeTest' object has no attribute
'_extra_browser_args'

[nduca, 2012-11-14 04:38:11]

Feel free to revert if the fix isn't obvious.

[marja, 2012-11-15 10:19:59]

https://chromiumcodereview.appspot.com/11360172/diff/13001/tools/telemetry/te...
File tools/telemetry/telemetry/tab_test_case.py (right):

https://chromiumcodereview.appspot.com/11360172/diff/13001/tools/telemetry/te...
tools/telemetry/telemetry/tab_test_case.py:12: super(TabTestCase,
self).__init__(*args)
Uhh, something has gone wrong, the committed version of this CL doesn't contain
the addition of this __init__, and this breaks...

(Tests fail because setUp tries to access self._extra_browser_args which doesn't
exist.)

Is the committed version something else than the latest patch set?

Since nduca suggests reverting, I'll revert this CL.

[commit-bot: I haz the power, 2012-11-17 20:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11360172/13001

[commit-bot: I haz the power, 2012-11-17 20:54:13]

Failed to apply patch for tools/telemetry/telemetry/tab.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/telemetry/telemetry/tab.py
  Hunk #1 FAILED at 22.
  1 out of 1 hunk FAILED -- saving rejects to file
tools/telemetry/telemetry/tab.py.rej

Patch:       tools/telemetry/telemetry/tab.py
Index: tools/telemetry/telemetry/tab.py
diff --git a/tools/telemetry/telemetry/tab.py b/tools/telemetry/telemetry/tab.py
index
527a2e00fb94aea172a4d30a64c7a9106a2856e4..28292535e33ac997a051da4d496b43b909486de7
100644
--- a/tools/telemetry/telemetry/tab.py
+++ b/tools/telemetry/telemetry/tab.py
@@ -22,9 +22,11 @@ class Tab(object):
   def __init__(self, browser, inspector_backend):
     self._browser = browser
     self._inspector_backend = inspector_backend
-    self._page = inspector_page.InspectorPage(self._inspector_backend)
-    self._runtime = inspector_runtime.InspectorRuntime(self._inspector_backend)
-    self._console = inspector_console.InspectorConsole(self._inspector_backend)
+    self._page = inspector_page.InspectorPage(self._inspector_backend, self)
+    self._runtime = inspector_runtime.InspectorRuntime(
+        self._inspector_backend, self)
+    self._console = inspector_console.InspectorConsole(
+        self._inspector_backend, self)
 
   def __del__(self):
     self.Close()

[commit-bot: I haz the power, 2012-11-18 18:49:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11360172/17001

[commit-bot: I haz the power, 2012-11-18 18:55:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-19 13:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/11360172/17001

[commit-bot: I haz the power, 2012-11-19 13:40:13]

Change committed as 168518