Enable zoom session history for synthetic documents

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

Trunk
Firefox 31
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
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.
Attachment #8411731 - Flags: review?(bugmail.mozilla)
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.
Attachment #8411731 - Flags: review?(bugmail.mozilla) → review-
Assignee

Comment 3

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #8411731 - Flags: review- → review+
Assignee

Comment 5

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bbcb7f37b176
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.