Bug 73235 – [Chromium] Support adding/removing page overlay to WebView

• Component: New Bugs
• Status: RESOLVED FIXED

[xiyuan]

Background dimming when a tab modal dialog shows up does not work when WebKit accelerated compositing is on (http://crbug.com/103386). The best way to fix the problem across platforms would probably be implementing the dimming as a PageOverlay (introduced in https://bugs.webkit.org/show_bug.cgi?id=62149). To do that, we need to support multiple page overlays in WebViewImpl and open an API to add/remove page overlays.

[xiyuan]

Created attachment 116793 [details]
Proposed patch.

[WebKit Review Bot]

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

[James Robinson]

Comment on attachment 116793 [details]
Proposed patch.

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

> Source/WebKit/chromium/public/WebView.h:416
> +    virtual void addWebPageOverlayClient(WebPageOverlayClient*) = 0;
> +    virtual void removeWebPageOverlayClient(WebPageOverlayClient*) = 0;

the function names here look like they are just manipulating clients, but these calls actually add and remove overlays which change the graphical appearance of this WebView. could you rename these be in terms of adding/removing Overlays instead of clients?

the visual appearance depends on the order in which the overlays are drawn. what should the rendered order be if multiple overlays are added? how should they interact with the inspector's overlay? should the user of this API be able to control the order of their overlays? these things need to be clearly specified and documented in this header

> Source/WebKit/chromium/src/PageOverlay.cpp:110
> +    explicit OverlayGraphicsLayerClientImpl(WebViewImpl* webViewImpl, WebPageOverlayClient* pageOverlayClient)

no need for explicit here, that's only needed for 1-arg constructors

> Source/WebKit/chromium/src/PageOverlayList.cpp:91
> +    return notFound;

where is notFound defined?

> Source/WebKit/chromium/src/PageOverlayList.h:45
> +    ~PageOverlayList() { }

can you define this d'tor in the .cpp, please? the d'tor for Vector<> is pretty big

> Source/WebKit/chromium/src/PageOverlayList.h:58
> +    PageOverlayList(WebViewImpl*);

explicit please

> Source/WebKit/chromium/src/PageOverlayList.h:62
> +    Vector<OwnPtr<PageOverlay>, 2> m_pageOverlays;

can you typedef this type? any iterators over this type will have to use this template string exactly (including the inline capacity)

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:286
>  void WebDevToolsAgentImpl::highlight()
>  {
> -    m_webViewImpl->setPageOverlayClient(this);
> +    m_webViewImpl->addWebPageOverlayClient(this);
>  }

i suspect that this will lead to z-order fighting between the inspector highlights and any client-specified overlays. what order should they render in? how can we keep the order consistent?

[James Robinson]

What's the intended rendering if there is a page overlay and the inspector is attempting to draw overlays? This patch puts the two into the same list, where they'll potentially fight for draw order.  I wonder if we should keep them separate.

[xiyuan]

Comment on attachment 116793 [details]
Proposed patch.

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

>> Source/WebKit/chromium/public/WebView.h:416

> 
> the function names here look like they are just manipulating clients, but these calls actually add and remove overlays which change the graphical appearance of this WebView. could you rename these be in terms of adding/removing Overlays instead of clients?
> 
> the visual appearance depends on the order in which the overlays are drawn. what should the rendered order be if multiple overlays are added? how should they interact with the inspector's overlay? should the user of this API be able to control the order of their overlays? these things need to be clearly specified and documented in this header

Renamed functions to addWebPageOverlay/removeWebPageOverlay and added comments per suggestion.

Z-order of added overlays is a problem and you are right that we should resolve that since these are in public API. I have added a new method of WebPageOverlayClient so that a client can provide a z-order number and we used to decide painting order in PageOverlayList::addClient.

>> Source/WebKit/chromium/src/PageOverlay.cpp:110
>> +    explicit OverlayGraphicsLayerClientImpl(WebViewImpl* webViewImpl, WebPageOverlayClient* pageOverlayClient)
> 
> no need for explicit here, that's only needed for 1-arg constructors

Done.

>> Source/WebKit/chromium/src/PageOverlayList.cpp:91
>> +    return notFound;
> 
> where is notFound defined?

Changed to WTF::notFound to be more clear.

>> Source/WebKit/chromium/src/PageOverlayList.h:45
>> +    ~PageOverlayList() { }
> 
> can you define this d'tor in the .cpp, please? the d'tor for Vector<> is pretty big

Done.

>> Source/WebKit/chromium/src/PageOverlayList.h:58
>> +    PageOverlayList(WebViewImpl*);
> 
> explicit please

Done

>> Source/WebKit/chromium/src/PageOverlayList.h:62
>> +    Vector<OwnPtr<PageOverlay>, 2> m_pageOverlays;
> 
> can you typedef this type? any iterators over this type will have to use this template string exactly (including the inline capacity)

Done.

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:286
>>  }
> 
> i suspect that this will lead to z-order fighting between the inspector highlights and any client-specified overlays. what order should they render in? how can we keep the order consistent?

You are right. They will fight and devtools would always win since it will update its overlay contents more frequently. I have made overlay client to provide a z-order number and we could use that to have a consistent painting order.

[xiyuan]

Created attachment 116995 [details]
Updated patch 1

[xiyuan]

Change updated.

Acutally, I found that having multiple overlay layers seems unnecessary since we are always re-painting all the overlay clients. I am thinking maybe I should get rid of the PageOverlayList and changing PageOverlay to support multiple clients. What do you guys think?

[Vsevolod Vlasov]

Comment on attachment 116995 [details]
Updated patch 1

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2067
> +            viewImpl()->pageOverlays()->paintWebFrame(gc);

I think it might worth noting that curently in non-composited case pageOverlay does not work correctly. It is possible that when paintWithContext is called only a part of page was invalidated (dirtyRect?), so pageOverlay can only paint in this part. 
E.g. if you run something like

var d = document.createElement("div"); document.body.appendChild(d);  d.style.height = "100px"; d.style.width = "500px"; d.textContent = "some text"; d.style.position = "absolute";
var left = 0; function upd() {left += 5; d.style.left = left + "px"; setTimeout(upd.bind(this), 1000)} 
upd();

in inspector console and try to highlight this div from inspector, it will correctly highlight moving div, but the tooltip with sizes will not be redrawn.

[Vsevolod Vlasov]

(In reply to comment #7)
> Change updated.
> 
> Acutally, I found that having multiple overlay layers seems unnecessary since we are always re-painting all the overlay clients. I am thinking maybe I should get rid of the PageOverlayList and changing PageOverlay to support multiple clients. What do you guys think?

Maybe PageOverlay::update() should have different behavior for dimming vs. inspector.
Why do you need to invalidate your dimming in each WebViewImpl::composite call?
Inspector needs invalidating on each composite because highlighted element might have moved, but I am not sure you ever need to invalidate dimming layer at all. 

Is that performance critical? James, what do you think?

[xiyuan]

(In reply to comment #9)
> (In reply to comment #7)
> > Change updated.
> > 
> > Acutally, I found that having multiple overlay layers seems unnecessary since we are always re-painting all the overlay clients. I am thinking maybe I should get rid of the PageOverlayList and changing PageOverlay to support multiple clients. What do you guys think?
> 
> Maybe PageOverlay::update() should have different behavior for dimming vs. inspector.
> Why do you need to invalidate your dimming in each WebViewImpl::composite call?
> Inspector needs invalidating on each composite because highlighted element might have moved, but I am not sure you ever need to invalidate dimming layer at all. 
> 
> Is that performance critical? James, what do you think?

Dimming is static and does not change and I should not draw it on every composite. Maybe add an isDirty() method to PageOverlayClient and have PageOverlay to check that to decide whether to redraw?

Right now, dimming is used to darken web contents so that the tab-modal dialog stands out. I would think performance is not of a concern for this case.

[Vsevolod Vlasov]

> Dimming is static and does not change and I should not draw it on every composite. Maybe add an isDirty() method to PageOverlayClient and have PageOverlay to check that to decide whether to redraw?
Yes, or getDirtyRect().

[xiyuan]

Created attachment 117264 [details]
Use getDirtyRect to avoid unnecessary paint and fixed a webview resize issue

[xiyuan]

Comment on attachment 117264 [details]
Use getDirtyRect to avoid unnecessary paint and fixed a webview resize issue

Change updated:
- Added a getDirtyRect to overlay client in order to avoid unnecessary painting during compositing;
- Handle webview resize in PageOverlay::update to ensure overlay layer is big enough and on top of scrollbars;

Please take another look. Thanks.

[Vsevolod Vlasov]

Comment on attachment 117264 [details]
Use getDirtyRect to avoid unnecessary paint and fixed a webview resize issue

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

> Source/WebKit/chromium/src/PageOverlay.cpp:181
> +    // WebPageOverlayClient does the actual painting of the overlay.

I think getDirtyRect should be used here as well.
Please move these FIXMEs to WebDevToolsAgentImpl::getDirtyRect() as well.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2868
> +                m_pageOverlays->update(true /*force*/);

I believe WebKit style is to use enums for that, not comments, e.g.:
    enum PageOverlayUpdateOption {
        ForceUpdate,
        NotForceUpdate
    };

> Source/WebKit/chromium/src/WebViewImpl.h:576
> +    // The DevTools agent.

This comment seems redundant.

> Source/WebKit/chromium/src/WebViewImpl.h:579
> +    // PageOverlays for this WebView.

This comment seems redundant.

[xiyuan]

Created attachment 117288 [details]
Address comments in previous patch.

[xiyuan]

Comment on attachment 117288 [details]
Address comments in previous patch.

Change updated to address comments.
Please take another look. Thanks.

[xiyuan]

Comment on attachment 117264 [details]
Use getDirtyRect to avoid unnecessary paint and fixed a webview resize issue

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

>> Source/WebKit/chromium/src/PageOverlay.cpp:181
>> +    // WebPageOverlayClient does the actual painting of the overlay.
> 
> I think getDirtyRect should be used here as well.
> Please move these FIXMEs to WebDevToolsAgentImpl::getDirtyRect() as well.

Done

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2868
>> +                m_pageOverlays->update(true /*force*/);
> 
> I believe WebKit style is to use enums for that, not comments, e.g.:
>     enum PageOverlayUpdateOption {
>         ForceUpdate,
>         NotForceUpdate
>     };

Done.

>> Source/WebKit/chromium/src/WebViewImpl.h:576
>> +    // The DevTools agent.
> 
> This comment seems redundant.

Removed.

>> Source/WebKit/chromium/src/WebViewImpl.h:579
>> +    // PageOverlays for this WebView.
> 
> This comment seems redundant.

Removed

[James Robinson]

Comment on attachment 117288 [details]
Address comments in previous patch.

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

> Source/WebKit/chromium/public/WebPageOverlayClient.h:48
> +    virtual WebRect getDirtyRect() const = 0;

webkit style is to omit the 'get' prefix for functions like this

> Source/WebKit/chromium/public/WebPageOverlayClient.h:49
> +};

can you please define a protected, virtual d'tor with an empty body?

> Source/WebKit/chromium/public/WebView.h:416
> +    // graphcical apperance of the WebView. WebPageOverlayClient paints the

typos: graphcical -> graphical, apperance -> appearance

> Source/WebKit/chromium/public/WebView.h:423
> +    virtual void addWebPageOverlay(WebPageOverlayClient*) = 0;

it looks like the z-order value is only checked in this add call, unless I'm missing something. in that case it seems like the z-order should be provided as a parameter here instead of being a client callback

> Source/WebKit/chromium/src/PageOverlay.cpp:54
> +    return canvas;

the way this is written is a bit odd - could you just return directly from inside the #if blocks?

> Source/WebKit/chromium/src/PageOverlayList.cpp:68
> +    bool zorderChanged = false;

nit: zOrderChanged, please. "zorder" isn't a word

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:285
> +    return 99;

could you add a comment explaining this value (it looks like it's just picked to be bigger than any normal overlays, correct?)

[xiyuan]

Comment on attachment 117288 [details]
Address comments in previous patch.

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

All done. I will upload updated patch shortly.

>> Source/WebKit/chromium/public/WebPageOverlayClient.h:48
>> +    virtual WebRect getDirtyRect() const = 0;
> 
> webkit style is to omit the 'get' prefix for functions like this

Done.

>> Source/WebKit/chromium/public/WebPageOverlayClient.h:49
>> +};
> 
> can you please define a protected, virtual d'tor with an empty body?

Done.

>> Source/WebKit/chromium/public/WebView.h:416
>> +    // graphcical apperance of the WebView. WebPageOverlayClient paints the
> 
> typos: graphcical -> graphical, apperance -> appearance

oops, Fixed.

>> Source/WebKit/chromium/public/WebView.h:423
>> +    virtual void addWebPageOverlay(WebPageOverlayClient*) = 0;
> 
> it looks like the z-order value is only checked in this add call, unless I'm missing something. in that case it seems like the z-order should be provided as a parameter here instead of being a client callback

Removed zorder client callback and passing it in the add call. The zorder is now stored as part of PageOverlay so that we could add an overlay to proper position.

>> Source/WebKit/chromium/src/PageOverlay.cpp:54
>> +    return canvas;
> 
> the way this is written is a bit odd - could you just return directly from inside the #if blocks?

Done.

>> Source/WebKit/chromium/src/PageOverlayList.cpp:68
>> +    bool zorderChanged = false;
> 
> nit: zOrderChanged, please. "zorder" isn't a word

Renamed to zOrderChanged

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:285
>> +    return 99;
> 
> could you add a comment explaining this value (it looks like it's just picked to be bigger than any normal overlays, correct?)

Done.

[xiyuan]

Created attachment 117299 [details]
Address comments from jamesr

[James Robinson]

From my point of view I think this looks good.  Vsevolod, would you mind looking over the dev tools stuff?

[Vsevolod Vlasov]

(In reply to comment #21)
> From my point of view I think this looks good.  Vsevolod, would you mind looking over the dev tools stuff?

All my comments are already addressed, this looks good to me now.

[xiyuan]

Comment on attachment 117299 [details]
Address comments from jamesr

Thanks for reviewing.

I am not a committer. Could any of you help to turn on CQ flag? Hopefully CQ could land this.

[xiyuan]

Comment on attachment 117299 [details]
Address comments from jamesr

Found a compilation error on mac in PageOverlay.h where I forward declared WebRect as "class", it should be struct.

Will upload a fix shortly.

[James Robinson]

Comment on attachment 117299 [details]
Address comments from jamesr

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

> Source/WebKit/chromium/public/WebPageOverlayClient.h:37
> +class WebPageOverlayClient {

Darin pointed out that this interface should actually be called WebPageOverlay, not Client

> Source/WebKit/chromium/public/WebPageOverlayClient.h:43
> +    // Returns dirty rect that needs repaint.
> +    virtual WebRect dirtyRect() const = 0;

I don't think this is quite accurate - what this is really returning is the rectangle that this overlay is interested in painting. There doesn't seem to be any mechanism to deal with changing the dirty rect.

I think this would be clearer if the addPageOverlay() function specified the bounds of the overlay, just as it specified the z-order, and the rule for changing it was that if you wanted to change the bounds of an overlay you needed to remove it and add it back. That would be consistent with the way you can change the z-order.

Although looking in detail we aren't using this functionality - every overlay covers the full viewport. So let's punt on this functionality for now and add it when we have a need.

> Source/WebKit/chromium/public/WebView.h:424
> +    virtual void addWebPageOverlay(WebPageOverlayClient*, int /*z-order*/) = 0;
> +    virtual void removeWebPageOverlay(WebPageOverlayClient*) = 0;

Darin let me know that we don't use the "web" prefix in the function names. So these should be called:
addPageOverlay(...)
removePageOverlay(...)

see the setVisibilityState() above as an example. We all apologize deeply for the inconsistencies :/

[xiyuan]

Comment on attachment 117299 [details]
Address comments from jamesr

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

Will update updated patch shortly.

>> Source/WebKit/chromium/public/WebPageOverlayClient.h:37
>> +class WebPageOverlayClient {
> 
> Darin pointed out that this interface should actually be called WebPageOverlay, not Client

Done.

>> Source/WebKit/chromium/public/WebPageOverlayClient.h:43
>> +    virtual WebRect dirtyRect() const = 0;
> 
> I don't think this is quite accurate - what this is really returning is the rectangle that this overlay is interested in painting. There doesn't seem to be any mechanism to deal with changing the dirty rect.
> 
> I think this would be clearer if the addPageOverlay() function specified the bounds of the overlay, just as it specified the z-order, and the rule for changing it was that if you wanted to change the bounds of an overlay you needed to remove it and add it back. That would be consistent with the way you can change the z-order.
> 
> Although looking in detail we aren't using this functionality - every overlay covers the full viewport. So let's punt on this functionality for now and add it when we have a need.

Agree with your assessments.

>> Source/WebKit/chromium/public/WebView.h:424
>> +    virtual void removeWebPageOverlay(WebPageOverlayClient*) = 0;
> 
> Darin let me know that we don't use the "web" prefix in the function names. So these should be called:
> addPageOverlay(...)
> removePageOverlay(...)
> 
> see the setVisibilityState() above as an example. We all apologize deeply for the inconsistencies :/

Done.

[xiyuan]

Created attachment 117515 [details]
Updated patch.

Please take another look.
Thanks.

[xiyuan]

Comment on attachment 117515 [details]
Updated patch.

Found a missed renaming in PageOverlay.h. Sorry about that.

[xiyuan]

Created attachment 117519 [details]
Updated patch.

[James Robinson]

Tip for the future: Tools/Scripts/do-webcore-rename can be used for renames pretty much anywhere in the WebKit repo, despite the name.

[WebKit Review Bot]

Attachment 117519 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 117519 [details]
Updated patch.

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

I thought I left some feedback on this earlier but I don't see it on the bug now. My apologies if you end up getting some comments twice. This is very close!

> Source/WebKit/chromium/WebKit.gyp:263
> +                'public/WebPageOverlayClient.h',

should be WebPageOverlay.h

> Source/WebKit/chromium/public/WebPageOverlay.h:43
> +    // Returns dirty rect that needs repaint.
> +    virtual WebRect dirtyRect() const = 0;

I think we should remove dirtyRect() completely from this patch and always consider the full viewport to be covered by every overlay.

> Source/WebKit/chromium/src/PageOverlay.cpp:150
> +    FloatSize size(m_viewImpl->size().width, m_viewImpl->size().height);

m_viewImpl->size() is a WebSize, which (within WebKit code) can be implicitly converted to an IntSize, which in turn can be implicitly converted to a FloatSize

so this should just be "FloatSize size = m_viewImpl->size();" or "FloatSize size(m_viewImpl->size());" if you prefer that initialization style

> Source/WebKit/chromium/src/PageOverlay.cpp:167
> +    if (updateOption == ForceUpdate)
> +        m_layer->setNeedsDisplay();
> +    else {
> +        const WebRect& dirtyRect = m_overlay->dirtyRect();
> +        if (!dirtyRect.isEmpty())
> +            m_layer->setNeedsDisplayInRect(WebCore::FloatRect(dirtyRect.x, dirtyRect.y, dirtyRect.width, dirtyRect.height));
> +    }

i think this can go back to what it previously did

[James Robinson]

Comment on attachment 117519 [details]
Updated patch.

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

> Source/WebKit/chromium/public/WebView.h:416
> +    // graphical appearance of the WebView. WebPageOverlayClient paints the

WebPageOverlayClient -> WebPageOverlay

> Source/WebKit/chromium/src/PageOverlay.cpp:182
> +    // WebPageOverlayClient does the actual painting of the overlay.

WebPageOverlayClient -> WebPageOverlay

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:275
> +// WebPageOverlayClient

WebPageOverlayClient -> WebPageOverlay

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:100
> +    // WebPageOverlayClient

WebPageOverlayClient -> WebPageOverlay

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:102
> +    virtual WebRect dirtyRect() const;

remove please

> Source/WebKit/chromium/src/WebViewImpl.h:96
> +class WebPageOverlayClient;

WebPageOverlayClient -> WebPageOverlay, or remove this forward declaration if it isn't needed

[xiyuan]

> > Source/WebKit/chromium/public/WebPageOverlay.h:43
> > +    // Returns dirty rect that needs repaint.
> > +    virtual WebRect dirtyRect() const = 0;
> 
> I think we should remove dirtyRect() completely from this patch and always consider the full viewport to be covered by every overlay.
> 

dirtyRect (or at least a needsRepaint flag) is needed for compositing mode so that we don't always repaint all overlays. This avoids unnecessary paintPageOverlay calls. For background dimming, it's static and does not change so we should only paint it once in composting mode.

[xiyuan]

Comment on attachment 117519 [details]
Updated patch.

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

All done except removing dirtyRect, which I think needs a bit more discussion.

>> Source/WebKit/chromium/WebKit.gyp:263
>> +                'public/WebPageOverlayClient.h',
> 
> should be WebPageOverlay.h

Done.

Strange, gyp_webkit does not buff on this. :(

>> Source/WebKit/chromium/public/WebView.h:416
>> +    // graphical appearance of the WebView. WebPageOverlayClient paints the
> 
> WebPageOverlayClient -> WebPageOverlay

Done.

>> Source/WebKit/chromium/src/PageOverlay.cpp:150
>> +    FloatSize size(m_viewImpl->size().width, m_viewImpl->size().height);
> 
> m_viewImpl->size() is a WebSize, which (within WebKit code) can be implicitly converted to an IntSize, which in turn can be implicitly converted to a FloatSize
> 
> so this should just be "FloatSize size = m_viewImpl->size();" or "FloatSize size(m_viewImpl->size());" if you prefer that initialization style

Done.

>> Source/WebKit/chromium/src/PageOverlay.cpp:167

> 
> i think this can go back to what it previously did

Using dirtyRect could save unnecessary paints in composite code path.

>> Source/WebKit/chromium/src/PageOverlay.cpp:182
>> +    // WebPageOverlayClient does the actual painting of the overlay.
> 
> WebPageOverlayClient -> WebPageOverlay

Done.

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:275
>> +// WebPageOverlayClient
> 
> WebPageOverlayClient -> WebPageOverlay

Done.

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.h:100
>> +    // WebPageOverlayClient
> 
> WebPageOverlayClient -> WebPageOverlay

Done.

>> Source/WebKit/chromium/src/WebViewImpl.h:96
>> +class WebPageOverlayClient;
> 
> WebPageOverlayClient -> WebPageOverlay, or remove this forward declaration if it isn't needed

Removed.

[xiyuan]

Created attachment 117558 [details]
Updated patch.

[James Robinson]

(In reply to comment #34)
> > > Source/WebKit/chromium/public/WebPageOverlay.h:43
> > > +    // Returns dirty rect that needs repaint.
> > > +    virtual WebRect dirtyRect() const = 0;
> > 
> > I think we should remove dirtyRect() completely from this patch and always consider the full viewport to be covered by every overlay.
> > 
> 
> dirtyRect (or at least a needsRepaint flag) is needed for compositing mode so that we don't always repaint all overlays. This avoids unnecessary paintPageOverlay calls. For background dimming, it's static and does not change so we should only paint it once in composting mode.

I don't understand - wouldn't a page dimming overlay simply call addPageOverlay() once, get one paint call, and be done with it? The devtools overlay code calls removePageOverlay()/addPageOverlay() every time the highlight changes, which triggers a repaint (fullscreen, currently).

If we do want to support overlays that are persistent and have partial repaints (although I can't imagine why) a better way to do that is to have the overlay inform the implementation that some rectangle inside the overlay is invalid and then have the implementation request all overlays paint the portion marked invalid.

[xiyuan]

(In reply to comment #37)
> (In reply to comment #34)
> > > > Source/WebKit/chromium/public/WebPageOverlay.h:43
> > > > +    // Returns dirty rect that needs repaint.
> > > > +    virtual WebRect dirtyRect() const = 0;
> > > 
> > > I think we should remove dirtyRect() completely from this patch and always consider the full viewport to be covered by every overlay.
> > > 
> > 
> > dirtyRect (or at least a needsRepaint flag) is needed for compositing mode so that we don't always repaint all overlays. This avoids unnecessary paintPageOverlay calls. For background dimming, it's static and does not change so we should only paint it once in composting mode.
> 
> I don't understand - wouldn't a page dimming overlay simply call addPageOverlay() once, get one paint call, and be done with it? The devtools overlay code calls removePageOverlay()/addPageOverlay() every time the highlight changes, which triggers a repaint (fullscreen, currently).
> 
> If we do want to support overlays that are persistent and have partial repaints (although I can't imagine why) a better way to do that is to have the overlay inform the implementation that some rectangle inside the overlay is invalid and then have the implementation request all overlays paint the portion marked invalid.

directRect is useful in composting code path. e.g. WebViewImpl::composite is called when devtools changes highlight and right now, we call PageOverlay::update on all overlays. And if we don't have dirtyRect, dimming overlay will setNeedsDispaly on its layer and paints the dimming again even though nothing changed. With dirtyRect, we could save this unnecessary paint.

On the non-composting code path, dirtyRect is not used and we always repaint all overlays when webview is repainted.

Also see  Vsevolod's comments @ 9.

[James Robinson]

(In reply to comment #38)

> 
> directRect is useful in composting code path. e.g. WebViewImpl::composite is called when devtools changes highlight and right now, we call PageOverlay::update on all overlays. And if we don't have dirtyRect, dimming overlay will setNeedsDispaly on its layer and paints the dimming again even though nothing changed. With dirtyRect, we could save this unnecessary paint.

Hmm...but whenever the devtools overlay changes its dirtyRect is the entire viewport, so I'm not sure what we are saving.

It is possible to optimize this case. However, it doesn't seem that the code today is getting any benefit, and if we do want to get this benefit we need to do it in a different way. I'd rather not worry about it. This is not a performance-critical codepath.

[xiyuan]

Created attachment 117564 [details]
Remove dirtyRect.

[xiyuan]

Comment on attachment 117564 [details]
Remove dirtyRect.

Please take another look. Thanks.

[James Robinson]

Comment on attachment 117564 [details]
Remove dirtyRect.

thanks

[xiyuan]

(In reply to comment #42)
> (From update of attachment 117564 [details])
> thanks

Thank you and Vsevolod for reviewing this, educating me and being patient with me. :)

[WebKit Review Bot]

Comment on attachment 117564 [details]
Remove dirtyRect.

Clearing flags on attachment: 117564

Committed r101758: <http://trac.webkit.org/changeset/101758>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.