Bug 73235 - [Chromium] Support adding/removing page overlay to WebView
Summary: [Chromium] Support adding/removing page overlay to WebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-28 11:30 PST by xiyuan
Modified: 2011-12-02 00:56 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch. (24.71 KB, patch)
2011-11-28 11:37 PST, xiyuan
no flags Details | Formatted Diff | Diff
Updated patch 1 (27.31 KB, patch)
2011-11-29 10:24 PST, xiyuan
no flags Details | Formatted Diff | Diff
Use getDirtyRect to avoid unnecessary paint and fixed a webview resize issue (29.01 KB, patch)
2011-11-30 13:49 PST, xiyuan
no flags Details | Formatted Diff | Diff
Address comments in previous patch. (30.37 KB, patch)
2011-11-30 15:54 PST, xiyuan
no flags Details | Formatted Diff | Diff
Address comments from jamesr (30.33 KB, patch)
2011-11-30 16:56 PST, xiyuan
no flags Details | Formatted Diff | Diff
Updated patch. (30.28 KB, patch)
2011-12-01 16:22 PST, xiyuan
no flags Details | Formatted Diff | Diff
Updated patch. (30.29 KB, patch)
2011-12-01 16:32 PST, xiyuan
jamesr: review-
Details | Formatted Diff | Diff
Updated patch. (30.04 KB, patch)
2011-12-01 20:46 PST, xiyuan
no flags Details | Formatted Diff | Diff
Remove dirtyRect. (27.79 KB, patch)
2011-12-01 22:10 PST, xiyuan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description xiyuan 2011-11-28 11:30:50 PST
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.
Comment 1 xiyuan 2011-11-28 11:37:56 PST
Created attachment 116793 [details]
Proposed patch.
Comment 2 WebKit Review Bot 2011-11-28 11:42:19 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 James Robinson 2011-11-28 13:26:36 PST
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?
Comment 4 James Robinson 2011-11-28 13:27:23 PST
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.
Comment 5 xiyuan 2011-11-29 10:23:12 PST
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.
Comment 6 xiyuan 2011-11-29 10:24:06 PST
Created attachment 116995 [details]
Updated patch 1
Comment 7 xiyuan 2011-11-29 10:26:29 PST
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?
Comment 8 Vsevolod Vlasov 2011-11-29 14:38:16 PST
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.
Comment 9 Vsevolod Vlasov 2011-11-29 14:46:49 PST
(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?
Comment 10 xiyuan 2011-11-29 15:09:59 PST
(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.
Comment 11 Vsevolod Vlasov 2011-11-30 02:10:40 PST
> 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().
Comment 12 xiyuan 2011-11-30 13:49:50 PST
Created attachment 117264 [details]
Use getDirtyRect to avoid unnecessary paint and fixed a webview resize issue
Comment 13 xiyuan 2011-11-30 13:55:38 PST
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.
Comment 14 Vsevolod Vlasov 2011-11-30 15:12:55 PST
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.
Comment 15 xiyuan 2011-11-30 15:54:16 PST
Created attachment 117288 [details]
Address comments in previous patch.
Comment 16 xiyuan 2011-11-30 15:56:20 PST
Comment on attachment 117288 [details]
Address comments in previous patch.

Change updated to address comments.
Please take another look. Thanks.
Comment 17 xiyuan 2011-11-30 15:57:04 PST
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
Comment 18 James Robinson 2011-11-30 16:17:15 PST
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?)
Comment 19 xiyuan 2011-11-30 16:55:00 PST
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.
Comment 20 xiyuan 2011-11-30 16:56:52 PST
Created attachment 117299 [details]
Address comments from jamesr
Comment 21 James Robinson 2011-11-30 19:01:03 PST
From my point of view I think this looks good.  Vsevolod, would you mind looking over the dev tools stuff?
Comment 22 Vsevolod Vlasov 2011-12-01 00:59:14 PST
(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.
Comment 23 xiyuan 2011-12-01 07:21:30 PST
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.
Comment 24 xiyuan 2011-12-01 14:46:16 PST
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.
Comment 25 James Robinson 2011-12-01 15:16:50 PST
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 :/
Comment 26 xiyuan 2011-12-01 16:20:22 PST
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.
Comment 27 xiyuan 2011-12-01 16:22:59 PST
Created attachment 117515 [details]
Updated patch.

Please take another look.
Thanks.
Comment 28 xiyuan 2011-12-01 16:25:42 PST
Comment on attachment 117515 [details]
Updated patch.

Found a missed renaming in PageOverlay.h. Sorry about that.
Comment 29 xiyuan 2011-12-01 16:32:06 PST
Created attachment 117519 [details]
Updated patch.
Comment 30 James Robinson 2011-12-01 16:32:39 PST
Tip for the future: Tools/Scripts/do-webcore-rename can be used for renames pretty much anywhere in the WebKit repo, despite the name.
Comment 31 WebKit Review Bot 2011-12-01 16:38:20 PST
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.
Comment 32 James Robinson 2011-12-01 17:37:35 PST
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
Comment 33 James Robinson 2011-12-01 17:38:45 PST
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
Comment 34 xiyuan 2011-12-01 19:47:40 PST
> > 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.
Comment 35 xiyuan 2011-12-01 20:43:24 PST
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.
Comment 36 xiyuan 2011-12-01 20:46:24 PST
Created attachment 117558 [details]
Updated patch.
Comment 37 James Robinson 2011-12-01 20:52:57 PST
(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.
Comment 38 xiyuan 2011-12-01 21:13:33 PST
(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.
Comment 39 James Robinson 2011-12-01 21:26:50 PST
(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.
Comment 40 xiyuan 2011-12-01 22:10:24 PST
Created attachment 117564 [details]
Remove dirtyRect.
Comment 41 xiyuan 2011-12-01 22:12:36 PST
Comment on attachment 117564 [details]
Remove dirtyRect.

Please take another look. Thanks.
Comment 42 James Robinson 2011-12-01 22:21:23 PST
Comment on attachment 117564 [details]
Remove dirtyRect.

thanks
Comment 43 xiyuan 2011-12-01 22:23:42 PST
(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. :)
Comment 44 WebKit Review Bot 2011-12-02 00:56:04 PST
Comment on attachment 117564 [details]
Remove dirtyRect.

Clearing flags on attachment: 117564

Committed r101758: <http://trac.webkit.org/changeset/101758>
Comment 45 WebKit Review Bot 2011-12-02 00:56:11 PST
All reviewed patches have been landed.  Closing bug.