Bug 29042 - Custom printing shrink factors
Summary: Custom printing shrink factors
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 30755
  Show dependency treegraph
 
Reported: 2009-09-08 11:44 PDT by Jakob Truelsen
Modified: 2010-07-12 01:55 PDT (History)
5 users (show)

See Also:


Attachments
Shrink factor patch (10.41 KB, patch)
2009-09-08 11:54 PDT, Jakob Truelsen
no flags Details | Formatted Diff | Diff
Shrink factor patch 1.1 (10.41 KB, patch)
2009-09-08 22:34 PDT, Jakob Truelsen
no flags Details | Formatted Diff | Diff
Shrink factor patch 1.2 (10.33 KB, patch)
2009-09-22 00:04 PDT, Jakob Truelsen
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Shrink factor patch 1.3 (10.34 KB, patch)
2009-10-14 02:08 PDT, Jakob Truelsen
abarth: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch now ^I free (10.84 KB, patch)
2009-10-18 23:56 PDT, Jakob Truelsen
no flags Details | Formatted Diff | Diff
Revert the commit r49769 (9.74 KB, patch)
2009-11-12 05:07 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakob Truelsen 2009-09-08 11:44:37 PDT
Currently webkit shrinks pages with a factor of 1.25 to 2.0. 
While this is fine in most cases, it might not be the best choice in some cases.

For for instance one might want to clamp these two values to a constant to get a predicable dpi on the printed document.

The following patch allows one to configure these two factors.
Comment 1 Jakob Truelsen 2009-09-08 11:54:31 PDT
Created attachment 39199 [details]
Shrink factor patch
Comment 2 Eric Seidel (no email) 2009-09-08 12:45:57 PDT
Comment on attachment 39199 [details]
Shrink factor patch

Dive-by nit:
 502 void Settings::setPrintingMaximumShrinkFactor(float printingMaximumShrinkFactor) {
 503     m_printingMaximumShrinkFactor = printingMaximumShrinkFactor;
Style issues { goes on a new line.
Comment 3 Jakob Truelsen 2009-09-08 22:34:30 PDT
Created attachment 39246 [details]
Shrink factor patch 1.1

Sorry about that, the braces are now the correct place.
Comment 4 Jakob Truelsen 2009-09-22 00:04:46 PDT
Created attachment 39905 [details]
Shrink factor patch 1.2
Comment 5 Adam Barth 2009-10-13 14:41:14 PDT
Comment on attachment 39905 [details]
Shrink factor patch 1.2

Ok.  This looks reasonable.
Comment 6 WebKit Commit Bot 2009-10-13 14:45:07 PDT
Comment on attachment 39905 [details]
Shrink factor patch 1.2

Rejecting patch 39905 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39905 from bug 29042 failed to download and apply.
Comment 7 Jakob Truelsen 2009-10-14 02:08:43 PDT
Created attachment 41154 [details]
Shrink factor patch 1.3

Make patch apply to trunk
Comment 8 Adam Barth 2009-10-15 16:41:06 PDT
Comment on attachment 41154 [details]
Shrink factor patch 1.3

Ok.  I don't have time to watch the bots right now, but someone else can mark this cq+
Comment 9 WebKit Commit Bot 2009-10-18 23:34:10 PDT
Comment on attachment 41154 [details]
Shrink factor patch 1.3

Rejecting patch 41154 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
t/qt/ChangeLog
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/WebCore/ChangeLog
        trunk/WebCore/page/Settings.h

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 10 Adam Barth 2009-10-18 23:35:28 PDT
Comment on attachment 41154 [details]
Shrink factor patch 1.3

Ack Hans!  It's the tabs!
Comment 11 Jakob Truelsen 2009-10-18 23:56:26 PDT
Created attachment 41399 [details]
Patch now ^I free

Sorry about that. Emacs apparently adds tabs on empty lines
Comment 12 WebKit Commit Bot 2009-10-19 00:17:49 PDT
Comment on attachment 41399 [details]
Patch now ^I free

Clearing flags on attachment: 41399

Committed r49769: <http://trac.webkit.org/changeset/49769>
Comment 13 WebKit Commit Bot 2009-10-19 00:17:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Simon Hausmann 2009-11-11 12:55:55 PST
This patch has API issues. The use of float in the API has to be replaced with the use of qreal and the documentation tags are incorrect, they should refer to \since 4.6 instead of 4.7.

Re-opening this bug (Hm, I can't use REOPEN, but I'll set it to UNCONFIRMED instead)
Comment 15 Benjamin Poulain 2009-11-12 00:51:24 PST
Note that the patch is not correct.
If QWebSettings is initialized from WebCore::Settings, QWebSettingsPrivate::printingMinimumShrinkFactor and QWebSettingsPrivate::printingMaximumShrinkFactor are not initialized.

When QWebSettingsPrivate::apply() is called, the following code has an undefined behavior: 
        float minimumShrinkFactor = printingMinimumShrinkFactor > 0.0f ? printingMinimumShrinkFactor : global->printingMinimumShrinkFactor;
        settings->setPrintingMinimumShrinkFactor(minimumShrinkFactor);

        float maximumShrinkFactor = printingMaximumShrinkFactor > 0.0f ? printingMaximumShrinkFactor : global->printingMaximumShrinkFactor;
        settings->setPrintingMaximumShrinkFactor(maximumShrinkFactor);
Comment 16 Jakob Truelsen 2009-11-12 01:25:14 PST
#15 is not news to me. I submitted a patch for that here
https://bugs.webkit.org/show_bug.cgi?id=30755

As for \since 4.7, I was under the impression that 4.6 had gone into feature freeze. I will submit a new patch to 30755 ASAP..
Comment 17 Benjamin Poulain 2009-11-12 05:07:55 PST
Created attachment 43058 [details]
Revert the commit r49769

Revert the original patch.

This API should not be part of Qt 4.6.
This could be submitted for Qt 4.7, but the public API should be reviewed by someone from Qt.
Comment 18 Jakob Truelsen 2009-11-12 05:22:06 PST
I see you just proposed reverting my original patch instead of fixing the issue. 
Is QT pulling WebKit trunk into the QT 4.6 tree, if yes is there real WebKit trunk I can submit patches against, or is it only possible to submit QT related patches into WebKit at time-windows according with the QT release schedule.  I have tried many different strategies for getting patches such as this in, submitting to the QT bug-tracker, QT Gitorius and so on but all of them state that I should submit here.  Now you seem to claim that one needs some sort of QT rewiew, how would that happen.  I'm beginning to wonder if it is at all possible to get a QT related patch into WebKit?
Comment 19 Benjamin Poulain 2009-11-12 05:35:12 PST
(In reply to comment #18)
> I see you just proposed reverting my original patch instead of fixing the
> issue. 
> Is QT pulling WebKit trunk into the QT 4.6 tree, if yes is there real WebKit
> trunk I can submit patches against, or is it only possible to submit QT related
> patches into WebKit at time-windows according with the QT release schedule.  I
> have tried many different strategies for getting patches such as this in,
> submitting to the QT bug-tracker, QT Gitorius and so on but all of them state
> that I should submit here.  Now you seem to claim that one needs some sort of
> QT rewiew, how would that happen.  I'm beginning to wonder if it is at all
> possible to get a QT related patch into WebKit?

You have done the right workflow, it is correct to submit the patches here for QtWebkit.
We have failed on this patch. Nobody from Qt did notice the patch and it has been merged without any API review (necessary for public APIs).

I suggest to revert the patch here instead of the 4.6 branch in order to have a proper API review before it's inclusion for 4.7.

I personally think we could introduce a new class for printing in order to add more flexibility in the future. E.g.:
void QWebFrame::print (QPrinter* printer, const QWebPrintSettings& settings)

This would have the advantage of giving the right visibility to the parameters (next to ::print, instead of hidden in the settings). And this would be more future proof in case of new parameters (zoomFactor, and CSS media type for example). This is just my point of view, that need to be discussed with the others.
Comment 20 WebKit Commit Bot 2009-11-12 05:52:57 PST
Comment on attachment 43058 [details]
Revert the commit r49769

Clearing flags on attachment: 43058

Committed r50876: <http://trac.webkit.org/changeset/50876>
Comment 21 WebKit Commit Bot 2009-11-12 05:53:04 PST
All reviewed patches have been landed.  Closing bug.