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): remove unused goOffline/goOnline methods #12135

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

patrickhulce
Copy link
Collaborator

Summary
Removes some unused parts of driver I noticed while refactoring pieces for #11313. I don't recall if we ever established a semver policy for removing esoteric driver methods, but if there was a conversation I missed about this please feel free to correct me :)

Given how much of a delineation we made for non-public artifacts that are technically still accessible, this seems perfectly in line with that policy.

Related Issues/PRs
ref #11313

@patrickhulce patrickhulce requested a review from a team as a code owner February 24, 2021 20:49
@patrickhulce patrickhulce requested review from adamraine and removed request for a team February 24, 2021 20:49
@google-cla google-cla bot added the cla: yes label Feb 24, 2021
@brendankenny
Copy link
Member

#578 📆 ⏳ 😢

Maaaaaybe it's worth keeping? It does seem like the kind of thing custom gatherers might use...but I'm also 100% for deleting unused things, and that's winning out for me right now :) Also I've never loved that call hierarchy.

@@ -7,14 +7,6 @@

/** @typedef {import('../gather/driver.js')} Driver */

const OFFLINE_METRICS = {
offline: true,
// values of 0 remove any active throttling. crbug.com/456324#c9
Copy link
Member

@brendankenny brendankenny Feb 24, 2021

Choose a reason for hiding this comment

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

this seems like the only tricky knowledge that could be lost if trying to recreate going offline at a later date, but based on that comment is it still true/useful today?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on the immediate followup commit, it looks like future attempts should be fine with just offline: true.

If we ever need to find this again, we know the file to use git history on :)

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Looks good, but I don't have any strong opinions on keeping some of these.

@patrickhulce
Copy link
Collaborator Author

Let the record show that @paulirish +1'd this in person but did not use the 👍 reaction on any comment or PR description as of 2/25/2021 6:05pm CST despite claims to the contrary.

:)

@paulirish
Copy link
Member

Let the record show that @paulirish +1'd this in person but did not use the 👍 reaction on any comment or PR description as of 2/25/2021 6:05pm CST despite claims to the contrary.

:)

i don't know what you're talkin about…

image

;)

@patrickhulce patrickhulce merged commit 0daa1c3 into master Feb 26, 2021
@patrickhulce patrickhulce deleted the driver_cleanup branch February 26, 2021 18:02
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.

None yet

5 participants