Bug 94182

Summary: [chromium] Implement Disambiguation Popup (a.k.a. link preview, on-demand zoom)
Product: WebKit Reporter: Tien-Ren Chen <trchen>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aelias, allan.jensen, dglazkov, feldstein, fishd, jamesr, klobag, rjkroege, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687, 91648    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Tien-Ren Chen 2012-08-15 20:54:39 PDT
[chromium] Implement Link Preview (a.k.a. on-demand zoom)
Comment 1 Tien-Ren Chen 2012-08-15 21:06:10 PDT
Created attachment 158702 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-15 21:07:28 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-08-15 22:18:13 PDT
*** Bug 79150 has been marked as a duplicate of this bug. ***
Comment 4 Alexandre Elias 2012-08-16 11:31:01 PDT
Comment on attachment 158702 [details]
Patch

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

> Source/WebKit/chromium/features.gypi:84
> +      'ENABLE_LINK_PREVIEW=1',

As I was saying earlier, before the confusing naming gets embedded into the code, can we rename this to "disambiguation popup"?

> Source/WebKit/chromium/public/WebInputEvent.h:382
> +    bool generatedByLinkPreview;

Here's a thought.  This value could be replaced by "float errorDistance;"  The browser could send down a value corresponding to basically the size of a finger and it could feed into the scoring function.  When the tap already has been disambiguated, the error can be set to zero.

> Source/WebKit/chromium/src/WebViewImpl.cpp:741
> +        score *= max((padding - (boundingBox.x() - touchPoint.x())) * divPadding, (float)0);

I think this code can be made more compact by using some of the fancier IntRect methods.
Comment 5 Adam Barth 2012-08-19 21:04:06 PDT
Comment on attachment 158702 [details]
Patch

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

This review is mostly questions.  The the main thing I'd like to change is to remove generatedByLinkPreview.  Is there a way around adding that state to WebKit?

>> Source/WebKit/chromium/public/WebInputEvent.h:382
>> +    bool generatedByLinkPreview;
> 
> Here's a thought.  This value could be replaced by "float errorDistance;"  The browser could send down a value corresponding to basically the size of a finger and it could feed into the scoring function.  When the tap already has been disambiguated, the error can be set to zero.

Is there a way to keep this state in Chromium?  It's not really a concern of WebKit...  All that might be required is for the RenderView to remember whether it is currently showing a link previous popup.

> Source/WebKit/chromium/public/WebViewClient.h:290
> +    // Return true if the embedder will start a link preview so the input event will be swallowed
> +    virtual bool triggersLinkPreview(const WebRect& windowZoomRect) { return false; }

triggersLinkPreview  ->  shouldCauseLinkDisambiguation ?

What is windowZoomRect?  How can a rect cause link disambiguation?  This is probably fine, it's just a bit confusing.

> Source/WebKit/chromium/src/WebViewImpl.cpp:710
> +bool isEventNode(Node *node)
> +{
> +    return node && (node->supportsFocus()
> +        || node->hasEventListeners(eventNames().clickEvent)
> +        || node->hasEventListeners(eventNames().mousedownEvent)
> +        || node->hasEventListeners(eventNames().mouseupEvent));
> +}

Should this be in WebCore?  It's not really specific to Chromium and it doesn't really seem like a concern of the entire WebView.

It's tempting to implement every feature in WebViewImpl.cpp, but if we did that, WebViewImpl.cpp would grow to be even more huge than it is today.

> Source/WebKit/chromium/src/WebViewImpl.cpp:713
> +IntRect calculateEventNodeBoundingBox(Node* eventNode)

Again, this doesn't seem specific to Chromium and seems like something that could live in WebCore somewhere.

> Source/WebKit/chromium/src/WebViewImpl.cpp:732
> +float scoreTouchTarget(IntPoint touchPoint, int padding, IntRect boundingBox)

Should this be in a separate file in WebKit/chromium/src ?  We try to avoid putting everything in WebViewImpl.cpp.

> Source/WebKit/chromium/src/WebViewImpl.cpp:772
> +    // Find event handler node in the ancestor chain for each hit test result

I wonder if a bunch of this should be moved into the same file as scoreTouchTarget

> Source/WebKit/chromium/src/WebViewImpl.cpp:778
> +            if (node->isDocumentNode() || node->hasTagName(HTMLNames::htmlTag) || node->hasTagName(HTMLNames::bodyTag))
> +                break;

How does this work for SVG documents?

> Source/WebKit/chromium/src/WebViewImpl.cpp:798
> +    // Only keep good touch targets with score >= max(score)/2, and calculate the zoom rect
> +    int numberOfGoodTargets = 0;
> +    IntRect windowZoomRect = IntRect(touchPoint, IntSize(1, 1));
> +    windowZoomRect.inflate(touchPointPadding);
> +    for (HashMap<Node*, TouchTargetData>::iterator it = touchTargets.begin(); it != touchTargets.end(); ++it) {
> +        if (it->second.score < bestScore * 0.5)
> +            continue;
> +        numberOfGoodTargets++;
> +        windowZoomRect.unite(it->second.windowBoundingBox);
> +    }

This too.

> Source/WebKit/chromium/src/WebViewImpl.cpp:800
> +    // TODO: replace touch adjustment code when numberOfGoodTargets == 1?

TODO -> FIXME

> Source/WebKit/chromium/src/WebViewImpl.cpp:804
> +    return m_client && m_client->triggersLinkPreview(windowZoomRect);

I see.  So windowZoomRect is something like a bounding box of the touch targets.

> Source/WebKit/chromium/src/WebViewImpl.cpp:833
> +        if (!event.generatedByLinkPreview) {

Maybe the Chromium side can use its own version of this state to answer triggersLinkPreview or another client call made here?
Comment 6 Alexandre Elias 2012-08-20 10:44:45 PDT
(In reply to comment #5)
> This review is mostly questions.  The the main thing I'd like to change is to remove generatedByLinkPreview.  Is there a way around adding that state to WebKit?
> 
> >> Source/WebKit/chromium/public/WebInputEvent.h:382
> >> +    bool generatedByLinkPreview;
> > 
> > Here's a thought.  This value could be replaced by "float errorDistance;"  The browser could send down a value corresponding to basically the size of a finger and it could feed into the scoring function.  When the tap already has been disambiguated, the error can be set to zero.
> 
> Is there a way to keep this state in Chromium?  It's not really a concern of WebKit...  All that might be required is for the RenderView to remember whether it is currently showing a link previous popup.

We don't want the renderer to make assumptions about what the browser is doing.  The browser could dismiss the popup for complex reasons.  And it would also be messy to add more criss-crossing ViewMsgs to keep the renderer in sync with whatever the browser is doing.

The "errorDistance" actually is a logical property of an input event -- conceptually, there is a zone of precision you can expect for a given input event, whether due to hardware or finger imprecision.   So I think we should add the value -- it's natural enough that I have the feeling we'll even find different uses for it in the future..
Comment 7 Adam Barth 2012-08-20 12:11:01 PDT
> The "errorDistance" actually is a logical property of an input event

Ok, that makes some amount of sense.  It sounds similar to radiusX and radiusY on WebTouchPoint.
Comment 8 Tien-Ren Chen 2012-08-20 15:02:18 PDT
(In reply to comment #5)
> (From update of attachment 158702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158702&action=review
> 
> This review is mostly questions.  The the main thing I'd like to change is to remove generatedByLinkPreview.  Is there a way around adding that state to WebKit?
> 
> >> Source/WebKit/chromium/public/WebInputEvent.h:382
> >> +    bool generatedByLinkPreview;
> > 
> > Here's a thought.  This value could be replaced by "float errorDistance;"  The browser could send down a value corresponding to basically the size of a finger and it could feed into the scoring function.  When the tap already has been disambiguated, the error can be set to zero.
> 
> Is there a way to keep this state in Chromium?  It's not really a concern of WebKit...  All that might be required is for the RenderView to remember whether it is currently showing a link previous popup.

After all I think we don't have to add another flag. I think we can reuse WebGestureEvent.boundingBox which stands for the touch point radius for the single tap events. For an artificial tap we can set that to a zero-sized rect.

> > Source/WebKit/chromium/public/WebViewClient.h:290
> > +    // Return true if the embedder will start a link preview so the input event will be swallowed
> > +    virtual bool triggersLinkPreview(const WebRect& windowZoomRect) { return false; }
> 
> triggersLinkPreview  ->  shouldCauseLinkDisambiguation ?

I think maybe handleLinkDisambiguation would be better.

> What is windowZoomRect?  How can a rect cause link disambiguation?  This is probably fine, it's just a bit confusing.

It is the bounding box of the touch target candidates, in window coordinate.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:710
> > +bool isEventNode(Node *node)
> > +{
> > +    return node && (node->supportsFocus()
> > +        || node->hasEventListeners(eventNames().clickEvent)
> > +        || node->hasEventListeners(eventNames().mousedownEvent)
> > +        || node->hasEventListeners(eventNames().mouseupEvent));
> > +}
> 
> Should this be in WebCore?  It's not really specific to Chromium and it doesn't really seem like a concern of the entire WebView.
> 
> It's tempting to implement every feature in WebViewImpl.cpp, but if we did that, WebViewImpl.cpp would grow to be even more huge than it is today.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:713
> > +IntRect calculateEventNodeBoundingBox(Node* eventNode)
> 
> Again, this doesn't seem specific to Chromium and seems like something that could live in WebCore somewhere.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:732
> > +float scoreTouchTarget(IntPoint touchPoint, int padding, IntRect boundingBox)
> 
> Should this be in a separate file in WebKit/chromium/src ?  We try to avoid putting everything in WebViewImpl.cpp.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:772
> > +    // Find event handler node in the ancestor chain for each hit test result
> 
> I wonder if a bunch of this should be moved into the same file as scoreTouchTarget

I think moving all these to WebCore/page/chromium/ may be a good idea.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:778
> > +            if (node->isDocumentNode() || node->hasTagName(HTMLNames::htmlTag) || node->hasTagName(HTMLNames::bodyTag))
> > +                break;
> 
> How does this work for SVG documents?

This is a non-exhaustive list of elements we don't want to zoom to. We added those tags because we've seen a few dumb pages that adds event handlers on the page body and disambiguation popups become annoying. So far we haven't seen many dumb SVG documents but we might have to tune it if we get reports from QA.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:798
> > +    // Only keep good touch targets with score >= max(score)/2, and calculate the zoom rect
> > +    int numberOfGoodTargets = 0;
> > +    IntRect windowZoomRect = IntRect(touchPoint, IntSize(1, 1));
> > +    windowZoomRect.inflate(touchPointPadding);
> > +    for (HashMap<Node*, TouchTargetData>::iterator it = touchTargets.begin(); it != touchTargets.end(); ++it) {
> > +        if (it->second.score < bestScore * 0.5)
> > +            continue;
> > +        numberOfGoodTargets++;
> > +        windowZoomRect.unite(it->second.windowBoundingBox);
> > +    }
> 
> This too.
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:800
> > +    // TODO: replace touch adjustment code when numberOfGoodTargets == 1?
> 
> TODO -> FIXME

Done
Comment 9 Tien-Ren Chen 2012-08-28 01:18:54 PDT
Created attachment 160927 [details]
Patch
Comment 10 WebKit Review Bot 2012-08-28 01:26:36 PDT
Comment on attachment 160927 [details]
Patch

Attachment 160927 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13665080
Comment 11 Adam Barth 2012-08-28 08:10:04 PDT
Comment on attachment 160927 [details]
Patch

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

> Source/WebCore/page/TouchDisambiguation.cpp:46
> +namespace {

Generally, WebKit prefers to use file-level static functions rather than anonymous namespaces.

> Source/WebCore/page/TouchDisambiguation.cpp:54
> +bool isEventNode(Node *node)
> +{
> +    return node && (node->supportsFocus()
> +        || node->hasEventListeners(eventNames().clickEvent)
> +        || node->hasEventListeners(eventNames().mousedownEvent)
> +        || node->hasEventListeners(eventNames().mouseupEvent));
> +}

This looks like it should be a member function on Node.  See also the discussion in https://bugs.webkit.org/show_bug.cgi?id=94727#c6

It sounds like there's already a willRespondToMouseClickEvents.  Should we use that instead?  If not, we should move this code near that code.

> Source/WebCore/page/TouchDisambiguation.cpp:56
> +// Calculate the union of the bounding boxes of all descendant nodes that propagates events up.

This comment just explains what the code does, not why it does it.  We should remove it.

> Source/WebCore/page/TouchDisambiguation.cpp:57
> +IntRect calculateEventNodeBoundingBox(Node* eventNode)

The word "calculate" doesn't really add anything to the name of this function.  Perhaps "boundingBoxForDescendantsThatRespondToMouseEvents" ?  That might be a bit long...  boundingBoxForEventNodes ?

> Source/WebCore/page/TouchDisambiguation.cpp:60
> +    if (!eventNode->document() || !eventNode->document()->view())
> +        return IntRect();

eventNode->document() can only be 0 for DocumentType nodes.  It seems unlikely eventNode can ever be a DocumentType node (i.e. <!DOCTYPE html>).  We should remove the 0 check.

> Source/WebCore/page/TouchDisambiguation.cpp:65
> +        // skip the whole sub-tree if the node doesn't propagate events

Please use proper sentence punctuation, including initial capital letters and trailing periods.

> Source/WebCore/page/TouchDisambiguation.cpp:78
> +    float divPadding = (float)1 / padding;

What does "divPadding" mean?

> Source/WebCore/page/TouchDisambiguation.cpp:82
> +    if (boundingBox.isEmpty())
> +        return 0;

Perhaps this should be the first statement in the function.

> Source/WebCore/page/TouchDisambiguation.cpp:98
> +void listGoodTouchTargets(Vector<IntRect>& goodTargets, const IntRect& touchBox, Frame* mainFrame, float pageScaleFactor)

Typically we place the out parameters at the end of the parameter list.

> Source/WebCore/page/TouchDisambiguation.cpp:102
> +    int touchPointPadding = (max(touchBox.width(), touchBox.height()) + 1) / 2;

Why do we add 1?

> Source/WebCore/page/TouchDisambiguation.cpp:104
> +    //         We have to pre-apply page scale factor here.

You've got an extra space before the "we".

> Source/WebCore/page/TouchDisambiguation.cpp:105
> +    int padding = touchPointPadding / pageScaleFactor;

Should touchPointPadding be a float so we don't lose precision?

> Source/WebCore/page/TouchDisambiguation.cpp:113
> +    // Find event handler node in the ancestor chain for each hit test result

This comment just says what the code does, not why.  We should remove it.

> Source/WebCore/page/TouchDisambiguation.cpp:130
> +    // Only keep good touch targets with score >= max(score)/2, and calculate the zoom rect

ditto

> Source/WebCore/page/TouchDisambiguation.cpp:132
> +        if (it->second.score < bestScore * 0.5)

I might add a comment explaining why this is a good heuristic.

> Source/WebCore/page/TouchDisambiguation.h:41
> +void listGoodTouchTargets(Vector<IntRect>& goodTargets, const IntRect& touchBox, Frame* mainFrame, float pageScaleFactor);

list -> find
Comment 12 Adam Barth 2012-08-28 08:13:11 PDT
Comment on attachment 160927 [details]
Patch

This patch looks pretty good.  The comments above are all just minor nit picks.  Let's iterate on the patch once more and then we'll likely be ready to land it.  Thanks!
Comment 13 Adam Barth 2012-08-28 08:19:24 PDT
Comment on attachment 160927 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:16
> +        FIXME: add tests

I should mention that you can probably test this patch by adding a unit test in WebFrameTests.cpp (in Source/WebKit/chromium/tests).
Comment 14 Tien-Ren Chen 2012-08-28 14:58:29 PDT
(In reply to comment #11)
> (From update of attachment 160927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160927&action=review
> 
> > Source/WebCore/page/TouchDisambiguation.cpp:46
> > +namespace {
> 
> Generally, WebKit prefers to use file-level static functions rather than anonymous namespaces.

Okay, but I'll need an anonymous namespace for struct TouchTargetData anyway. Do we still prefer static function in this case?

> > Source/WebCore/page/TouchDisambiguation.cpp:54
> > +bool isEventNode(Node *node)
> > +{
> > +    return node && (node->supportsFocus()
> > +        || node->hasEventListeners(eventNames().clickEvent)
> > +        || node->hasEventListeners(eventNames().mousedownEvent)
> > +        || node->hasEventListeners(eventNames().mouseupEvent));
> > +}
> 
> This looks like it should be a member function on Node.  See also the discussion in https://bugs.webkit.org/show_bug.cgi?id=94727#c6
> 
> It sounds like there's already a willRespondToMouseClickEvents.  Should we use that instead?  If not, we should move this code near that code.

Looks interesting. There is some subtle difference but generally it catches everything we want to catch. I think we can try that first.

> > Source/WebCore/page/TouchDisambiguation.cpp:56
> > +// Calculate the union of the bounding boxes of all descendant nodes that propagates events up.
> 
> This comment just explains what the code does, not why it does it.  We should remove it.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:57
> > +IntRect calculateEventNodeBoundingBox(Node* eventNode)
> 
> The word "calculate" doesn't really add anything to the name of this function.  Perhaps "boundingBoxForDescendantsThatRespondToMouseEvents" ?  That might be a bit long...  boundingBoxForEventNodes ?

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:60
> > +    if (!eventNode->document() || !eventNode->document()->view())
> > +        return IntRect();
> 
> eventNode->document() can only be 0 for DocumentType nodes.  It seems unlikely eventNode can ever be a DocumentType node (i.e. <!DOCTYPE html>).  We should remove the 0 check.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:65
> > +        // skip the whole sub-tree if the node doesn't propagate events
> 
> Please use proper sentence punctuation, including initial capital letters and trailing periods.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:78
> > +    float divPadding = (float)1 / padding;
> 
> What does "divPadding" mean?

Cached reciprocal for DIVision ... Yea div can be confused with <div>, let me change this.

> > Source/WebCore/page/TouchDisambiguation.cpp:82
> > +    if (boundingBox.isEmpty())
> > +        return 0;
> 
> Perhaps this should be the first statement in the function.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:98
> > +void listGoodTouchTargets(Vector<IntRect>& goodTargets, const IntRect& touchBox, Frame* mainFrame, float pageScaleFactor)
> 
> Typically we place the out parameters at the end of the parameter list.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:102
> > +    int touchPointPadding = (max(touchBox.width(), touchBox.height()) + 1) / 2;
> 
> Why do we add 1?

Rounds up

> > Source/WebCore/page/TouchDisambiguation.cpp:104
> > +    //         We have to pre-apply page scale factor here.
> 
> You've got an extra space before the "we".

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:105
> > +    int padding = touchPointPadding / pageScaleFactor;
> 
> Should touchPointPadding be a float so we don't lose precision?

I think it's ok. The semantics is more like padding=pixelSnapped(touchPointPadding.scale(1/pageScaleFactor)) as in transformation. We do need to make it round up though.

> > Source/WebCore/page/TouchDisambiguation.cpp:113
> > +    // Find event handler node in the ancestor chain for each hit test result
> 
> This comment just says what the code does, not why.  We should remove it.

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:130
> > +    // Only keep good touch targets with score >= max(score)/2, and calculate the zoom rect
> 
> ditto

Done

> > Source/WebCore/page/TouchDisambiguation.cpp:132
> > +        if (it->second.score < bestScore * 0.5)
> 
> I might add a comment explaining why this is a good heuristic.

Done

> > Source/WebCore/page/TouchDisambiguation.h:41
> > +void listGoodTouchTargets(Vector<IntRect>& goodTargets, const IntRect& touchBox, Frame* mainFrame, float pageScaleFactor);
> 
> list -> find

Done
Comment 15 Adam Barth 2012-08-28 15:49:29 PDT
> > View in context: https://bugs.webkit.org/attachment.cgi?id=160927&action=review
> > 
> > > Source/WebCore/page/TouchDisambiguation.cpp:46
> > > +namespace {
> > 
> > Generally, WebKit prefers to use file-level static functions rather than anonymous namespaces.
> 
> Okay, but I'll need an anonymous namespace for struct TouchTargetData anyway. Do we still prefer static function in this case?

We can also just leak the symbol for TouchTargetData.  It's not a big deal either way.
Comment 16 Adam Barth 2012-08-28 15:51:07 PDT
*** Bug 92043 has been marked as a duplicate of this bug. ***
Comment 17 Tien-Ren Chen 2012-08-28 20:29:00 PDT
Created attachment 161122 [details]
Patch
Comment 18 Adam Barth 2012-08-28 20:33:58 PDT
Comment on attachment 161122 [details]
Patch

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

Thanks!

> Source/WebCore/page/TouchDisambiguation.cpp:32
> +

We usually skip this blank line.
Comment 19 Adam Barth 2012-08-28 20:34:33 PDT
Looks like you've got a merge conflict in WebViewImpl.cpp.  Otherwise, this looks ready to land.
Comment 20 Tien-Ren Chen 2012-08-29 14:55:33 PDT
Created attachment 161322 [details]
Patch
Comment 21 Tien-Ren Chen 2012-08-29 14:56:02 PDT
(In reply to comment #20)
> Created an attachment (id=161322) [details]
> Patch

Nothing changed. Rebase only.
Comment 22 WebKit Review Bot 2012-08-29 21:08:15 PDT
Comment on attachment 161322 [details]
Patch

Clearing flags on attachment: 161322

Committed r127095: <http://trac.webkit.org/changeset/127095>
Comment 23 WebKit Review Bot 2012-08-29 21:08:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Allan Sandfeld Jensen 2012-09-04 02:19:57 PDT
I think this should be merged with the code in page/TouchAdjustment.cpp. The main difference seems to be the result is a number of "good" targets instead of a single one?
Comment 25 Adam Barth 2012-09-04 11:02:01 PDT
(In reply to comment #24)
> I think this should be merged with the code in page/TouchAdjustment.cpp. The main difference seems to be the result is a number of "good" targets instead of a single one?

Would you be willing to file a new bug to this effect and CC trchen and myself?
Comment 26 Tien-Ren Chen 2012-09-04 11:15:40 PDT
(In reply to comment #24)
> I think this should be merged with the code in page/TouchAdjustment.cpp. The main difference seems to be the result is a number of "good" targets instead of a single one?

Yes I'm also aware of the similarity of these two classes.

My priority was to upstream whatever we have to make things work, but in the long term I'm happy to try to unify them.

Also, I'm not certain but there may be some weird patent issue. Let me CC klobag@ to make sure there will be no potential troubles.
Comment 27 Adam Barth 2012-09-04 12:01:26 PDT
> My priority was to upstream whatever we have to make things work, but in the long term I'm happy to try to unify them.

It's important that we don't make a mess while upstreaming this code.