Skip to content

Get zoom level from the browser host instead #2022

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

Closed
wants to merge 2 commits into from
Closed

Get zoom level from the browser host instead #2022

wants to merge 2 commits into from

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Apr 14, 2017

Resolves #1778

  • Get zoom level from the browser host instead of ZoomLevelProperty

@merceyz merceyz added the wpf WPF Implementation label Apr 14, 2017
@amaitland
Copy link
Member

Is it safe to Wait for the task to execute? I really dislike Wait, I've seen far too many deadlocks

@merceyz
Copy link
Member Author

merceyz commented Apr 14, 2017

In my tests it didn't deadlock, it can of course happen.

In my application (Offscreen processes doing work) I use Wait on JS execution and haven't seen a deadlock yet

EDIT:
I could make CefBrowserHostWrapper::GetZoomLevelOnUI() public, then the task wouldn't be needed

@amaitland
Copy link
Member

I'm still not keen on this. It's also we would be blocking the UI thread.

@merceyz
Copy link
Member Author

merceyz commented Apr 14, 2017

Another solution is to have the zoom commands send the new zoom value to all other browser instances on the same domain.

Though that wouldn't work if you were to open the page after the initial zoom as it would show 0 no matter what the value is

@amaitland
Copy link
Member

Is a fix actually required? The simplest workaround is to isolate each browser and let them zoom independently.

@merceyz
Copy link
Member Author

merceyz commented Apr 14, 2017

If one wants the domain shared zoom to work properly yes, if not, no.

we would be blocking the UI thread.

Waiting for GetZoomLevelAsync to finish took 00:00:00.000473

@amaitland
Copy link
Member

How about providing an example of your idea as an attached property? Get some real world feedback. There is nothing stopping a user from implementing their own solution.

@merceyz
Copy link
Member Author

merceyz commented Apr 20, 2017

How about providing an example of your idea as an attached property?

I'm not quite sure what you mean, could you attempt to clarify?

@amaitland
Copy link
Member

Using this in conjunction with my proposed fix for #1915 causes a deadlock. The simplest fix may well be updating ZoomLevelProperty when the browser becomes visible. Something like:

if(isVisible)
{
	browser.GetHost().GetZoomLevelAsync().ContinueWith(previous =>
	{
		if (!IsDisposed)
		{
			SetCurrentValue(ZoomLevelProperty, previous.Result);
		}
	}, TaskContinuationOptions.OnlyOnRanToCompletion);
}

The ZoomReset issue still needs to be addressed, changing ZoomLevelProperty from using the property changed to the CoerceValueCallback is probably the easiest fix (always called even if the values are the same).

EDIT:
I could make CefBrowserHostWrapper::GetZoomLevelOnUI() public, then the task wouldn't be needed

Adding IBrowserHost.GetZoomLevel is fine with me. I think the GetZoomLevelOnUI method should be private with a new public method being added that includes the standard disposed checks. You would only be able to use that version when MultiThreadedMessageLoop = false though.

@merceyz
Copy link
Member Author

merceyz commented Mar 19, 2018

I don't have any plans to do anything with this PR, so i'm closing this.

@merceyz merceyz closed this Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-ready-for-merge wpf WPF Implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants