Bug 84591 - [Qt] Restore x-position when restoring previous zoom-level
: [Qt] Restore x-position when restoring previous zoom-level
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Allan Sandfeld Jensen
:
Depends on:
Blocks: 84602
  Show dependency treegraph
 
Reported: 2012-04-23 06:17 PDT by Allan Sandfeld Jensen
Modified: 2012-04-24 02:41 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.98 KB, patch)
2012-04-23 06:25 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.18 KB, patch)
2012-04-23 08:48 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2012-04-23 09:14 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2012-04-24 01:12 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-04-23 06:17:02 PDT
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.
Comment 1 Allan Sandfeld Jensen 2012-04-23 06:25:25 PDT
Created attachment 138333 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-04-23 06:51:50 PDT
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?
Comment 3 Allan Sandfeld Jensen 2012-04-23 06:56:44 PDT
(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.
Comment 4 Allan Sandfeld Jensen 2012-04-23 08:48:05 PDT
Created attachment 138355 [details]
Patch
Comment 5 WebKit Review Bot 2012-04-23 08:51:42 PDT
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.
Comment 6 Andras Becsi 2012-04-23 09:04:44 PDT
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.
Comment 7 Allan Sandfeld Jensen 2012-04-23 09:06:19 PDT
(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.
Comment 8 Andras Becsi 2012-04-23 09:14:15 PDT
(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.
Comment 9 Allan Sandfeld Jensen 2012-04-23 09:14:43 PDT
Created attachment 138361 [details]
Patch
Comment 10 Allan Sandfeld Jensen 2012-04-23 09:32:58 PDT
(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.
Comment 11 Andras Becsi 2012-04-23 09:47:36 PDT
(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.
Comment 12 Allan Sandfeld Jensen 2012-04-24 01:12:08 PDT
Created attachment 138512 [details]
Patch
Comment 13 zalan 2012-04-24 01:13:51 PDT
discussed on #irc. the updated(coming) patch looks good. :)
Comment 14 WebKit Review Bot 2012-04-24 02:41:51 PDT
Comment on attachment 138512 [details]
Patch

Clearing flags on attachment: 138512

Committed r115022: <http://trac.webkit.org/changeset/115022>
Comment 15 WebKit Review Bot 2012-04-24 02:41:57 PDT
All reviewed patches have been landed.  Closing bug.