Bug 184344 - WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll is directly accessing NSScreen
Summary: WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll is direct...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 184322
  Show dependency treegraph
 
Reported: 2018-04-05 16:52 PDT by Brent Fulgham
Modified: 2018-04-09 16:46 PDT (History)
7 users (show)

See Also:


Attachments
Patch (17.17 KB, patch)
2018-04-06 17:14 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.52 MB, application/zip)
2018-04-06 18:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.63 MB, application/zip)
2018-04-06 19:11 PDT, Build Bot
no flags Details
Patch (11.62 KB, patch)
2018-04-07 12:37 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (11.58 KB, patch)
2018-04-09 11:46 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (17.55 KB, patch)
2018-04-09 12:13 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (18.20 KB, patch)
2018-04-09 12:48 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (18.19 KB, patch)
2018-04-09 13:41 PDT, Brent Fulgham
pvollan: review+
bfulgham: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2018-04-05 16:52:19 PDT
The WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll method is retrieving NSScreen information directly. These calls should be brokered.
Comment 1 Radar WebKit Bug Importer 2018-04-05 16:52:40 PDT
<rdar://problem/39224969>
Comment 2 Brent Fulgham 2018-04-06 17:14:50 PDT
Created attachment 337403 [details]
Patch
Comment 3 Build Bot 2018-04-06 18:46:32 PDT
Comment on attachment 337403 [details]
Patch

Attachment 337403 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7231943

New failing tests:
fast/events/autoscroll-with-non-scrollable-parent.html
fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html
fast/events/no-scroll-on-input-text-selection.html
fast/forms/select-multiple-elements-with-mouse-drag.html
fast/events/autoscroll-in-iframe.html
fast/forms/input-readonly-autoscroll.html
fast/events/autoscroll-in-textfield.html
fast/events/autoscroll-should-not-stop-on-keypress.html
fast/forms/select/listbox-disabled-no-autoscroll.html
fast/events/autoscroll-in-iframe-body.html
Comment 4 Build Bot 2018-04-06 18:46:33 PDT
Created attachment 337408 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Build Bot 2018-04-06 19:11:55 PDT
Comment on attachment 337403 [details]
Patch

Attachment 337403 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7232000

New failing tests:
fast/events/autoscroll-with-non-scrollable-parent.html
fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html
fast/events/no-scroll-on-input-text-selection.html
fast/forms/select-multiple-elements-with-mouse-drag.html
fast/events/autoscroll-in-iframe.html
fast/forms/input-readonly-autoscroll.html
fast/events/autoscroll-in-textfield.html
fast/events/autoscroll-should-not-stop-on-keypress.html
fast/forms/select/listbox-disabled-no-autoscroll.html
fast/events/autoscroll-in-iframe-body.html
Comment 6 Build Bot 2018-04-06 19:11:56 PDT
Created attachment 337409 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 7 Brent Fulgham 2018-04-07 12:37:37 PDT
Created attachment 337430 [details]
Patch
Comment 8 Brent Fulgham 2018-04-09 11:46:04 PDT
Created attachment 337515 [details]
Patch
Comment 9 Brent Fulgham 2018-04-09 12:13:07 PDT
Created attachment 337521 [details]
Patch
Comment 10 Per Arne Vollan 2018-04-09 12:20:03 PDT
Comment on attachment 337515 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:1169
> +    auto frame = toUserSpaceForPrimaryScreen(screenProperties(page->chrome().displayID()).screenRect);

Will the screen properties map be empty on WK1?
Comment 11 Brent Fulgham 2018-04-09 12:30:28 PDT
(In reply to Per Arne Vollan from comment #10)
> Comment on attachment 337515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337515&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:1169
> > +    auto frame = toUserSpaceForPrimaryScreen(screenProperties(page->chrome().displayID()).screenRect);
> 
> Will the screen properties map be empty on WK1?

Oh, you are right. This will crash in WebKitLegacy. :-(
Comment 12 Brent Fulgham 2018-04-09 12:48:51 PDT
Created attachment 337529 [details]
Patch
Comment 13 Brent Fulgham 2018-04-09 13:41:48 PDT
Created attachment 337535 [details]
Patch
Comment 14 Per Arne Vollan 2018-04-09 13:47:34 PDT
Comment on attachment 337535 [details]
Patch

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

> Source/WebCore/page/mac/EventHandlerMac.mm:-1167
> -    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);

It seems the original transformation will be incorrect if the window is not on the first screen, since the NSWindow parameter is nil here. Should we also address that in this patch?
Comment 15 Brent Fulgham 2018-04-09 15:41:39 PDT
(In reply to Per Arne Vollan from comment #14)
> Comment on attachment 337535 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337535&action=review
> 
> > Source/WebCore/page/mac/EventHandlerMac.mm:-1167
> > -    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);
> 
> It seems the original transformation will be incorrect if the window is not
> on the first screen, since the NSWindow parameter is nil here. Should we
> also address that in this patch?

Which NSWindow would we be passing as the second argument? If you know the right answer one to use, I’m in favor of fixing it now.
Comment 16 Brent Fulgham 2018-04-09 15:44:42 PDT
Comment on attachment 337535 [details]
Patch

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

>>> Source/WebCore/page/mac/EventHandlerMac.mm:-1167
>>> -    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);
>> 
>> It seems the original transformation will be incorrect if the window is not on the first screen, since the NSWindow parameter is nil here. Should we also address that in this patch?
> 
> Which NSWindow would we be passing as the second argument? If you know the right answer one to use, I’m in favor of fixing it now.

Actually, I think it is correct as-is. A nil window was used by the other eiginslncode to compute coordinates with respect to the primary display (I.e., not “no display”). I think that is the correct transformation.
Comment 17 Per Arne Vollan 2018-04-09 16:21:58 PDT
Comment on attachment 337535 [details]
Patch

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

Very nice! r=me.

> Source/WebKit/WebProcess/WebProcess.cpp:138
> +#if PLATFORM(MAC)
> +#include <WebCore/PlatformScreen.h>
> +#endif

This include might not be needed.
Comment 18 Brent Fulgham 2018-04-09 16:30:12 PDT
Comment on attachment 337535 [details]
Patch

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

>> Source/WebKit/WebProcess/WebProcess.cpp:138
>> +#endif
> 
> This include might not be needed.

You are right. I'll remove it.
Comment 19 Brent Fulgham 2018-04-09 16:34:19 PDT
Committed r230454: <https://trac.webkit.org/changeset/230454>