[Slimming Paint] Paint the inspector overlay with GraphicsLayer DisplayList.

[Slimming Paint] Paint the inspector overlay with GraphicsLayer DisplayList.

Since some overlays are implemented in Chromium using WebCanvas,
this makes blink::PageOverlay capable of being painted with either
DisplayList or WebCanvas (in the latter case, one DrawingDisplayItem
is emitted).

Makes the inspector overlay work with slimming paint enabled.

Only one GraphicsLayer is used, and only full invalidation is
here right now, but adding partial invalidation support will
likely come "for free" when compositor layerization is
implemented.

Also adds unit tests for overlay painting. I'd also like to have
a LayoutTest which uses the proper inspector overlay, but that's
probably a separate patch (requires insight from dev tools folk).

BUG=450761
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189950

[jbroman, 2015-02-09 15:06:34]

jbroman@chromium.org changed reviewers:
+ pdr@chromium.org, schenney@chromium.org, wangxianzhu@chromium.org

[jbroman, 2015-02-09 15:06:35]

Someone else already fixed the crash issue, but I'd still like to land this:
drawing with the original DisplayItemList belonging to the GraphicsLayer seems
cleaner and likely to open the door to making this path less special.

This CL is big, yes, but most of the LOC is a new unit test. :)

[Xianzhu, 2015-02-09 17:57:11]

I like this kind of improvement, but I think it would be better to avoid the
parallel paths. Can we go further, e.g. to let the Web interfaces (WebCanvas,
WebPageOverlay, etc.) support display list? (Perhaps a new interface
WebGraphicsContext to replace WebCanvas?)

https://codereview.chromium.org/867063004/diff/260001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/867063004/diff/260001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:74: DrawingLast = PageOverlay,
Please keep the above list sorted.

[jbroman, 2015-02-09 18:13:37]

On 2015/02/09 17:57:11, Xianzhu wrote:
> I like this kind of improvement, but I think it would be better to avoid the
> parallel paths. Can we go further, e.g. to let the Web interfaces (WebCanvas,
> WebPageOverlay, etc.) support display list? (Perhaps a new interface
> WebGraphicsContext to replace WebCanvas?)

Do we want to reflect the GraphicsContext creating-display-items interface
back across the API boundary? Currently this would be the only use, and that
interface is subject to a lot of change...

One simple version might be something like:

class WebGraphicsContext {
public:
  // Creates a DrawingRecorder with a particular display item ID
  // specified when the WebGraphicsContext is created by Blink.
  WebCanvas* beginDrawing() = 0;
  void endDrawing() = 0;

#if BLINK_IMPLEMENTATION
  GraphicsContext& graphicsContext() = 0;
#endif
};

With the use looking something like:

WebGraphicsContextImpl contextWrapper(graphicsContext, displayItemClient(),
DisplayItem::SomeDrawing);
webPageOverlay->paintOverlay(&contextWrapper);

Which would let the Chromium side get a canvas for simple drawing, and Blink
get a proper GraphicsContext it can pass to the painting code.

Exposing all the other stuff with the display lists (clients, types, etc.)
requires
making the Blink API boundary a lot wider than it is right now.

It's worth noting that currently the only implementations of WebPageOverlay are:
- the inspector overlay (implemented in Blink)
- a few solid-color overlays (one in content, one in chrome)

So you could also make the argument that we should just kill the WebPageOverlay
interface and just give WebView an "addSolidColorOverlay" method. I'm scared to
add a lot of complexity for what is essentially that feature right now.

https://codereview.chromium.org/867063004/diff/260001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/867063004/diff/260001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:74: DrawingLast = PageOverlay,
On 2015/02/09 17:57:10, Xianzhu wrote:
> Please keep the above list sorted.

Ah, I hadn't realized it was sorted (I saw D, B, C and assumed it was
order-of-creation). Fixed.

[Xianzhu, 2015-02-09 18:41:04]

On 2015/02/09 18:13:37, jbroman wrote:
> On 2015/02/09 17:57:11, Xianzhu wrote:
> > I like this kind of improvement, but I think it would be better to avoid the
> > parallel paths. Can we go further, e.g. to let the Web interfaces
(WebCanvas,
> > WebPageOverlay, etc.) support display list? (Perhaps a new interface
> > WebGraphicsContext to replace WebCanvas?)
> 
> Do we want to reflect the GraphicsContext creating-display-items interface
> back across the API boundary? Currently this would be the only use, and that
> interface is subject to a lot of change...
> 
> One simple version might be something like:
> 
> class WebGraphicsContext {
> public:
>   // Creates a DrawingRecorder with a particular display item ID
>   // specified when the WebGraphicsContext is created by Blink.
>   WebCanvas* beginDrawing() = 0;
>   void endDrawing() = 0;
> 
> #if BLINK_IMPLEMENTATION
>   GraphicsContext& graphicsContext() = 0;
> #endif
> };
> 
> With the use looking something like:
> 
> WebGraphicsContextImpl contextWrapper(graphicsContext, displayItemClient(),
> DisplayItem::SomeDrawing);
> webPageOverlay->paintOverlay(&contextWrapper);
> 
> Which would let the Chromium side get a canvas for simple drawing, and Blink
> get a proper GraphicsContext it can pass to the painting code.
> 

This seems good.

Or can we just simply extend WebCanvas:

class WebCanvas : public SkCanvas {
#if BLINK_IMPLEMENTATION
    // Constructor with GraphicsContext and graphicsContext() getter.
#endif
};

> Exposing all the other stuff with the display lists (clients, types, etc.)
> requires
> making the Blink API boundary a lot wider than it is right now.
> 
> It's worth noting that currently the only implementations of WebPageOverlay
are:
> - the inspector overlay (implemented in Blink)
> - a few solid-color overlays (one in content, one in chrome)
> 
> So you could also make the argument that we should just kill the
WebPageOverlay
> interface and just give WebView an "addSolidColorOverlay" method. I'm scared
to
> add a lot of complexity for what is essentially that feature right now.
>
>
https://codereview.chromium.org/867063004/diff/260001/Source/platform/graphic...
> File Source/platform/graphics/paint/DisplayItem.h (right):
> 
>
https://codereview.chromium.org/867063004/diff/260001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItem.h:74: DrawingLast = PageOverlay,
> On 2015/02/09 17:57:10, Xianzhu wrote:
> > Please keep the above list sorted.
> 
> Ah, I hadn't realized it was sorted (I saw D, B, C and assumed it was
> order-of-creation). Fixed.

[jbroman, 2015-02-09 18:44:02]

On 2015/02/09 18:41:04, Xianzhu wrote:
> Or can we just simply extend WebCanvas:
> 
> class WebCanvas : public SkCanvas {
> #if BLINK_IMPLEMENTATION
>     // Constructor with GraphicsContext and graphicsContext() getter.
> #endif
> };

I don't think that will work; WebCanvas is frequently used to pass SkCanvas
created on either side to the other. I don't think we can make it anything other
than a typedef.

[jbroman, 2015-02-09 23:44:22]

Alright, refactored to add a GraphicsContext/WebCanvas shim type to the Blink
API as requested.

This CL is slightly larger, mostly due to the paperwork required to add a Blink
API type.

If approved, this would also require two followup changes (one to Chromium, one
to Blink) to clean up the API change.

[jbroman, 2015-02-10 23:29:12]

On 2015/02/09 23:44:22, jbroman wrote:
> Alright, refactored to add a GraphicsContext/WebCanvas shim type to the Blink
> API as requested.
> 
> This CL is slightly larger, mostly due to the paperwork required to add a
Blink
> API type.
> 
> If approved, this would also require two followup changes (one to Chromium,
one
> to Blink) to clean up the API change.

bump

[Xianzhu, 2015-02-10 23:34:02]

lgtm (non-owner).

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebDevToolsA...
File Source/web/WebDevToolsAgentImpl.h (right):

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebDevToolsA...
Source/web/WebDevToolsAgentImpl.h:50: class IntPoint;
Nit: Is this used?

https://codereview.chromium.org/867063004/diff/340001/public/web/WebGraphicsC...
File public/web/WebGraphicsContext.h (right):

https://codereview.chromium.org/867063004/diff/340001/public/web/WebGraphicsC...
public/web/WebGraphicsContext.h:16: class WebGraphicsContext {
Nit: Can we just make it a concrete class (like wrappers such as WebNode etc)?

[pdr., 2015-02-10 23:47:09]

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebGraphicsC...
File Source/web/WebGraphicsContextImpl.h (right):

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebGraphicsC...
Source/web/WebGraphicsContextImpl.h:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
Can we combine this with WebGraphicsContext?

[jbroman, 2015-02-11 00:32:55]

tl;dr: Yes, we could merge them, but complications arise, and I think this is
cleaner than the other solutions I'm aware of.

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebDevToolsA...
File Source/web/WebDevToolsAgentImpl.h (right):

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebDevToolsA...
Source/web/WebDevToolsAgentImpl.h:50: class IntPoint;
On 2015/02/10 23:34:01, Xianzhu wrote:
> Nit: Is this used?

Yes. m_lastPinchAnchorDip requires this forward declaration, and changes to
include files elsewhere caused this declaration to be missing.

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebGraphicsC...
File Source/web/WebGraphicsContextImpl.h (right):

https://codereview.chromium.org/867063004/diff/340001/Source/web/WebGraphicsC...
Source/web/WebGraphicsContextImpl.h:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2015/02/10 23:47:08, pdr wrote:
> Can we combine this with WebGraphicsContext?

The interface/impl pattern seems to be more common, from what I can tell. It's
not impossible, but it would also require:

1. guarding references to GraphicsContext with BLINK_IMPLEMENTATION or similar
2. declaring DisplayItemClient (and you cannot forward-declare typedefs nor can
you include platform/graphics from public/, so this would require
forward-declaring DisplayItemClientInternalVoid as well)
3. declaring DisplayItem::Type (and enums within classes cannot be
forward-declared, so this would require the full definition of DisplayItem,
which cannot be included from here)

2 and 3 could be fixed by with another level of indirection (the so-called PIMPL
pattern; forward-declaring a private struct, holding it in an OwnPtr in
WebGraphicsContext, and then providing the definition in a source file). That
would look something like:

class WebGraphicsContext {
public:
    // ...
private:
    struct PrivateData;
    WebPrivateOwnPtr<PrivateData> m_privateData;
};

I'm not sure that's a win in readability, though. It also would make it harder
to mock out WebGraphicsContext for test (for example, the Chromium-side
implementations of WebPageOverlay could be unit tested by providing a mock
implementation of WebGraphicsContext).

https://codereview.chromium.org/867063004/diff/340001/public/web/WebGraphicsC...
File public/web/WebGraphicsContext.h (right):

https://codereview.chromium.org/867063004/diff/340001/public/web/WebGraphicsC...
public/web/WebGraphicsContext.h:16: class WebGraphicsContext {
On 2015/02/10 23:34:01, Xianzhu wrote:
> Nit: Can we just make it a concrete class (like wrappers such as WebNode etc)?

See reply to pdr's comment.

[pdr., 2015-02-11 00:44:03]

LGTM

[jbroman, 2015-02-11 05:45:16]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-02-11 05:46:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/867063004/340001

[commit-bot: I haz the power, 2015-02-11 05:51:51]

Committed patchset #18 (id:340001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189950