Bug 79150 – [chromium] Add Link Preview behavior to WebViewImpl

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Michael Feldstein]

[chromium] Add Link Preview behavior to WebViewImpl

[Michael Feldstein]

Created attachment 128049 [details]
Patch

[Michael Feldstein]

Created attachment 128058 [details]
Patch

[Adam Barth]

Comment on attachment 128058 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:35
> +#if defined(ANDROID)

This functionality probably shouldn't be dependent on ANDROID.  It should be available all the time.

[Adam Barth]

fishd should probably review this patch.

The main question here is which layer should do this work.  You're adding a lot of code to the WebKit layer, which is something we usually try to avoid.  It's unclear to me whether this work should move into WebCore or should be moved into the embedder.

[Michael Feldstein]

Let me know if there's a better place to put this and I'll try to change it.

[WebKit Commit Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 128058 [details]
Patch

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

>> Source/WebKit/chromium/public/WebView.h:35
>> +#if defined(ANDROID)
> 
> This functionality probably shouldn't be dependent on ANDROID.  It should be available all the time.

Agreed.

> Source/WebKit/chromium/public/WebView.h:44
> +struct WebTouchCandidatesInfo {

style nits:
1- each top-level type gets its own header file.
2- this struct should be declared within the WebKit namespace.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3183
> +struct TouchNodeData {

This is way too much code to be adding to WebView.  You need to refactor this into some self-contained piece.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3184
> +    Node* mNode;

style nit: public member variables do not have a "m" prefix.  they just use normal variable naming style.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3318
> +    outTouchInfo->numberOfCandidates = 0;

nit: this function is massive.  it needs to be decomposed into smaller units.

I'm not sure how to best advise you on improving the design here.  It might help
to step back and have you educate a WebKit reviewer about what you are trying to
accomplish.

[Adam Barth]

Michael, do you have thoughts on fishd's comments above?  I tend to agree with fishd that 230 line functions are too large to be maintainable.  The path forward here is likely to discuss how best to factor this code into something more maintainable.

[Michael Feldstein]

I no longer work on this.  I'll ask the new owner to follow up on it.

We did talk about a design where all the code is moved into the client, and WebView would just expose a hitTestWithPadding method, which would minimize the amount of code needed in webkit.

[Adam Barth]

Who is the new owner?

[Michael Feldstein]

Tien-Ren Chen I believe.  I've forwarded the bug to him.

[Tien-Ren Chen]

(In reply to comment #9)
> I no longer work on this.  I'll ask the new owner to follow up on it.

Hi all, I'll be responsible for upstreaming our link preview code.

(In reply to comment #8)
> Michael, do you have thoughts on fishd's comments above?  I tend to agree with fishd that 230 line functions are too large to be maintainable.  The path forward here is likely to discuss how best to factor this code into something more maintainable.

Totally agree. Our WebViewImpl::getTouchHighlightQuads function is multiplexed with too many functionality at this time, including
* hit test & touch targets disambiguation
* find out the highlight quads (when only 1 touch target is available)
* find out a reasonable rect for link preview (when 2 or more touch targets are available)

Currently I'm working on the first and second part, cooperating with CrOS people who also want link highlighting.

For the hit test & disambiguation code I'm still trying to understand it and refactor it. In the meantime the CrOS people will reuse EventHandler.cpp::findBestClickableCandidate.

The highlight quads code has been rewritten to support threaded scrolling/animation, currently waiting for review:
https://bugs.webkit.org/show_bug.cgi?id=84936 --- depends on --> https://bugs.webkit.org/show_bug.cgi?id=85725

As for link preview, I'm not certain what is the current plan on this. As far as I know we're going to allow panning in the preview area, and move much of the functionality into the compositor. Need to discuss that after getting the highlight to work first.

[Adam Barth]

Hi trchen.  If you run into any difficulty getting patches reviewed, please let me know.  I can help you find appropriate reviews.  Thanks for picking up this work.

[Tien-Ren Chen]

The work has been replaced by https://bugs.webkit.org/show_bug.cgi?id=94182

[Adam Barth]

*** This bug has been marked as a duplicate of bug 94182 ***