Bug 19125 - [Qt] add functions to get/set the zoom level
Summary: [Qt] add functions to get/set the zoom level
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-19 11:15 PDT by Siraj razick
Modified: 2008-08-13 06:43 PDT (History)
3 users (show)

See Also:


Attachments
A Patch that adds functions to get/set the zoom level to QtWebKit (4.08 KB, patch)
2008-05-21 07:02 PDT, Siraj razick
no flags Details | Formatted Diff | Diff
Add functions to get/set the zoom level (3.68 KB, patch)
2008-05-21 10:49 PDT, Siraj razick
hausmann: review-
Details | Formatted Diff | Diff
Add functions to get/set the zoom level V2. (4.11 KB, patch)
2008-06-24 09:42 PDT, Siraj razick
no flags Details | Formatted Diff | Diff
Zoom factor patch v3 (6.47 KB, patch)
2008-07-26 05:56 PDT, Simon Hausmann
zecke: review+
Details | Formatted Diff | Diff
Zoom factor patch v4 (8.47 KB, patch)
2008-08-05 08:28 PDT, Simon Hausmann
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Siraj razick 2008-05-19 11:15:13 PDT
Qt WebKit currently seems to have text only zoom. I'm going to write a patch to expose an API function to zoom all the contents, including text and images.
Comment 1 Siraj razick 2008-05-21 07:02:14 PDT
Created attachment 21267 [details]
A Patch that adds functions to get/set the zoom level to QtWebKit 

A Patch that adds functions to get/set the zoom level to QtWebKit API
Comment 2 Siraj razick 2008-05-21 10:49:26 PDT
Created attachment 21271 [details]
Add functions to get/set the zoom level
Comment 3 Pierre-Luc Beaudoin 2008-05-21 11:28:46 PDT
Comment on attachment 21271 [details]
Add functions to get/set the zoom level

Actually you have to use "review?" first :)
Comment 4 Simon Hausmann 2008-06-06 01:37:37 PDT
Comment on attachment 21271 [details]
Add functions to get/set the zoom level

In principle I agree with the patch, but I see two problems:

1) Documentation is missing for this property.

2) The return value of Frame::zoomFactor() can be the text size multiplier or the zoom factor. I think we should use the newly added pageZoomFactor() and textZoomFactor() getters. But I'm not sure what to do about the two conflicting and symmetry. Either way we have to document the interaction of the two properties or how they exclude each other.

Another option is to add support for both...

So r- because this needs more work, but don't let that discourage you! It's great that you've started on this missing feature :)
Comment 5 Siraj razick 2008-06-12 09:27:42 PDT
(In reply to comment #4) 
Sorry for the delay, I was stuck with some work. I'll re-prepare a patch along the lines you suggested. I really appreciate the review, it's all part of the learning curve.
Comment 6 Siraj razick 2008-06-24 09:42:13 PDT
Created attachment 21911 [details]
Add functions to get/set the zoom level V2. 

Hi,
Sorry for the delay. Here is the patch which adds page zoom and text zoom API properties. I have added the required documentation and also this patch is using   the new pageZoomFactor()/textZoomFactor() functions from the webCore, as suggested by Simon.
Comment 7 Eric Seidel (no email) 2008-07-24 11:57:39 PDT
Comment on attachment 21911 [details]
Add functions to get/set the zoom level V2. 

Looks fine to me.
Comment 8 Simon Hausmann 2008-07-26 05:56:17 PDT
Created attachment 22494 [details]
Zoom factor patch v3

This patch is based on Siraj's patch but proposes a slightly different API. I think zoomFactor is more consistent with the rest of Qt and the boolean zoomTextOnly property gives clarity over the meaning of the zoomFactor.
Comment 9 Mark Rowe (bdash) 2008-07-26 22:34:19 PDT
Comment on attachment 21911 [details]
Add functions to get/set the zoom level V2. 

Clearing review flag since Comment #8 suggests that the  API is still under discussion.
Comment 10 Holger Freyther 2008-08-02 14:27:09 PDT
Comment on attachment 22494 [details]
Zoom factor patch v3

Look's good. What made you pick zoomTextOnly over zoomOnlyText?
Comment 11 Simon Hausmann 2008-08-05 08:28:17 PDT
Created attachment 22655 [details]
Zoom factor patch v4

This is a new revision of the previous patch that maps the implementation and moves zoomTextOnly into QWebSettings.

After a discussion with the doc team we concluded that having 'Only' at the end is more consistent with the rest of Qt, which has for example setReadOnly or setForwardOnly.
Comment 12 Holger Freyther 2008-08-11 18:13:38 PDT
Comment on attachment 22655 [details]
Zoom factor patch v4

- API is okay
- WebCore::Settings change is fine
Comment 13 Simon Hausmann 2008-08-13 06:43:53 PDT
Landed in r35716