[webkit-reviews] review denied: [Bug 73235] [Chromium] Support adding/removing page overlay to WebView : [Attachment 117519] Updated patch.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 117519: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=117519&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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