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(full-page-screenshot): use dpr 1 #11688

Merged
merged 7 commits into from
Nov 24, 2020
Merged

core(full-page-screenshot): use dpr 1 #11688

merged 7 commits into from
Nov 24, 2020

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Nov 19, 2020

followup from #11118 and #11536

I originally hesitated about changing DPR but patrick convinced me here (and in some followup conversations) it's better to change DPR to 1 for our FPS.

Taller image. more coverage. The risk is lower than the risk we already encounter by changing the height (viz observers, etc).

To test this.

node lighthouse-cli https://paulirish.github.io/tiny-demos-on-https/pixel-ruler.html?maxPxHeight=20000 --only-audits=full-page-screenshot --preset=experimental

then in the report:

const img = new Image(); img.src = __LIGHTHOUSE_JSON__.audits["full-page-screenshot"].details.data;  document.body.prepend(img);

scale: 1,
positionX: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

removing all of this is necessary? IIRC you suggested I add all the parameters in the original PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah i was being overly cautious back then. i looked and neither CDT nor PPTR uses these props.
i tested this heavily after these changes and feel good that they aren't important.

@connorjclark
Copy link
Collaborator

Did you check if the cases that were fixed recently are still good with this change?

#11538
#11402
#11536

@connorjclark
Copy link
Collaborator

iffy on fpss (does the full thing fit)? but it should certainly be fps :)

@connorjclark connorjclark changed the title core(fpss): take screenshot with dpr: 1 core(fps): take screenshot with dpr: 1 Nov 19, 2020
@paulirish
Copy link
Member Author

paulirish commented Nov 20, 2020

good question

Did you check if the cases that were fixed recently are still good with this change?

#11538

ya

paulirish.github.io/tiny-demos-on-https/pixel-ruler.html?maxPxHeight=20000&color=ccc

#11402

ya

#11536

yeaup

https://googlechrome.github.io/lighthouse/viewer/?gist=b01f0140619ad5aca1953f532bf85149

@connorjclark
Copy link
Collaborator

Thanks for checking.

googlechrome.github.io/lighthouse/viewer/?gist=b01f0140619ad5aca1953f532bf85149

I don't see any images.

@paulirish
Copy link
Member Author

paulirish commented Nov 20, 2020

Thanks for checking.

googlechrome.github.io/lighthouse/viewer/?gist=b01f0140619ad5aca1953f532bf85149

I don't see any images.

TRUE!

it was fine locally but it put it up on viewer.. and.. looks like some problems.

image

... investigating...


update:

oh its just the changes to the FPS audit payload. you changed it in the resolve-nodes branch.. and to verify this PR works.. i merged the two locally.. but the hosted viewer doesn't have the changes.

anyway we all good.

image

Copy link
Contributor

@Beytoven Beytoven left a comment

Choose a reason for hiding this comment

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

LGTM. Gonna lean on @connorjclark to give the final approval though.

@@ -20,7 +19,7 @@ function createMockDriver({contentSize, screenSize, screenshotData}) {
return contentSize.width;
}
if (code === 'window.devicePixelRatio') {
return screenSize ? screenSize.dpr : 1;
return screenSize ? screenSize.dpr : 2;
Copy link
Collaborator

@connorjclark connorjclark Nov 21, 2020

Choose a reason for hiding this comment

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

i don't understand this change to the default

Copy link
Member Author

Choose a reason for hiding this comment

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

good call. it's kind of a separate change. This is the mock driver so we're returning a DPR back to lighthouse.

Since we're now emulating DPR 1 to take the screenshot.. I figured I'd just pretend the browser is a non-1 DPR just to keep things separate

afaik this value is never even used.......

......

okay in that case i'll just delete this part of the mock.
done.

@connorjclark
Copy link
Collaborator

oops I happened to already do a commit like core(full-page-screenshot): ... (7845154) soooo for consistencies sake....

@connorjclark connorjclark changed the title core(fps): take screenshot with dpr: 1 core(full-page-screenshot): use dpr 1 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants