Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

Adding maxTimeout setting and onTimeoutError for webpage #414

Closed
wants to merge 2 commits into from
Closed

Adding maxTimeout setting and onTimeoutError for webpage #414

wants to merge 2 commits into from

Conversation

Tomtomgo
Copy link
Contributor

When requesting a page using page.open() there is no way of setting a maximum timeout.

I added a setting so you can use page.settings.maxTimeout = integer ms. If the request times out it is aborted. The event can be caught using page.onTimeoutError.

This was discussed here: https://code.google.com/p/phantomjs/issues/detail?id=1129

@JamesMGreene
Copy link
Collaborator

I'd like to see a few changes, personally:

  1. Change the NetworkTimeout constructor (or add a setter method) such that it has access to the data object that has already been put together rather than constructing its own later. The big gain by modifying that is that the onTimeoutError callback will then receive data containing — amongst other properties missing from the later-constructed data object — the request's ID. This makes it much easier to match it up if you are cataloging the requests (e.g. for HARs).
  2. Change the name of page.onTimeoutError to page.onResourceTimeout. Although I wonder if perhaps this new handler shouldn't even been added at all but rather delegate to the existing page.onResourceError handler instead (and just specifying that the status is QNetworkReply::TimeoutError). @vitallium @ariya, thoughts?
  3. page.settings.maxTimeout makes me (personally) think that the timeout is for the page itself but not also for each sub-resource request (which is actually is). So, I would suggest changing the name of page.settings.maxTimeout to, well, something else (e.g. .resourceTimeout, .requestTimeout, etc.). Any prior art in other browsers to suggest a good name here?

@Tomtomgo
Copy link
Contributor Author

I will wait a bit to see if @ariya and @vitallium have some comments, then I'll adapt it!

@JamesMGreene
Copy link
Collaborator

@Tomtomgo: Sounds fine to me. Question for you: although your original goal was just to introduce a timeout for the page request, you have effectively introduced a timeout for all resources (which is, IMHO, much more useful). Does that still help you achieve your goal?

@ariya
Copy link
Owner

ariya commented Mar 18, 2013

Some suggestions for renaming:

  • maxTimeout -> timeout (as it already implies maximum)
  • NetworkTimeout ->TimeoutTimer

@Tomtomgo
Copy link
Contributor Author

Thanks for the comments!

The onTimeoutError event is now onResourceTimeout and it has a copy of the data available and I changed maxTimeout to resourceTimeout. I also renamed NetworkTimeout to TimeoutTimer.

@JamesMGreene to me it doesn't matter that this solution is per resource instead of per page.

@ariya
Copy link
Owner

ariya commented Mar 20, 2013

Landed as one commit with some tweaks (don't change indentation style, don't add superfluous spaces and commented code). Thanks!

@ariya ariya closed this Mar 20, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants