Bug 84591 – [Qt] Restore x-position when restoring previous zoom-level

• Component: WebKit2
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

When zooming out on tap to zoom the previous zoom-level is reestablished, but the previous position is not. 

The X-position needs to be restored to ensure the columns that were visible before zooming-in will be visible again after zooming out.

Y-position does not need to be restored since zooming is fit-to-width, and moving Y-position is an intended feature of double-tap.

[Allan Sandfeld Jensen]

Created attachment 138333 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 138333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138333&action=review

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:96
> +static inline bool fuzzyCompare(qreal a, qreal b, qreal f)

qFuzzyCompare is not good enough?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 138333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138333&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:96
> > +static inline bool fuzzyCompare(qreal a, qreal b, qreal f)
> 
> qFuzzyCompare is not good enough?

I tried it, but I still had cases where a double-tap didn't zoom out, and qFuzzyCompare couldn't take an argument for epsilon. I am kinda of surprised I couldn't find a proper Qt floating-point compare with epsilon argument.

[Allan Sandfeld Jensen]

Created attachment 138355 [details]
Patch

[WebKit Review Bot]

Attachment 138355 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andras Becsi]

Comment on attachment 138355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138355&action=review
> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:99
> +    return std::abs(a - b) < epsilon;

I think you could use qAbs() here, then you wouldn't need cmath.
Looks good otherwise.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> Attachment 138355 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.

[Andras Becsi]

(In reply to comment #7)
> (In reply to comment #5)
> > Attachment 138355 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> > Total errors found: 1 in 3 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.

I think the bot uses C locale for sort (LC_COLLATE=C) which would expect cmath after QWheelEvent.

[Allan Sandfeld Jensen]

Created attachment 138361 [details]
Patch

[Allan Sandfeld Jensen]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Attachment 138355 [details] [details] [details] did not pass style-queue:
> > > 
> > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> > > Total errors found: 1 in 3 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.
> 
> I think the bot uses C locale for sort (LC_COLLATE=C) which would expect cmath after QWheelEvent.

That was where I first placed it. There was no place the style-checker would accept it. Anyway it is gone now, qAbs was a better choice.

[Andras Becsi]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > Attachment 138355 [details] [details] [details] [details] did not pass style-queue:
> > > > 
> > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> > > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
> > > > Total errors found: 1 in 3 files
> > > > 
> > > > 
> > > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > > 
> > > Style-bot is wrong by the way. The includes are sorted with system-includes after other includes.
> > 
> > I think the bot uses C locale for sort (LC_COLLATE=C) which would expect cmath after QWheelEvent.
> 
> That was where I first placed it. There was no place the style-checker would accept it. Anyway it is gone now, qAbs was a better choice.

That's probably because your system has something like en_US.UTF-8 as a locale, 
using LC_COLLATE=C Tools/Scripts/check-webkit-style might give the correct result.

[Allan Sandfeld Jensen]

Created attachment 138512 [details]
Patch

[alan]

discussed on #irc. the updated(coming) patch looks good. :)

[WebKit Review Bot]

Comment on attachment 138512 [details]
Patch

Clearing flags on attachment: 138512

Committed r115022: <http://trac.webkit.org/changeset/115022>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.