Closed Bug 1000828 Opened 10 years ago Closed 10 years ago

Enable zoom session history for synthetic documents

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(1 file)

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.
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-
(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+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bbcb7f37b176
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.