Bug 76914 - [Qt] Implement tap feedback respecting -webkit-tap-highlight-color
Summary: [Qt] Implement tap feedback respecting -webkit-tap-highlight-color
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-24 07:44 PST by Kenneth Rohde Christiansen
Modified: 2012-01-25 10:19 PST (History)
5 users (show)

See Also:


Attachments
Patch (27.38 KB, patch)
2012-01-24 08:07 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (27.37 KB, patch)
2012-01-24 08:29 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (26.74 KB, patch)
2012-01-24 09:02 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (27.25 KB, patch)
2012-01-25 02:55 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (27.78 KB, patch)
2012-01-25 04:26 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (28.09 KB, patch)
2012-01-25 06:21 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (30.03 KB, patch)
2012-01-25 07:32 PST, Kenneth Rohde Christiansen
kenneth: commit-queue-
Details | Formatted Diff | Diff
Patch (30.47 KB, patch)
2012-01-25 07:57 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-01-24 07:44:26 PST
SSIA
Comment 1 Kenneth Rohde Christiansen 2012-01-24 08:07:25 PST
Created attachment 123736 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-24 08:14:39 PST
Attachment 123736 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/qt/tap-highlight..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/page/GestureTapHighlighter.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/page/GestureTapHighlighter.cpp:48:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kenneth Rohde Christiansen 2012-01-24 08:29:37 PST
Created attachment 123740 [details]
Patch
Comment 4 Antonio Gomes 2012-01-24 08:36:45 PST
Comment on attachment 123736 [details]
Patch

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

nits:

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:100
> +

unneeded new line?

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:70
> +        m_webPage->uninstallPageOverlay(m_overlay, true);

/* what true means */

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:81
> +    if (webPage)
> +        return;

just curious: !webPage?

also is it different from m_webpage?

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:100
> +bool TapHighlightController::mouseEvent(PageOverlay* pageOverlay, const WebMouseEvent& mouseEvent)

maybe omit the parameters?
Comment 5 Kenneth Rohde Christiansen 2012-01-24 08:42:44 PST
Uh this wasnt up for review yet but thanks :-)

(In reply to comment #4)
> (From update of attachment 123736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123736&action=review
> 
> nits:
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:100
> > +
> 
> unneeded new line?
> 
> > Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:70
> > +        m_webPage->uninstallPageOverlay(m_overlay, true);
> 
> /* what true means */
> 
> > Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:81
> > +    if (webPage)
> > +        return;
> 
> just curious: !webPage?

No it is moving to another page, this code is two other places in webkit2

> also is it different from m_webpage?
> 
> > Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:100
> > +bool TapHighlightController::mouseEvent(PageOverlay* pageOverlay, const WebMouseEvent& mouseEvent)
> 
> maybe omit the parameters?
Comment 6 Kenneth Rohde Christiansen 2012-01-24 09:02:08 PST
Created attachment 123744 [details]
Patch
Comment 7 Antonio Gomes 2012-01-24 09:37:41 PST
question: when you have a link that wraps line, being the beginning of it at the right of the page, and its wrapped content at the left of it at the line below, do you get the bounding box of the link? it would show a very big tap highlight...
Comment 8 Kenneth Rohde Christiansen 2012-01-24 10:34:27 PST
(In reply to comment #7)
> question: when you have a link that wraps line, being the beginning of it at the right of the page, and its wrapped content at the left of it at the line below, do you get the bounding box of the link? it would show a very big tap highlight...

That is not handled yet. There are also a few other things that I would like to treat as follow-ups. You are of course very welcome to help me with that :-)
Comment 9 Simon Hausmann 2012-01-25 00:43:43 PST
Comment on attachment 123744 [details]
Patch

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

I think this is a nice solution! (nicer than what we have in Grob :)

However there's one thing that was better in Grob compared to the way it's done here: The fade animation in Grob was just the result of composing the "highlight" pixmap with a different opacity, i.e. the animation required only composition and no re-rendering of content/texture-upload. The current way the page overlay implements the fade animation requires constant re-painting of the tiles AFAICS.

Although outside of the scope of this patch directly, in an ideal world we'd apply the same technique to this: If supported by the overlay (client), the content could be rendered into a layer once and the animation could be implemented via AC.

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:96
> +void TapHighlightController::drawRect(PageOverlay* pageOverlay, GraphicsContext& graphicsContext, const IntRect& dirtyRect)
> +{
> +    if (!m_node)
> +        return;
> +
> +    GestureTapHighlighter::drawHighlight(&graphicsContext, m_node);
> +}

I think for the fade animation to work, you need to respect pageOverlay->fractionFadedIn();
Comment 10 Kenneth Rohde Christiansen 2012-01-25 02:55:48 PST
Created attachment 123907 [details]
Patch

Now with animations
Comment 11 Kenneth Rohde Christiansen 2012-01-25 02:57:06 PST
> However there's one thing that was better in Grob compared to the way it's done here: The fade animation in Grob was just the result of composing the "highlight" pixmap with a different opacity, i.e. the animation required only composition and no re-rendering of content/texture-upload. The current way the page overlay implements the fade animation requires constant re-painting of the tiles AFAICS.
> 
> Although outside of the scope of this patch directly, in an ideal world we'd apply the same technique to this: If supported by the overlay (client), the content could be rendered into a layer once and the animation could be implemented via AC.

Yes, it would be the proper way to do this. Maybe No'am can have a look or help me when we are in Hungary.
Comment 12 Kenneth Rohde Christiansen 2012-01-25 04:26:03 PST
Created attachment 123921 [details]
Patch
Comment 13 Simon Hausmann 2012-01-25 05:26:26 PST
Comment on attachment 123921 [details]
Patch

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

> Source/WebCore/page/GestureTapHighlighter.cpp:153
> +void drawHighlight(GraphicsContext* context, Node* node)

I suggest to change the API to just return you the path for the highlight. Then the caller can call it once on initialization time (the node doesn't change after all) and then decide how to continously draw the path with the fade animation, instead of calculating the same path for every rendered frame.

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:62
> +    WebCore::Node* m_node;

Is it intentional to store the Node as raw pointer instead of RefPtr? It seems the highlight controller will dereference node many times during the animation, and during that time the node could easily get deleted, no?

But I would suggest to avoid storing the Node* altogether and instead calculate the path you need for highlighting once on initialization time and then store that instead.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1434
> +    Node* activationNode = 0;
> +    Frame* mainframe = m_page->mainFrame();
> +
> +    if (point != IntPoint::zero()) {
> +        HitTestResult result = mainframe->eventHandler()->hitTestResultAtPoint(mainframe->view()->windowToContents(point), /*allowShadowContent*/ false, /*ignoreClipping*/ true);
> +        activationNode = result.innerNode();

You could add a // ### FIXME to point out that when using touch we need to do touch adjustment here to locate the correct node.
Comment 14 Kenneth Rohde Christiansen 2012-01-25 06:21:41 PST
Created attachment 123929 [details]
Patch

Changed to incorporate some of Simon's ideas
Comment 15 WebKit Review Bot 2012-01-25 06:25:19 PST
Attachment 123929 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/qt/tap-highlight..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Kenneth Rohde Christiansen 2012-01-25 07:32:36 PST
Created attachment 123936 [details]
Patch

ChangeLogs method listing will be updated before any commit.
Comment 17 Kenneth Rohde Christiansen 2012-01-25 07:57:45 PST
Created attachment 123941 [details]
Patch
Comment 18 WebKit Review Bot 2012-01-25 08:39:20 PST
Comment on attachment 123941 [details]
Patch

Rejecting attachment 123941 [details] from commit-queue.

New failing tests:
media/audio-garbage-collect.html
Full output: http://queues.webkit.org/results/11242819
Comment 19 Kenneth Rohde Christiansen 2012-01-25 09:40:13 PST
Comment on attachment 123941 [details]
Patch

Must be flaky, resetting
Comment 20 WebKit Review Bot 2012-01-25 10:19:06 PST
Comment on attachment 123941 [details]
Patch

Clearing flags on attachment: 123941

Committed r105893: <http://trac.webkit.org/changeset/105893>
Comment 21 WebKit Review Bot 2012-01-25 10:19:12 PST
All reviewed patches have been landed.  Closing bug.