Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2228623002:
Adding Android specific behavior when overriding YouTube Flash embeds. (Closed)
DescriptionRewrites YouTube embeds on Android devices even if the URL contains enablejsapi=1.
BUG=633216
Committed: https://crrev.com/621478c1990044b573a7b25c5d16427fe79a3018
Cr-Commit-Position: refs/heads/master@{#410726}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments #Patch Set 3 : Addressed comments #
Total comments: 10
Patch Set 4 : Addressed comments #
Depends on Patchset: Messages
Total messages: 29 (18 generated)
Description was changed from ========== Adding Android specific behavior when overriding YouTube Flash embeds. BUG= ========== to ========== Rewrites YouTube embeds on Android devices even if the URL contains enablejsapi=1. BUG=633216 ==========
kdsilva@google.com changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:1454: #endif I think this would be more readable if you had: ``` internal::YouTubeRewriteStatus result = internal::THE_MAX_VALUE; if (corrected_url.query().find("enablejsapi=1") != std::string::npos) { #if defined(OS_ANDROID) result = internal::SUCCESS_ENABLEJSAPI; #else RecordYouTubeRewriteUMA(...); return GURL(); #endif } [...] if (result == internal) { result = invalid_url ? internal::SUCCESS_PARAMS_REWRITE : internal::SUCCESS); } RecordYouTubeRewriteUMA(result); return [...]; ``` https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:1466: : internal::SUCCESS); style: add { } when there is more than a line. https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:504: #if defined(OS_ANDROID) Maybe better to put the ifdef outside of TEST_F() so the test isn't run at all on desktop. https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:535: #if !defined(OS_ANDROID) ditto, you could even have a #else
The CQ bit was checked by kdsilva@google.com 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...
The CQ bit was checked by kdsilva@google.com 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...
https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:1454: #endif On 2016/08/08 19:50:25, Mounir Lamouri wrote: > I think this would be more readable if you had: > ``` > internal::YouTubeRewriteStatus result = internal::THE_MAX_VALUE; > > if (corrected_url.query().find("enablejsapi=1") != std::string::npos) { > #if defined(OS_ANDROID) > result = internal::SUCCESS_ENABLEJSAPI; > #else > RecordYouTubeRewriteUMA(...); > return GURL(); > #endif > } > > [...] > > if (result == internal) { > result = invalid_url ? internal::SUCCESS_PARAMS_REWRITE > : internal::SUCCESS); > } > > RecordYouTubeRewriteUMA(result); > > return [...]; > ``` Done. https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:1466: : internal::SUCCESS); On 2016/08/08 19:50:25, Mounir Lamouri wrote: > style: add { } when there is more than a line. Done. https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:504: #if defined(OS_ANDROID) On 2016/08/08 19:50:25, Mounir Lamouri wrote: > Maybe better to put the ifdef outside of TEST_F() so the test isn't run at all > on desktop. Done. https://codereview.chromium.org/2228623002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:535: #if !defined(OS_ANDROID) On 2016/08/08 19:50:25, Mounir Lamouri wrote: > ditto, you could even have a #else Done.
lgtm with comments applied. https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:530: client.OverrideFlashEmbedWithHTML(GURL(data.original))); style: this requires { } https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:532: nit: you could remove this empty line. https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:556: client.OverrideFlashEmbedWithHTML(GURL(data.original))); style: this requires { } https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:704: ditto https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:731: nit: remove this empty line
The CQ bit was checked by kdsilva@google.com 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...
kdsilva@google.com changed reviewers: + jam@chromium.org
kdsilva@google.com changed required reviewers: + jam@chromium.org
https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:530: client.OverrideFlashEmbedWithHTML(GURL(data.original))); On 2016/08/09 11:12:38, Mounir Lamouri wrote: > style: this requires { } Done. https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:532: On 2016/08/09 11:12:39, Mounir Lamouri wrote: > nit: you could remove this empty line. Done. https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:556: client.OverrideFlashEmbedWithHTML(GURL(data.original))); On 2016/08/09 11:12:38, Mounir Lamouri wrote: > style: this requires { } Done. https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:704: On 2016/08/09 11:12:38, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/2228623002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:731: On 2016/08/09 11:12:38, Mounir Lamouri wrote: > nit: remove this empty line Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jam@, I noticed you responded to the CL but didn't leave a message. Could I ask you to take another quick look?
lgtm
The CQ bit was checked by kdsilva@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2228623002/#ps60001 (title: "Addressed comments")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2206343002 Patch 260001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by kdsilva@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Rewrites YouTube embeds on Android devices even if the URL contains enablejsapi=1. BUG=633216 ========== to ========== Rewrites YouTube embeds on Android devices even if the URL contains enablejsapi=1. BUG=633216 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Rewrites YouTube embeds on Android devices even if the URL contains enablejsapi=1. BUG=633216 ========== to ========== Rewrites YouTube embeds on Android devices even if the URL contains enablejsapi=1. BUG=633216 Committed: https://crrev.com/621478c1990044b573a7b25c5d16427fe79a3018 Cr-Commit-Position: refs/heads/master@{#410726} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/621478c1990044b573a7b25c5d16427fe79a3018 Cr-Commit-Position: refs/heads/master@{#410726} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
