Closed
Bug 905650
Opened 11 years ago
Closed 8 years ago
Add a way to get just the hash of a screenshot
Categories
(Testing :: Marionette Client and Harness, defect)
Testing
Marionette Client and Harness
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
mozilla49
People
(Reporter: jgraham, Assigned: kb15, Mentored)
Details
(Keywords: pi-marionette-client, pi-marionette-server, Whiteboard: [screenshot][good first bug][lang=py][lang=js])
Attachments
(1 file)
For many use cases it isn't necessary to get all the screenshot data; you just want to compare the screenshot against some known reference. One obvious example of this is implementing reftests where you only care about whether two screenshots are the same or different, not about the details of the individual pixel data. This could be implemented by allowing a hash of the screenshot to be returned by the screenShot() function in marionette-listener.js, and adding a parameter indicating that only the hash is required, or by having a separate function that does largely the same thing as screenShot, but returns a hash. One use case that could be common is "match the hash to an existing one and only return the screenshot data if there is a mismatch". This would be useful for testing scenarios where the screenshot data is debugging information for the case that hashes fail to match. For full generality, one should be able to return only on match or only on mismatch (this is the reftest usecase with == and != reftests).
Comment 1•11 years ago
|
||
cool idea
Updated•10 years ago
|
Whiteboard: [screenshot]
Updated•9 years ago
|
Keywords: ateam-marionette-client,
ateam-marionette-server
Comment 2•9 years ago
|
||
Having it as a separate command alleviates any typing problems in static languages. Otherwise you need to look at the parameter the user gave it to know what the return type will be. Because JSON doesn't distinguish between a base64 string of a PNG and a stringified byte array of a hash, it would be impossible to distinguish based on the JSON return type.
Updated•9 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [screenshot] → [screenshot][good first bug] [mentor=ato][lang=py][lang=js]
Updated•9 years ago
|
Whiteboard: [screenshot][good first bug] [mentor=ato][lang=py][lang=js] → [screenshot][good first bug][lang=py][lang=js]
Comment 3•9 years ago
|
||
import difflib def diff_ratio(screen_ref, screen_new): s = difflib.SequenceMatcher(None, screen_ref, screen_new) return s.quick_ratio() This will return a parameter in between 0 and 1. If the two images are identical, you get a 1, if it's totally different you get a 0.
Comment 4•9 years ago
|
||
I think what is suggested here is to add this as a server-side command to reduce wire protocol delays.
Comment 5•9 years ago
|
||
I would like to take this bug.
Comment 6•9 years ago
|
||
(In reply to Chris Galecki from comment #5) > I would like to take this bug. Assigned the bug to you. Please let me know if you need any help. I’m known as ato in the #ateam IRC channel.
Assignee: nobody → csg35
Hi Andreas I am interested in working on this. Can you please mentor me? Thanks Dhruva
Comment 8•9 years ago
|
||
Reassigning this to es13b1022 since there has been no activity. I’m happy to mentor. The source code for the existing screenshot feature is in testing/marionette/driver.js and testing/marionette/listener.js. All remote calls go to the driver handler that lives in chrome space (driver.js) and gets propagated to the listener (listener.js) if a content screenshot is desired. The result is returned to chrome context, then marshaled and passed back to the local end. What I imagine we want to do here is to add a separate command endpoint that returns a hash of the screenshot raw data. We could abstract the code in the existing screenshot implementation into a function that can get called by multiple different users.
Assignee: csg35 → es13b1022
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 9•9 years ago
|
||
Hi, Are you still working on this or can I reassign this back in case anyone else wants to work on it?
Flags: needinfo?(es13b1022)
Comment 10•9 years ago
|
||
Resetting assignee
Assignee: es13b1022 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(es13b1022)
Comment 11•9 years ago
|
||
I would like to work on this bug can you please assign this bug to me also I will try to sent the patch soon and is there anything else that I need to know to fix this bug ?
Flags: needinfo?(james)
Reporter | ||
Comment 12•9 years ago
|
||
I don't think there's anything else you need to know; see the comments from :ato above, and ask on irc when you get stuck.
Assignee: nobody → dhanvicse
Flags: needinfo?(james)
Comment 13•9 years ago
|
||
Since last time I commented on this bug, I've refactored the screen capture code as mentioned above to be possible to share between chrome- and content space. The refactor actually makes it easier to fix this bug as the functions to capture screenshots are now more compartmentalised and not so tightly coupled as before. See the source code in testing/marionette/capture.js: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/capture.js?from=marionette%2Fcapture.js
Updated•9 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Assignee: dhanvicse → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Mentor: ato
Updated•8 years ago
|
Assignee: nobody → dhanvicse
Status: NEW → ASSIGNED
Comment 14•8 years ago
|
||
I would like to still work on this bug so I am assigning it to myself
Comment 16•8 years ago
|
||
I am afraid that I made a little progress on this one Yes I sure need your help, I will ping you in IRC
Flags: needinfo?(dhanvicse)
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45117/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45117/
Attachment #8739608 -
Flags: review?(ato)
Updated•8 years ago
|
Attachment #8739608 -
Flags: review?(ato) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8739608 [details] MozReview Request: Bug 905650 - Added ability to get the hash of a screenshot; r?ato https://reviewboard.mozilla.org/r/45117/#review41859 This looks very good! My comments are almost entirely nitpicks and I’m giving this an r+. Do you have access to test code on try or push to inbound? If not we can get the ball rolling on that. ::: testing/marionette/capture.js:7 (Diff revision 1) > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > +var {utils: Cu} = Components; `const` ::: testing/marionette/capture.js:158 (Diff revision 1) > +* @return {string} > +* A hex digest of the SHA-256 hash of the base64 encoded string. > +*/ > +capture.toHash = function(canvas) { > + let u = capture.toBase64(canvas); > + var buffer = new TextEncoder("utf-8").encode(u); `let` ::: testing/marionette/capture.js:159 (Diff revision 1) > + return crypto.subtle.digest("SHA-256", buffer).then(function (hash){ > + return hex(hash); > + }); This can be shortened using ES6 arrow functions, but you can decide if you want to use it: ```js return crypto.subtle.digest("SHA-256", buffer).then(hash => hex(hash)); ``` ::: testing/marionette/capture.js:162 (Diff revision 1) > + let u = capture.toBase64(canvas); > + var buffer = new TextEncoder("utf-8").encode(u); > + return crypto.subtle.digest("SHA-256", buffer).then(function (hash){ > + return hex(hash); > + }); > +} Missing semicolon ::: testing/marionette/capture.js:163 (Diff revision 1) > + var buffer = new TextEncoder("utf-8").encode(u); > + return crypto.subtle.digest("SHA-256", buffer).then(function (hash){ > + return hex(hash); > + }); > +} > + Whitespace ::: testing/marionette/capture.js:174 (Diff revision 1) > + var hexCodes = []; > + var view = new DataView(buffer); > + for (var i = 0; i < view.byteLength; i += 4) { > + var value = view.getUint32(i) > + var stringValue = value.toString(16) > + var padding = '00000000' > + var paddedValue = (padding + stringValue).slice(-padding.length) Use `let` throughout. ::: testing/marionette/capture.js:177 (Diff revision 1) > + var value = view.getUint32(i) > + var stringValue = value.toString(16) > + var padding = '00000000' > + var paddedValue = (padding + stringValue).slice(-padding.length) Missing semicolons ::: testing/marionette/driver.js:2514 (Diff revision 1) > * @return {string} > - * PNG image encoded as base64 encoded string. > + * If 'hash' is False, PNG image encoded as base64 encoded string. If > + * 'hash' is True, hex digest of the SHA-256 hash of the base64 encoded > + * string. 'hash' → {@code hash} False → false ::: testing/marionette/driver.js:2564 (Diff revision 1) > let dataUrl = canvas.toDataURL("image/png", ""); > let data = dataUrl.substring(dataUrl.indexOf(",") + 1); > resp.body.value = data; > break; > > - case Context.CONTENT: > + case Context.CONTENT: Whitespace ::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:5 (Diff revision 1) > import base64 > import imghdr > import struct > import urllib > +import hashlib Sort alphabetically ::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:9 (Diff revision 1) > > import base64 > import imghdr > import struct > import urllib > +import hashlib Whitespace ::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:200 (Diff revision 1) > + > + def test_hash_format(self): > + self.marionette.navigate(box) > + el = self.marionette.find_element(By.TAG_NAME, "div") > + content = self.marionette.screenshot(element=el, format="hash") > + pyHash = hashlib.sha256(ELEMENT).hexdigest() pyHash → hash ::: testing/marionette/harness/marionette/tests/unit/test_screenshot.py:201 (Diff revision 1) > + def test_hash_format(self): > + self.marionette.navigate(box) > + el = self.marionette.find_element(By.TAG_NAME, "div") > + content = self.marionette.screenshot(element=el, format="hash") > + pyHash = hashlib.sha256(ELEMENT).hexdigest() > + self.assertEqual(content, pyHash) Whitespace ::: testing/marionette/listener.js:1756 (Diff revision 1) > * references. > * > * @return {string} > * Base64 encoded string of an image/png type. > */ > + Remove blank line ::: testing/marionette/listener.js:1800 (Diff revision 1) > +* references. > +* > +* @return {HTMLCanvasElement} > +* The canvas element to be encoded or hashed. > +*/ > + Remove blank line
Updated•8 years ago
|
Assignee: dhanvicse → kbmoz1515
Assignee | ||
Comment 19•8 years ago
|
||
I don't have access to try or push to inbound. Thanks for the quick review; I'll get to work on these changes!
Flags: needinfo?(ato)
Updated•8 years ago
|
Flags: needinfo?(ato)
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8739608 [details] MozReview Request: Bug 905650 - Added ability to get the hash of a screenshot; r?ato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45117/diff/1-2/
Comment 21•8 years ago
|
||
https://reviewboard.mozilla.org/r/45117/#review44489 Sorry for the late reply to this, but I’ve been on holiday! This looks excellent! Thank you so much for fixing this bug. I will run a try run (CI) tomorrow (unless you have access to do so already?), and if all is fine push it to inbound.
Comment 22•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddafb86bfdb8
Comment 23•8 years ago
|
||
I notice that the patches have not be rebased for quite some time. The test that is failing in the try run was disabled by philor in bug 1267012 and should be of no concern. kb15: Can you rebase the patch and we’ll do another try run?
Flags: needinfo?(kbmoz1515)
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8739608 [details] MozReview Request: Bug 905650 - Added ability to get the hash of a screenshot; r?ato Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45117/diff/2-3/
Assignee | ||
Comment 25•8 years ago
|
||
I pulled inbound today and rebased the patch.
Flags: needinfo?(kbmoz1515)
Comment 26•8 years ago
|
||
Thanks! Triggered another try run.
Comment 27•8 years ago
|
||
One failing Mn-e10s job, but it might be an intermittent so retriggering.
Comment 29•8 years ago
|
||
The failing Mn-e10s job was indeed intermittent. I’ve landed it on inbound now. That means it will hopefully pass all tests and make its way to central within 24 hours.
Comment 30•8 years ago
|
||
I forgot to mention: Thank you for fixing!
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69267d635ade
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 32•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/31d4316fa44c
status-firefox48:
--- → fixed
Comment 33•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/31d4316fa44c
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 35•1 year ago
|
||
Moving bugs for Marionette client due to component changes.
Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in
before you can comment on or make changes to this bug.
Description
•