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

misc(emulation): Use Nexus 5X emulation real screen size #6932

Merged
merged 1 commit into from
Jan 10, 2019

Conversation

exterkamp
Copy link
Member

Summary
I'll bite and try to adjust this. I just changed the emulation heights to 660. I wasn't sure if height should be 732 and then screenHeight should be 660 or the other way around? Docs didn't explain which was which, so I just made them both 660 for now.

When changing the emulated device in the frontend it only sends a screenWidth and screenHeight with width & height set to 0:
image
Note the 660 is derived on the frontend from: 732px - 24px (top bar) - 48px (bottom bar) = 660px

Related Issues/PRs
fixes: #6615

@exterkamp
Copy link
Member Author

There is a lot of height: 732 visibleHeight: 732 etc. in the test/fixtures/traces files, don't know if we need to change those too 🤷‍♂️

@patrickhulce
Copy link
Collaborator

I wasn't sure if height should be 732 and then screenHeight should be 660 or the other way around? Docs didn't explain which was which, so I just made them both 660 for now.

https://cs.chromium.org/chromium/src/third_party/blink/public/web/web_device_emulation_params.h?sq=package:chromium&g=0&l=23-33 suggests that width/height is the viewport which can be scrolled maybe using view_position? Not sure either 🤷‍♂️ Either way, copying what frontend does SGTM :)

This is probably going to have some unexpected ramifications down the line. I still think it needs to be done obviously since we're claiming Nexus 5X and the right aspect ratio should be used for it 😃, just trying to think of what could happen.

  • Speed Index and anything using ViewportDimensions height will likely change (just offscreen images I think that needs height?)
  • Screenshots will be different dimensions (probably doesn't matter to us so much but maybe consumers? @benschwarz @mattzeunert both of yours don't rely on default emulation and our screen size do they?)
  • Something else??

@mattzeunert
Copy link
Contributor

Yeah, I'm not relying on the default emulation settings.

Can't think of anything else that would change (as long as the page doesn't render differently based on the changed window height).

@brendankenny
Copy link
Member

DZL, do a barrel roll!

@devtools-bot
Copy link

DZL is done! Go check it out http://lh-dzl-6932.surge.sh

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 on the impl side

I'll defer to others on the impact on metrics :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM too!

For DZL changes, there's just no way this window size is responsible for that drastic TTI shift. Even if it was, people will just have to deal it's a major version bump anyhow 😆

More details on what we gotta do about this persistent problem to come 😞

@brendankenny brendankenny merged commit 0b223e5 into master Jan 10, 2019
@brendankenny brendankenny deleted the adjust-nexus-5x-height branch January 10, 2019 19:43
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.

Change mobile viewport size to match Nexus 5X
5 participants