Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by Srirama Modified:
4 years, 10 months ago Reviewers:
philipj_slow, fs CC:
fs, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary variable HTMLMediaElement::m_haveVisibleTextTrack
m_haveVisibleTextTrack is just used in the function
configureTextTrackDisplay, so use a local variable instead
Committed: https://crrev.com/39245052a1de08122ae972abf0c5ac337168ba8f
Cr-Commit-Position: refs/heads/master@{#374659}
Patch Set 1 #
Total comments: 3
Created: 4 years, 10 months ago
Messages
Total messages: 13 (7 generated)
Description was changed from ========== fix BUG= ========== to ========== Remove unnecessary member variable m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ==========
srirama.m@samsung.com changed reviewers: + fs@opera.com, philipj@opera.com
fs@opera.com: Please review changes in philipj@opera.com: Please review changes in https://codereview.chromium.org/1686063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1686063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3368: if (!haveVisibleTextTrack && !mediaControls()) Local variable can be removed and use m_closedCaptionsVisible directly but i kept it for better readability. WDYT? should i remove it as well?
lgtm Maybe mention HTMLMediaElement in the subject (HTMLMediaElement::m_haveVisibleTextTrack if that doesn't overflow.) https://codereview.chromium.org/1686063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1686063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3368: if (!haveVisibleTextTrack && !mediaControls()) On 2016/02/10 at 12:50:50, Srirama wrote: > Local variable can be removed and use m_closedCaptionsVisible directly but i kept it for better readability. WDYT? should i remove it as well? I don't mind either way. I think I agree that using the local is slightly more readable though.
Description was changed from ========== Remove unnecessary member variable m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ========== to ========== Remove unneeded member variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ==========
Description was changed from ========== Remove unneeded member variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ========== to ========== Remove unnecessary variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ==========
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1686063002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1686063002/1
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ========== to ========== Remove unnecessary variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead ========== to ========== Remove unnecessary variable HTMLMediaElement::m_haveVisibleTextTrack m_haveVisibleTextTrack is just used in the function configureTextTrackDisplay, so use a local variable instead Committed: https://crrev.com/39245052a1de08122ae972abf0c5ac337168ba8f Cr-Commit-Position: refs/heads/master@{#374659} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/39245052a1de08122ae972abf0c5ac337168ba8f Cr-Commit-Position: refs/heads/master@{#374659}
Message was sent while issue was closed.
Post-commit LGTM https://codereview.chromium.org/1686063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1686063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3368: if (!haveVisibleTextTrack && !mediaControls()) On 2016/02/10 14:29:34, fs wrote: > On 2016/02/10 at 12:50:50, Srirama wrote: > > Local variable can be removed and use m_closedCaptionsVisible directly but i > kept it for better readability. WDYT? should i remove it as well? > > I don't mind either way. I think I agree that using the local is slightly more > readable though. WFM too |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
