Bug 152241 - [iOS] REGRESSION (r191849): There's no yellow bouncy highlight when using Find on Page
Summary: [iOS] REGRESSION (r191849): There's no yellow bouncy highlight when using Fin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-14 00:52 PST by Tim Horton
Modified: 2016-01-08 11:50 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.11 KB, patch)
2015-12-14 00:53 PST, Tim Horton
no flags Details | Formatted Diff | Diff
New patch with two tests (33.25 KB, patch)
2015-12-14 20:53 PST, Tim Horton
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.01 MB, application/zip)
2015-12-14 21:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (802.60 KB, application/zip)
2015-12-14 21:50 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (946.69 KB, application/zip)
2015-12-14 21:51 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-12-14 00:52:45 PST
[iOS] REGRESSION (r191849): There's no yellow bouncy highlight when using Find on Page
Comment 1 Tim Horton 2015-12-14 00:53:06 PST
Created attachment 267284 [details]
Patch
Comment 2 WebKit Commit Bot 2015-12-14 00:53:52 PST
Attachment 267284 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tim Horton 2015-12-14 00:54:06 PST
Going to figure out how to make a test, because this is fairly fundamental functionality that was broken for months with no tests catching it.
Comment 4 Andreas Kling 2015-12-14 08:43:34 PST
Comment on attachment 267284 [details]
Patch

Oh damn, sorry about that.
r=me!
Comment 5 Darin Adler 2015-12-14 08:51:48 PST
Comment on attachment 267284 [details]
Patch

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

> Source/WebCore/page/PageOverlay.cpp:78
> +        return IntRect(IntPoint(), m_overrideFrame.size());

I like to write it like this:

    return { { }, m_overrideFrame.size() };

Maybe less clear, but I like how it’s more terse and the type is implicit and so it’s guaranteed no extra type conversion happens.
Comment 6 Tim Horton 2015-12-14 20:53:30 PST
Created attachment 267339 [details]
New patch with two tests
Comment 7 Simon Fraser (smfr) 2015-12-14 21:07:59 PST
Comment on attachment 267339 [details]
New patch with two tests

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

> Source/WebCore/ChangeLog:15
> +        if we frame is manually overriden.

if we frame?

> Source/WebCore/ChangeLog:47
> +        * testing/MockPageOverlay.cpp: Added.
> +        (WebCore::MockPageOverlay::create):
> +        (WebCore::MockPageOverlay::MockPageOverlay):
> +        (WebCore::MockPageOverlay::setFrame):
> +        * testing/MockPageOverlay.h: Added.
> +        (WebCore::MockPageOverlay::overlay):
> +        * testing/MockPageOverlay.idl: Added.
> +        * testing/MockPageOverlayClient.cpp:
> +        (WebCore::MockPageOverlayClient::installOverlay):
> +        (WebCore::MockPageOverlayClient::uninstallAllOverlays):
> +        (WebCore::MockPageOverlayClient::pageOverlayDestroyed):
> +        (WebCore::MockPageOverlayClient::drawRect):
> +        (WebCore::MockPageOverlayClient::mouseEvent):
> +        * testing/MockPageOverlayClient.h:
> +        Make internals.installMockPageOverlay return a MockPageOverlay object so
> +        tests can manipulate their overlay. For now, expose setFrame.
> +        Also, log when MockPageOverlayClient gets asked to paint or receives a mouse event,
> +        which will show up in test output. Slightly unconventional, but very convenient.

I'm close to saying that you've gone too far here. Does all this code ship?

> Source/WebCore/page/PageOverlay.cpp:190
> +    mousePositionInOverlayCoordinates.moveBy(-frame().location());

Doesn't the overlay have a "contentsToOverlay" or something that should take care of this?
Comment 8 Build Bot 2015-12-14 21:48:38 PST
Comment on attachment 267339 [details]
New patch with two tests

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

New failing tests:
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-small-frame-mouse-events.html
pageoverlay/overlay-large-document.html
Comment 9 Build Bot 2015-12-14 21:48:41 PST
Created attachment 267345 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2015-12-14 21:50:39 PST
Comment on attachment 267339 [details]
New patch with two tests

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

New failing tests:
pageoverlay/overlay-large-document.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-small-frame-mouse-events.html
Comment 11 Build Bot 2015-12-14 21:50:44 PST
Created attachment 267346 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 12 Build Bot 2015-12-14 21:51:49 PST
Comment on attachment 267339 [details]
New patch with two tests

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

New failing tests:
pageoverlay/overlay-large-document.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-small-frame-mouse-events.html
Comment 13 Build Bot 2015-12-14 21:51:53 PST
Created attachment 267347 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 14 Tim Horton 2015-12-14 22:13:05 PST
> > Source/WebCore/ChangeLog:47
> > +        * testing/MockPageOverlay.cpp: Added.
> > +        (WebCore::MockPageOverlay::create):
> > +        (WebCore::MockPageOverlay::MockPageOverlay):
> > +        (WebCore::MockPageOverlay::setFrame):
> > +        * testing/MockPageOverlay.h: Added.
> > +        (WebCore::MockPageOverlay::overlay):
> > +        * testing/MockPageOverlay.idl: Added.
> > +        * testing/MockPageOverlayClient.cpp:
> > +        (WebCore::MockPageOverlayClient::installOverlay):
> > +        (WebCore::MockPageOverlayClient::uninstallAllOverlays):
> > +        (WebCore::MockPageOverlayClient::pageOverlayDestroyed):
> > +        (WebCore::MockPageOverlayClient::drawRect):
> > +        (WebCore::MockPageOverlayClient::mouseEvent):
> > +        * testing/MockPageOverlayClient.h:
> > +        Make internals.installMockPageOverlay return a MockPageOverlay object so
> > +        tests can manipulate their overlay. For now, expose setFrame.
> > +        Also, log when MockPageOverlayClient gets asked to paint or receives a mouse event,
> > +        which will show up in test output. Slightly unconventional, but very convenient.
> 
> I'm close to saying that you've gone too far here.

I'd say we're pretty far past too far.

> Does all this code ship?

No, it's built into WebCoreTestSupport!
Comment 15 Tim Horton 2015-12-14 22:13:52 PST
Somehow I forgot the rebaselines.
Comment 16 Tim Horton 2015-12-15 14:46:54 PST
http://trac.webkit.org/changeset/194117
Comment 17 Tim Horton 2015-12-15 14:55:14 PST
Cmake build fix in http://trac.webkit.org/changeset/194118
Comment 18 Michael Catanzaro 2016-01-08 11:50:09 PST
Loren discovered the following tests have been failing for GTK since r194117:

pageoverlay/overlay-installation.html [ Failure ]
pageoverlay/overlay-large-document-scrolled.html [ Failure ]
pageoverlay/overlay-large-document.html [ Failure ]
pageoverlay/overlay-small-frame-mouse-events.html [ Failure ]
pageoverlay/overlay-small-frame-paints.html [ Failure ]

The test are not showing the 'MockPageOverlayClient::drawRect dirtyRect()' logs introduced on the expectations.

He filed bug #152908.