[Webkit-unassigned] [Bug 113786] Throttle repaints during page loading in tile controller
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 2 09:24:04 PDT 2013
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.
More information about the webkit-unassigned
mailing list