Skip to content

misc: avoid splitting node labels in the middle of unicode surrogate pairs#11698

Merged
devtools-bot merged 12 commits intoGoogleChrome:masterfrom
wildlyinaccurate:truncate-codepoints-not-bytes
Dec 6, 2020
Merged

misc: avoid splitting node labels in the middle of unicode surrogate pairs#11698
devtools-bot merged 12 commits intoGoogleChrome:masterfrom
wildlyinaccurate:truncate-codepoints-not-bytes

Conversation

@wildlyinaccurate
Copy link
Contributor

@wildlyinaccurate wildlyinaccurate commented Nov 23, 2020 •
edited by patrickhulce
Loading

Summary

Potential fix for #11697. Takes advantage of the fact that the ES6 string iterator is unicode-aware.

Related Issues/PRs

fix for part of #11697

@wildlyinaccurate wildlyinaccurate requested a review from a team as a code owner November 23, 2020 02:53
@wildlyinaccurate wildlyinaccurate requested review from patrickhulce and removed request for a team November 23, 2020 02:53
@google-cla google-cla bot added the cla: yes label Nov 23, 2020
patrickhulce
patrickhulce approved these changes Nov 23, 2020
View reviewed changes
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.

thanks very much @wildlyinaccurate! 🎉

great find, PR much appreciated :)

@wildlyinaccurate
Copy link
Contributor Author

wildlyinaccurate commented Nov 23, 2020

Thanks for the suggestions @patrickhulce - much appreciated!

@wildlyinaccurate wildlyinaccurate changed the title Avoid splitting node labels in the middle of unicode surrogate pairs misc: Avoid splitting node labels in the middle of unicode surrogate pairs Nov 23, 2020
@wildlyinaccurate wildlyinaccurate changed the title misc: Avoid splitting node labels in the middle of unicode surrogate pairs misc: avoid splitting node labels in the middle of unicode surrogate pairs Nov 23, 2020
@wildlyinaccurate
Copy link
Contributor Author

wildlyinaccurate commented Nov 23, 2020

yarn update:test-devtools is failing on my machine. I'm not sure if I need to manually update some of the expected output fixtures or not?

@patrickhulce
Copy link
Collaborator

patrickhulce commented Nov 23, 2020

It's actually failing on master too @wildlyinaccurate , so not your doing. We'll let this chill for a bit until we're back up and running 👍

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 23, 2020

master on my machine:
image

@patrickhulce
Copy link
Collaborator

patrickhulce commented Nov 23, 2020

@connorjclark it's in CI too https://github.com/GoogleChrome/lighthouse/runs/1434349934#step:15:206

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 24, 2020

Just landed #11702 to fix master.

How is yarn update:test-devtools failing, can you share the output? FYI it only works on Mac at the moment.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Nov 24, 2020

FWIW I don't think you'll need to run yarn update:test-devtools once this is rebased though

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 24, 2020

I think the other 2 usages of .slice here are susceptible to the same issue. Hard to analyze the entire codebase tho, would be nice if one could search for [String].slice using the typescript parser.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Nov 24, 2020

I think the other 2 usages of .slice here are susceptible to the same issue. Hard to analyze the entire codebase tho, would be nice if one could search for [String].slice using the typescript parser.

There are many usages of .slice across LH that are susceptible to this issue. How do you feel about just leaving open #11697 for investigation by the team but still landing this PR?

@google-cla
Copy link

google-cla bot commented Nov 24, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 24, 2020
patrickhulce
patrickhulce reviewed Dec 4, 2020
View reviewed changes
.github/scripts/git-get-shared-history.sh
# We can always use some more history
git -c protocol.version=2 fetch --deepen=100

# Find out if the PR is coming from a fork
Copy link
Collaborator

@patrickhulce patrickhulce Dec 4, 2020

Choose a reason for hiding this comment

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

@paulirish how do you feel about this skipping this check entirely if it's from a fork for now instead and we can sort out the fix later?

Copy link
Member

@paulirish paulirish Dec 6, 2020

Choose a reason for hiding this comment

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

that works for me.. though determining that would probably have required almost this much iteration as well... so i think it's all moot.

ALL MOOT!

@wildlyinaccurate
Copy link
Contributor Author

wildlyinaccurate commented Dec 5, 2020

Hey, uh, does it help if I resubmit this against master as a single commit?

@paulirish
Copy link
Member

paulirish commented Dec 6, 2020

Hey, uh, does it help if I resubmit this against master as a single commit?

haha nah man it's all good. we'll just all pretend I never had to make all that noise. the bug i had is fixed (as of my last commit), but it ends up not mattering.

image

This should be good to go once the yarn install flake timeout is gone.

paulirish
paulirish approved these changes Dec 6, 2020
View reviewed changes
Copy link
Member

@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.

@wildlyinaccurate btw this is a fantastic find and fix. Nice work. :D

@devtools-bot devtools-bot merged commit 2fa473f into GoogleChrome:master Dec 6, 2020
@patrickhulce patrickhulce mentioned this pull request Dec 8, 2020
Merged
@wildlyinaccurate wildlyinaccurate deleted the truncate-codepoints-not-bytes branch January 7, 2021 20:48
@paulirish paulirish mentioned this pull request Jan 15, 2021
Closed
@paulirish paulirish mentioned this pull request Mar 21, 2023
Merged
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

+1 more reviewer

@patrickhulce patrickhulce patrickhulce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@paulirish paulirish

@patrickhulce patrickhulce

Labels

cla: yes waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wildlyinaccurate @patrickhulce @connorjclark @brendankenny @paulirish @devtools-bot