-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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(devtools-log): consolidate implementation into gatherer #14080
Conversation
lighthouse-core/gather/driver.js
Outdated
@@ -12,7 +12,7 @@ const {fetchResponseBodyFromCache} = require('../gather/driver/network.js'); | |||
const EventEmitter = require('events').EventEmitter; | |||
|
|||
const log = require('lighthouse-logger'); | |||
const DevtoolsLog = require('./devtools-log.js'); | |||
const DevToolsMessageLog = require('./gatherers/devtools-log.js').DevToolsMessageLog; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: casing does not match file/how we typically stylize "devtools"
|
||
/** | ||
* This class saves all protocol messages whose method match a particular | ||
* regex filter. Used when saving assets for later analysis by another tool such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (sorry): spacing is off
module.exports = DevtoolsLog; | ||
module.exports.DevToolsMessageLog = DevToolsMessageLog; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing does not match
|
||
describe('DevtoolsLog', () => { | ||
describe('DevToolsMessageLog', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approval but some requested changes for consistency
currently we have two
devtools-log.js
files.the gatherer one is only used in FR, not legacy. in legacy, the equivalent functionality is done in driver.
so i'm changing the legacy one's name to be what it does. also… relevant 2017 threads: #1665 (comment) and #1669