Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(screenshots): match format of final-screenshot #8299

Merged
merged 8 commits into from
Apr 17, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
Reworks our screenshot-thumbnails to match final-screenshot. I personally prefer the format used in screenshot-thumbnails but I see the value in an immediately consumable data URI without misinterpretation of what its contents are. I played around with just a mime-type property but then that seemed like overkill so I went back to just matching the data:image/jpeg format.

Potentially we might consider a rename of .data to .dataURL or something at the same time?

Related Issues/PRs
#7752

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially we might consider a rename of .data to .dataURL or something at the same time?

this makes sense to me

lighthouse-core/audits/screenshot-thumbnails.js Outdated Show resolved Hide resolved
timestamp: targetTimestamp * 1000,
data: base64Data,
timestamp: targetTimestamp,
data: `data:image/jpeg;base64,${base64Data}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, having the prefix makes sense so no one has to guess what type of image it is, even if it's annoying to add it here

@@ -30,14 +30,14 @@ describe('Screenshot thumbnails', () => {
const framePath = path.join(__dirname,
`../fixtures/traces/screenshots/progressive-app-frame-${index}.jpg`);
const expectedData = fs.readFileSync(framePath, 'base64');
assert.equal(expectedData.length, result.data.length);
expect(result.data.length - 'data:image/jpeg;base64,'.length).toEqual(expectedData.length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird test :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, I can slice it instead to make it a little more explicit :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird test :)

heh, I can slice it instead to make it a little more explicit :)

ha, I meant the original test is a weird one, but I like the change :)

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Messing with proto details again 😮

@paulirish paulirish mentioned this pull request Apr 16, 2019
67 tasks
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

(with one last suggestion )

const screenshots = await Screenshots.request(trace, context);
const finalScreenshot = screenshots[screenshots.length - 1];

if (!finalScreenshot) {
throw new LHError(LHError.errors.NO_SCREENSHOTS);
}

// trace-of-tab uses microseconds for the timestamp, so we'll convert for consistency
const finalScreenshotTs = finalScreenshot.timestamp * 1000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

computed/screenshots.js is dividing the timestamp by 1000, and this is the only audit using that computed artifact (did we use it for something else at some point?), so, want to just not divide it there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sg, I wanted to kill it at some point for being mostly useless, but that'll have to wait for another day :)

@brendankenny
Copy link
Member

lol how does that computed artifact have no test for the timestamp :)

it should definitely get culled at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants