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

core(driver): don't clear indexedb, websql, or localstorage before run #11438

Merged
merged 17 commits into from
Sep 29, 2020

Conversation

adamraine
Copy link
Member

Part of #772

Prevents loss of important data in any of indexeddb, websql, or localstorage. DevTools panel should display a warning to use an incognito window when there is data in these resources rather than clearing them.

In addition to DevTools warning, should we add a toplevel warning in the report if there was data in these resources?

@adamraine adamraine requested a review from a team as a code owner September 16, 2020 16:57
@adamraine adamraine requested review from brendankenny and removed request for a team September 16, 2020 16:57
@brendankenny
Copy link
Member

In addition to DevTools warning, should we add a toplevel warning in the report if there was data in these resources?

I think a warning makes a lot of sense. Most people running into this in DevTools will likely be accidental (just trying out Lighthouse on their site) and not because they're trying out behavior of a warm load, so a warning on the report itself will be a good reminder of what they should do for an honest assessment of performance.

OTOH, there won't be a way to get rid of the warning if you are doing it on purpose and we'll may have to revisit for Fraggle Rock.

Any other opinions?

@patrickhulce
Copy link
Collaborator

How about the toplevel warning only gets added when disableStorageReset is false?

lighthouse-core/test/gather/gather-runner-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/gather/gather-runner-test.js Outdated Show resolved Hide resolved
lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved
lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved
lighthouse-core/gather/driver.js Show resolved Hide resolved
lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved
lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved
lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved
lighthouse-core/test/gather/driver-test.js Show resolved Hide resolved
lighthouse-core/gather/driver.js Show resolved Hide resolved
lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved
lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved
lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved
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!

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.

6 participants