[webkit-reviews] review granted: [Bug 108864] [wk2] REGRESSION (r138858): GIFs don't start playing when they come out of background tabs : [Attachment 197649] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 108864: [wk2] REGRESSION (r138858): GIFs don't start playing when they come
out of background tabs
https://bugs.webkit.org/show_bug.cgi?id=108864

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=197649&action=review


> Source/WebCore/dom/Document.cpp:6171
> +	   CachedResourceHandle<CachedResource> resource = it->value;

Can this be a const CachedResourceHandle& instead to avoid copying? Or is
copying the handle valuable in some way?

> Source/WebCore/dom/Document.h:1218
> +    void advanceAllAnimatedImages();

Is this the best class for this function? For example, should it be in the
cached resource loader instead? Seems to be mission-creep for Document.

> Source/WebCore/platform/graphics/BitmapImage.h:186
>      void reportMemoryUsage(MemoryObjectInfo*) const OVERRIDE;

Yuck, missing virtual keyword.

> Source/WebCore/platform/graphics/BitmapImage.h:188
> +    bool canAnimate();

Should this be const?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2006
> +	       for (Frame* frame = m_page->mainFrame(); frame; frame =
frame->tree()->traverseNext()) {

I think this needs a why comment. A very brief one, and one that answers the
questions the code itself doesn’t.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2007
> +		   if (Document* document = frame->document())

Is this null check really needed? I don’t think we allow frames in the frame
tree that have null documents. But obviously if I am wrong we will regret it
later.