Bug 29042 – Custom printing shrink factors

• Component: Printing
• Status: RESOLVED LATER

[Jakob Truelsen]

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.

[Jakob Truelsen]

Created attachment 39199 [details]
Shrink factor patch

[Eric Seidel (no email)]

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.

[Jakob Truelsen]

Created attachment 39246 [details]
Shrink factor patch 1.1

Sorry about that, the braces are now the correct place.

[Jakob Truelsen]

Created attachment 39905 [details]
Shrink factor patch 1.2

[Adam Barth]

Comment on attachment 39905 [details]
Shrink factor patch 1.2

Ok.  This looks reasonable.

[WebKit Commit Bot]

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.

[Jakob Truelsen]

Created attachment 41154 [details]
Shrink factor patch 1.3

Make patch apply to trunk

[Adam Barth]

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+

[WebKit Commit Bot]

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

[Adam Barth]

Comment on attachment 41154 [details]
Shrink factor patch 1.3

Ack Hans!  It's the tabs!

[Jakob Truelsen]

Created attachment 41399 [details]
Patch now ^I free

Sorry about that. Emacs apparently adds tabs on empty lines

[WebKit Commit Bot]

Comment on attachment 41399 [details]
Patch now ^I free

Clearing flags on attachment: 41399

Committed r49769: <http://trac.webkit.org/changeset/49769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

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)

[Benjamin Poulain]

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);

[Jakob Truelsen]

#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..

[Benjamin Poulain]

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.

[Jakob Truelsen]

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?

[Benjamin Poulain]

(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.

[WebKit Commit Bot]

Comment on attachment 43058 [details]
Revert the commit r49769

Clearing flags on attachment: 43058

Committed r50876: <http://trac.webkit.org/changeset/50876>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.