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

Do not use Emulation.setVisibleSize #2831

Closed
dgozman opened this issue Aug 2, 2017 · 4 comments · Fixed by #3536
Closed

Do not use Emulation.setVisibleSize #2831

dgozman opened this issue Aug 2, 2017 · 4 comments · Fixed by #3536

Comments

@dgozman
Copy link
Contributor

dgozman commented Aug 2, 2017

It's not needed anymore, since setDeviceMetricsOverride does it automatically.
We are going to remove setVisibleSize soon, so we better stop calling it from lighthouse.


EDIT: dmitry indicates this was removed in m61, but we need to verify that.
verified that the protocol changes are in 61.0.3155.0

@brendankenny
Copy link
Member

Hmm, was setDeviceMetricsOverride fixed after a screenshot bug a few months ago? We added setVisibleSize in #1839 when we our screenshots weren't cropped to the emulated viewport size. Not sure what the story was at the time (did we file a crbug? or is that WAI and we need to do something else for screenshots?)

@paulirish

@patrickhulce
Copy link
Collaborator

@brendankenny I think it was fixed recently to be unnecessary, so we might need to wait for m61 to hit stable before removing it entirely?

@dgozman
Copy link
Contributor Author

dgozman commented Aug 2, 2017

That's correct, it was fixed recently. Might even have to wait for m62 to be on the safe side. I just filed this to not forget about cleanup opportunity.

@wardpeet
Copy link
Collaborator

wardpeet commented Aug 3, 2017

@brendankenny @patrickhulce how do we feel to already beta test this with wrapping it around an if so we can test on canary or isn't this something we want.

if (chrome.version < 62) {
  setVisibleSize(...)
}

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

Successfully merging a pull request may close this issue.

5 participants