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

Adding case for unhandled page events #780

Closed
wants to merge 2 commits into from

Conversation

diogogmt
Copy link
Contributor

@diogogmt diogogmt commented Mar 27, 2021

Adding a case for the EventScreencastVisibilityChanged and EventScreencastFrame page events to supress the unhandled page event %T error log statements.

cc @kenshaw @ZekeLu

@ZekeLu
Copy link
Member

ZekeLu commented Mar 28, 2021

LGTM 👍

I want to take the chance to list all the page events which can be ignored and do a little refactoring.

	// ignored events
	case
		*page.EventCompilationCacheProduced,
		*page.EventDocumentOpened,
		*page.EventDomContentEventFired,
		*page.EventDownloadProgress,
		*page.EventDownloadWillBegin,
		*page.EventFileChooserOpened,
		*page.EventFrameRequestedNavigation,
		*page.EventFrameResized,
		*page.EventInterstitialHidden,
		*page.EventInterstitialShown,
		*page.EventJavascriptDialogClosed,
		*page.EventJavascriptDialogOpening,
		*page.EventLifecycleEvent,
		*page.EventLoadEventFired,
		*page.EventNavigatedWithinDocument,
		*page.EventScreencastFrame,
		*page.EventScreencastVisibilityChanged,
		*page.EventWindowOpen:
		return

The changes are:

  1. use the comma expression to list all the ignored events in a single case branch
  2. sort the events
  3. add these extra events to ignore:
    • page.EventCompilationCacheProduced
    • page.EventFileChooserOpened
    • page.EventInterstitialHidden
    • page.EventInterstitialShown

@diogogmt Can you help to add the changes to your PR? Please note that I'm not sure whether it's okay to do such changes. Let's wait for @kenshaw 's final review.

@kenshaw
Copy link
Member

kenshaw commented Mar 28, 2021

Yes, I'm OK with these kinds of changes. I don't think it really modifies any behavior though, so I'm not sure what the point would be.

@ZekeLu
Copy link
Member

ZekeLu commented Mar 28, 2021

I think the purpose of t.errf("unhandled page event %T", ev) is to catch neglected events which are generated into https://github.com/chromedp/cdproto automatically. If we don't add the known events to the ignored list, we will get some false alarms.

@diogogmt
Copy link
Contributor Author

I think the purpose of t.errf("unhandled page event %T", ev) is to catch neglected events which are generated into https://github.com/chromedp/cdproto automatically. If we don't add the known events to the ignored list, we will get some false alarms.

Exactly, depending on the set of tasks executed in the chrome session it can get very noisy with the t.errf("unhandled page event %T", ev) logs and make it hard to spot legit error messages.

@kenshaw
Copy link
Member

kenshaw commented Apr 27, 2021

Merged.

@kenshaw kenshaw closed this Apr 27, 2021
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

3 participants