[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





--- Comment #3 from James Robinson <jamesr at chromium.org>  2011-11-28 13:26:36 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
> +    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?

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