Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561823003:
Ensure LayoutMedia notifies MediaControls of panel width (Closed)
|
Created:
4 years ago by billorr Modified:
4 years ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, ddorwin, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, mlamouri (slow - plz ping), mlamouri+watch-blink_chromium.org, pdr+renderingwatchlist_chromium.org, Srirama, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure LayoutMedia notifies MediaControls of panel width
Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content.
LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes.
In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed.
The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width.
BUG=672227
Committed: https://crrev.com/1d8e2478297992731014b1dc6661a420aaa3f1e9
Cr-Commit-Position: refs/heads/master@{#437437}
Patch Set 1 #
Total comments: 2
Patch Set 2 : feedback #
Messages
Total messages: 24 (17 generated)
billorr@chromium.org changed reviewers: + eae@chromium.org
eae, please review all files
Description was changed from ========== android: native media controls not shown Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia tries to notify MediaControls of the panel width so MediaControls can decide what controls will fit. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. This happens because LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, layout occurs without the MediaControls occuring as a child of LayoutMedia, so it misses the first notification. Subsequent notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls even if it hasn't changed if we haven't yet sent the width. BUG=672227 ========== to ========== Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ==========
Description was changed from ========== Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ========== to ========== Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ==========
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
non-owner LGTM Thanks for looking into the issue! Sorry I couldn't get to it sooner. https://codereview.chromium.org/2561823003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/2561823003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:108: // store the last value we reported, so we know if it has changed nit: Use proper sentence format for consistency (start with a capital and end with a dot).
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
https://codereview.chromium.org/2561823003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/2561823003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:108: // store the last value we reported, so we know if it has changed On 2016/12/08 23:21:06, whywhat wrote: > nit: Use proper sentence format for consistency (start with a capital and end > with a dot). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
Description was changed from ========== Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ========== to ========== Ensure LayoutMedia notifies MediaControls of panel width Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ==========
The CQ bit was unchecked by billorr@chromium.org
The CQ bit was checked by billorr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2561823003/#ps20001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481247614402030,
"parent_rev": "9760d8f4be887e6fab39482fe3d66adf993e7110", "commit_rev":
"18e1980681be21176c5e50a4b0b7c0c478a5293b"}
Message was sent while issue was closed.
Description was changed from ========== Ensure LayoutMedia notifies MediaControls of panel width Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ========== to ========== Ensure LayoutMedia notifies MediaControls of panel width Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Ensure LayoutMedia notifies MediaControls of panel width Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 ========== to ========== Ensure LayoutMedia notifies MediaControls of panel width Some pages weren't showing native media controls on Android. This was easiest to repro when using an iframe to embed youtube content. LayoutMedia notifies MediaControls of the panel width so MediaControls can decide what controls will fit. LayoutMedia calculates the size during layout, and only notifies the MediaControls if the size changes. In the case of the bug, LayoutMedia never notifies MediaControls of the correct size. The first time layout occurs, there is no child MediaControls. During subsequent layouts, notifications aren't sent because the size hasn't changed. The fix is to ensure we send the panel width to MediaControls if the size change or if we haven't yet sent the width. BUG=672227 Committed: https://crrev.com/1d8e2478297992731014b1dc6661a420aaa3f1e9 Cr-Commit-Position: refs/heads/master@{#437437} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1d8e2478297992731014b1dc6661a420aaa3f1e9 Cr-Commit-Position: refs/heads/master@{#437437} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
