Bug 112505 – [wk2] Should support multiple page overlays, like the API suggests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

WebKit2 allows the injected bundle to install page overlays, and also installs some itself. The API is structured in such a way that it makes sense to be able to install multiple page overlays, but that isn't currently supported by WebPage/DrawingArea/etc.

You can also get yourself in a sticky situation with the find-in-page overlay and the inspector overlay, by using find-in-page and then invoking the inspector and making the inspector overlay appear. With TiledCoreAnimationDrawingArea, the find-in-page overlay becomes a zombie.

<rdar://problem/13424796>

[Tim Horton]

Created attachment 193447 [details]
preliminary patch to see how bad the damage is on other platforms

[Early Warning System Bot]

Comment on attachment 193447 [details]
preliminary patch to see how bad the damage is on other platforms

Attachment 193447 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17125376

[EFL EWS Bot]

Comment on attachment 193447 [details]
preliminary patch to see how bad the damage is on other platforms

Attachment 193447 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17233105

[Tim Horton]

Created attachment 193453 [details]
futzing with coordinatedgraphics

[Tim Horton]

I'm not going to implement it for EFL/Qt but I'll at least keep it building and behaving close to how it currently does.

[Tim Horton]

Created attachment 193457 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 193457 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:732
> +            Vector<RefPtr<PageOverlay> >& pageOverlays = m_webPage->pageOverlays();
> +            Vector<RefPtr<PageOverlay> >::iterator end = pageOverlays.end();
> +            for (Vector<RefPtr<PageOverlay> >::iterator it = pageOverlays.begin(); it != end; ++it)

A typedef for Vector<RefPtr<PageOverlay> > would clean this up.

> Source/WebKit2/WebProcess/WebPage/mac/LayerTreeHostMac.h:96
> +    PageOverlayLayerMap m_pageOverlays;

I think m_pageOverlays would be better called m_pageOverlayLayers

[Noam Rosenthal]

(In reply to comment #5)
> I'm not going to implement it for EFL/Qt but I'll at least keep it building and behaving close to how it currently does.

Thank you for that Tim.

[Build Bot]

Comment on attachment 193457 [details]
patch

Attachment 193457 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16996443

New failing tests:
editing/selection/anchor-focus1.html
editing/selection/4895428-2.html
editing/pasteboard/4944770-2.html
editing/selection/5195166-1.html
editing/selection/block-with-positioned-lastchild.html
editing/execCommand/findString-2.html
editing/selection/caret-ltr-right.html
editing/selection/caret-at-bidi-boundary.html
editing/selection/5131716-1.html
editing/selection/5209984.html
editing/selection/5131716-3.html
editing/selection/5057506-2.html
editing/selection/4895428-3.html
editing/selection/5131716-2.html
css3/viewport-percentage-lengths/vh-resize.html
editing/selection/caret-ltr-2-left.html
editing/selection/6476.html
editing/deleting/smart-delete-004.html
editing/deleting/smart-delete-across-editable-boundaries-2.html
editing/selection/5131716-4.html
compositing/iframes/layout-on-compositing-change.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
editing/selection/caret-bidi-first-and-last-letters.html
editing/deleting/smart-delete-002.html
editing/selection/caret-ltr.html
editing/selection/caret-ltr-2.html
editing/undo/undo-smart-delete-word.html
editing/deleting/smart-delete-003.html
editing/deleting/smart-delete-001.html
editing/selection/after-line-break.html

[Tim Horton]

http://trac.webkit.org/changeset/146183

[Ryosuke Niwa]

It seems like this patch broke multiple editing tests :(

[WebKit Review Bot]

Re-opened since this is blocked by bug 112673

[Ryosuke Niwa]

Created attachment 193762 [details]
Layout Test Results

Here's a local output of Tools/Scripts/run-webkit-tests --debug LayoutTests/editing/ -2.

[Ryosuke Niwa]

I'm sorry but we need to roll out this patch. It's causing buildbot and EWS bots to clog:
e.g.
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/7053
has been running for well over an hour and hasn't finished running yet.

[Tim Horton]

(In reply to comment #14)
> I'm sorry but we need to roll out this patch. It's causing buildbot and EWS bots to clog:
> e.g.
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/7053
> has been running for well over an hour and hasn't finished running yet.

Hmm, interesting. Thanks for rolling out! I have no idea whatsoever how that could be the case, but I'll take a look.

[Tim Horton]

(In reply to comment #15)
> (In reply to comment #14)
> > I'm sorry but we need to roll out this patch. It's causing buildbot and EWS bots to clog:
> > e.g.
> > http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/7053
> > has been running for well over an hour and hasn't finished running yet.
> 
> Hmm, interesting. Thanks for rolling out! I have no idea whatsoever how that could be the case, but I'll take a look.

Rolled back in with a single-line change to initialize the 'handled' argument in mouseEventSyncForTesting, in http://trac.webkit.org/changeset/146193.