Remove the composited path from WebViewImpl::paint().

Remove the composited path from WebViewImpl::paint().

Now that we have WebWidget::compositeAndReadbackAsync() for layout
tests and WebWidget::paintCompositedDeprecated() for android, the
WebWidget::paint() method is only used for actually painting into
a GraphicsContext when the WebViewImpl is not composited.

Remove the composited path from this method and ASSERT that the
WebViewImpl is not being composited. Also remove the
WebViewImpl::doPixelReadbackToCanvas().

Depends on: https://codereview.chromium.org/266243002/
Depends on: https://codereview.chromium.org/276003002/

R=abarth, enne, aelias
BUG=251960, 365810
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173789

[danakj, 2014-05-02 23:12:53]

https://codereview.chromium.org/264713014/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/264713014/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1786: #if OS(ANDROID)
This OS(ANDROID) stuff is a disaster.

abarth: WDYT of adding another WebWidget::paintCompositedWebViewDeprecated()
method for android that wraps a call to paint() in this setPaintBehaviour stuff,
so it's clear when this is happening and that it's wrong?

[abarth-chromium, 2014-05-02 23:15:46]

I'm not sure what this CL gains us.  This codepath is already only reachable on
Android.

[enne (OOO), 2014-05-02 23:16:23]

https://codereview.chromium.org/264713014/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/264713014/diff/40001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1770: void WebViewImpl::paint(WebCanvas* canvas,
const WebRect& rect)
I do think this needs to be split into two functions for readability at least. 
If you don't want to change the public API in this patch, could you add an
internal paintAndroid and paintNonComposited (or whatever) and just have one
#ifdef?

[danakj, 2014-05-02 23:17:12]

On Fri, May 2, 2014 at 7:15 PM, <abarth@chromium.org> wrote:

> I'm not sure what this CL gains us.  This codepath is already only
> reachable on
> Android.
>

I've removing the call to composteAndReadback, ie the if() portion of the
if/else.


>
> https://codereview.chromium.org/264713014/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2014-05-02 23:20:39]

On 2014/05/02 23:15:46, abarth wrote:
> I'm not sure what this CL gains us.  This codepath is already only reachable
on
> Android.

Also, this code path is reached by anything that calls paint() when
isAccleratedCompositing is off, including things like LayoutTests (for now) and
WebViewPlugin. I think there's a few more.

[abarth-chromium, 2014-05-02 23:21:12]

On 2014/05/02 23:17:12, danakj wrote:
> I've removing the call to composteAndReadback, ie the if() portion of the
> if/else.

Ah!  That makes a lot of sense.

+1 to enne's comment about splitting this function into two pieces.

[danakj, 2014-05-02 23:24:51]

Updated on top of https://codereview.chromium.org/269723008/. This is how it
will look with the android thing split off into its own method. Now ::paint() is
only for use by things that call paint on a software mode WebView such as
WebViewPlugin.

[aelias_OOO_until_Jul13, 2014-05-02 23:31:46]

lgtm.  We'll get tap disambiguation onto the async readback path eventually, but
this is a good incremental step.

[enne (OOO), 2014-05-02 23:43:19]

lgtm2

[abarth-chromium, 2014-05-03 01:23:44]

public/ LGTM

[danakj, 2014-05-06 15:28:36]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-06 15:29:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/80001

[commit-bot: I haz the power, 2014-05-06 15:29:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-06 15:29:08]

Failed to apply patch for Source/web/WebViewImpl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/WebViewImpl.h
  Hunk #1 FAILED at 102.
  Hunk #2 succeeded at 550 (offset 3 lines).
  1 out of 2 hunks FAILED -- saving rejects to file Source/web/WebViewImpl.h.rej

Patch:       Source/web/WebViewImpl.h
Index: Source/web/WebViewImpl.h
diff --git a/Source/web/WebViewImpl.h b/Source/web/WebViewImpl.h
index
c23864b1fcb14dd4a3aa59e5d51703bf57d94edf..d10246f67d8fa5cdc72b05d4d05a11d0bc7120d4
100644
--- a/Source/web/WebViewImpl.h
+++ b/Source/web/WebViewImpl.h
@@ -102,7 +102,8 @@ public:
     virtual void animate(double) OVERRIDE;
     virtual void layout() OVERRIDE;
     virtual void enterForceCompositingMode(bool enable) OVERRIDE;
-    virtual void paint(WebCanvas*, const WebRect&, PaintOptions =
ReadbackFromCompositorIfAvailable) OVERRIDE;
+    virtual void paint(WebCanvas*, const WebRect&) OVERRIDE;
+    virtual void paintCompositedDeprecated(WebCanvas*, const WebRect&)
OVERRIDE;
     virtual bool
compositeAndReadbackAsync(WebCompositeAndReadbackAsyncCallback*) OVERRIDE;
     virtual bool isTrackingRepaints() const OVERRIDE;
     virtual void themeChanged() OVERRIDE;
@@ -547,7 +548,6 @@ private:
 
     void setIsAcceleratedCompositingActive(bool);
     void doComposite();
-    void doPixelReadbackToCanvas(WebCanvas*, const WebCore::IntRect&);
     void reallocateRenderer();
     void updateLayerTreeViewport();
     void updateLayerTreeBackgroundColor();

[danakj, 2014-05-06 15:32:36]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-06 15:32:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/100001

[commit-bot: I haz the power, 2014-05-06 17:07:49]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
  linux_blink_rel on tryserver.blink
  mac_blink_rel on tryserver.blink
  win_blink_rel on tryserver.blink

[danakj, 2014-05-06 17:08:17]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-05-07 22:06:56]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-07 22:07:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/100001

[danakj, 2014-05-07 22:41:41]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-05-07 22:42:13]

Looks like there are tests in webkit_unit_tests that call WebViewImpl::paint()
but aren't composited, I will investigate.

[danakj, 2014-05-07 23:18:02]

I fixed the WebViewTest.SetBaseBackgroundColorAndBlendWithExistingContent test
by turning off compositing on that WebView it creates, since it wants to call
paint() on that WebView.

[danakj, 2014-05-07 23:18:08]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-07 23:19:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/120001

[danakj, 2014-05-07 23:23:35]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-05-07 23:24:24]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-07 23:24:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/120001

[commit-bot: I haz the power, 2014-05-08 00:35:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
  linux_blink_rel on tryserver.blink
  mac_blink_rel on tryserver.blink
  win_blink_rel on tryserver.blink

[commit-bot: I haz the power, 2014-05-08 01:57:24]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
  linux_blink_rel on tryserver.blink
  mac_blink_rel on tryserver.blink
  win_blink_rel on tryserver.blink

[commit-bot: I haz the power, 2014-05-08 02:31:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-08 02:31:21]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink

[danakj, 2014-05-08 15:43:36]

Confusingly, these tests appear to be calling WebViewImpl::paint() on a
composited WebView still..

virtual/implsidepainting/inspector/timeline/timeline-paint.html
virtual/deferred/inspector/timeline/timeline-paint.html
virtual/deferred/inspector/timeline/timeline-gc-event.html
virtual/gpu/fast/canvas/webgl/webgl-composite-modes-repaint.html
virtual/deferred/inspector/timeline/timeline-decode-resize.html
virtual/implsidepainting/inspector/timeline/timeline-gc-event.html
fast/canvas/webgl/webgl-composite-modes-repaint.html

[danakj, 2014-05-08 17:14:03]

On 2014/05/08 15:43:36, danakj wrote:
> Confusingly, these tests appear to be calling WebViewImpl::paint() on a
> composited WebView still..
> 
> virtual/implsidepainting/inspector/timeline/timeline-paint.html
> virtual/deferred/inspector/timeline/timeline-paint.html
> virtual/deferred/inspector/timeline/timeline-gc-event.html
> virtual/gpu/fast/canvas/webgl/webgl-composite-modes-repaint.html
> virtual/deferred/inspector/timeline/timeline-decode-resize.html
> virtual/implsidepainting/inspector/timeline/timeline-gc-event.html
> fast/canvas/webgl/webgl-composite-modes-repaint.html

Aha, these are all cases of displayThenAsync(), and they are fixed by
https://codereview.chromium.org/266243002/ so I'll wait for that to land first.

[danakj, 2014-05-09 16:56:40]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-09 16:57:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/140001

[commit-bot: I haz the power, 2014-05-09 18:26:55]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7099)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6840)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/6338)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6808)

[danakj, 2014-05-09 18:28:31]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-05-09 18:29:16]

Now depends on https://codereview.chromium.org/276003002/ as well, to resolve
some new failures with this in some virtual test suites where they appear to
enter compositing mode during the software paint() path. FCM solves all.

[danakj, 2014-05-09 19:33:26]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-09 19:35:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/140001

[danakj, 2014-05-09 20:40:15]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-09 20:41:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/264713014/160001

[commit-bot: I haz the power, 2014-05-09 21:54:35]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6836)

[commit-bot: I haz the power, 2014-05-09 22:56:16]

Change committed as 173789