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

convert DoBetterWeb gatherers to new error system #1641

Merged
merged 7 commits into from
Feb 6, 2017

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Feb 4, 2017

with #1624 this completes the transition of the gatherers to using actual errors instead of -1 artifacts :)

These are all pretty straightforward so I grouped them more coarsely by similar gatherers per commit.

The biggest change was I changed the artifact returned by the call site gatherers (Date.now, document.write, etc) so instead of returning {usage: [/* locations of call sites */]} they just return [/* locations of call sites */]. If that's bad I can revert, but the indirection seems unnecessary now that we've lived with these for a while now.

/**
* Returns WebSQL database information or null if none was found.
* @param {!Object} options
* @return {?{id: string, domain: string, name: string, version: string}}
Copy link
Member Author

Choose a reason for hiding this comment

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

this one also changed a bit. Before this was returning the object returned by the protocol (which has all information on a property named database) on the artifact on a property also named database, so to get e.g. the name you'd have to do artifacts.WebSQL.database.database.name. Now it's artifacts.WebSQL.name, and artifacts.WebSQL is null if no database was found.

@ebidel
Copy link
Contributor

ebidel commented Feb 6, 2017

The biggest change was I changed the artifact returned by the call site gatherers (Date.now, document.write, etc) so instead of returning {usage: [/* locations of call sites /]} they just return [/ locations of call sites */]. If that's bad I can revert, but the indirection seems unnecessary now that we've lived with these for a while now.

I think part of this was a symptom of the -1 error system. It was easy to check artifacts.AnchorsWithNoRelNoopener === -1 to know something went wrong with the gatherer. Coercing multiple values into the artifact result always felt weird to me. So we stuck actual results on a .usages property. With your changes, all that weirdness goes bye bye :)

Copy link
Contributor

@ebidel ebidel left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -147,15 +147,14 @@ class TagsBlockingFirstPaint extends Gatherer {
return options.driver.evaluateScriptOnLoad(scriptSrc);
}

/**
* @param {!Object} options
* @param {{networkRecords: !Array<!NetworkRecord>}} tracingData
Copy link
Contributor

Choose a reason for hiding this comment

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

{!{networkRecords: !Array<!NetworkRecord>}}?

Copy link
Member Author

Choose a reason for hiding this comment

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

record types like this are non-nullable, so the ! is technically correct but unnecessary. It would have been really nice if everything had been non-nullable by default from the start :(

@brendankenny
Copy link
Member Author

I think part of this was a symptom of the -1 error system

Yeah, good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants