Bug 92043

Created attachment 153924 [details] Patch

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.

@fishd: This is the software read back API for link preview that we discussed briefly yesterday.

Comment on attachment 153924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153924&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:1651 > +static PageWidgetDelegate::CanvasBackground canvasBackgroundForTransparencey(bool isTransparent) nit: avoid interleaving non-class methods with class methods. move to the top of the file. interleaved non-class methods tend to attract more non-class methods, which then seems to lead to a messy .cpp file. > Source/WebKit/chromium/src/WebViewImpl.cpp:1681 > + FrameView* view = page()->mainFrame()->view(); It is not obvious why this is called paintOnDemandZoom. I was expecting to see some code here that triggered a temporary zoom of the content. As is, it just seems like the paint() method except that it forces software rendering and compositing. It seems like WebViewImpl::paint() might produce exactly the same result. In the accelerated case, it'll just do the work on the GPU and then read back the pixels. Do we really need this new paint function?

(In reply to comment #4) > (From update of attachment 153924 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153924&action=review > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1681 > > + FrameView* view = page()->mainFrame()->view(); > > It is not obvious why this is called paintOnDemandZoom. I was expecting > to see some code here that triggered a temporary zoom of the content. > As is, it just seems like the paint() method except that it forces > software rendering and compositing. It seems like WebViewImpl::paint() > might produce exactly the same result. In the accelerated case, it'll > just do the work on the GPU and then read back the pixels. The intent is to apply the zoom by scaling the canvas that gets passed in. > Do we really need this new paint function? The comment above the function says this function avoids aliasing, but I wonder if that comment is still correct. @trchen: Do you know if we still need this second painting function?

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 153924 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=153924&action=review > > > > > Source/WebKit/chromium/src/WebViewImpl.cpp:1681 > > > + FrameView* view = page()->mainFrame()->view(); > > > > It is not obvious why this is called paintOnDemandZoom. I was expecting > > to see some code here that triggered a temporary zoom of the content. > > As is, it just seems like the paint() method except that it forces > > software rendering and compositing. It seems like WebViewImpl::paint() > > might produce exactly the same result. In the accelerated case, it'll > > just do the work on the GPU and then read back the pixels. > > The intent is to apply the zoom by scaling the canvas that gets passed in. > > > Do we really need this new paint function? > > The comment above the function says this function avoids aliasing, but I wonder if that comment is still correct. > > @trchen: Do you know if we still need this second painting function? Yes, this function basically works the same as WebViewImpl::paint() in pure sof tware rendering path. The purpose is to explicitly avoid reusing composted result, which is essentially resampling a low-res image. I agree the behavior of the function is a little bit confusing in the sense that the scale matrix is actually applied by the client in advance. As for the function name, I think paintOnDemandZoom should be appropriate since it's sole purpose is for the on-demand zoom to redraw a high-res image. And I believe the function is only a temporary solution until we get something more powerful such as SkPicture and UberCompositor.

Comment on attachment 153924 [details] Patch I'll update this patch based on the discussion above.

Created attachment 155903 [details] Patch

This appears to be solving some of the same goals as bug 92275.

Comment on attachment 155903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155903&action=review We have a few different versions of "software" rendering when composited: 1.) Composite and do a readback 2.) Software render for printing - ignores 3d transforms, but tries to do a readback of GPU-resident content 3.) Software flattened rendering - ignores 3d transforms, ignores GPU-resident content Should we try to call out the different in WebWidget.h? The print behavior is currently down on WebFrame (I'm not sure exactly why) and it's pretty subtle. This behavior seems like the right one for link highlighting, at least in most circumstances - there will be cases where it misses the desired rect by an epic amount (due to ignoring 3d) but I'm guessing it's good enough. > Source/WebKit/chromium/public/WebWidget.h:97 > + // Force the widget to rerender onto the canvas using sofware. typo "sofware" I think you should note that this will result in some content not showing up as expected (i.e. ignoring 3d transforms) and some content to not show up at all (video, canvas, webgl).

Created attachment 157790 [details] Patch

Comment on attachment 157790 [details] Patch Attachment 157790 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13472499

Created attachment 157807 [details] Patch

Comment on attachment 157807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157807&action=review Works for me, but I understand Darin had some API feedback so I'll defer to him for the final review. > Source/WebKit/chromium/public/WebWidget.h:102 > + // Note: This option exists on OS(ANDROID) and will hopefully be did you want to hide the enum value too (is that possible in an API header)?

> > Source/WebKit/chromium/public/WebWidget.h:102 > > + // Note: This option exists on OS(ANDROID) and will hopefully be > > did you want to hide the enum value too (is that possible in an API header)? I wanted to, but I couldn't find an example. If there's a good way to do this, let me know and I'll be happy to use it.

> Works for me, but I understand Darin had some API feedback so I'll defer to him for the final review. @fishd: You mentioned you might have some feedback. Thanks.

@jamesr: It sounds like fishd is on vacation. Do you still want to wait for his feedback on this patch before landing it?

I don't know what his feedback was going to be, but he said he had some. I suppose we could land and then fix it up later if you need it to land in the next week - is it blocking anything specific?

Yes. This one of three patches blocking API compatibility between upstream and downstream chromium-android.

Comment on attachment 157807 [details] Patch OK. I think this looks reasonable and we can iterate when Darin gets back if we need to make more changes.

Thanks. @fishd: If you have comments, please don't hesitate to ask for a followup patch.

Comment on attachment 157807 [details] Patch Rejecting attachment 157807 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ChangeLog CONFLICT (content): Merge conflict in Source/WebKit/chromium/ChangeLog Failed to merge in the changes. Patch failed at 0001 Remove redundant TOUCH_LISTENER event type When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/13539804

Comment on attachment 157807 [details] Patch Clearing flags on attachment: 157807 Committed r126095: <http://trac.webkit.org/changeset/126095>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 94555

This guy broke: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_widget_fullscreen_pepper.cc&exact_package=chromium&q=render_widget_fullscreen_pepper&l=72

Downstream change in https://chromiumcodereview.appspot.com/10828420

Created attachment 159724 [details] now with #define

Created attachment 159989 [details] Patch for landing

Comment on attachment 159989 [details] Patch for landing Clearing flags on attachment: 159989 Committed r126344: <http://trac.webkit.org/changeset/126344>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 95253

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

With the reworked disambiguation popup we still need the ForceSoftware flag. Chromium side CL: http://codereview.chromium.org/10885004/ Can we reopen this?

> Can we reopen this? Oh! I misunderstood. Sorry for the confusion.

I'm rolling out the rollout.