WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
118548
[CoordinatedGraphics][WK2] Correct wrong usage of m_contentPosition variable in the WebView.
https://bugs.webkit.org/show_bug.cgi?id=118548
Summary
[CoordinatedGraphics][WK2] Correct wrong usage of m_contentPosition variable ...
Eunmi Lee
Reported
2013-07-10 21:34:19 PDT
The m_contentPosition variable is scaled position which is scaled with content scale factor. It can be confused with real position (which is known in the webcore), so it should be used carefully. I fix the wrong usage of m_contentPosition in this bug.
Attachments
Patch
(2.51 KB, patch)
2013-07-10 21:41 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
(540.66 KB, application/zip)
2013-07-11 00:47 PDT
,
Build Bot
no flags
Details
Patch
(2.84 KB, patch)
2013-07-18 01:44 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(2.65 KB, patch)
2013-07-18 04:47 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
(707.03 KB, application/zip)
2013-07-18 06:56 PDT
,
Build Bot
no flags
Details
Patch
(2.72 KB, patch)
2013-07-18 19:28 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(2.60 KB, patch)
2013-10-18 03:17 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Eunmi Lee
Comment 1
2013-07-10 21:41:05 PDT
Created
attachment 206426
[details]
Patch
Build Bot
Comment 2
2013-07-11 00:47:38 PDT
Comment on
attachment 206426
[details]
Patch
Attachment 206426
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/1048524
New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Build Bot
Comment 3
2013-07-11 00:47:40 PDT
Created
attachment 206430
[details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Gyuyoung Kim
Comment 4
2013-07-16 18:53:12 PDT
Comment on
attachment 206426
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206426&action=review
Noam, could you take a look this patch ?
> Source/WebKit2/ChangeLog:3 > + [EFL][WK2] Correct wrong usage of m_contentPosition variable in the WebView.
It looks you need to change [EFL] with [CoordnatiedGraphics].
Noam Rosenthal
Comment 5
2013-07-17 19:09:58 PDT
Comment on
attachment 206426
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206426&action=review
> Source/WebKit2/ChangeLog:10 > + The m_contentPosition variable is scaled position which is scaled with > + content scale factor. It can be confused with real position which is > + known in the webcore, so it should be used carefully.
I don't get what this says, or what this patch does differently.
> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:248 > + FloatPoint position = contentPosition(); > + position.scale(1 / m_contentScaleFactor, 1 / m_contentScaleFactor); > + FloatRect visibleContentsRect(position, visibleContentsSize());
This does not explain itself, nor it is explained in the Changelog.
Eunmi Lee
Comment 6
2013-07-18 00:25:51 PDT
Comment on
attachment 206426
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206426&action=review
>> Source/WebKit2/ChangeLog:3 >> + [EFL][WK2] Correct wrong usage of m_contentPosition variable in the WebView. > > It looks you need to change [EFL] with [CoordnatiedGraphics].
Yes, I will change that.
>> Source/WebKit2/ChangeLog:10 >> + known in the webcore, so it should be used carefully. > > I don't get what this says, or what this patch does differently.
OK, I will re-write description.
>> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:248 >> + FloatRect visibleContentsRect(position, visibleContentsSize()); > > This does not explain itself, nor it is explained in the Changelog.
the contentPosition() returns the scaled position, so we have to divide that with content scale factor before setting to the visibleContentrect. I will add description about this to the changelog.
Eunmi Lee
Comment 7
2013-07-18 01:44:13 PDT
Created
attachment 206965
[details]
Patch Re-write the changelog.
Noam Rosenthal
Comment 8
2013-07-18 02:40:14 PDT
Comment on
attachment 206965
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206965&action=review
> Source/WebKit2/ChangeLog:16 > + > + The CoordinatedGraphics has its own scaling logic which is called as > + contents scaling, so the content position of WebCore should be used > + after scaling with contents scale factor in the CoordinatedGraphics. > + The scaled content position is maintained in the WebView as a > + m_contentPosition, and we have to recognize that is scaled value. > + > + The m_contentPosition is regarded as a non-scaled value in the > + transformToScene() and updateViewportSize(), so I fix them correctly. > +
Correct English please... I hate being grammar police (I'm not a native English speaker myself) but we have to maintain readability standard in the Changelog.
Eunmi Lee
Comment 9
2013-07-18 04:47:50 PDT
Created
attachment 206980
[details]
Patch Re-write changelog.
Build Bot
Comment 10
2013-07-18 06:56:21 PDT
Comment on
attachment 206980
[details]
Patch
Attachment 206980
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/1090892
New failing tests: media/video-zoom.html
Build Bot
Comment 11
2013-07-18 06:56:23 PDT
Created
attachment 206988
[details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Eunmi Lee
Comment 12
2013-07-18 19:28:06 PDT
Created
attachment 207049
[details]
Patch Rebase.
Eunmi Lee
Comment 13
2013-10-18 03:17:53 PDT
Created
attachment 214554
[details]
Patch re-write Changelog.
Praveen Jadhav
Comment 14
2013-11-26 16:51:29 PST
***
Bug 124396
has been marked as a duplicate of this bug. ***
Ryuan Choi
Comment 15
2013-11-28 18:24:41 PST
(In reply to
comment #13
)
> Created an attachment (id=214554) [details] > Patch > > re-write Changelog.
I think that we need this. Are there still issues to block this?
WebKit Commit Bot
Comment 16
2013-11-28 23:48:43 PST
Comment on
attachment 214554
[details]
Patch Clearing flags on attachment: 214554 Committed
r159864
: <
http://trac.webkit.org/changeset/159864
>
WebKit Commit Bot
Comment 17
2013-11-28 23:48:48 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug