1000828 - Enable zoom session history for synthetic documents

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Eugen Sawin [:esawin]]

Some synthetic documents don't allow zooming, but restoring the zoom level for documents that do allow it, like images, should provide a more consistent user experience.

I'm keeping this separate from bug 611556, should be decide to handle synthetic documents differently in this regard.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: Enable zoom session history for synthetic documents

This should do it, we simply have to prevent re-fitting of the zoom level for synthetic documents when a restored session zoom is available.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8411731 [details] [diff] [review]
Enable zoom session history for synthetic documents

Review of attachment 8411731 [details] [diff] [review]:
-----------------------------------------------------------------

The call to sendViewportUpdate that happens inside this block still needs to run, I think. It would be better to have nested if statements, with the restoredSessionZoom() guarding the fitZoom calculation and setResolution call only.

Comment 3

[Eugen Sawin [:esawin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Comment on attachment 8411731 [details] [diff] [review]
> Enable zoom session history for synthetic documents
> 
> Review of attachment 8411731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The call to sendViewportUpdate that happens inside this block still needs to
> run, I think. It would be better to have nested if statements, with the
> restoredSessionZoom() guarding the fitZoom calculation and setResolution
> call only.

We have the following function call chain: |ViewportHandler.updateMetadata -> updateViewportMetadata -> updateViewportSize -> sendViewportUpdate| before the synthetic document check. So I think that it would be redundant to call |sendViewportUpdate| again without actually changing the resolution.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8411731 [details] [diff] [review]
Enable zoom session history for synthetic documents

Review of attachment 8411731 [details] [diff] [review]:
-----------------------------------------------------------------

You may have a point. I'm still a little uneasy but maybe this is ok. This code tends to fail in unexpected ways so keep an eye out for regressions after this lands.

Comment 5

[Eugen Sawin [:esawin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8411731 [details] [diff] [review]
> Enable zoom session history for synthetic documents
> 
> Review of attachment 8411731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You may have a point. I'm still a little uneasy but maybe this is ok. This
> code tends to fail in unexpected ways so keep an eye out for regressions
> after this lands.

Right, I was a bit uneasy with having this in the main patch, but catching regressions on this one should be easy with its limited scope.

Also, given that we know the proper resolution before drawing when restoring from history, I think we could refactor some things out to reduce the number of viewport permutation for that case.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bbcb7f37b176

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbcb7f37b176