Bug 84936 - [chromium] Compute highlight for touch targets
Summary: [chromium] Compute highlight for touch targets
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 84487 85725
Blocks: 66687 79150 91648
  Show dependency treegraph
 
Reported: 2012-04-26 01:53 PDT by Tien-Ren Chen
Modified: 2012-10-05 14:11 PDT (History)
22 users (show)

See Also:


Attachments
Patch (17.15 KB, patch)
2012-04-26 01:54 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
A few basic test cases (1.05 KB, text/html)
2012-04-26 01:56 PDT, Tien-Ren Chen
no flags Details
Patch (22.92 KB, patch)
2012-05-01 03:31 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (15.92 KB, patch)
2012-05-07 21:06 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (15.87 KB, patch)
2012-05-10 19:39 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (16.02 KB, patch)
2012-05-11 13:32 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (23.57 KB, patch)
2012-05-21 20:44 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
some additional test cases (2.26 KB, text/html)
2012-05-21 20:48 PDT, Tien-Ren Chen
no flags Details
Patch (25.08 KB, patch)
2012-05-23 22:12 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch (36.40 KB, patch)
2012-05-29 17:08 PDT, Tien-Ren Chen
jchaffraix: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tien-Ren Chen 2012-04-26 01:53:12 PDT
[chromium] Compute highlight for touch targets
Comment 1 Tien-Ren Chen 2012-04-26 01:54:36 PDT
Created attachment 138954 [details]
Patch
Comment 2 Tien-Ren Chen 2012-04-26 01:56:32 PDT
Created attachment 138956 [details]
A few basic test cases
Comment 3 Tien-Ren Chen 2012-04-26 02:22:37 PDT
I couldn't find chromium account for the others, could you help me CC them?

This is an early WIP. I did open some pages and hover over some links to make sure it doesn't crash, but the actual generated paths are not verified. I believe there are still some bugs need to be fixed before requesting for review.

Also there are still some design concerns:

1. When do we generate the highlight, and how do we attach them to the GraphicsLayer?
The best timing is probably do it when painting the texture, but is it safe to keep a WebCore::Node* reference to the touch target? The node can be removed by javascript in the meantime.

2. How do we remove the highlight from the GraphicsLayer?

3. We probably want to refactor RenderInline::absoluteQuads.
I added another virtual function RenderObject::containerQuads that works similar to absoluteQuads except that it return the quads in a container RenderBoxModelObject's cords instead of the absolute cords. For RenderInline it is generating too much duplicate code. Better make RenderObject::absoluteQuads non-virtual and just call into containerQuads with NULL container.

4. Need to make the function more efficient.
It probably doesn't need to generate quads all descendant nodes... For example if there is a <div style="overflow:hidden"> then all its descendants can be skipped (except for fixed/absolute positioned descendant). I'm still thinking a clean way to do this.

5. absoluteQuads is probably not the perfect way to determine hit test region. There is an interesting example. See the first and second touch target in the attached example. The space between "Hello" and "link" is still occupied by the line box of the relative-positioned <div>, but it is only clickable when applied with a background color. Can't figure out how to do this right. :(

By the way there is an interesting bug in WebKit rendering. See the 4th touch target in the example. If you click on it, the blue box can escape from its parent's clipping.
Comment 4 W. James MacLean 2012-04-26 08:58:23 PDT
(In reply to comment #3)
> I couldn't find chromium account for the others, could you help me CC them?
> 
> This is an early WIP. I did open some pages and hover over some links to make sure it doesn't crash, but the actual generated paths are not verified. I believe there are still some bugs need to be fixed before requesting for review.
> 
> Also there are still some design concerns:
> 
> 1. When do we generate the highlight, and how do we attach them to the GraphicsLayer?
> The best timing is probably do it when painting the texture, but is it safe to keep a WebCore::Node* reference to the touch target? The node can be removed by javascript in the meantime.

This is being worked out in https://bugs.webkit.org/show_bug.cgi?id=84487

> 2. How do we remove the highlight from the GraphicsLayer?

Ditto.

> 3. We probably want to refactor RenderInline::absoluteQuads.
> I added another virtual function RenderObject::containerQuads that works similar to absoluteQuads except that it return the quads in a container RenderBoxModelObject's cords instead of the absolute cords. For RenderInline it is generating too much duplicate code. Better make RenderObject::absoluteQuads non-virtual and just call into containerQuads with NULL container.
> 
> 4. Need to make the function more efficient.
> It probably doesn't need to generate quads all descendant nodes... For example if there is a <div style="overflow:hidden"> then all its descendants can be skipped (except for fixed/absolute positioned descendant). I'm still thinking a clean way to do this.
> 
> 5. absoluteQuads is probably not the perfect way to determine hit test region. There is an interesting example. See the first and second touch target in the attached example. The space between "Hello" and "link" is still occupied by the line box of the relative-positioned <div>, but it is only clickable when applied with a background color. Can't figure out how to do this right. :(
> 
> By the way there is an interesting bug in WebKit rendering. See the 4th touch target in the example. If you click on it, the blue box can escape from its parent's clipping.
Comment 5 WebKit Review Bot 2012-04-26 12:28:50 PDT
Attachment 138954 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  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: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Build Bot 2012-04-26 12:47:27 PDT
Comment on attachment 138954 [details]
Patch

Attachment 138954 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12547008
Comment 7 WebKit Review Bot 2012-04-26 19:02:56 PDT
Comment on attachment 138954 [details]
Patch

Attachment 138954 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12548064
Comment 8 W. James MacLean 2012-04-27 11:33:42 PDT
Comment on attachment 138954 [details]
Patch

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

Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!

> Source/WebKit/chromium/src/WebViewImpl.cpp:3561
> +        SkPath* path = highlight->get(graphicsLayer);

I would prefer if we can use Path here, as it's more general and is used by the proposed patch for drawing the highlight layers.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3586
> +            path->moveTo(quad.p1().x(), quad.p1().y());

Can use Path::addRect(const FloatRect&) here, it's simpler. Can we add an option to call Path::addRoundedRect(), perhaps as an argument to this function?

> Source/WebKit/chromium/src/WebViewImpl.h:590
> +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;

Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.
Comment 9 Tien-Ren Chen 2012-04-27 13:28:08 PDT
(In reply to comment #8)
> (From update of attachment 138954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138954&action=review
> 
> Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!

I saw your patch that uses GestureTapHighlighter, that is actually a very interesting piece of code. I think we'd rather try to extend it instead of invent our own stuff.
Be more specific these are the 2 functionality we want to add:

1. To support transformed elements, using the Quads family functions. IMO all the Rects family should be deprecated sooner or later...
2. Support compositor side scrolling/animation. That is, the highlights should be assigned to individual GraphicsLayers with layer-relative cords. Example:
<a href="">
<div style="width:100px;height:100px"/>
<div style="width:100px;height:100px;-webkit-transform:translateZ(0);-webkit-animation-name:someanimation"/>
</a>

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3561
> > +        SkPath* path = highlight->get(graphicsLayer);
> 
> I would prefer if we can use Path here, as it's more general and is used by the proposed patch for drawing the highlight layers.

Sure I will upload a new patch uses WebCore::Path.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3586
> > +            path->moveTo(quad.p1().x(), quad.p1().y());
> 
> Can use Path::addRect(const FloatRect&) here, it's simpler. Can we add an option to call Path::addRoundedRect(), perhaps as an argument to this function?

It won't work for transformed touch target. For example: <div style="width:100px;height:100px;-webkit-transform:rotated(30deg)"></div>
Only 3D-transformed render objects will create its own GraphicsLayer.

> > Source/WebKit/chromium/src/WebViewImpl.h:590
> > +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;
> 
> Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.

I wanted to, but GraphicsLayer is not ref-counted, and modifying all existing code to use RefPtr will be a pain. :X
Comment 10 W. James MacLean 2012-04-30 08:53:53 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 138954 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138954&action=review
> > 
> > Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!
> 
> I saw your patch that uses GestureTapHighlighter, that is actually a very interesting piece of code. I think we'd rather try to extend it instead of invent our own stuff.

That patch has changed a lot. First, using the existing code gave very poor targets in my opinion. Your code worked quite nicely for me though. I may try again with the touch highlight region from my patch, and your node selector.

> Be more specific these are the 2 functionality we want to add:
> 
> 1. To support transformed elements, using the Quads family functions. IMO all the Rects family should be deprecated sooner or later...
> 2. Support compositor side scrolling/animation. That is, the highlights should be assigned to individual GraphicsLayers with layer-relative cords. Example:
> <a href="">
> <div style="width:100px;height:100px"/>
> <div style="width:100px;height:100px;-webkit-transform:translateZ(0);-webkit-animation-name:someanimation"/>
> </a>
> 
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:3561
> > > +        SkPath* path = highlight->get(graphicsLayer);
> > 
> > I would prefer if we can use Path here, as it's more general and is used by the proposed patch for drawing the highlight layers.
> 
> Sure I will upload a new patch uses WebCore::Path.

Great, thanks!

> > > Source/WebKit/chromium/src/WebViewImpl.cpp:3586
> > > +            path->moveTo(quad.p1().x(), quad.p1().y());
> > 
> > Can use Path::addRect(const FloatRect&) here, it's simpler. Can we add an option to call Path::addRoundedRect(), perhaps as an argument to this function?
> 
> It won't work for transformed touch target. For example: <div style="width:100px;height:100px;-webkit-transform:rotated(30deg)"></div>
> Only 3D-transformed render objects will create its own GraphicsLayer.
> 
> > > Source/WebKit/chromium/src/WebViewImpl.h:590
> > > +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;
> > 
> > Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.
> 
> I wanted to, but GraphicsLayer is not ref-counted, and modifying all existing code to use RefPtr will be a pain. :X

OK, so we need to figure out how to work around this then. Is there anything that points to the graphics layer that is ref-counted, that we could store instead?
Comment 11 Tien-Ren Chen 2012-05-01 03:31:41 PDT
Created attachment 139605 [details]
Patch
Comment 12 WebKit Review Bot 2012-05-01 03:35:44 PDT
Attachment 139605 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Build Bot 2012-05-01 03:40:27 PDT
Comment on attachment 139605 [details]
Patch

Attachment 139605 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12593519
Comment 14 Tien-Ren Chen 2012-05-01 03:44:36 PDT
I won't be in the office until Friday. I will try to keep in touch by email but I won't have too much time to work on code.

I changed the patch a little bit to make the highlight generation code a separate class. It is very likely I will have to improve the implementation later, but the interface will stay the same.

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 138954 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=138954&action=review
> > > 
> > > Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!
> > 
> > I saw your patch that uses GestureTapHighlighter, that is actually a very interesting piece of code. I think we'd rather try to extend it instead of invent our own stuff.
> 
> That patch has changed a lot. First, using the existing code gave very poor targets in my opinion. Your code worked quite nicely for me though. I may try again with the touch highlight region from my patch, and your node selector.

The node selector from my patch is very minimal. We have some disambiguation logic in downstream code that I don't 100% understand yet. I'll try to upstream those incrementally in later patches.

As for the highlight region, I think addFocusRingRects is actually very similar to our implementation, except that it uses absolute rects (thus can behave very wrong when transformation is used).

> > > > Source/WebKit/chromium/src/WebViewImpl.h:590
> > > > +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;
> > > 
> > > Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.
> > 
> > I wanted to, but GraphicsLayer is not ref-counted, and modifying all existing code to use RefPtr will be a pain. :X
> 
> OK, so we need to figure out how to work around this then. Is there anything that points to the graphics layer that is ref-counted, that we could store instead?

I don't think anything in the render tree is ref-counted.

LayerChromium is ref-counted but it doesn't back-reference to GraphicsLayer. One possibility is to do everything on LayerChromium. That is what I did with the compositor scrollbar layers.
Comment 15 WebKit Review Bot 2012-05-01 03:54:14 PDT
Comment on attachment 139605 [details]
Patch

Attachment 139605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12591519
Comment 16 Tien-Ren Chen 2012-05-07 21:06:50 PDT
Created attachment 140662 [details]
Patch
Comment 17 Build Bot 2012-05-07 21:28:52 PDT
Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12650253
Comment 18 WebKit Review Bot 2012-05-07 21:29:00 PDT
Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12651249
Comment 19 Early Warning System Bot 2012-05-07 21:29:25 PDT
Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12652264
Comment 20 Early Warning System Bot 2012-05-07 21:33:40 PDT
Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12656212
Comment 21 Build Bot 2012-05-07 21:34:55 PDT
Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12645278
Comment 22 Gyuyoung Kim 2012-05-07 23:10:20 PDT
Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12654215
Comment 23 Tom Zakrajsek 2012-05-10 16:32:51 PDT
Comment on attachment 140662 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:1495
> +    generateLineBoxRects(RepaintContainerHighlightQuadsGeneratorContext(this, quads, repaintContainer));

generateLineBoxRects isn't defined anywhere.  Breaks all builds.
Comment 24 Tien-Ren Chen 2012-05-10 16:54:55 PDT
(In reply to comment #23)
> (From update of attachment 140662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140662&action=review
> 
> > Source/WebCore/rendering/RenderInline.cpp:1495
> > +    generateLineBoxRects(RepaintContainerHighlightQuadsGeneratorContext(this, quads, repaintContainer));
> 
> generateLineBoxRects isn't defined anywhere.  Breaks all builds.

Yes, waiting for 85725 to land
Comment 25 Tien-Ren Chen 2012-05-10 19:39:52 PDT
Created attachment 141314 [details]
Patch
Comment 26 Tien-Ren Chen 2012-05-10 19:42:40 PDT
Nothing changed. Just rebase to the latest version.
Comment 27 Build Bot 2012-05-10 20:30:33 PDT
Comment on attachment 141314 [details]
Patch

Attachment 141314 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12677073
Comment 28 Tien-Ren Chen 2012-05-11 13:32:54 PDT
Created attachment 141485 [details]
Patch
Comment 29 Tien-Ren Chen 2012-05-21 20:44:53 PDT
Created attachment 143183 [details]
Patch
Comment 30 Tien-Ren Chen 2012-05-21 20:48:11 PDT
This new version is based on the recent version of https://bugs.webkit.org/show_bug.cgi?id=84487
(https://bugs.webkit.org/attachment.cgi?id=142729)

Provides much more accurate highlight for many test cases.
Comment 31 Tien-Ren Chen 2012-05-21 20:48:57 PDT
Created attachment 143184 [details]
some additional test cases
Comment 32 Adam Barth 2012-05-21 23:48:42 PDT
@trchen: I'm hoping Eric or jchaffraix will review your patch.
Comment 33 Julien Chaffraix 2012-05-22 13:14:18 PDT
Comment on attachment 143183 [details]
Patch

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

I will only comment on the general bits in the rendering which are clunky. Added the compositor people to look at what are the implication for the compositor's and what you need to do.

> Source/WebCore/ChangeLog:14
> +        One important difference from RenderObject::absoluteQuads is that
> +        the returned quads will be in the coordinates of a repaint container
> +        specified by the caller. This will allow threaded-scrolling/animation
> +        without having to regenerate the highlights.

What happens if your repaintContainer change? (layer removed or added for example or promotion / demotion from being hardware accelerated)

> Source/WebCore/ChangeLog:17
> +        No new tests. Tests will be added with later patches that actually
> +        add highlight to layers.

You are exposing the new function to Chromium. Will it be tested there?

What do you mean by layers: RenderLayers or GraphicsLayers?

> Source/WebCore/rendering/RenderBlock.h:450
> +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;

Not a huge fan of this function name: I thought it was a repaint* function (which obviously it's not). How about collectHighlightQuadsForContainer(...) or something similar?

> Source/WebCore/rendering/RenderBox.cpp:596
> +    if (!size().isEmpty())

This should be an early return. I also find this check inconsistent with the rest of the code (you would not check the other classes for empty quads?)

> Source/WebCore/rendering/RenderBox.cpp:597
> +        quads.append(localToContainerQuad(FloatRect(0, 0, width(), height()), repaintContainer, false, 0));

You don't need the 2 last parameters here. They make the code less readable and more confusing as you wonder why you need them here. Same comment later at the other call sites.

> Source/WebCore/rendering/RenderBox.h:155
> +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;

We annotate those functions with OVERRIDE (not repeated).

> Source/WebCore/rendering/RenderInline.cpp:1465
> +

This is inconsistent: you don't call your parent class' method here.

> Source/WebCore/rendering/RenderInline.cpp:1467
> +    if (continuation())
> +        continuation()->repaintContainerHighlightQuads(quads, repaintContainer);

continuation is RenderBoxModelObject concept which makes me dubious of the duplication between RenderInline and RenderBlock: there should be a RenderBoxModelObject function that handles the continuation for you.

> Source/WebKit/chromium/src/WebHighlight.cpp:54
> +    for (Node *node = topNode; node; node = node->traverseNextNode(topNode)) {

Style violation

> Source/WebKit/chromium/src/WebHighlight.cpp:57
> +        if (!renderer)
> +            continue;

My question here: have you considered SVG? Do you want to support it or not?

If you don't care about SVG, you don't care about anything that is not a RenderBoxModelObject anyway. That would simplify the code above.

> Source/WebKit/chromium/src/WebHighlight.cpp:59
> +        RenderLayer* repaintLayer = renderer->enclosingLayer()->enclosingCompositingLayerForRepaint();

NO! Using RenderLayer in the WebKit/ directory is a show-stopper. It means you need access to something you shouldn't be knowing about.

RenderBoxModelObject* repaintContainer = renderer->containerForRepaint();

Will give you the renderer you need. If you need more of the hidden bits, it looks like you should rethink your architecture.

> Source/WebKit/chromium/src/WebHighlight.cpp:65
> +        GraphicsLayer* graphicsLayer = repaintLayer->backing()->graphicsLayer();

What proofs do you have that this RenderLayer is hardware accelerated? If it is not, this is a NULL crasher.

> Source/WebKit/chromium/src/WebHighlight.cpp:68
> +        Path* path = m_quadMap.get(graphicsLayer);
> +        if (!path)
> +            m_quadMap.set(graphicsLayer, adoptPtr(path = new Path()));

This is super confusing and inefficient as you are doing 2 hash lookups if you have no entry:

Path* path = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator->second;

> Source/WebKit/chromium/src/WebHighlight.cpp:73
> +        for (size_t i = 0; i < newquads.size(); i++) {

I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?
Comment 34 Tien-Ren Chen 2012-05-22 14:27:18 PDT
(In reply to comment #33)
> (From update of attachment 143183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143183&action=review
> 
> I will only comment on the general bits in the rendering which are clunky. Added the compositor people to look at what are the implication for the compositor's and what you need to do.

Thanks for the valuable comments!

> > Source/WebCore/ChangeLog:14
> > +        One important difference from RenderObject::absoluteQuads is that
> > +        the returned quads will be in the coordinates of a repaint container
> > +        specified by the caller. This will allow threaded-scrolling/animation
> > +        without having to regenerate the highlights.
> 
> What happens if your repaintContainer change? (layer removed or added for example or promotion / demotion from being hardware accelerated)

Yes that would be a problem. The current implementation generates the highlight at the time a tap gesture happened, then it stays the same no matter the nodes are moved / transformed / zapped / whatever. Ideally we should store a reference to the node, then regenerate the highlight on every compositor commit. I doubt if it'd be an overkill though.

> > Source/WebCore/ChangeLog:17
> > +        No new tests. Tests will be added with later patches that actually
> > +        add highlight to layers.
> 
> You are exposing the new function to Chromium. Will it be tested there?
> 
> What do you mean by layers: RenderLayers or GraphicsLayers?

Sorry the comment no longer applies. I'll add tests in revised patch.

> > Source/WebCore/rendering/RenderBlock.h:450
> > +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;
> 
> Not a huge fan of this function name: I thought it was a repaint* function (which obviously it's not). How about collectHighlightQuadsForContainer(...) or something similar?

Totally agree... Another thing aelias@ is suggesting that what the function actually does is to mimic the hit-test area, and later we might use it to generate the non-fast touch events region for threaded input event handling. So maybe collectHittestQuadsForContainer would be a better name?

> > Source/WebCore/rendering/RenderBox.cpp:596
> > +    if (!size().isEmpty())
> 
> This should be an early return. I also find this check inconsistent with the rest of the code (you would not check the other classes for empty quads?)

Empty quad probably doesn't matter, but I'll add check for all of them for the sake of consistency.

> > Source/WebCore/rendering/RenderBox.cpp:597
> > +        quads.append(localToContainerQuad(FloatRect(0, 0, width(), height()), repaintContainer, false, 0));
> 
> You don't need the 2 last parameters here. They make the code less readable and more confusing as you wonder why you need them here. Same comment later at the other call sites.
> 
> > Source/WebCore/rendering/RenderBox.h:155
> > +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;
> 
> We annotate those functions with OVERRIDE (not repeated).

Done

> > Source/WebCore/rendering/RenderInline.cpp:1465
> > +
> 
> This is inconsistent: you don't call your parent class' method here.

I don't understand this comment. Could you explain in detail?

> > Source/WebCore/rendering/RenderInline.cpp:1467
> > +    if (continuation())
> > +        continuation()->repaintContainerHighlightQuads(quads, repaintContainer);
> 
> continuation is RenderBoxModelObject concept which makes me dubious of the duplication between RenderInline and RenderBlock: there should be a RenderBoxModelObject function that handles the continuation for you.

I don't think there is one, as absoluteRects and absoluteQuads use the same practice. continuation() is declared as protected instead of private, I guess that means subclasses can be aware of it?

> > Source/WebKit/chromium/src/WebHighlight.cpp:54
> > +    for (Node *node = topNode; node; node = node->traverseNextNode(topNode)) {
> 
> Style violation

Done

> > Source/WebKit/chromium/src/WebHighlight.cpp:57
> > +        if (!renderer)
> > +            continue;
> 
> My question here: have you considered SVG? Do you want to support it or not?
> 
> If you don't care about SVG, you don't care about anything that is not a RenderBoxModelObject anyway. That would simplify the code above.

Currently I don't care about SVG as I almost never see them used on the internet... But I'll say we'd like to support at a later time when we have more time budget.

> > Source/WebKit/chromium/src/WebHighlight.cpp:59
> > +        RenderLayer* repaintLayer = renderer->enclosingLayer()->enclosingCompositingLayerForRepaint();
> 
> NO! Using RenderLayer in the WebKit/ directory is a show-stopper. It means you need access to something you shouldn't be knowing about.
> 
> RenderBoxModelObject* repaintContainer = renderer->containerForRepaint();
> 
> Will give you the renderer you need. If you need more of the hidden bits, it looks like you should rethink your architecture.

Ah ha, this is exactly what I need. Thanks!

> > Source/WebKit/chromium/src/WebHighlight.cpp:65
> > +        GraphicsLayer* graphicsLayer = repaintLayer->backing()->graphicsLayer();
> 
> What proofs do you have that this RenderLayer is hardware accelerated? If it is not, this is a NULL crasher.

Because it's returned from enclosingCompositingLayerForRepaint(), either it is NULL or it has a backing?

> > Source/WebKit/chromium/src/WebHighlight.cpp:68
> > +        Path* path = m_quadMap.get(graphicsLayer);
> > +        if (!path)
> > +            m_quadMap.set(graphicsLayer, adoptPtr(path = new Path()));
> 
> This is super confusing and inefficient as you are doing 2 hash lookups if you have no entry:
> 
> Path* path = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator->second;

This one allocates a new Path even when there is existing entry. :(

How about this?
HashMap<...>::iterator i = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator;
if (!i->second)
    i->second = new Path();
Path* path = i->second;

> > Source/WebKit/chromium/src/WebHighlight.cpp:73
> > +        for (size_t i = 0; i < newquads.size(); i++) {
> 
> I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?

What I'm thinking is that the WebCore function should only return the exact hit test area, while the WebKit function adds decorations (like round smooth corners), and try to do some basic clipping.
Comment 35 Alexandre Elias 2012-05-22 19:32:39 PDT
(In reply to comment #33)
> > Source/WebKit/chromium/src/WebHighlight.cpp:73
> > +        for (size_t i = 0; i < newquads.size(); i++) {
> 
> I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?

I had a chat offline with Tien-Ren about this.  First, I think we want the WebCore part to return rects because it's not guaranteed that the highlight rect generation will always support them (personally I think it's overkill and we may want to strip out Path support at some point).

But I think you have a point.  As Tien-Ren mentioned above, I think we should rename "repaintContainerHighlightQuads()" to "hitRectContainerQuads()" as ultimately what we want to highlight is the hit rect area of the element.  Moreover, this function will be usable in the future for the touch event quick-reject feature that we've been discussing.  Then, we can move the bulk of the hocus pocus -- the clipping for overflow: hidden elements -- into WebCore, since that logic matches the "hit rect" concept.
Comment 36 Tien-Ren Chen 2012-05-23 22:12:53 PDT
Created attachment 143728 [details]
Patch
Comment 37 Tien-Ren Chen 2012-05-23 22:15:55 PDT
The new patch fixed some of the issue addressed by jchaffraix@.

Still need to add new test and update the existing LinkHighlightTest.

By the way how do I tell the script to regenerate the modification list in ChangeLog?
Comment 38 Adam Barth 2012-05-24 08:30:40 PDT
> By the way how do I tell the script to regenerate the modification list in ChangeLog?

There isn't a very good way to do that.  One thing you can do is to run ./Tools/Scripts/prepare-ChangeLog manually, which will add a new ChangeLog entry to the top of the file.  You'll then need to merge the two entries manually.
Comment 39 Tien-Ren Chen 2012-05-29 17:08:26 PDT
Created attachment 144645 [details]
Patch
Comment 40 Adam Barth 2012-06-04 16:46:40 PDT
@jchaffraix: Are you happy with this patch now?
Comment 41 Julien Chaffraix 2012-06-07 10:23:51 PDT
Comment on attachment 144645 [details]
Patch

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

Not good yet. The patch is too massive to be properly reviewed, please find some ways to split it as it would help to get it landed: I couldn't look at everything due to the size.

> Source/WebCore/ChangeLog:9
> +        to generate the clickable region for link highlighting.

If it's only for link, it should be more explicit in the naming.

> Source/WebCore/rendering/RenderObject.cpp:2867
> +void RenderObject::hitQuadsForContainerWithClipping(Vector<FloatQuad>& quads, RenderBoxModelObject* repaintContainer) const

The naming could really use a *verb* and be more precise. I don't understand what the 'with clipping' means, if you mean clipped then please use 'clipped' instead (which would match the rest of the code, e.g. clippedOverflowRectForRepaint (which may do something similar to what you are doing btw, I don't have the strength of looking into that)).

> Source/WebCore/rendering/RenderObject.cpp:2869
> +    size_t oldSize = quads.size();

This is useless as quads is always empty when the function is called.

> Source/WebCore/rendering/RenderObject.cpp:2880
> +        for (RenderBlock* blk = containingBlock(); blk && blk->isDescendantOf(repaintContainer); blk = blk->containingBlock()) {

I hope you understand this is a quadratic operation thanks to the isDescendantOf call and because it's in another loop it's horribly slow. 2 ways to make it less sucky:
* look-ahead and you cache the top-most containing block to avoid the isDescendantOf() call.
* cache the whole containing block chain (likely faster as containingBlock() is a non-trivial function)

> Source/WebCore/rendering/RenderObject.cpp:2881
> +            if (!blk->hasOverflowClip())

please don't abbreviate, especially since it should be containingBlock for preciseness.

> Source/WebCore/rendering/RenderObject.cpp:2883
> +            FloatQuad boundQuad = blk->localToContainerQuad(FloatRect(blk->visualOverflowRect()), repaintContainer);

visualOverflowRect is more or less the border-box for elements with a clip: it would include only a couple of operations that bleed outside the box (like box-shadow) but would miss some like outline.

I am not sure why you don't just use the border-box.

> Source/WebCore/rendering/RenderObject.cpp:2884
> +            if (!boundQuad.isRectilinear()) // don't know how to clip this. :(

You can always do the intersection with the quad's bounding box but it will be inaccurate - as in the resulting FloatRect will cover a bigger surface - if it's not rectilinear.

If you need accuracy, you would need to manipulate Path and not rects.

> Source/WebKit/chromium/src/WebHighlight.cpp:63
> +        GraphicsLayer* graphicsLayer = 0;
> +        if (repaintContainer && repaintContainer->layer() && repaintContainer->layer()->backing())
> +            graphicsLayer = repaintContainer->layer()->backing()->graphicsLayer();

It makes me cry that you need to know so much about how rendering works just to collect some simple hit quads.

> Source/WebKit/chromium/src/WebHighlight.h:48
> +class WebHighlight {

This refactoring is somehow tangential to the change (I guess it's to be able to test more easily). Please move it to a blocking bug as it would shrink your already massive patch.

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:176
> +#if 0

Why are you disabling this test? I see no mention of it anywhere.

> Source/WebKit/chromium/tests/data/webhighlight_test.html:4
> +<a href="" id="link1" style="position:absolute;left:50px;top:50px;background-color:#ccffcc">
> +    <div style="display:inline-block;width:500px;height:20px;background-color:#ccffcc"></div>

Please factor the style into some common classes and some id selectors. It's difficult to read the test as-is.
Comment 42 Antonio Gomes 2012-06-08 11:33:47 PDT
Comment on attachment 144645 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        This patch adds RenderObject::hitQuadsForContainerWithClipping call,
>> +        to generate the clickable region for link highlighting.
> 
> If it's only for link, it should be more explicit in the naming.

is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
Comment 43 Tien-Ren Chen 2012-06-08 13:26:36 PDT
Comment on attachment 144645 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        to generate the clickable region for link highlighting.
>> 
>> If it's only for link, it should be more explicit in the naming.
> 
> is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)

GestureTapHighlighter has some major flaws that I don't think are easily fixable.
1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.

>> Source/WebCore/rendering/RenderObject.cpp:2869
>> +    size_t oldSize = quads.size();
> 
> This is useless as quads is always empty when the function is called.

True for our current code, but there may be a chance we will batch multiple calls to the same vector. The overhead is negligible anyway.

>> Source/WebCore/rendering/RenderObject.cpp:2880
>> +        for (RenderBlock* blk = containingBlock(); blk && blk->isDescendantOf(repaintContainer); blk = blk->containingBlock()) {
> 
> I hope you understand this is a quadratic operation thanks to the isDescendantOf call and because it's in another loop it's horribly slow. 2 ways to make it less sucky:
> * look-ahead and you cache the top-most containing block to avoid the isDescendantOf() call.
> * cache the whole containing block chain (likely faster as containingBlock() is a non-trivial function)

Hmm I'm just not sure if blk->containingBlock would ever jump over a repainContainer. If not I guess we can simply use blk && blk != repaintContainer.

>> Source/WebCore/rendering/RenderObject.cpp:2883
>> +            FloatQuad boundQuad = blk->localToContainerQuad(FloatRect(blk->visualOverflowRect()), repaintContainer);
> 
> visualOverflowRect is more or less the border-box for elements with a clip: it would include only a couple of operations that bleed outside the box (like box-shadow) but would miss some like outline.
> 
> I am not sure why you don't just use the border-box.

Sounds good

>> Source/WebCore/rendering/RenderObject.cpp:2884
>> +            if (!boundQuad.isRectilinear()) // don't know how to clip this. :(
> 
> You can always do the intersection with the quad's bounding box but it will be inaccurate - as in the resulting FloatRect will cover a bigger surface - if it's not rectilinear.
> 
> If you need accuracy, you would need to manipulate Path and not rects.

I think you're right. Clip loosely is better than not clipping at all in this case.
We can allow some false positive. Manipulating polygons is simply too expensive.

>> Source/WebKit/chromium/src/WebHighlight.cpp:63
>> +            graphicsLayer = repaintContainer->layer()->backing()->graphicsLayer();
> 
> It makes me cry that you need to know so much about how rendering works just to collect some simple hit quads.

I agree it is too much leak of internals. But in this case the caller must know something about graphicsLayer in order to support threaded-scrolling/CSS animation.

>> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:176
>> +#if 0
> 
> Why are you disabling this test? I see no mention of it anywhere.

Ah, actually the whole LinkHighlightTest needs to be reworked for the new code. I'm still figuring out how to keep these tests.
Comment 44 Antonio Gomes 2012-06-08 13:29:17 PDT
> > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> 
> GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.

Zalan would be interested in improving this, I think.
Comment 45 Tien-Ren Chen 2012-06-08 13:32:25 PDT
(In reply to comment #41)
> (From update of attachment 144645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144645&action=review
> > Source/WebCore/ChangeLog:9
> > +        to generate the clickable region for link highlighting.
> 
> If it's only for link, it should be more explicit in the naming.

Forgot to say, this will be re-used at a later time for threaded-scrolling/touch event handling. The idea is, we want to generate the hit quads for the elements with Javascript touch handler, store them in the compositor side, so the compositor will know whether we can do a quick reject to touch events instead of forward everything to the main thread. It will be very helpful for UI responsiveness.
Comment 46 Tien-Ren Chen 2012-06-08 13:37:16 PDT
(In reply to comment #44)
> > > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> > 
> > GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> > 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> > 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.
> 
> Zalan would be interested in improving this, I think.

Why not just use our version? ;)

I tried the GestureTapHighlighter based version with various test cases (see "some additional test cases" in the attachments), most of them result in a unusable highlight.

Besides transformation, it also generates wrong box for inline elements for some cases.
Comment 47 zalan 2012-06-11 05:52:16 PDT
(In reply to comment #46)
> (In reply to comment #44)
> > > > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> > > 
> > > GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> > > 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> > > 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.
> > 
> > Zalan would be interested in improving this, I think.
> 
> Why not just use our version? ;)
> 
> I tried the GestureTapHighlighter based version with various test cases (see "some additional test cases" in the attachments), most of them result in a unusable highlight.
> 
> Besides transformation, it also generates wrong box for inline elements for some cases.
Agree, GestureTapHighlighter has multiple known issues, which were planned to be improved gradually. Since it doesn't make much sense to maintain 2 different implementations of the same feature, I'll check if we can have qt to use this.
Comment 48 Tien-Ren Chen 2012-06-14 15:15:45 PDT
(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #44)
> > > > > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> > > > 
> > > > GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> > > > 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> > > > 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.
> > > 
> > > Zalan would be interested in improving this, I think.
> > 
> > Why not just use our version? ;)
> > 
> > I tried the GestureTapHighlighter based version with various test cases (see "some additional test cases" in the attachments), most of them result in a unusable highlight.
> > 
> > Besides transformation, it also generates wrong box for inline elements for some cases.
> Agree, GestureTapHighlighter has multiple known issues, which were planned to be improved gradually. Since it doesn't make much sense to maintain 2 different implementations of the same feature, I'll check if we can have qt to use this.

Sorry for the delay in reply. I'm currently on a 2-weeks vacation. Will try to reply within 48 hours.

I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:

1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.

2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.
Comment 49 Peter Beverloo 2012-07-17 03:33:27 PDT
Hi Tien-Ren, did you had a chance to look at this yet?
Comment 50 Allan Sandfeld Jensen 2012-08-22 01:46:31 PDT
(In reply to comment #48)
> I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:
> 
> 1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.
> 
> 2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.

Would it make sense to change addFocusRing, to return quads? It is intended for this purpose but I guess has just not been upgraded to handle transformations. Alternatively, leave it mostly as is (on RenderObject level), but let RenderLayer apply the transformations and still end up returning quads.
Comment 51 Allan Sandfeld Jensen 2012-08-22 01:48:56 PDT
Additionally, I would suggest removing the [Chromium] tag from the bug. If this lands, it could also solve problems for other platforms, for instance bug #94345, which is an assertion hit due to encountering transforms in highlight path code that can not handle transforms.
Comment 52 Kenneth Rohde Christiansen 2012-08-22 01:53:37 PDT
(In reply to comment #50)
> (In reply to comment #48)
> > I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:
> > 
> > 1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.
> > 
> > 2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.
> 
> Would it make sense to change addFocusRing, to return quads? It is intended for this purpose but I guess has just not been upgraded to handle transformations. Alternatively, leave it mostly as is (on RenderObject level), but let RenderLayer apply the transformations and still end up returning quads.

What about making addFocusRing use hitQuadsForContainer and then just do the beautifying code on top? Drawing highlights or focus rings should be quite similar
Comment 53 Allan Sandfeld Jensen 2012-08-22 02:18:38 PDT
(In reply to comment #52)
> (In reply to comment #50)
> > (In reply to comment #48)
> > > I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:
> > > 
> > > 1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.
> > > 
> > > 2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.
> > 
> > Would it make sense to change addFocusRing, to return quads? It is intended for this purpose but I guess has just not been upgraded to handle transformations. Alternatively, leave it mostly as is (on RenderObject level), but let RenderLayer apply the transformations and still end up returning quads.
> 
> What about making addFocusRing use hitQuadsForContainer and then just do the beautifying code on top? Drawing highlights or focus rings should be quite similar

It turns out to be unnecessary. AddFocusRing is used by normal paint logic, which means it is already clipped and transformed automatically by RenderLayer, so it doesn't need quads.

But that does give me an idea: What we are doing with hitQuadsForContainer (and GestureTapHighlighter), is duplicating the clipping code from painting, because we are doing the painting outside of the normal painting logic. If the painting was instead performed by RenderObject like it is for focusRings, this would not be necessary. So would it be possible to call a function similar to paintFocusRing to paint the tap-highlight, and give it the right graphicscontext making it paint it on another layer?
Comment 54 Kenneth Rohde Christiansen 2012-08-22 02:45:25 PDT
> It turns out to be unnecessary. AddFocusRing is used by normal paint logic, which means it is already clipped and transformed automatically by RenderLayer, so it doesn't need quads.
> 
> But that does give me an idea: What we are doing with hitQuadsForContainer (and GestureTapHighlighter), is duplicating the clipping code from painting, because we are doing the painting outside of the normal painting logic. If the painting was instead performed by RenderObject like it is for focusRings, this would not be necessary. So would it be possible to call a function similar to paintFocusRing to paint the tap-highlight, and give it the right graphicscontext making it paint it on another layer?

I think this should be possible, especially if we want the same clipping. I thinks that the highlight itself might be slightly bigger than the element being highlighted (and clipped). I guess we also want to make sure that the actual highlighting is always painted on a layer different from root.
Comment 55 Adam Barth 2012-10-05 14:11:25 PDT
I think this work got done in another patch.