Update media controls time formatting to use hours [chromium/src : master]
Tommy Steimel (Gerrit)
PTAL
Patch set 3:Commit-Queue +1
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Steimel (Gerrit)
Tommy Steimel has uploaded this change for review.
Update media controls time formatting to use hours
This CL changes the formatting rules of the media controls as follows:
[0-10) minutes => m:ss
[10-60) minutes => mm:ss
[1-10) hours => h:mm:ss
[10-100) hours => hh:mm:ss
[100-1000) hours => hhh:mm:ss
etc.
The current time format will no longer be affected by the duration. For
example, if we're 1 minute and 51 seconds into a 12 minute video, we
would show "1:51 / 12:00" (not zero-padding the minutes).
Bug: 739152
Change-Id: Ibed50426593dff0c5541a3a9c88a0b6229a896b3
---
M third_party/WebKit/Source/core/layout/LayoutTheme.cpp
M third_party/WebKit/Source/core/layout/LayoutTheme.h
M third_party/WebKit/Source/core/layout/LayoutThemeTest.cpp
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
4 files changed, 26 insertions(+), 34 deletions(-)
Mounir Lamouri (Gerrit)
sgtm, left two comments to improve this further.
2 comments:
File third_party/WebKit/Source/core/layout/LayoutTheme.h:
Patch Set #3, Line 205:
String FormatMediaControlsCurrentTime(float current_time) const;Actually, while you are at it, could you move this out of LayoutTheme and move it to the util class in media_controls/elements/?
File third_party/WebKit/Source/core/layout/LayoutTheme.cpp:
Patch Set #3, Line 285:
const char* separator = include_separator ? "/ " : "";If we move this code out of `FormatChromiumMediaControlsTime`, could we have the same method to format the current time and the duration?
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
CJ DiMeglio (Gerrit)
1 comment:
File third_party/WebKit/Source/core/layout/LayoutThemeTest.cpp:
Add a test that is both negative and has hours?
How do you trigger the leading "/"? If easy test?
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Steimel (Gerrit)
PTAL :)
Patch set 4:Commit-Queue +1
3 comments:
File third_party/WebKit/Source/core/layout/LayoutTheme.h:
Patch Set #3, Line 205:
// along the track.Actually, while you are at it, could you move this out of LayoutTheme and move it to the util class […]
Done. With the change, I also went ahead and made it happen anytime SetCurrentValue is called (since we were manually doing it each time anyway)
File third_party/WebKit/Source/core/layout/LayoutTheme.cpp:
Patch Set #3, Line 285:
Color LayoutTheme::InactiveSelectionForegroundColor() const {If we move this code out of `FormatChromiumMediaControlsTime`, could we have the same method to form […]
Yes, though I override the method in the duration one to prepend the "/ "
File third_party/WebKit/Source/core/layout/LayoutThemeTest.cpp:
Add a test that is both negative and has hours? […]
With the big change to move into media_controls module, I also added a quick check that the duration time has the "/ ". Added negative hour test
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
apacible (Gerrit)
2 comments:
File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:
Since the format of the current time no longer depends on
// the duration, do we still need this?nit: "Determine if this is still needed since the format..."
Patch Set #5, Line 20:
New UI"New" is relative. How about having an example of expected time after formatting?
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
lgtm with comments applied. We should wait for rachelis@ before landing though.
Patch set 5:Code-Review +1
3 comments:
Since the format of the current time no longer depends on
// the duration, do we still need this?
nit: "Determine if this is still needed since the format... […]
Maybe worth filing a bug and link it here.
Patch Set #5, Line 21:
String time("/ ");Probably not worth doing now but we could always introduce a DOM element for the "/". It will come with pros (no need for this special casing) and cons (need to hide it when duration is hidden) so probably not worth doing in this CL.
File third_party/WebKit/Source/modules/media_controls/elements/MediaControlTimeDisplayElement.cpp:
Patch Set #5, Line 18:
IGNORE_EXCEPTION_FOR_TESTINGASSERT_NO_EXCEPTION
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Steimel (Gerrit)
Patch set 6:Commit-Queue +1
4 comments:
756698): Determine if this is still needed since the format
// of the current time no longer depend
Maybe worth filing a bug and link it here.
Done and done
Patch Set #5, Line 20:
For th"New" is relative. […]
Done
Patch Set #5, Line 21:
// from the duration, e.g. "0:12 / 3:45".Probably not worth doing now but we could always introduce a DOM element for the "/". […]
Ack. Should I create a bug?
File third_party/WebKit/Source/modules/media_controls/elements/MediaControlTimeDisplayElement.cpp:
Patch Set #5, Line 18:
ASSERT_NO_EXCEPTION);ASSERT_NO_EXCEPTION
Done
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Mounir Lamouri (Gerrit)
1 comment:
Patch Set #5, Line 21:
// from the duration, e.g. "0:12 / 3:45".Ack. […]
I don't think it's needed.
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Steimel (Gerrit)
+eae@ for OWNERs review of third_party/WebKit/Source/core/layout
1 comment:
Patch Set #5, Line 21:
// from the duration, e.g. "0:12 / 3:45".
I don't think it's needed.
Sounds good
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
apacible (Gerrit)
Patch set 6:Code-Review +1
Tommy Steimel (Gerrit)
eae@ friendly ping :)
Tommy Steimel (Gerrit)
+f...@opera.com OWNERs review of third_party/WebKit/Source/core/layout
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Steimel (Gerrit)
Tommy Steimel removed Emil A Eklund from this change.
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
Patch set 6:Code-Review +1
1 comment:
String time("/ ");
time.append(MediaControlTimeDisplayElementI believe you could write this as just:
return "/ " + MediaControlTimeDisplayElement::FormatTime();
saving one String allocation.
(May need to include platform/wtf/text/StringOperators.h)
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Steimel (Gerrit)
Patch set 7:Commit-Queue +2
1 comment:
return "/ " + MediaControlTimeDisplayElement::FormatTime();
}I believe you could write this as just: […]
Much cleaner. Thanks!
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"rebase and cleaner formattime" https://chromium-review.googlesource.com/c/611703/7
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/611703/7
Bot data: {"action": "start", "triggered_at": "2017-08-24T18:33:17.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "f5312d75b1a938be5b3f0502db1376cd70486de7"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/249529)
Tommy Steimel (Gerrit)
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"rebase and cleaner formattime" https://chromium-review.googlesource.com/c/611703/7
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/611703/7
Bot data: {"action": "start", "triggered_at": "2017-08-24T22:25:55.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "f5312d75b1a938be5b3f0502db1376cd70486de7"}
To view, visit change 611703. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Update media controls time formatting to use hours
This CL changes the formatting rules of the media controls as follows:
[0-10) minutes => m:ss
[10-60) minutes => mm:ss
[1-10) hours => h:mm:ss
[10-100) hours => hh:mm:ss
[100-1000) hours => hhh:mm:ss
etc.
The current time format will no longer be affected by the duration. For
example, if we're 1 minute and 51 seconds into a 12 minute video, we
would show "1:51 / 12:00" (not zero-padding the minutes).
Bug: 739152
Change-Id: Ibed50426593dff0c5541a3a9c88a0b6229a896b3
Reviewed-on: https://chromium-review.googlesource.com/611703
Commit-Queue: Tommy Steimel <ste...@chromium.org>
Reviewed-by: apacible <apac...@chromium.org>
Reviewed-by: Fredrik Söderquist <f...@opera.com>
Reviewed-by: Mounir Lamouri <mlam...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497265}
---
M third_party/WebKit/Source/core/layout/LayoutTheme.cpp
M third_party/WebKit/Source/core/layout/LayoutTheme.h
M third_party/WebKit/Source/core/layout/LayoutThemeTest.cpp
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
M third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlRemainingTimeDisplayElement.cpp
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlRemainingTimeDisplayElement.h
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlTimeDisplayElement.cpp
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlTimeDisplayElement.h
9 files changed, 89 insertions(+), 81 deletions(-)