Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(53)

Issue 2154233003: Rewrite YouTube Flash embeds (Closed)

Created:
2 years, 9 months ago by kdsilva
Modified:
2 years, 8 months ago
CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, esprehn, gavinp+loader_chromium.org, jam, Nate Chapin, kinuko+watch, loading-reviews_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Rewrite YouTube Flash embeds. When a Flash embed for YouTube is detected, we automatically use HTML5 instead. This is done to reduce the overall usage of Flash in Chrome. BUG=625984 Committed: https://crrev.com/e0135598f6bf547463bc943100da4574c5f89930 Cr-Commit-Position: refs/heads/master@{#410715}

Patch Set 1 #

Patch Set 2 : Added unit tests #

Total comments: 32

Patch Set 3 : Changed method definition #

Patch Set 4 : Addressed comments #

Total comments: 36

Patch Set 5 : Addressed comments #

Patch Set 6 : Clean up #

Total comments: 1

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : Clean up #

Patch Set 10 : Clean up #

Total comments: 57

Patch Set 11 : Addressed comments #

Total comments: 22

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : Addressed comments #

Patch Set 15 : merging #

Patch Set 16 : #

Patch Set 17 : #

Patch Set 18 : #

Patch Set 19 : #

Patch Set 20 : #

Total comments: 20

Patch Set 21 #

Patch Set 22 : Addressed comments #

Total comments: 22

Patch Set 23 : Addressed comments #

Total comments: 2

Patch Set 24 : #

Patch Set 25 : #

Total comments: 1

Patch Set 26 : #

Patch Set 27 : #

Patch Set 28 : #

Total comments: 12

Patch Set 29 : rebase #

Total comments: 6

Patch Set 30 : Addressed comments #

Total comments: 2

Patch Set 31 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -0 lines) Patch
M chrome/common/chrome_features.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/renderer/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +56 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 3 chunks +104 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +94 lines, -0 lines 0 comments Download
A chrome/test/data/flash_embeds.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +16 lines, -0 lines 0 comments Download
M content/public/renderer/content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/renderer/content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +5 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 2 chunks +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoaderClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebFrameClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +7 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 132 (85 generated)
mlamouri (slow - plz ping)
https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_content_renderer_client.cc#newcode1399 chrome/renderer/chrome_content_renderer_client.cc:1399: gurl.DomainIs("www.youtube.com") && gurl.path().find("/v/") == 0) { Could this be ...
2 years, 9 months ago (2016-07-20 13:22:03 UTC) #4
kdsilva
https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_content_renderer_client.cc#newcode1399 chrome/renderer/chrome_content_renderer_client.cc:1399: gurl.DomainIs("www.youtube.com") && gurl.path().find("/v/") == 0) { On 2016/07/20 13:22:02, ...
2 years, 9 months ago (2016-07-21 13:59:46 UTC) #5
mlamouri (slow - plz ping)
looks good :) BTW, chromium's coding style uses "foo_bar" for variable names instead of "fooBar" ...
2 years, 9 months ago (2016-07-21 15:55:08 UTC) #6
kdsilva
https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome_content_renderer_client.cc#newcode1396 chrome/renderer/chrome_content_renderer_client.cc:1396: On 2016/07/21 15:55:07, Mounir Lamouri wrote: > style: remove ...
2 years, 9 months ago (2016-07-22 13:55:58 UTC) #7
mlamouri (slow - plz ping)
I think you've changed the indentation of the unit test file by mistake. https://codereview.chromium.org/2154233003/diff/140001/chrome/renderer/chrome_content_renderer_client_unittest.cc File ...
2 years, 9 months ago (2016-07-25 12:40:35 UTC) #8
kdsilva
2 years, 9 months ago (2016-07-28 11:23:23 UTC) #9
mlamouri (slow - plz ping)
Can you change the youtube video id you use to be something that doesn't actually ...
2 years, 9 months ago (2016-07-28 13:05:13 UTC) #10
kdsilva
https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_features.cc File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_features.cc#newcode58 chrome/common/chrome_features.cc:58: base::FEATURE_ENABLED_BY_DEFAULT}; On 2016/07/28 13:05:12, Mounir Lamouri wrote: > style: ...
2 years, 9 months ago (2016-07-28 15:17:18 UTC) #11
mlamouri (slow - plz ping)
Looks good! Only some details below. https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Source/core/DEPS File third_party/WebKit/Source/core/DEPS (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Source/core/DEPS#newcode7 third_party/WebKit/Source/core/DEPS:7: "+media/base", On 2016/07/28 ...
2 years, 9 months ago (2016-07-28 19:46:54 UTC) #16
kdsilva
https://codereview.chromium.org/2154233003/diff/240001/chrome/common/chrome_features.cc File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/common/chrome_features.cc#newcode58 chrome/common/chrome_features.cc:58: base::FEATURE_ENABLED_BY_DEFAULT}; On 2016/07/28 19:46:54, Mounir Lamouri wrote: > style-wise, ...
2 years, 9 months ago (2016-07-29 17:59:07 UTC) #37
mlamouri (slow - plz ping)
https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome_content_renderer_client.cc#newcode1388 chrome/renderer/chrome_content_renderer_client.cc:1388: return ""; Instead of checking if the string is ...
2 years, 8 months ago (2016-07-30 17:46:06 UTC) #44
kdsilva
https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome_content_renderer_client.cc#newcode1388 chrome/renderer/chrome_content_renderer_client.cc:1388: return ""; On 2016/07/30 17:46:06, Mounir Lamouri wrote: > ...
2 years, 8 months ago (2016-08-01 12:42:45 UTC) #51
mlamouri (slow - plz ping)
lgtm with comments applied. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome_content_renderer_client.cc#newcode1396 chrome/renderer/chrome_content_renderer_client.cc:1396: // any URLs that contain ...
2 years, 8 months ago (2016-08-01 13:44:00 UTC) #55
dcheng
Drive-by comments. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome_content_renderer_client.cc#newcode1401 chrome/renderer/chrome_content_renderer_client.cc:1401: return ""; This should just use GURLs, ...
2 years, 8 months ago (2016-08-01 14:50:34 UTC) #57
jochen (gone - plz use gerrit)
I'm deferring to dcheng@
2 years, 8 months ago (2016-08-02 15:31:47 UTC) #60
kdsilva
I've updated the CL to address your comments PTAL! https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome_content_renderer_client.cc#newcode1396 chrome/renderer/chrome_content_renderer_client.cc:1396: ...
2 years, 8 months ago (2016-08-02 15:34:47 UTC) #61
Avi (use Gerrit)
drive-by https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome_content_renderer_client.cc#newcode1402 chrome/renderer/chrome_content_renderer_client.cc:1402: // for each subsequent parameter. We do this ...
2 years, 8 months ago (2016-08-02 15:59:38 UTC) #62
kdsilva
https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome_content_renderer_client.cc#newcode1402 chrome/renderer/chrome_content_renderer_client.cc:1402: // for each subsequent parameter. We do this becuase ...
2 years, 8 months ago (2016-08-02 16:55:36 UTC) #67
omattos
https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp#newcode155 third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:155: m_serviceType = "text/html"; I take it this m_serviceType override ...
2 years, 8 months ago (2016-08-03 11:36:04 UTC) #73
mlamouri (slow - plz ping)
On 2016/08/03 at 11:36:04, omattos wrote: > https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp > File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): > > https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp#newcode155 ...
2 years, 8 months ago (2016-08-03 12:55:29 UTC) #80
Mike West
//third_party/WebKit LGTM, and I left some comments in the implementation higher up the stack (though ...
2 years, 8 months ago (2016-08-04 12:41:39 UTC) #85
kdsilva
https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome_content_renderer_client.cc#newcode1404 chrome/renderer/chrome_content_renderer_client.cc:1404: if (!url.DomainIs("youtube.com") || url.path().find("/v/") != 0) On 2016/08/04 at ...
2 years, 8 months ago (2016-08-05 11:06:03 UTC) #88
jochen (gone - plz use gerrit)
is there a particular reason you plumb this through so many layers? couldn't this just ...
2 years, 8 months ago (2016-08-05 11:56:04 UTC) #89
mlamouri (slow - plz ping)
On 2016/08/05 at 11:56:04, jochen wrote: > is there a particular reason you plumb this ...
2 years, 8 months ago (2016-08-05 12:31:25 UTC) #92
jochen (gone - plz use gerrit)
On 2016/08/05 at 12:31:25, mlamouri wrote: > On 2016/08/05 at 11:56:04, jochen wrote: > > ...
2 years, 8 months ago (2016-08-05 12:32:20 UTC) #93
mlamouri (slow - plz ping)
On 2016/08/05 at 12:32:20, jochen wrote: > On 2016/08/05 at 12:31:25, mlamouri wrote: > > ...
2 years, 8 months ago (2016-08-05 12:34:42 UTC) #94
jochen (gone - plz use gerrit)
did you try using the same logic as the mobile youtube thing? I wonder why ...
2 years, 8 months ago (2016-08-05 13:42:59 UTC) #95
mlamouri (slow - plz ping)
On 2016/08/05 at 13:42:59, jochen wrote: > did you try using the same logic as ...
2 years, 8 months ago (2016-08-05 14:31:05 UTC) #96
dcheng
On 2016/08/05 14:31:05, Mounir Lamouri wrote: > On 2016/08/05 at 13:42:59, jochen wrote: > > ...
2 years, 8 months ago (2016-08-05 15:46:22 UTC) #97
mlamouri (slow - plz ping)
On 2016/08/05 at 15:46:22, dcheng wrote: > On 2016/08/05 14:31:05, Mounir Lamouri wrote: > > ...
2 years, 8 months ago (2016-08-05 16:09:00 UTC) #98
mlamouri (slow - plz ping)
On 2016/08/05 at 16:09:00, Mounir Lamouri wrote: > On 2016/08/05 at 15:46:22, dcheng wrote: > ...
2 years, 8 months ago (2016-08-05 16:22:31 UTC) #99
dcheng
I talked with mlamouri. I'm OK with not using the placeholder plugin approach, since this ...
2 years, 8 months ago (2016-08-05 16:29:10 UTC) #100
dcheng
On 2016/08/05 16:29:10, dcheng (OOO Aug 1 - Aug 11) wrote: > I talked with ...
2 years, 8 months ago (2016-08-05 16:29:46 UTC) #101
dcheng
lgtm
2 years, 8 months ago (2016-08-05 16:32:44 UTC) #102
kdsilva
Hi John, Jochen, who was originally marked as a reviewer, is OOO for the next ...
2 years, 8 months ago (2016-08-08 09:52:12 UTC) #105
mlamouri (slow - plz ping)
https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome_content_renderer_client.cc#newcode1404 chrome/renderer/chrome_content_renderer_client.cc:1404: if (!url.DomainIs("youtube.com") && !url.DomainIs("youtube-nocookie.com")) I can't find a unit ...
2 years, 8 months ago (2016-08-08 12:45:17 UTC) #106
jam
lgtm
2 years, 8 months ago (2016-08-08 18:14:04 UTC) #111
kdsilva
https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome_content_renderer_client.cc#newcode1404 chrome/renderer/chrome_content_renderer_client.cc:1404: if (!url.DomainIs("youtube.com") && !url.DomainIs("youtube-nocookie.com")) On 2016/08/08 12:45:17, Mounir Lamouri ...
2 years, 8 months ago (2016-08-08 19:09:04 UTC) #112
mlamouri (slow - plz ping)
https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/public/web/WebFrameClient.h File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/public/web/WebFrameClient.h#newcode706 third_party/WebKit/public/web/WebFrameClient.h:706: // A null URL is returned if the URL ...
2 years, 8 months ago (2016-08-08 19:43:07 UTC) #113
kdsilva
https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/public/web/WebFrameClient.h File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/public/web/WebFrameClient.h#newcode706 third_party/WebKit/public/web/WebFrameClient.h:706: // A null URL is returned if the URL ...
2 years, 8 months ago (2016-08-09 09:54:39 UTC) #116
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2154233003/640001
2 years, 8 months ago (2016-08-09 09:55:54 UTC) #120
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/118661)
2 years, 8 months ago (2016-08-09 10:28:22 UTC) #122
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2154233003/640001
2 years, 8 months ago (2016-08-09 10:36:56 UTC) #124
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/269813)
2 years, 8 months ago (2016-08-09 13:14:34 UTC) #126
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2154233003/640001
2 years, 8 months ago (2016-08-09 13:18:48 UTC) #128
commit-bot: I haz the power
Committed patchset #31 (id:640001)
2 years, 8 months ago (2016-08-09 17:08:33 UTC) #130
commit-bot: I haz the power
2 years, 8 months ago (2016-08-09 17:13:14 UTC) #132
Message was sent while issue was closed.
Patchset 31 (id:??) landed as
https://crrev.com/e0135598f6bf547463bc943100da4574c5f89930
Cr-Commit-Position: refs/heads/master@{#410715}

Powered by Google App Engine
This is Rietveld 408576698