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.

Sign up for GitHub

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

Jump to bottom

add desktop support, refactor scoring data #10

Merged
merged 13 commits into from
May 28, 2020
Merged

add desktop support, refactor scoring data #10

merged 13 commits into from
May 28, 2020

Conversation

connorjclark
Copy link
Collaborator

I started work on getting different scoring curve based on devices to work, but first had to refactor much of the scoring data.

I got to the point where I realized we need to support the new scoring curve definition. Need to pick this up another time.

@connorjclark connorjclark requested a review from paulirish May 15, 2020 21:00
paulirish
paulirish reviewed May 18, 2020
View reviewed changes
Copy link
Owner

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

i left some ideas on what metrics.js looks like...

I got to the point where I realized we need to support the new scoring curve definition.

as for p10 scoring, there's two approaches we can take 1) support both p10-style & podr style and have the data determine. or 2) we only support p10 style and fix the data to match.

i'm good with whatever

script/metrics.js

// TODO need to support defining curves with p10 and podr.

const scoringGuides_WIP_mobile = {
v6: {

This comment was marked as resolved.

Sign in to view

script/main.js Outdated
@@ -220,7 +222,10 @@ class App extends Component {
// debounce just a tad, as its noisy
debounce(_ => {
const url = new URL(location.href);
const auditIdValuePairs = Object.entries(this.state).map(([id, value]) => [scoring[id].auditId,value]);
const auditIdValuePairs = Object.entries(this.state).map(([id, value]) => {
const auditId = (scoringGuides.v5[id] || scoringGuides.v6[id]).auditId;
Copy link
Owner

Choose a reason for hiding this comment

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

this is a bit awkward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeaeh, let's make a metrics object that is just the metric name, audit id.... exactly what you named audits. 👍

@connorjclark
Copy link
Collaborator Author

  1. support both p10-style & podr style and have the data determine.

I'd prefer this

@connorjclark connorjclark changed the title refactor scoring data add device query parameter to configure scoring guide, refactor scoring data May 18, 2020
@connorjclark
Copy link
Collaborator Author

connorjclark commented May 19, 2020 •
edited
Loading

scoring curves now derive podr from p10.

verify it's all cool with:

http://localhost:8000/#first-contentful-paint=4143.975&speed-index=8527.592402791312&largest-contentful-paint=8930.763000000003&interactive=20955.31199999999&total-blocking-time=2375.5&cumulative-layout-shift=0.134375&first-cpu-idle=11466.559000000001&first-meaningful-paint=6435.7245&device=mobile&version=6.0.0&version=5.6.0

image

image

@paulirish paulirish mentioned this pull request May 24, 2020
Closed
@paulirish
Copy link
Owner

ruhroh.

GoogleChrome/lighthouse#10836 has a WPT report where "device" is 'none' as it's not using desktop emulation. (But it is a desktop run)

We'll have to figure out how we handle that case.

@connorjclark
Copy link
Collaborator Author

Shouldn't we just use the desktop curves for emulation none? Is that what LH does?

@paulirish
Copy link
Owner

paulirish commented May 24, 2020 via email

@patrickhulce
Copy link
Collaborator

Shouldn't we just use the desktop curves for emulation none? Is that what LH does?

Problem: when you run WPT on an actual phone it also uses none and we definitely don't want to use the desktop curves there :/ We might need to update the report to infer from the UA or use TestedAsMobileDevice or something when device emulation is none.

Example test https://webpagetest.org/result/200524_3N_9f238902d8910c6a5dd67ad3a05ab1eb/

@patrickhulce
Copy link
Collaborator

Honestly maybe the better message is to say "warning: this test didn't match the two lighthouse presets" and just have a toggle for mobile v. desktop?

@fvpDev
Copy link

fvpDev commented May 25, 2020 •
edited
Loading

Honestly maybe the better message is to say "warning: this test didn't match the two lighthouse presets" and just have a toggle for mobile v. desktop?

As a user, I would prefer this^...In general, having a toggle for that would be nice. Though in the context of wpt, they are already separating mobile tests from desktop tests, thus already separating LH runs. I think on the wpt channel it would be could to keep them separated as they do; and not have a toggle there, so as to avoid potential confusion. However on web.dev, the toggle would be nice to have for sure...currently right now it is an Emulated Nexus 5X when running in desktop view, which is a big no no.

@connorjclark
Copy link
Collaborator Author

Let's just add a toggle to the scoring calculator. if it's none, just default to something (it'd be wrong in some cases for whatever we pick, so let's just pick mobile?)

we determine the scoring guide in LH w/ artifacts.TestedAsMobileDevice, which is a mix of emulated form factor/hosted device, so perhaps we just need to surface that in the LHR and use that in the report calc URL.

in any case, let's land this PR and follow up with a device toggle.

@benschwarz
Copy link

Let's just add a toggle to the scoring calculator. if it's none, just default to something (it'd be wrong in some cases for whatever we pick, so let's just pick mobile?)

@connorjclark Isn't none always going to be a Desktop run? (Unless the user has purposefully set their own screen dimensions/throttling/etc etc)

@connorjclark
Copy link
Collaborator Author

@connorjclark Isn't none always going to be a Desktop run? (Unless the user has purposefully set their own screen dimensions/throttling/etc etc)

exactly :)

see wpt: this code runs for both mobile or desktop host device. it doesn't configure what type it is–we derive that in LH via TestedOnMobileDevice
https://github.com/connorjclark/wptagent/blob/master/internal/devtools_browser.py#L490

paulirish
paulirish approved these changes May 28, 2020
View reviewed changes
Copy link
Owner

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

Double-checked some v6 and v5 scores and they're all looking great.

Agree with following up this PR with a device toggle.

@paulirish paulirish changed the title add device query parameter to configure scoring guide, refactor scoring data add desktop support, refactor scoring data May 28, 2020
@paulirish paulirish merged commit 5b2d729 into master May 28, 2020
@paulirish paulirish deleted the device branch May 28, 2020 22:13
@paulirish
Copy link
Owner

This is deployed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulirish paulirish paulirish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@connorjclark @paulirish @patrickhulce @fvpDev @benschwarz