[Webkit-unassigned] [Bug 113786] Throttle repaints during page loading in tile controller

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=113786





--- Comment #5 from Darin Adler <darin at apple.com>  2013-04-02 09:22:15 PST ---
(From update of attachment 196155)
View in context: https://bugs.webkit.org/attachment.cgi?id=196155&action=review

I gave the same kinds of comments that I would for cases where you planned to land it; hope they are helpful.

> Source/WebCore/ChangeLog:12
> +        (WebCore):

If you’d be willing, please take a moment to remove the bogus lines like this one listing just class names that the prepare-ChangeLog script adds.

Even better, find someone to fix the darned script!

> Source/WebCore/loader/ProgressTracker.cpp:54
> +static const double activeProgressTimerHeartbeatRate = 0.1;

I’d call this progressHeartbeatInterval.

Since this is a time in seconds, I don’t think it should be called a “rate”. It should probably be called an “interval”. I’m also not sure what the word “active” adds here.

I’d like to see a comment explaining the concept behind the constants; why did you choose these values. That can help people make intelligent choices about changing or not changing them in the future.

> Source/WebCore/loader/ProgressTracker.cpp:55
> +static const unsigned maximumHeartbeatsWithNoProgress= 4;

Missing space before "=" sign.

> Source/WebCore/loader/ProgressTracker.cpp:284
> +    return m_progressValue && m_progressValue < finalProgressValue && m_heartbeatsWithNoProgress < maximumHeartbeatsWithNoProgress;

I think this be <= maximumHeartbeatsWithNoProgress given the name of that constant. Or we could name the constant something different.

> Source/WebCore/loader/ProgressTracker.cpp:294
> +//    unsigned delta = unsigned(m_totalBytesReceived - m_totalBytesReceivedBeforePreviousHeartbeat);

If you did want to land this we’d want to remove all commented lines, but since this is a work in progress, I won’t belabor that point.

> Source/WebCore/loader/ProgressTracker.cpp:298
> +    m_originatingProgressFrame->loader()->loadProgressingStatusChanged();

Can we optimize this to only run if the boolean state of isLoadProgressing() actually changed, rather than leaving that to callers? Is that an important optimization?

> Source/WebCore/loader/ProgressTracker.cpp:302
> +    if (!m_progressValue || m_progressValue >= finalProgressValue)
> +        m_progressHeartbeatTimer.stop();

I don’t understand the reason for stopping the timer if m_progressValue is 0. Since it’s not self-evident it probably needs a comment.

> Source/WebCore/page/FrameView.cpp:2288
> +    if (TiledBacking* tiledBacking = this->tiledBacking())
> +        tiledBacking->disableRepaintThrottlingTemporarilyForInteraction();

I’d normally call a local variable with such a tight scope just “backing”; my rule of thumb is that names can be more terse and ambiguous if they have very small scopes. Certainly not an important point, and there’s also an argument for using a variable name with the identical title to the member function for clarity.

> Source/WebCore/platform/graphics/Region.cpp:130
> +void Region::clear()
> +{
> +    m_bounds = IntRect();
> +    m_shape = Shape();
> +}

Should this be an inline function?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:153
> +void TileController::setRepaintThrottlingEnabled(bool b)

I’d name this “enabled” instead of “b”.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:260
> +    // The delaying will start on top of the runloop.
> +    static const double throttledRepaintDelay = .5;

Normally we’d want a comment saying why 0.5 seconds is a good interval and we’d put this at the top of the file.

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