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

add url blocking function to driver #1195

Merged
merged 3 commits into from
Jan 4, 2017

Conversation

weiwei-lin
Copy link
Contributor

Added request blocking method in Driver and it will be called during GatherRunner.setupDriver().
Blocked URLs will be passed in flags.blockedUrlPatterns. The flag is not exposed to users since copy-pasting a bunch of URLs seems inconvenient in cli. It will mainly been used by Perf-X server #1143.

@weiwei-lin weiwei-lin requested review from brendankenny, paulirish and samuelli and removed request for brendankenny, paulirish and samuelli December 21, 2016 06:07
@paulirish
Copy link
Member

Just to clarify these blocked patterns will apply to ALL passes in the config, right?

Given that, the blocking will stop when we disconnect from the protocol. So it appears we don't have to clean up with Network.removeBlockedURL or anything. sweet! 🍭

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.

This feels great.

Excited to see the next PR where we hook it up.

@@ -701,6 +701,14 @@ class Driver {

return collectUsage;
}

blockUrlPatterns(urlPatterns) {
const promiseArr = [];
Copy link
Member

Choose a reason for hiding this comment

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

could simplify to

    const promiseArr = urlPatterns.map(url => this.sendCommand('Network.addBlockedURL', {url}));
    return Promise.all(promiseArr);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Already changed.

@@ -96,7 +96,8 @@ class GatherRunner {
.then(_ => driver.enableRuntimeEvents())
.then(_ => driver.evaluateScriptOnLoad('window.__nativePromise = Promise;'))
.then(_ => driver.cleanAndDisableBrowserCaches())
.then(_ => driver.clearDataForOrigin(options.url));
.then(_ => driver.clearDataForOrigin(options.url))
.then(_ => driver.blockUrlPatterns(options.flags.blockedUrlPatterns || []));
Copy link
Member

Choose a reason for hiding this comment

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

I was mulling over if blockedUrlPatterns should live right on options rather than options.flags, but it'd probably require a line of plumbing in index.js that I don't love.

So yeah, I'm also fine with it hanging out on options.flags even if its not user-exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. As you mentioned, the main reason I did this is because I don't want to add an extra (and optional) parameter in lighthouse() and runLighthouse(), which looks ugly.

@weiwei-lin
Copy link
Contributor Author

Just to clarify these blocked patterns will apply to ALL passes in the config, right?

Yes. Actually, the blocking patterns are (mainly) to be used by perfX server when rerunning lighthouse. And we probably will use perf config or something similar when we rerun because we only care about the performance difference. So although it applies to all passes in one run, it won't affect passes defined by user (or default configs) since they are used in a separate run.

@paulirish paulirish merged commit 811c219 into GoogleChrome:master Jan 4, 2017
@paulirish
Copy link
Member

boom. level 🆙

@weiwei-lin weiwei-lin deleted the request-blocking branch January 4, 2017 22:39
andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
* add url blocking function to driver
* update fake driver to match new driver
* simplified Driver.blockUrlPatterns
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.

2 participants