Bug 79150

Created attachment 128049 [details] Patch

Created attachment 128058 [details] Patch

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.

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.

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

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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.

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.

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.

Who is the new owner?

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

(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.

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.

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

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