RESOLVED INVALID Bug 55157
Simplify RenderTheme::volumeSliderOffsetFromMuteButton, unduplicate code. 
https://bugs.webkit.org/show_bug.cgi?id=55157
Summary Simplify RenderTheme::volumeSliderOffsetFromMuteButton, unduplicate code.
Dimitri Glazkov (Google)
Reported 2011-02-24 09:33:42 PST
Simplify RenderTheme::volumeSliderOffsetFromMuteButton, unduplicate code.
Attachments
Patch (11.96 KB, patch)
2011-02-24 09:41 PST, Dimitri Glazkov (Google) 		eric.carlson: review+
DetailsFormatted DiffDiff
Uses volumeSliderOffsetRelativeToMuteButton. (12.22 KB, patch)
2011-02-24 12:21 PST, Dimitri Glazkov (Google) 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2011-02-24 09:41:12 PST
Created attachment 83674 [details] Patch
Dimitri Glazkov (Google)
Comment 2 2011-02-24 09:41:59 PST
I'll give the bots to cook this patch once bug 55152 patch lands.
Dimitri Glazkov (Google)
Comment 3 2011-02-24 10:55:34 PST
Comment on attachment 83674 [details] Patch Cook, bots, cook!
Eric Carlson
Comment 4 2011-02-24 11:43:21 PST
Comment on attachment 83674 [details] Patch I like the code simplification here, but I don't so much like RenderTheme::volumeSliderOffsetFromMuteButton changing to RenderTheme:: volumeSliderOffset because the new name says nothing about what the offset is relative to. Please consider a more descriptive name: volumeSliderOffsetRelativeToMuteButton, volumeSliderOffsetFromMuteButton, ...
Dimitri Glazkov (Google)
Comment 5 2011-02-24 12:21:16 PST
Created attachment 83696 [details] Uses volumeSliderOffsetRelativeToMuteButton.
Dimitri Glazkov (Google)
Comment 6 2011-02-24 13:10:12 PST
Committed r79607: <http://trac.webkit.org/changeset/79607>
Dimitri Glazkov (Google)
Comment 7 2011-02-24 13:45:26 PST
Reverted r79607 for reason: Broke Chromium layout tests. Committed r79613: <http://trac.webkit.org/changeset/79613>
Dimitri Glazkov (Google)
Comment 8 2011-02-24 13:46:31 PST
(In reply to comment #7) > Reverted r79607 for reason: > > Broke Chromium layout tests. > > Committed r79613: <http://trac.webkit.org/changeset/79613> I messed up. The logic for calculating offsets wasn't the same. Need to think on this a bit more.
WebKit Review Bot
Comment 9 2011-02-24 15:59:55 PST
http://trac.webkit.org/changeset/79613 might have broken Qt Linux Release
Dimitri Glazkov (Google)
Comment 10 2011-03-03 13:12:07 PST
Went with a milder version of this refactoring instead. See bug 55099.
Note You need to log in before you can comment on or make changes to this bug.