Bug 55157

Summary: Simplify RenderTheme::volumeSliderOffsetFromMuteButton, unduplicate code.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: New BugsAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED INVALID    
Severity: Normal CC: abarth, darin, eric.carlson, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 55152    
Bug Blocks: 53020    
Attachments:
Description Flags
Patch
eric.carlson: review+
Uses volumeSliderOffsetRelativeToMuteButton. darin: review+

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.