[webkit-reviews] review canceled: [Bug 73235] [Chromium] Support adding/removing page overlay to WebView : [Attachment 116793] Proposed patch.

[bugzilla-daemon at webkit.org]

xiyuan <xiyuan at chromium.org> has canceled xiyuan <xiyuan at chromium.org>'s
request for review:
Bug 73235: [Chromium] Support adding/removing page overlay to WebView
https://bugs.webkit.org/show_bug.cgi?id=73235

Attachment 116793: Proposed patch.
https://bugs.webkit.org/attachment.cgi?id=116793&action=review

------- Additional Comments from xiyuan <xiyuan at chromium.org>
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.