[webkit-reviews] review denied: [Bug 38233] Slow rendering w/multiple animated resizes : [Attachment 58284] Patch

Wed Jun 9 20:26:33 PDT 2010

David Levin <levin at chromium.org> has denied Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 38233: Slow rendering w/multiple animated resizes
https://bugs.webkit.org/show_bug.cgi?id=38233

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

------- Additional Comments from David Levin <levin at chromium.org>
These are my 1st pass comments, but there are enough of them that I decided to
enter them before doing my second pass comments. (My 1st pass tends to be
simpler to spot items, more stylistic, etc. My second pass is where I finish
getting a big picture and try to verify correctness. I'll try to start a second
pass soon.)

---------------------------------
http://wkrietveld.appspot.com/38233/diff/1/2
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/38233/diff/1/2#newcode6
WebCore/ChangeLog:6: https://bugs.webkit.org/show_bug.cgi?id=38233
I would add a return here.

http://wkrietveld.appspot.com/38233/diff/1/3
File WebCore/rendering/RenderBoxModelObject.cpp (left):

http://wkrietveld.appspot.com/38233/diff/1/3#oldcode134
WebCore/rendering/RenderBoxModelObject.cpp:134: bool contextIsScaled =
!currentTransform.isIdentityOrTranslationOrFlipped();
Why are we now using "size" instead of the transform as was done here before? 

I see this matched what the other instance of this code was doing, but did we
loose something from this instance by doing this change?

http://wkrietveld.appspot.com/38233/diff/1/3
File WebCore/rendering/RenderBoxModelObject.cpp (right):

http://wkrietveld.appspot.com/38233/diff/1/3#newcode6
WebCore/rendering/RenderBoxModelObject.cpp:6: * Copyright (C) 2005, 2006, 2007,
2008, 2009 Apple Inc. All rights reserved.
You should add a copyright line here and other files where you made changes of
more than a few lines.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode51
WebCore/rendering/RenderBoxModelObject.cpp:51:
RenderBoxModelObject::ScaleObserver::ScaleObserver() : m_objects(0),
m_timer(this, &ScaleObserver::highQualityRepaintTimerFired) {}
The initializers are typically put on separate lines (as
RenderBoxModelScaleData did before) and then the {} would be on separate lines
as well.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode62
WebCore/rendering/RenderBoxModelObject.cpp:62: if (m_objects) {
Please consider the fail fast approach.

if (!m_objects)
    return;

etc.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode63
WebCore/rendering/RenderBoxModelObject.cpp:63: m_objects->take(object);
Just use "remove" instead of "take" since you don't need the object returned.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode66
WebCore/rendering/RenderBoxModelObject.cpp:66: m_objects = 0;
m_timer.stop(); ?

Maybe that isn't worth it.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode73
WebCore/rendering/RenderBoxModelObject.cpp:73: if (m_objects) {
fail fast?

http://wkrietveld.appspot.com/38233/diff/1/3#newcode81
WebCore/rendering/RenderBoxModelObject.cpp:81: m_timer.stop();
There is no need to call stop, since you are setting the time on the next line.
There is only one firing time that this timer may hold (see
WebCore/platform/Timer.cpp).

http://wkrietveld.appspot.com/38233/diff/1/3#newcode102
WebCore/rendering/RenderBoxModelObject.cpp:102: // There is no scale in effect.
 If we had a scale in effect before, we can just remove this object from the
list.
One space after periods in WebKit comments.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode103
WebCore/rendering/RenderBoxModelObject.cpp:103: if (time) {
The { should go away since the statement is now one line.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode103
WebCore/rendering/RenderBoxModelObject.cpp:103: if (time) {
What is "if (time)" suppose to be checking for? that m_objects exists? If so,
why not just check m_objects?

"m_objects->get(object)" will return "static T emptyValue() { return
std::numeric_limits<T>::infinity(); }" if the object doesn't exist in the
hashmap.

Before when the value was a pointer, the default value would be 0, so this code
is different from before.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode118
WebCore/rendering/RenderBoxModelObject.cpp:118: if (!time) {
What is !time checking for? (see comment above).

http://wkrietveld.appspot.com/38233/diff/1/3#newcode127
WebCore/rendering/RenderBoxModelObject.cpp:127: // If it has been at least
<threshold> seconds since the last time a
Is "<threshold>" == cLowQualityTimeThreshold ?

http://wkrietveld.appspot.com/38233/diff/1/3#newcode128
WebCore/rendering/RenderBoxModelObject.cpp:128: // resize was requested, draw
at hi quality and don't set the timer.
s/hi/high/

http://wkrietveld.appspot.com/38233/diff/1/3#newcode132
WebCore/rendering/RenderBoxModelObject.cpp:132: // Draw at low quality first,
and set a timer for HQ.
HQ: Please avoid abreviations.

Also no comma before and (just like you did above for a similar comment).

http://wkrietveld.appspot.com/38233/diff/1/4
File WebCore/rendering/RenderBoxModelObject.h (right):

http://wkrietveld.appspot.com/38233/diff/1/4#newcode108
WebCore/rendering/RenderBoxModelObject.h:108: typedef
HashMap<RenderBoxModelObject*, double> ScaleMap;
It seems like this file should include wtf/HashMap.

ScaleMap doesn't really give you scales or map scales to something. It maps
RenderBoxModelObject to the time that object last painted. Can you come up with
a better name?

http://wkrietveld.appspot.com/38233/diff/1/4#newcode110
WebCore/rendering/RenderBoxModelObject.h:110: class ScaleObserver : public
Noncopyable {
Nice use of Noncopyable!

http://wkrietveld.appspot.com/38233/diff/1/4#newcode112
WebCore/rendering/RenderBoxModelObject.h:112: ScaleObserver();
Why is the constructor public?

http://wkrietveld.appspot.com/38233/diff/1/4#newcode113
WebCore/rendering/RenderBoxModelObject.h:113: static ScaleObserver*
getInstance();
Misc idea: What about "instance()" as the name?

I wonder getInstance() is even exposed.  It looks like every time this is
called, one does this:

ScaleObserver::getInstance()>method...

So why not just hide getInstance and make a few static methods that do this for
you?

In fact, you could just expose the the static methods on  RenderBoxModelObject
(not sure if method names will need to change) and then just hide the whole
class inside of RenderBoxModelObject.cpp.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode115
WebCore/rendering/RenderBoxModelObject.h:115: void
objectDestroyed(RenderBoxModelObject* object);
"object" shouldn't be here.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode116
WebCore/rendering/RenderBoxModelObject.h:116: void
highQualityRepaintTimerFired(Timer<ScaleObserver>*);
highQualityRepaintTimerFire should be private.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode117
WebCore/rendering/RenderBoxModelObject.h:117: void restartTimer();
Please add a blank line here before private:

http://wkrietveld.appspot.com/38233/diff/1/4#newcode117
WebCore/rendering/RenderBoxModelObject.h:117: void restartTimer();
restartTimer should be private.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode119
WebCore/rendering/RenderBoxModelObject.h:119: ScaleMap* m_objects;
Every time I see m_objects, I don't know what it is.

It seems to be a mapping of RenderBoxModelObject's that are painted at low
quality to the last time at which they were painted. Is that correct? Can this
name be more descriptive (but not as long as my sentence :) )?

http://wkrietveld.appspot.com/38233/diff/1/4#newcode121
WebCore/rendering/RenderBoxModelObject.h:121: static ScaleObserver* gInstance;
Recent style guideline addition "Static data members should be prefixed by
"s_". "