The screen capture code in chrome space is still not using the new testing/marionette/capture.js module. There is not reason it cannot share the same screenshot code as with content, but likely a few modifications will need to happen. I suspect this has caused issues such as bug 1140542.

Tests for the try build are looking fine, except that we have some strange element sizes on Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9fc451512f&selectedJob=32380579 A follow-up patch for the tests will fix that by converting them to int.

Comment on attachment 8817403 [details] Bug 1243415 - Capture methods should use the window as parameter. https://reviewboard.mozilla.org/r/97676/#review98076

Comment on attachment 8817405 [details] Bug 1243415 - Remove B2G related code from screenshot methods. https://reviewboard.mozilla.org/r/97678/#review98078

Comment on attachment 8817407 [details] Bug 1243415 - Add highlighting of elements for screenshots in chrome scope. https://reviewboard.mozilla.org/r/97682/#review98080

Comment on attachment 8817408 [details] Bug 1243415 - Add hash format support for screenshots in chrome scope. https://reviewboard.mozilla.org/r/97684/#review98082

Comment on attachment 8817409 [details] Bug 1243415 - Add support to take full screenshots in chrome scope. https://reviewboard.mozilla.org/r/97686/#review98084 ::: testing/marionette/driver.js:2409 (Diff revision 1) > let el = this.curBrowser.seenEls.get(h, container); > highlightEls.push(el); > } > > - let canvas = capture.viewport(this.getCurrentWindow(), highlightEls); > + // viewport > + if (!id && !full) { Can the chrome code be larger than the viewport?

Comment on attachment 8817406 [details] Bug 1243415 - Make use of capture.js in chrome scope. https://reviewboard.mozilla.org/r/97680/#review98086 ::: testing/marionette/capture.js:94 (Diff revision 1) > * @return {HTMLCanvasElement} > * The canvas on which the selection from the window's framebuffer > * has been painted on. > */ > capture.canvas = function(win, left, top, width, height, highlights=[]) { > + let scale = win.devicePixelRatio; Why have we added the scaling code? Just curious for my own learning

Comment on attachment 8817410 [details] Bug 1243415 - Check for a valid DOMWindow for screenshot in chrome scope. https://reviewboard.mozilla.org/r/97688/#review98088

Comment on attachment 8817406 [details] Bug 1243415 - Make use of capture.js in chrome scope. https://reviewboard.mozilla.org/r/97680/#review98086 > Why have we added the scaling code? Just curious for my own learning This is probably needed because of pixel scaling on retina displays. I’ve seen similar code in devtools.

Comment on attachment 8817409 [details] Bug 1243415 - Add support to take full screenshots in chrome scope. https://reviewboard.mozilla.org/r/97686/#review98084 > Can the chrome code be larger than the viewport? Yes, I had to try myself for a while to figure that out. So when you load a HTML test page into a chrome window - something a new test will do - the scrollbars will appear, if the window is opened too small. With pure XUL it might have also been possible somehow but I gave up for now.

Comment on attachment 8817406 [details] Bug 1243415 - Make use of capture.js in chrome scope. https://reviewboard.mozilla.org/r/97680/#review98086 > This is probably needed because of pixel scaling on retina displays. I’ve seen similar code in devtools. Correct. A good overview about how it works you can find eg. here: http://www.quirksmode.org/blog/archives/2012/06/devicepixelrati.html Generally it's that we do not want to get oversized images created if multiple device pixels map to a single pixel on screen.

I noticed that my changes to the tests even for content has increased the test failures for Fennec a lot. I will try to figure out why that is happening. Most likely this is due to the devicePixelRatio, because failures look like: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9fc451512f&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=32380259 > Tuples differ: (300, 462) != (600, 924) Which would mean there is a ratio of 2 used for Android emulator. Also while looking through code on DXR I noticed that a lot is using the drawing flags like caret, widget layer and such we we had in before.

By adding the drawing flags I was able to catch a crash of Firefox for our tests which is tracked as bug 1009762. Here the try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7758e3f58733&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=32456548&bugfiler For now I will disable using 'ctx.DRAWWINDOW_USE_WIDGET_LAYERS'. We can re-enable with the fix of bug 1009762 or once the patch for it landed.

David, beside the remaining review for the tests, I would need a follow-up review for the following changes: https://reviewboard.mozilla.org/r/97680/diff/1-3/ Thanks.

The last try build shows two failures for content tests which already existed before and which are about a single line dimension difference for screenshots. I would suggest that we get that fixed in a follow-up bug. Otherwise I wonder if we should better make use of the hash in most cases of this test. It has two benefits... we do not have to transfer that much data to the client, and on the other side make the tracing log smaller which is enabled for all builds on Treeherder. Before I do the change, I would like to know what others think. The downside will be that if a failure occurred you cannot simply copy and paste the data and open as data url.

Redirecting last question to Andreas given that David will not be around the next days and it would be good to get this patch set landed before Christmas.

Comment on attachment 8817411 [details] Bug 1243415 - Improve screenshot unit tests and add tests for chrome scope. https://reviewboard.mozilla.org/r/97690/#review98922 ::: testing/marionette/chrome/test_dialog.xul:9 (Diff revision 6) > -<dialog id="dialogTest" > - buttons="accept, cancel" > - xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > > > - <vbox id="things"> > +<dialog id="testDialog" Trusting you that these changes are correct. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:33 (Diff revision 6) > -class ScreenCaptureTestCase(MarionetteTestCase): > - def assert_png(self, string): > - """Test that string is a Base64 encoded PNG file.""" > - image = base64.decodestring(string) > +class ScreenshotTestCase(MarionetteTestCase): > + > + def setUp(self): > + super(ScreenshotTestCase, self).setUp() The original name was intended. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:38 (Diff revision 6) > + self._device_pixel_ratio = None > + > + @property > + def device_pixel_ratio(self): > + if self._device_pixel_ratio is None: > + self._device_pixel_ratio = self.marionette.execute_script(""" > + return window.devicePixelRatio > + """) > + return self._device_pixel_ratio Not opening an issue on this, but since we don’t memoise the document element or the Y offset of the window, it’s inconsistent to memoise this. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:54 (Diff revision 6) > + value = self.marionette.execute_script("return window.pageYOffset") > + return value Just return the value. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:62 (Diff revision 6) > + @property > + def viewport_dimensions(self): > + return self.marionette.execute_script(""" > + return [arguments[0].clientWidth, > + arguments[0].clientHeight]; > + """, script_args=[self.document_element]) Even though I’ve coded it so you can use a list for `script_args`, it’s more Pythonic to use a tuple. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:70 (Diff revision 6) > + """Test that screenshot is a Base64 encoded PNG file.""" > + image = base64.decodestring(screenshot) > self.assertEqual(imghdr.what("", image), "png") > > - def get_image_dimensions(self, string): > - self.assert_png(string) > + def assert_formats(self, element=None): > + element = element or self.document_element Use `if element is not None` here to avoid a falsiness check. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:96 (Diff revision 6) > + self.assertEqual(screenshot_image, screenshot_default) > + self.assertEqual(binary1, binary2) > + self.assertEqual(hash1, hash2) > + > + def get_element_dimensions(self, element): > + return [element.rect["width"], element.rect["height"]] Return tuple and reuse `element.rect` so that it does not have to be requested twice. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:102 (Diff revision 6) > + > + def get_image_dimensions(self, screenshot): > + self.assert_png(screenshot) > + image = base64.decodestring(screenshot) > width, height = struct.unpack(">LL", image[16:24]) > - return int(width), int(height) > + return [int(width), int(height)] Return tuple, as it did before. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:105 (Diff revision 6) > + return [int(rect[0] * self.device_pixel_ratio), > + int(rect[1] * self.device_pixel_ratio)] Return tuple. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:109 (Diff revision 6) > + def scale(self, rect): > + return [int(rect[0] * self.device_pixel_ratio), > + int(rect[1] * self.device_pixel_ratio)] > + > > +class TestScreenshotChrome(WindowManagerMixin, ScreenshotTestCase): Keep old name. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:113 (Diff revision 6) > - current_window = self.marionette.current_chrome_window_handle > + return self.marionette.execute_script(""" > - self.marionette.switch_to_window(self.start_window) > - with self.marionette.using_context("chrome"): Return this as a tuple. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:119 (Diff revision 6) > def setUp(self): > - super(TestScreenCaptureChrome, self).setUp() > + super(TestScreenshotChrome, self).setUp() > self.marionette.set_context("chrome") > > def tearDown(self): > self.close_all_windows() > - super(TestScreenCaptureChrome, self).tearDown() > + super(TestScreenshotChrome, self).tearDown() Keep old name. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:140 (Diff revision 6) > - # <window> element, which does not exist for all windows. > - def test_secondary_windows(self): > - def open_window_with_js(): > self.marionette.execute_script(""" > - window.open('chrome://marionette/content/test_dialog.xul', 'foo', > - 'dialog,height=200,width=300'); > + window.open(arguments[0], "", arguments[1]); > + """, script_args=[url, features]) Tuple again. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:175 (Diff revision 6) > + screenshot_focus = self.marionette.screenshot() > > -class Content(ScreenCaptureTestCase): > - @property > + self.marionette.execute_script("arguments[0].blur();", script_args=[textbox]) > + screenshot_no_focus = self.marionette.screenshot() > - def body_scroll_dimensions(self): > - return tuple(self.marionette.execute_script( > - "return [document.body.scrollWidth, document.body.scrollHeight]")) > > - @property > - def viewport_dimensions(self): > + self.marionette.close_chrome_window() > + self.marionette.switch_to_window(self.start_window) > - return tuple(self.marionette.execute_script(""" > - let docEl = document.documentElement; > - return [docEl.clientWidth, docEl.clientHeight];""")) > > - @property > + self.assertNotEqual(screenshot_focus, screenshot_no_focus) Good test. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:279 (Diff revision 6) > +class TestScreenshotContent(WindowManagerMixin, ScreenshotTestCase): > > def setUp(self): > - ScreenCaptureTestCase.setUp(self) > + super(TestScreenshotContent, self).setUp() > self.marionette.set_context("content") > > - def test_html_document_element(self): > + def tearDown(self): > + self.close_all_tabs() > + super(TestScreenshotContent, self).tearDown() Keep old name. ::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:291 (Diff revision 6) > + return self.marionette.execute_script(""" > + return [document.body.scrollWidth, document.body.scrollHeight] > + """) Return as a tuple.

Comment on attachment 8817411 [details] Bug 1243415 - Improve screenshot unit tests and add tests for chrome scope. https://reviewboard.mozilla.org/r/97690/#review98928 This is great work, but I miss more verbose commit messages. I think it’s worth pointing out in particular that we now support device pixel ratios for screenshots. If you could spare some time to expand each commit message, I would be very grateful.

Comment on attachment 8817407 [details] Bug 1243415 - Add highlighting of elements for screenshots in chrome scope. https://reviewboard.mozilla.org/r/97682/#review98930 ::: testing/marionette/driver.js:2397 (Diff revision 4) > - resp.body.value = capture.toBase64(canvas); > - break; > + let highlightEls = []; > + > + for (let h of highlights) { > + let el = this.curBrowser.seenEls.get(h, container); > + highlightEls.push(el); > + } You could replace all of this with: ```js let highlightEls = highlights.map(h => this.curBrowser.seenEls(h, container)); ``` Not making this an issue because I tend to like seeing the performance penalties of code, which you get by writing out loops.

(In reply to Henrik Skupin (:whimboo) from comment #37) > The last try build shows two failures for content tests which already > existed before and which are about a single line dimension difference for > screenshots. I would suggest that we get that fixed in a follow-up bug. Noted. I think these are about using the bounding client rects instead of the client rects for calculating the area to capture. > Otherwise I wonder if we should better make use of the hash in most cases of > this test. It has two benefits... we do not have to transfer that much data > to the client, and on the other side make the tracing log smaller which is > enabled for all builds on Treeherder. Before I do the change, I would like > to know what others think. The downside will be that if a failure occurred > you cannot simply copy and paste the data and open as data url. I think for this test, it is fine to transfer the data. We want to test that it actually does capture what is intended. Consumers of Marionette should definitely be encouraged to use hashing, as transferring raw binary data is rather expensive since the Marionette protocol isn’t compressed. However, I don’t think you should use this bug to follow up on making the tracing logs smaller. I suspect the gecko.log file can be made substantially smaller though.

Comment on attachment 8817411 [details] Bug 1243415 - Improve screenshot unit tests and add tests for chrome scope. https://reviewboard.mozilla.org/r/97690/#review98922 > The original name was intended. Not sure to what this comment refers to. ScreenCapture? I wouldn't call it that way. Webdriver uses Screenshot and our client implementation too. > Not opening an issue on this, but since we don’t memoise the document element or the Y offset of the window, it’s inconsistent to memoise this. devicePixelRatio will never change so I do not see a need to spend extra hundreds of milliseconds to send a command to the server and wait for its return value. It's just lost time. We don't need it for others because those values can change. > Just return the value. Ups, left from debugging. > Even though I’ve coded it so you can use a list for `script_args`, it’s more Pythonic to use a tuple. We use lists everywhere. I would suggest to file a new bug to get all this converted at once. > Use `if element is not None` here to avoid a falsiness check. I assume you meant `if element is None`. :) > Keep old name. See above. This is inconsistent. > Tuple again. As mentioned above for consistency right now.

Comment on attachment 8817407 [details] Bug 1243415 - Add highlighting of elements for screenshots in chrome scope. https://reviewboard.mozilla.org/r/97682/#review98930 > You could replace all of this with: > > ```js > let highlightEls = highlights.map(h => this.curBrowser.seenEls(h, container)); > ``` > > Not making this an issue because I tend to like seeing the performance penalties of code, which you get by writing out loops. I assume we have a couple of those instances across the Marionette server code. If you feel that we should change this all can you please file a bug? I wonder how large the performance improvements are in general for such a change.

(In reply to Andreas Tolfsen ‹:ato› from comment #50) > (In reply to Henrik Skupin (:whimboo) from comment #37) > > The last try build shows two failures for content tests which already > > existed before and which are about a single line dimension difference for > > screenshots. I would suggest that we get that fixed in a follow-up bug. > > Noted. I think these are about using the bounding client rects instead of > the client rects for calculating the area to capture. Whatever this is. In such a case I would expect a difference of 2 pixels but not of 1, given that this should affect the top and bottom. > I think for this test, it is fine to transfer the data. We want to test > that it actually does capture what is intended. Consumers of Marionette > should definitely be encouraged to use hashing, as transferring raw binary > data is rather expensive since the Marionette protocol isn’t compressed. Ok. Thanks.

Comment on attachment 8817411 [details] Bug 1243415 - Improve screenshot unit tests and add tests for chrome scope. https://reviewboard.mozilla.org/r/97690/#review98922 > Not sure to what this comment refers to. ScreenCapture? I wouldn't call it that way. Webdriver uses Screenshot and our client implementation too. The chapter dealing with screen capture in WebDriver is named ‘Screen Capture’. Please just keep the old names.

Merge conflicts prevented autolander from landing this patch series. Fixed it now.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6091803e801 Capture methods should use the window as parameter. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/7ec63ef95514 Remove B2G related code from screenshot methods. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/1a07b3fcb30b Make use of capture.js in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/62c28040b388 Add highlighting of elements for screenshots in chrome scope. r=ato,automatedtester https://hg.mozilla.org/integration/autoland/rev/b7e547b2f592 Add hash format support for screenshots in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/f108db686ca2 Add support to take full screenshots in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/4bd98277bf9e Check for a valid DOMWindow for screenshot in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/56ff773fe562 Improve screenshot unit tests and add tests for chrome scope. r=ato

backed out for wr tests failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8006311&repo=autoland

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e29301348389 Backed out changeset 56ff773fe562 https://hg.mozilla.org/integration/autoland/rev/ccb5e521a7a0 Backed out changeset 4bd98277bf9e https://hg.mozilla.org/integration/autoland/rev/c0cc234caead Backed out changeset f108db686ca2 https://hg.mozilla.org/integration/autoland/rev/e2b17c710f99 Backed out changeset b7e547b2f592 https://hg.mozilla.org/integration/autoland/rev/5d233baf72f7 Backed out changeset 62c28040b388 https://hg.mozilla.org/integration/autoland/rev/903c798cf788 Backed out changeset 1a07b3fcb30b https://hg.mozilla.org/integration/autoland/rev/6bee764b55cd Backed out changeset 7ec63ef95514 https://hg.mozilla.org/integration/autoland/rev/0e6ef171f8cf Backed out changeset e6091803e801 for wr test bustage

The problem as seen in the above test job are different hashes for the image and the reference. I'm fairly sure that this is because of the newly implemented scaling of images. I have to see how the reftests under webplatform tests are handling the reference image. If there is no easy way to get this fixed we will have to revert the scaling logic.

So examining such a test it looks like it is somewhat related to highlighting. There is a red line at the bottom of this image which is no present in the reference image.

The issues with web-platform-tests appear when I use the ctx.DRAWWINDOW_DRAW_VIEW flag for drawing the window into the canvas. It basically means "Draw scrollbars and scroll the viewport if they are present." With that included the screenshots are identical which would mean in a reftest that the test has to pass. What I don't understand in this case are the gray bars at the lower end of the screenshots. Those are no scrollbars nor other nodes, and I don't see it on my screen when running the tests locally. James, can you please have a look at those screenshots and tell me what you think? If we don't want to make this change right now I'm happy to move the addition of DRAWWINDOW_DRAW_VIEW out to a follow-up bug.

Btw when I enlarge the popup window where the tests are running in, I can still see this text but at the very right bottom corner. The popup doesn't have scrollbars, and as such we fail I think. I wonder if the popup is opened correctly because I think it should have scrollbars. So the unexpected pass is clearly a failure!

Ok, so while investigating the failure in 2_tracks.html I noticed that at least for this test we set "overflow:hidden" on the html element. The popup window which runs all the tests has a size of 600x600 pixels, and as such the content we want to compare is getting drawn outside of the viewport. Due to the hidden value no scrollbars are drawn and as such drawWindow doesn't reach that area. James it would be good to know why we do all that outside of the viewport.

We should not be doing anything outside the viewport; that's a test bug. In general we unfortunately have no good way to figure out which tests go outside the viewport (with anything relevant to the test). I filed <https://github.com/w3c/web-platform-tests/issues/4383>.

That's good to hear. Thank you Ms2ger. I would say given that this is an external dependency I will disable the drawing flag in content for now, and we can revisit once the webplatform tests follow the 600x600 window size.

Removing the flag made all tests passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c0c68bbbb02bbd213a756f5d27f86c919ef3f09 So I will better comment it out completely and we can add it again once the referenced web-platform-tests issue has been solved and tests are getting imported into mozilla-central.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/11b5edf1c2b0 Capture methods should use the window as parameter. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/8f3e84d364e7 Remove B2G related code from screenshot methods. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/b775ed2eb015 Make use of capture.js in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/31e61178ca03 Add highlighting of elements for screenshots in chrome scope. r=ato,automatedtester https://hg.mozilla.org/integration/autoland/rev/804ae2ac1d9b Add hash format support for screenshots in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/a96fb5d278bf Add support to take full screenshots in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/b8f4c98724eb Check for a valid DOMWindow for screenshot in chrome scope. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/7702f475a614 Improve screenshot unit tests and add tests for chrome scope. r=ato

https://hg.mozilla.org/mozilla-central/rev/11b5edf1c2b0 https://hg.mozilla.org/mozilla-central/rev/8f3e84d364e7 https://hg.mozilla.org/mozilla-central/rev/b775ed2eb015 https://hg.mozilla.org/mozilla-central/rev/31e61178ca03 https://hg.mozilla.org/mozilla-central/rev/804ae2ac1d9b https://hg.mozilla.org/mozilla-central/rev/a96fb5d278bf https://hg.mozilla.org/mozilla-central/rev/b8f4c98724eb https://hg.mozilla.org/mozilla-central/rev/7702f475a614

We would like to have this test-only patch for the next ESR release. Please uplift to aurora. Thanks.

needs rebasing grafting 381360:11b5edf1c2b0 "Bug 1243415 - Capture methods should use the window as parameter. r=automatedtester" merging testing/marionette/capture.js merging testing/marionette/listener.js warning: conflicts while merging testing/marionette/capture.js! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')

The problem here might be the missing patch on bug 1316975 for 52. I will check which parts need a fix.

The patch applies cleanly when bug 1316975 has been uplifted first. So please care about that for the test-only uplift to aurora. Thanks.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e5a2d42d984b https://hg.mozilla.org/releases/mozilla-aurora/rev/7a55b008ca3b https://hg.mozilla.org/releases/mozilla-aurora/rev/daf5f79fa5db https://hg.mozilla.org/releases/mozilla-aurora/rev/a7da8bdcf0f7 https://hg.mozilla.org/releases/mozilla-aurora/rev/7b31d0beacbc https://hg.mozilla.org/releases/mozilla-aurora/rev/88b2329f2448 https://hg.mozilla.org/releases/mozilla-aurora/rev/c509d6923794 https://hg.mozilla.org/releases/mozilla-aurora/rev/643be9e47b48