Skip to content

Conversation

@paulirish
Copy link
Member

We've discovered that some webservers misbehave when they get a mobile useragent but empty clienthints headers.

And since a while ago... when we set Emulation.setUserAgentOverride .. it appropriately sets the useragent string, but since no userAgentMetadata was provided, all those values are undefined. As client hints are being adopted, this leads to weird and brokenish scenarios. (depending on how defensive folks are configuring their ua handling on the webserver side).

We'll want to make sure clienthint headers are appropriately populated.

I went for making a change that minimizes impact on people who configure lighthouse with custom UA. I think we can revisit exactly how to expose those fields. I'd go for something less verbose than pptr's choice of replicating the exact same payload.


@paulirish paulirish requested a review from a team as a code owner November 9, 2021 00:47
@paulirish paulirish requested review from connorjclark and removed request for a team November 9, 2021 00:47
@google-cla google-cla bot added the cla: yes label Nov 9, 2021
const version = fullVersion.split('.', 1)[0];
const brands = [
{brand: 'Chromium', version},
{brand: 'Google Chrome', version},
Copy link
Collaborator

Choose a reason for hiding this comment

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

wdyt of adding a entry for LH version here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and if we do that, we should randomly sort the array, to support the GREASE stuffs (or will chrome do that for us in the request headers?)

Copy link
Member Author

Choose a reason for hiding this comment

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

wdyt of adding a entry for LH version here?

done

and if we do that, we should randomly sort the array, to support the GREASE stuffs (or will chrome do that for us in the request headers?)

seems like fun.. and its jsut adding a .sort(_ => Math.random() - 0.5)

... but we add some non-determinimism without much gain so i think we should put it off. leave the greasing to the big boiz.

userAgentMetadata: parseUseragentIntoMetadata(userAgent, settings.formFactor),
});
}
// See devtools-entry for one usecase for disabling screenEmulation
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs tests, esp. for the case where the UA is user-provided and doesn't even have a chrome version in it

function parseUseragentIntoMetadata(userAgent, formFactor) {
const match = userAgent.match(/Chrome\/([\d.]+)/); // eg 'Chrome/(71.0.3577.0)'
const fullVersion = (match && match[1]) || '99.0.1234.0';
const version = fullVersion.split('.', 1)[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const version = fullVersion.split('.', 1)[0];
const [version] = fullVersion.split('.', 1);

😎

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.

3 participants