[Webkit-unassigned] [Bug 73235] [Chromium] Support adding/removing page overlay to WebView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73235


xiyuan <xiyuan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116793|review?                     |
               Flag|                            |




--- Comment #5 from xiyuan <xiyuan at chromium.org>  2011-11-29 10:23:12 PST ---
(From update of attachment 116793)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.