Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2154233003:
Rewrite YouTube Flash embeds (Closed)
|
Created:
4 years, 5 months ago by kdsilva Modified:
4 years, 4 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. |
DescriptionRewrite 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 #Dependent Patchsets: Messages
Total messages: 132 (85 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1399: gurl.DomainIs("www.youtube.com") && gurl.path().find("/v/") == 0) { Could this be "youtube.com" instead of "www.youtube.com"? In other words, would it work? Can YouTube embed not have "www"? https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1406: bool invalidUrl = false; nit: maybe have the `bool` first. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1409: if (indexQm == std::string::npos || indexQm > indexAmp) { Does this mean that "www.youtube.com/v/VIDEO_ID" is an invalid URL? How does string::npos compare to other values (ie. is it per spec equal to 0 or INT_MAX?)? https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1411: } style: we [unfortunately] don't put { } for one-liners. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1428: } nit: you should favour early returns. In this case, you could do: ``` if (!gurl.DomainIs(...)) return false; if (no "/v/" or has "enablejsapi=1") return false; // the real code ``` https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:421: expected = "http://www.youtube.com"; Maybe you should add a test without "www." and also one with some other bogus subdomain like "foobar.youtube.com". Also, could you add a test with "https://" ? https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:437: // URL isn't using Flash, has JS API enabled You don't test enablejsapi=1 when /v/ is used. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:473: // URL is using Flash, is invalid, has multiple parameters Can you add one like this with &foo=a&bar=b?tulip=c https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:477: EXPECT_EQ(original, expected); The tests look fine. I think it might be interesting to have all the input/output in a big data structure and run a loop to test instead of copy/pasting. Feel free to ignore this for now though. https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... File content/public/renderer/content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... content/public/renderer/content_renderer_client.cc:228: nit: empty line. https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... content/public/renderer/content_renderer_client.h:363: // Overwrites the URL and MIME type of a YouTube Flash embed to use an Don't mention YouTube here. It can be implemented by anything else. Chrome's implementation is YouTube specific. https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: } nit: you can do: ``` if (GetContentClient()->renderer()->OverrideEmbedInfo(&url_str)) { *url = blink::WebString::fromUTF8(url_str); return true; } return false; ``` https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:238: // Overwrites the URL and MIME type of a YouTube Flash embed to use an ditto https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:855: nit: empty line again https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1044: if (m_webFrame->client()) { if (!m_webFrame->client()) return false; https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:731: // HTML5 video player when possible same as above
https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... 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, Mounir Lamouri wrote: > Could this be "youtube.com" instead of "www.youtube.com"? In other words, would > it work? Can YouTube embed not have "www"? Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1406: bool invalidUrl = false; On 2016/07/20 13:22:02, Mounir Lamouri wrote: > nit: maybe have the `bool` first. Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1409: if (indexQm == std::string::npos || indexQm > indexAmp) { On 2016/07/20 13:22:02, Mounir Lamouri wrote: > Does this mean that "www.youtube.com/v/VIDEO_ID" is an invalid URL? > > How does string::npos compare to other values (ie. is it per spec equal to 0 or > INT_MAX?)? You're right that youtube.com/v/ID should't be considered an invalid URL. I've updated the code to reflect that! string:npos is the is the largest possible value. That simplifies the if statement a bit. PTAL. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1411: } On 2016/07/20 13:22:02, Mounir Lamouri wrote: > style: we [unfortunately] don't put { } for one-liners. Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1428: } On 2016/07/20 13:22:02, Mounir Lamouri wrote: > nit: you should favour early returns. In this case, you could do: > ``` > if (!gurl.DomainIs(...)) > return false; > > if (no "/v/" or has "enablejsapi=1") > return false; > > // the real code > ``` Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:421: expected = "http://www.youtube.com"; On 2016/07/20 13:22:02, Mounir Lamouri wrote: > Maybe you should add a test without "www." and also one with some other bogus > subdomain like "foobar.youtube.com". > > Also, could you add a test with "https://" ? Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:437: // URL isn't using Flash, has JS API enabled On 2016/07/20 13:22:02, Mounir Lamouri wrote: > You don't test enablejsapi=1 when /v/ is used. Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:473: // URL is using Flash, is invalid, has multiple parameters On 2016/07/20 13:22:02, Mounir Lamouri wrote: > Can you add one like this with > &foo=a&bar=b?tulip=c Done. https://codereview.chromium.org/2154233003/diff/60001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:477: EXPECT_EQ(original, expected); On 2016/07/20 13:22:02, Mounir Lamouri wrote: > The tests look fine. I think it might be interesting to have all the > input/output in a big data structure and run a loop to test instead of > copy/pasting. Feel free to ignore this for now though. Acknowledged. https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... File content/public/renderer/content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... content/public/renderer/content_renderer_client.cc:228: On 2016/07/20 13:22:02, Mounir Lamouri wrote: > nit: empty line. Done. https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/60001/content/public/renderer... content/public/renderer/content_renderer_client.h:363: // Overwrites the URL and MIME type of a YouTube Flash embed to use an On 2016/07/20 13:22:02, Mounir Lamouri wrote: > Don't mention YouTube here. It can be implemented by anything else. Chrome's > implementation is YouTube specific. Done. https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: } On 2016/07/20 13:22:02, Mounir Lamouri wrote: > nit: you can do: > ``` > if (GetContentClient()->renderer()->OverrideEmbedInfo(&url_str)) { > *url = blink::WebString::fromUTF8(url_str); > return true; > } > > return false; > ``` Acknowledged. https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:238: // Overwrites the URL and MIME type of a YouTube Flash embed to use an On 2016/07/20 13:22:02, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/2154233003/diff/60001/content/renderer/render... content/renderer/render_frame_impl.h:855: On 2016/07/20 13:22:02, Mounir Lamouri wrote: > nit: empty line again Done. https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1044: if (m_webFrame->client()) { On 2016/07/20 13:22:03, Mounir Lamouri wrote: > if (!m_webFrame->client()) > return false; Acknowledged. https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrameClient.h:731: // HTML5 video player when possible On 2016/07/20 13:22:03, Mounir Lamouri wrote: > same as above Done.
looks good :) BTW, chromium's coding style uses "foo_bar" for variable names instead of "fooBar" in Blink. Sorry :( https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1396: style: remove empty line https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1397: GURL gurl = GURL(url); nit: I would call the `url` you get as param `str_url` because you actually only use it once (to convert to GURL). https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1398: nit: you could add `DCHECK(url.isValid())` so it guarantees that no one tries to send garbage. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1399: std::string retUrl = url; This should move after the early return so we avoid allocating a string that we will not use. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1401: if (!gurl.DomainIs("youtube.com") || gurl.path().find("/v/") != 0 Maybe add a comment explaining why these checks? :) https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1402: || retUrl.find("enablejsapi=1") != (size_t) -1) `(size_t) -1` should be `string::npos`, right? https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1416: (indexAmp != std::string::npos || indexQm != std::string::npos)) What about using std::string::find_first_of? http://www.cplusplus.com/reference/string/string/find_first_of/ I was thinking of a logic like: ``` size_it index = retUrl.find_first_of("&?"); bool invalidUrl = index != std::string::npos && retUrl[index] == '&'; ``` You might be able to re-use this `index` to do the replace after. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1432: retUrl.replace(retUrl.find("/v/"), 3, "/embed/"); Could we do this before? Like just after we know it's an URL we want to rewrite. It would allow us to do something like: ``` if (!invalid_url) return result; // Fix invalid url. [...] return result; ``` It would also make things easier to read because really, the most important part is this replace :) https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:422: EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original)); I think always defining "expected" will help to read because one can quickly look at "original" and "expected". It adds some cognitive overhead to check the EXPECT_EQ() to check what's in there. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:430: original = "https://www.foo.youtube.com"; You are testing "www.youtube.com", "youtube.com", "https://...", "www.foo.youtube.com" on URLs that are not rewritten (they don't match "/v/" requirement). You should do the same on the ones that are matching the requirements IMO. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:486: } Something you can do to improve readability is: ``` struct TestData { std::string original; std::string expected; }; std::vector<TestData> test_data = { { "https://youtube.com", "" }, // original / exppected }; for (auto data : test_data) EXPECT_EQ(data.expected, client.OverrideFlashEmbedWithHTML(data.original); ``` https://codereview.chromium.org/2154233003/diff/100001/content/public/rendere... File content/public/renderer/content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/100001/content/public/rendere... content/public/renderer/content_renderer_client.cc:233: return url; Shouldn't it be the empty string? https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:1061: return blink::WebString::fromUTF8(ret); nit: you might as well have this in one statement: ``` return blink::WebString::fromUTF8(GetContentClient()->renderer()->OverrideFlashEMbedWithHTML(url.utf8())); ``` https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:238: // Overwrites the given URL to use an HTML5 video player if possible. s/video player/embed/ https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:153: overridenUrl = document().frame()->loader().client()-> No need to have two statements. Also, Blink has no line limits so this could likely be all on one line: ``` String overridenUrl = document().frame()->loader().client()->OverrideFlashEmbedWithHTML(document().completeURL(m_url)); ``` https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/EmptyClients.h (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/EmptyClients.h:187: String OverrideFlashEmbedWithHTML(const String& url) override { return url; } empty string? https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:278: virtual String OverrideFlashEmbedWithHTML(const String&) = 0; Remove the TODO and add a real comment? :) Maybe you could have a return empty string here so you don't need to implement something in EmptyClients? https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:733: return url; Shouldn't it be the empty string?
https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1396: On 2016/07/21 15:55:07, Mounir Lamouri wrote: > style: remove empty line Done. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1397: GURL gurl = GURL(url); On 2016/07/21 15:55:07, Mounir Lamouri wrote: > nit: I would call the `url` you get as param `str_url` because you actually only > use it once (to convert to GURL). Done. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1398: On 2016/07/21 15:55:07, Mounir Lamouri wrote: > nit: you could add `DCHECK(url.isValid())` so it guarantees that no one tries to > send garbage. Done. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1399: std::string retUrl = url; On 2016/07/21 15:55:07, Mounir Lamouri wrote: > This should move after the early return so we avoid allocating a string that we > will not use. Done. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1401: if (!gurl.DomainIs("youtube.com") || gurl.path().find("/v/") != 0 On 2016/07/21 15:55:07, Mounir Lamouri wrote: > Maybe add a comment explaining why these checks? :) Done. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1402: || retUrl.find("enablejsapi=1") != (size_t) -1) On 2016/07/21 15:55:07, Mounir Lamouri wrote: > `(size_t) -1` should be `string::npos`, right? Yup! My bad. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1416: (indexAmp != std::string::npos || indexQm != std::string::npos)) On 2016/07/21 15:55:07, Mounir Lamouri wrote: > What about using std::string::find_first_of? > http://www.cplusplus.com/reference/string/string/find_first_of/ > > I was thinking of a logic like: > ``` > size_it index = retUrl.find_first_of("&?"); > bool invalidUrl = index != std::string::npos && retUrl[index] == '&'; > ``` > > You might be able to re-use this `index` to do the replace after. Wow, that is a much better way of doing it than what I'd written. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1432: retUrl.replace(retUrl.find("/v/"), 3, "/embed/"); On 2016/07/21 15:55:07, Mounir Lamouri wrote: > Could we do this before? Like just after we know it's an URL we want to rewrite. > It would allow us to do something like: > ``` > if (!invalid_url) > return result; > > // Fix invalid url. > [...] > return result; > ``` > > It would also make things easier to read because really, the most important part > is this replace :) Done. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:422: EXPECT_EQ(notOverriden, client.OverrideFlashEmbedWithHTML(original)); On 2016/07/21 15:55:07, Mounir Lamouri wrote: > I think always defining "expected" will help to read because one can quickly > look at "original" and "expected". It adds some cognitive overhead to check the > EXPECT_EQ() to check what's in there. Acknowledged. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:430: original = "https://www.foo.youtube.com"; On 2016/07/21 15:55:07, Mounir Lamouri wrote: > You are testing "www.youtube.com", "youtube.com", "https://...", > "www.foo.youtube.com" on URLs that are not rewritten (they don't match "/v/" > requirement). You should do the same on the ones that are matching the > requirements IMO. Done. Although something like "youtube.com/v/VIDEO_ID" wouldn't be considered valid since we assume we're receiving a complete URL. https://codereview.chromium.org/2154233003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:486: } On 2016/07/21 15:55:07, Mounir Lamouri wrote: > Something you can do to improve readability is: > ``` > struct TestData { > std::string original; > std::string expected; > }; > > std::vector<TestData> test_data = { > { "https://youtube.com", "" }, // original / exppected > }; > > for (auto data : test_data) > EXPECT_EQ(data.expected, client.OverrideFlashEmbedWithHTML(data.original); > ``` Done. https://codereview.chromium.org/2154233003/diff/100001/content/public/rendere... File content/public/renderer/content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/100001/content/public/rendere... content/public/renderer/content_renderer_client.cc:233: return url; On 2016/07/21 15:55:07, Mounir Lamouri wrote: > Shouldn't it be the empty string? Done. https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:1061: return blink::WebString::fromUTF8(ret); On 2016/07/21 15:55:07, Mounir Lamouri wrote: > nit: you might as well have this in one statement: > ``` > return > blink::WebString::fromUTF8(GetContentClient()->renderer()->OverrideFlashEMbedWithHTML(url.utf8())); > ``` Done. https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/2154233003/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:238: // Overwrites the given URL to use an HTML5 video player if possible. On 2016/07/21 15:55:08, Mounir Lamouri wrote: > s/video player/embed/ Done. https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:153: overridenUrl = document().frame()->loader().client()-> On 2016/07/21 15:55:08, Mounir Lamouri wrote: > No need to have two statements. Also, Blink has no line limits so this could > likely be all on one line: > ``` > String overridenUrl = > document().frame()->loader().client()->OverrideFlashEmbedWithHTML(document().completeURL(m_url)); > ``` Done. https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/EmptyClients.h (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/EmptyClients.h:187: String OverrideFlashEmbedWithHTML(const String& url) override { return url; } On 2016/07/21 15:55:08, Mounir Lamouri wrote: > empty string? Done. https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:278: virtual String OverrideFlashEmbedWithHTML(const String&) = 0; On 2016/07/21 15:55:08, Mounir Lamouri wrote: > Remove the TODO and add a real comment? :) > > Maybe you could have a return empty string here so you don't need to implement > something in EmptyClients? Done. https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:733: return url; On 2016/07/21 15:55:08, Mounir Lamouri wrote: > Shouldn't it be the empty string? Done.
I think you've changed the indentation of the unit test file by mistake. https://codereview.chromium.org/2154233003/diff/140001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/140001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:417: std::vector<TestData> test_data = { Could you move this and TestData inside TEST_F? Also, you can use this syntax: ``` struct TestData { std::string original; std::string expected; } test_data[] = { { "youtube.com", ""}, [...] }; ```
Can you change the youtube video id you use to be something that doesn't actually exist? Maybe "abcdef" or "deadbeef"?. It's not ideal to link to a real video given that we have no control over its content -- unless it's a Google/Chrome video obviously. https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... chrome/common/chrome_features.cc:58: base::FEATURE_ENABLED_BY_DEFAULT}; style: you should align the " of line 57 with the b of line 58. https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... chrome/common/chrome_features.h:46: extern const base::Feature kOverrideFlashEmbed; I think "kOverrideYouTubeFlashEmbed" would be a better name. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/DEPS File chrome/renderer/DEPS (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/DEPS#n... chrome/renderer/DEPS:3: "+chrome/browser/ui/tabs", You should have "specific_include_rules" so the expception only applies to the test, see: https://cs.chromium.org/chromium/src/chrome/renderer/safe_browsing/DEPS?q=f:c... https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1398: GURL url = GURL(str_url); You should move the `url` declaration after the FeatureList check. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1406: // style: remove this // https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1409: } style: no { } https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1419: // fix the URL by ensuring that IF there are multiple parameters, s/IF/if/ https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1428: // Replace all instances of ? with & Is this the fastest way to do what you want to do? We already have `index` that points to the first &. It sounds like going back to `start_pos = 0` would waste a few cycles. What about an algorithm that does something like: - set ret_url[index] to '?' - find all '?' after `index` and replace them with '&'. This should guarantee that we at max parse the string only once. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:110: void RunMessageRunner() { nit: maybe rename "WaitForYouTubeRequest()"? It will make the test easier to read. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:115: void SetExpectedURL(std::string given) { style: set_expected_url() https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:126: IN_PROC_BROWSER_TEST_F(ChromeContentRendererClientBrowserTest, RewriteYouTubeFlashEmbedTest) { No need to suffix with Test. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:127: net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTP); Can you simply use embedded_test_server() ? https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:133: https_server.RegisterRequestHandler(base::Bind( Can you try to use the monitor function? We don't need to handle any request here so if monitor works, it would be better. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:179: // Invalid URL, many parameters I think you have too many tests here. Browsertests take time to run, we should just make sure that the basics are working. The details and edge cases should be left to the unit tests. I would suggest to test: - normal case - subdomain - multiple parameters - multiple parameters with ? in the wrong place - enablejsapi=1 https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:412: TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbedTest) { No need to have a Test suffix in RewriteYouTubeFlashEmbed. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:417: { "youtube.com", "" }, // original, expected Can you move the "// original, expected" above and maybe have "// { original, expected }" https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:426: {"http://www.youtube.com/embed/cW44BpXpjYw", ""}, Starting from here, you seem to no longer have spaces after { and before }. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:428: {"youtube.com/embed/cW44BpXpjYw", ""}, No "www" indeed but you forgot the "http://" which makes the URL entirely invalid. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:430: {"http://www.youtube.com/embed/cW44BpXpjYw?enablejsapi=1"}, You forgot the expected value. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:437: "https://www.youtube.com/embed/cW44BpXpjYw"}, style: here and below, you have one space too many. The two strings should be aligned. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:462: "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1"} Can you have a unit test that succeeds where the URL is "http://youtube.com/..."? https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:463: }; Can you add a test with the URL being something like: - https://www.youtube.com/foo/v/ABCDEF (should fail) - http://youtube.com/v/a/ (should succeed) - http://youtube.com/v/123/ (should suceed) https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:469: } style: the coding style says that we should have { } for one-line statements. https://codereview.chromium.org/2154233003/diff/220001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/220001/content/public/rendere... content/public/renderer/content_renderer_client.h:364: virtual std::string OverrideFlashEmbedWithHTML(const std::string& url); The description should say that the empty string should be returned if there is no override. https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/DEPS (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/DEPS:7: "+media/base", I don't think you need that, do you? https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:277: // Overwrites the given URL to use an HTML5 video player if possible. The description should say that the empty string should be returned if there is no override. https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1046: return ret; I think it will read better this way: ``` if (m_webFrame->client()) return ""; return m_webFrame->client()->overrideFlashEmbedWithHTML(WebString(url)); ``` https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:730: // Overwrites the given URL to use an HTML5 video player if possible. The description should say that the empty string should be returned if there is no override.
https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... chrome/common/chrome_features.cc:58: base::FEATURE_ENABLED_BY_DEFAULT}; On 2016/07/28 13:05:12, Mounir Lamouri wrote: > style: you should align the " of line 57 with the b of line 58. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... File chrome/common/chrome_features.h (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/common/chrome_f... chrome/common/chrome_features.h:46: extern const base::Feature kOverrideFlashEmbed; On 2016/07/28 13:05:12, Mounir Lamouri wrote: > I think "kOverrideYouTubeFlashEmbed" would be a better name. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/DEPS File chrome/renderer/DEPS (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/DEPS#n... chrome/renderer/DEPS:3: "+chrome/browser/ui/tabs", On 2016/07/28 13:05:12, Mounir Lamouri wrote: > You should have "specific_include_rules" so the expception only applies to the > test, see: > https://cs.chromium.org/chromium/src/chrome/renderer/safe_browsing/DEPS?q=f:c... Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1398: GURL url = GURL(str_url); On 2016/07/28 13:05:12, Mounir Lamouri wrote: > You should move the `url` declaration after the FeatureList check. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1406: // On 2016/07/28 13:05:12, Mounir Lamouri wrote: > style: remove this // Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1409: } On 2016/07/28 13:05:12, Mounir Lamouri wrote: > style: no { } Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1419: // fix the URL by ensuring that IF there are multiple parameters, On 2016/07/28 13:05:12, Mounir Lamouri wrote: > s/IF/if/ Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1428: // Replace all instances of ? with & On 2016/07/28 13:05:12, Mounir Lamouri wrote: > Is this the fastest way to do what you want to do? We already have `index` that > points to the first &. It sounds like going back to `start_pos = 0` would waste > a few cycles. > > What about an algorithm that does something like: > - set ret_url[index] to '?' > - find all '?' after `index` and replace them with '&'. > > This should guarantee that we at max parse the string only once. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:110: void RunMessageRunner() { On 2016/07/28 13:05:13, Mounir Lamouri wrote: > nit: maybe rename "WaitForYouTubeRequest()"? It will make the test easier to > read. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:115: void SetExpectedURL(std::string given) { On 2016/07/28 13:05:12, Mounir Lamouri wrote: > style: set_expected_url() Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:126: IN_PROC_BROWSER_TEST_F(ChromeContentRendererClientBrowserTest, RewriteYouTubeFlashEmbedTest) { On 2016/07/28 13:05:13, Mounir Lamouri wrote: > No need to suffix with Test. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:127: net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTP); On 2016/07/28 13:05:12, Mounir Lamouri wrote: > Can you simply use embedded_test_server() ? Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:133: https_server.RegisterRequestHandler(base::Bind( On 2016/07/28 13:05:12, Mounir Lamouri wrote: > Can you try to use the monitor function? We don't need to handle any request > here so if monitor works, it would be better. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:179: // Invalid URL, many parameters On 2016/07/28 13:05:12, Mounir Lamouri wrote: > I think you have too many tests here. Browsertests take time to run, we should > just make sure that the basics are working. The details and edge cases should be > left to the unit tests. > > I would suggest to test: > - normal case > - subdomain > - multiple parameters > - multiple parameters with ? in the wrong place > - enablejsapi=1 Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:412: TEST_F(ChromeContentRendererClientTest, RewriteYouTubeFlashEmbedTest) { On 2016/07/28 13:05:13, Mounir Lamouri wrote: > No need to have a Test suffix in RewriteYouTubeFlashEmbed. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:417: { "youtube.com", "" }, // original, expected On 2016/07/28 13:05:13, Mounir Lamouri wrote: > Can you move the "// original, expected" above and maybe have "// { original, > expected }" Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:426: {"http://www.youtube.com/embed/cW44BpXpjYw", ""}, On 2016/07/28 13:05:13, Mounir Lamouri wrote: > Starting from here, you seem to no longer have spaces after { and before }. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:428: {"youtube.com/embed/cW44BpXpjYw", ""}, On 2016/07/28 13:05:13, Mounir Lamouri wrote: > No "www" indeed but you forgot the "http://" which makes the URL entirely > invalid. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:430: {"http://www.youtube.com/embed/cW44BpXpjYw?enablejsapi=1"}, On 2016/07/28 13:05:13, Mounir Lamouri wrote: > You forgot the expected value. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:437: "https://www.youtube.com/embed/cW44BpXpjYw"}, On 2016/07/28 13:05:13, Mounir Lamouri wrote: > style: here and below, you have one space too many. The two strings should be > aligned. Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:462: "http://www.youtube.com/embed/cW44BpXpjYw?start=4&fs=1"} On 2016/07/28 13:05:13, Mounir Lamouri wrote: > Can you have a unit test that succeeds where the URL is > "http://youtube.com/..."? Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:463: }; On 2016/07/28 13:05:13, Mounir Lamouri wrote: > Can you add a test with the URL being something like: > - https://www.youtube.com/foo/v/ABCDEF (should fail) > - http://youtube.com/v/a/ (should succeed) > - http://youtube.com/v/123/ (should suceed) Done. https://codereview.chromium.org/2154233003/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:469: } On 2016/07/28 13:05:13, Mounir Lamouri wrote: > style: the coding style says that we should have { } for one-line statements. Done. https://codereview.chromium.org/2154233003/diff/220001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/220001/content/public/rendere... content/public/renderer/content_renderer_client.h:364: virtual std::string OverrideFlashEmbedWithHTML(const std::string& url); On 2016/07/28 13:05:13, Mounir Lamouri wrote: > The description should say that the empty string should be returned if there is > no override. Done. https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/DEPS (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/DEPS:7: "+media/base", On 2016/07/28 13:05:13, Mounir Lamouri wrote: > I don't think you need that, do you? Looks like I don't! Not sure why I thought I did... https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:277: // Overwrites the given URL to use an HTML5 video player if possible. On 2016/07/28 13:05:13, Mounir Lamouri wrote: > The description should say that the empty string should be returned if there is > no override. Done. https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1046: return ret; On 2016/07/28 13:05:13, Mounir Lamouri wrote: > I think it will read better this way: > ``` > if (m_webFrame->client()) > return ""; > return m_webFrame->client()->overrideFlashEmbedWithHTML(WebString(url)); > ``` Done. Assuming you meant "if (!m_webFrame->client())" https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:730: // Overwrites the given URL to use an HTML5 video player if possible. On 2016/07/28 13:05:13, Mounir Lamouri wrote: > The description should say that the empty string should be returned if there is > no override. Done.
The CQ bit was checked by mlamouri@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Looks good! Only some details below. https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/DEPS (right): https://codereview.chromium.org/2154233003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/DEPS:7: "+media/base", On 2016/07/28 at 15:17:18, kdsilva wrote: > On 2016/07/28 13:05:13, Mounir Lamouri wrote: > > I don't think you need that, do you? > > Looks like I don't! Not sure why I thought I did... Maybe you added the feature flag to the media list first? https://codereview.chromium.org/2154233003/diff/240001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/common/chrome_f... chrome/common/chrome_features.cc:58: base::FEATURE_ENABLED_BY_DEFAULT}; style-wise, that doesn't really look like most of the other feature definition in the file but it matches the coding style (indentation has multiple rules) so it's fine to keep https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1399: return ""; nit: leave spaces around the `if` block: ``` DCHECK() if (...) return ""; GURL url = ... ``` https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1430: while ((start_pos = ret_url.find("?", start_pos)) != std::string::npos) { That really sounds like something that should be written as a `for` loop. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:90: if (request.relative_url.find("/embed") != std::string::npos) { Can you check that the host contains "youtube.com" too? Also, maybe you can change the check to be `find("/embed") == 0` because we expect this to start with "/embed", right? Furthermore, I think this will gain in clarity with an early return. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:96: base::Unretained(this))); Looking at MessageLoopRunner class, it sounds like you can do: ``` content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, message_runner_->QuitClosure()); ``` https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:102: } If you do the change above, I think you can remove this :) https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:159: // Invalid URL, many parameters Can you rephrase to "invalid parameters construct" or something similar. I think that the URL is technically correct, just not what the website would expect. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:165: // Invalid URL, many parameters Maybe you can drop this one? https://codereview.chromium.org/2154233003/diff/240001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/240001/content/public/rendere... content/public/renderer/content_renderer_client.h:366: style: remove the empty line after the declaration. https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:151: // HTML5 video player when possible. This OverrideFlashEmbedWithHTML is generic. As far as Blink is concerned, it can be used for many websites. The comment should not mention YouTube. https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1044: } style: no { }
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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/2154233003/diff/240001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/common/chrome_f... chrome/common/chrome_features.cc:58: base::FEATURE_ENABLED_BY_DEFAULT}; On 2016/07/28 19:46:54, Mounir Lamouri wrote: > style-wise, that doesn't really look like most of the other feature definition > in the file but it matches the coding style (indentation has multiple rules) so > it's fine to keep Acknowledged. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1399: return ""; On 2016/07/28 19:46:54, Mounir Lamouri wrote: > nit: leave spaces around the `if` block: > ``` > DCHECK() > > if (...) > return ""; > > GURL url = ... > ``` Done. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1430: while ((start_pos = ret_url.find("?", start_pos)) != std::string::npos) { On 2016/07/28 19:46:54, Mounir Lamouri wrote: > That really sounds like something that should be written as a `for` loop. Done. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:90: if (request.relative_url.find("/embed") != std::string::npos) { On 2016/07/28 19:46:54, Mounir Lamouri wrote: > Can you check that the host contains "youtube.com" too? Also, maybe you can > change the check to be `find("/embed") == 0` because we expect this to start > with "/embed", right? > > Furthermore, I think this will gain in clarity with an early return. Done. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:96: base::Unretained(this))); On 2016/07/28 19:46:54, Mounir Lamouri wrote: > Looking at MessageLoopRunner class, it sounds like you can do: > ``` > content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, > message_runner_->QuitClosure()); > ``` Done. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:102: } On 2016/07/28 19:46:54, Mounir Lamouri wrote: > If you do the change above, I think you can remove this :) Done. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:159: // Invalid URL, many parameters On 2016/07/28 19:46:54, Mounir Lamouri wrote: > Can you rephrase to "invalid parameters construct" or something similar. I think > that the URL is technically correct, just not what the website would expect. Done. https://codereview.chromium.org/2154233003/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:165: // Invalid URL, many parameters On 2016/07/28 19:46:54, Mounir Lamouri wrote: > Maybe you can drop this one? Done. https://codereview.chromium.org/2154233003/diff/240001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/240001/content/public/rendere... content/public/renderer/content_renderer_client.h:366: On 2016/07/28 19:46:54, Mounir Lamouri wrote: > style: remove the empty line after the declaration. Done. https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:151: // HTML5 video player when possible. On 2016/07/28 19:46:54, Mounir Lamouri wrote: > This OverrideFlashEmbedWithHTML is generic. As far as Blink is concerned, it can > be used for many websites. The comment should not mention YouTube. Done. https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1044: } On 2016/07/28 19:46:54, Mounir Lamouri wrote: > style: no { } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1388: return ""; Instead of checking if the string is empty, could you check below if `url` is invalid? https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1393: GURL url = GURL(str_url); Maybe add: ``` if (!url.is_valid()) return ""; ``` instead of the early return above? https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: void MonitorRequestHandler(const net::test_server::HttpRequest& request) { style: - only one space before {public,protected,private}. - the method should then be indented one more space https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:90: if (request.headers.at("Host").find("youtube.com") == std::string::npos) { style: this is missing indentation https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:92: } style: remove the { } https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:95: request.relative_url.find("?enablejsapi=1") != std::string::npos) { style: `request` and `request` should be aligned https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:96: return; I think it would be fine to check for "/v" and "/embed" instead of special casing "?enablejsapi". https://codereview.chromium.org/2154233003/diff/420001/chrome/test/data/flash... File chrome/test/data/flash_embeds.html (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/test/data/flash... chrome/test/data/flash_embeds.html:11: document.getElementsByTagName('body')[0].appendChild(object); `document.body` is shorter than `document.getElementsByTagName('body')[0]` ;) https://codereview.chromium.org/2154233003/diff/420001/chrome/test/data/flash... chrome/test/data/flash_embeds.html:14: </body> Can you close the body before opening the script? https://codereview.chromium.org/2154233003/diff/420001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/420001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:717: return WebString(); I believe WebString is utf16 and has an null state. Would that return a null string instead of an empty string?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1388: return ""; On 2016/07/30 17:46:06, Mounir Lamouri wrote: > Instead of checking if the string is empty, could you check below if `url` is > invalid? Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1393: GURL url = GURL(str_url); On 2016/07/30 17:46:06, Mounir Lamouri wrote: > Maybe add: > ``` > if (!url.is_valid()) > return ""; > ``` > > instead of the early return above? Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:88: void MonitorRequestHandler(const net::test_server::HttpRequest& request) { On 2016/07/30 17:46:06, Mounir Lamouri wrote: > style: > - only one space before {public,protected,private}. > - the method should then be indented one more space Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:90: if (request.headers.at("Host").find("youtube.com") == std::string::npos) { On 2016/07/30 17:46:06, Mounir Lamouri wrote: > style: this is missing indentation Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:92: } On 2016/07/30 17:46:06, Mounir Lamouri wrote: > style: remove the { } Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:95: request.relative_url.find("?enablejsapi=1") != std::string::npos) { On 2016/07/30 17:46:06, Mounir Lamouri wrote: > style: `request` and `request` should be aligned Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_browsertest.cc:96: return; On 2016/07/30 17:46:06, Mounir Lamouri wrote: > I think it would be fine to check for "/v" and "/embed" instead of special > casing "?enablejsapi". Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/test/data/flash... File chrome/test/data/flash_embeds.html (right): https://codereview.chromium.org/2154233003/diff/420001/chrome/test/data/flash... chrome/test/data/flash_embeds.html:11: document.getElementsByTagName('body')[0].appendChild(object); On 2016/07/30 17:46:06, Mounir Lamouri wrote: > `document.body` is shorter than `document.getElementsByTagName('body')[0]` ;) Done. https://codereview.chromium.org/2154233003/diff/420001/chrome/test/data/flash... chrome/test/data/flash_embeds.html:14: </body> On 2016/07/30 17:46:06, Mounir Lamouri wrote: > Can you close the body before opening the script? Done. https://codereview.chromium.org/2154233003/diff/420001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/420001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:717: return WebString(); On 2016/07/30 17:46:06, Mounir Lamouri wrote: > I believe WebString is utf16 and has an null state. Would that return a null > string instead of an empty string? Yup! You're right.
Description was changed from ========== Rewriting YouTube Flash embeds BUG= ========== to ========== 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 ==========
kdsilva@google.com changed reviewers: + jochen@chromium.org, mkwst@chromium.org
kdsilva@google.com changed required reviewers: + jochen@chromium.org
lgtm with comments applied. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1396: // any URLs that contain the enablejsapi=1 parameter since the page may be Can you open a follow-up bug to actually do the rewrite when enablejsapi=1 on Android? https://codereview.chromium.org/2154233003/diff/460001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/460001/content/public/rendere... content/public/renderer/content_renderer_client.h:363: // Overwrites the given URL to use an HTML5 video player if possible. s/video player/embed/ https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:151: // HTML5 video player when possible. s/video player/embed/ https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:276: // Overwrites the given URL to use an HTML5 video player if possible. s/video player/embed/ https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:713: // Overwrites the given URL to use an HTML5 video player if possible. s/video player/embed/
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Drive-by comments. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1401: return ""; This should just use GURLs, but note that converting "" to std::string() like this is surprisingly expensive in general: it's better to just use the std::string default constructor. A similar comment applies throughout the rest of this CL for WebString/WebURL https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: ret_url.replace(ret_url.find("/v/"), 3, "/embed/"); Can we just use GURL and avoid cracking URLs manually? At least we can make it just match the patch component that way and simplify the matching logic below. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1409: // for each subsequent parameter. Is it important to fix invalid URLs? Wouldn't they be broken anyway? https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:278: virtual String OverrideFlashEmbedWithHTML(const String&) { return ""; } Please pass URLs as KURL and WebURL: Document::completeURL already returns a KURL. Also Blink naming convention is overrideFlashEmbedWithHTML https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1040: if (!m_webFrame->client()) Why would we call this when client() is null? We shouldn't be loading plugins in detached frames. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1041: return ""; As noted elsewhere, prefer returning the default constructed object rather than constructing from an empty string literal.
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...
I'm deferring to dcheng@
I've updated the CL to address your comments PTAL! https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1396: // any URLs that contain the enablejsapi=1 parameter since the page may be On 2016/08/01 13:43:59, Mounir Lamouri wrote: > Can you open a follow-up bug to actually do the rewrite when enablejsapi=1 on > Android? Done. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1401: return ""; On 2016/08/01 14:50:34, dcheng (OOO Aug 1 - Aug 11) wrote: > This should just use GURLs, but note that converting "" to std::string() like > this is surprisingly expensive in general: it's better to just use the > std::string default constructor. A similar comment applies throughout the rest > of this CL for WebString/WebURL Done. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: ret_url.replace(ret_url.find("/v/"), 3, "/embed/"); On 2016/08/01 14:50:34, dcheng (OOO Aug 1 - Aug 11) wrote: > Can we just use GURL and avoid cracking URLs manually? At least we can make it > just match the patch component that way and simplify the matching logic below. Done. https://codereview.chromium.org/2154233003/diff/460001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1409: // for each subsequent parameter. On 2016/08/01 14:50:34, dcheng (OOO Aug 1 - Aug 11) wrote: > Is it important to fix invalid URLs? Wouldn't they be broken anyway? The Flash video player has some URL correction capabilities, so we didn't want the move to HTML5 to break pages that were already working. I've updated the comment to mention this, let me know if you have any more questions! https://codereview.chromium.org/2154233003/diff/460001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/460001/content/public/rendere... content/public/renderer/content_renderer_client.h:363: // Overwrites the given URL to use an HTML5 video player if possible. On 2016/08/01 13:43:59, Mounir Lamouri wrote: > s/video player/embed/ Done. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:151: // HTML5 video player when possible. On 2016/08/01 13:43:59, Mounir Lamouri wrote: > s/video player/embed/ Done. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:276: // Overwrites the given URL to use an HTML5 video player if possible. On 2016/08/01 13:43:59, Mounir Lamouri wrote: > s/video player/embed/ Done. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoaderClient.h:278: virtual String OverrideFlashEmbedWithHTML(const String&) { return ""; } On 2016/08/01 14:50:34, dcheng (OOO Aug 1 - Aug 11) wrote: > Please pass URLs as KURL and WebURL: Document::completeURL already returns a > KURL. Also Blink naming convention is overrideFlashEmbedWithHTML Done. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1040: if (!m_webFrame->client()) On 2016/08/01 14:50:34, dcheng (OOO Aug 1 - Aug 11) wrote: > Why would we call this when client() is null? We shouldn't be loading plugins in > detached frames. Done. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1041: return ""; On 2016/08/01 14:50:34, dcheng (OOO Aug 1 - Aug 11) wrote: > As noted elsewhere, prefer returning the default constructed object rather than > constructing from an empty string literal. Done. https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/460001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:713: // Overwrites the given URL to use an HTML5 video player if possible. On 2016/08/01 13:43:59, Mounir Lamouri wrote: > s/video player/embed/ Done.
drive-by https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1402: // for each subsequent parameter. We do this becuase the Flash vide player typo: because typo: video
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/2154233003/diff/480001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/480001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1402: // for each subsequent parameter. We do this becuase the Flash vide player On 2016/08/02 15:59:38, Avi wrote: > typo: because > typo: video Wow, what an embarrassing mistake. Thanks for the heads up!
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 unchecked by commit-bot@chromium.org
Dry run: 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_androi...)
omattos@gmail.com changed reviewers: + omattos@gmail.com
https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:155: m_serviceType = "text/html"; I take it this m_serviceType override is the real required change. Why not drop the URL rewriting bit and do that serverside instead? Even better, why not make HTMLEmbedElement simply render text/html, then no Chromium changes are required at all?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/08/03 at 11:36:04, omattos wrote: > https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): > > https://codereview.chromium.org/2154233003/diff/520001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:155: m_serviceType = "text/html"; > I take it this m_serviceType override is the real required change. Why not drop the URL rewriting bit and do that serverside instead? > > Even better, why not make HTMLEmbedElement simply render text/html, then no Chromium changes are required at all? omattos@, this might not be the best place to discuss why we are implementing this. There was a design doc sent to chromium-dev@. Otherwise, discussing in the bug is probably more appropriate than the CL. To answer your question, your suggestion doesn't sound practical. YouTube would have to work with every browser that want to do this and make sure that text/html requests for their Flash embed actually was meant for the IFrame embed. As you seem to know, it's not possible for YouTube alone to do the rewrite, the UA need to be involved and it's unclear why we should only do half of the work. FWIW, the solution we are implementing is very similar to what Firefox and Safari are doing.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
//third_party/WebKit LGTM, and I left some comments in the implementation higher up the stack (though I'm not an owner there). Thanks for taking a few passes at this! https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: if (!url.DomainIs("youtube.com") || url.path().find("/v/") != 0) I'd suggest that we handle `youtube-nocookie.com` here as well. Skimming through other browsers' code, WebKit handles this case (https://github.com/WebKit/webkit/blob/master/Source/WebCore/Modules/plugins/Y...), but Firefox doesn't (http://searchfox.org/mozilla-central/source/dom/base/nsObjectLoadingContent.c...). It might be worth filing a bug against Gecko to do the same. :) WebKit also processes `youtu.be`, but that doesn't appear to redirect to Flash, so I think we can safely ignore it here. https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1424: (pos = url_str.find("?",pos )) != std::string::npos; pos += 1) Nit: Please add braces here, as the for loop is multi-line. Also, the indentation is a bit strange. Did `git cl format` produce this? https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1428: GURL corrected_url = GURL(url_str); Nit: Did you consider wrapping this up in the `url::Replacement` bit below? That is, creating a Replacements with the new path and query rather than reparsing the URL? https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1433: if (corrected_url.query().find("enablejsapi=1") != std::string::npos) This will also match `?XXXenablejsapi=1` and `?a=enablejsapi=1` and `?enablejsapi=100`. This might or might not matter, since we're dealing with the pretty constrained world of YouTube embeds. *shrug* https://codereview.chromium.org/2154233003/diff/580001/chrome/test/data/flash... File chrome/test/data/flash_embeds.html (right): https://codereview.chromium.org/2154233003/diff/580001/chrome/test/data/flash... chrome/test/data/flash_embeds.html:10: embed.src = url; Nit: I'd like to see you check the `type` behavior as well, verifying that an explicit type is overwritten. https://codereview.chromium.org/2154233003/diff/580001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/580001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:154: m_url = overridenUrl.getString(); As an unrelated aside, it's super-weird that `m_url` is a string here and not a KURL. :/
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/2154233003/diff/580001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: if (!url.DomainIs("youtube.com") || url.path().find("/v/") != 0) On 2016/08/04 at 12:41:38, Mike West wrote: > I'd suggest that we handle `youtube-nocookie.com` here as well. > > Skimming through other browsers' code, WebKit handles this case (https://github.com/WebKit/webkit/blob/master/Source/WebCore/Modules/plugins/Y...), but Firefox doesn't (http://searchfox.org/mozilla-central/source/dom/base/nsObjectLoadingContent.c...). It might be worth filing a bug against Gecko to do the same. :) > > WebKit also processes `youtu.be`, but that doesn't appear to redirect to Flash, so I think we can safely ignore it here. Done. https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1424: (pos = url_str.find("?",pos )) != std::string::npos; pos += 1) On 2016/08/04 at 12:41:38, Mike West wrote: > Nit: Please add braces here, as the for loop is multi-line. Also, the indentation is a bit strange. Did `git cl format` produce this? Done. Looks like `git cl format` did fix the indentation. https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1428: GURL corrected_url = GURL(url_str); On 2016/08/04 12:41:38, Mike West wrote: > Nit: Did you consider wrapping this up in the `url::Replacement` bit below? That > is, creating a Replacements with the new path and query rather than reparsing > the URL? Ack. https://codereview.chromium.org/2154233003/diff/580001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1433: if (corrected_url.query().find("enablejsapi=1") != std::string::npos) On 2016/08/04 12:41:38, Mike West (OOO until 15th) wrote: > This will also match `?XXXenablejsapi=1` and `?a=enablejsapi=1` and > `?enablejsapi=100`. This might or might not matter, since we're dealing with the > pretty constrained world of YouTube embeds. *shrug* Ack. Happy to discuss, but I think this is okay for our purposes. https://codereview.chromium.org/2154233003/diff/580001/chrome/test/data/flash... File chrome/test/data/flash_embeds.html (right): https://codereview.chromium.org/2154233003/diff/580001/chrome/test/data/flash... chrome/test/data/flash_embeds.html:10: embed.src = url; On 2016/08/04 12:41:39, Mike West wrote: > Nit: I'd like to see you check the `type` behavior as well, verifying that an > explicit type is overwritten. Done. https://codereview.chromium.org/2154233003/diff/580001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right): https://codereview.chromium.org/2154233003/diff/580001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:154: m_url = overridenUrl.getString(); On 2016/08/04 at 12:41:39, Mike West wrote: > As an unrelated aside, it's super-weird that `m_url` is a string here and not a KURL. :/ Haha, you're not wrong.
is there a particular reason you plumb this through so many layers? couldn't this just all be done inside blink?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/05 at 11:56:04, jochen wrote: > is there a particular reason you plumb this through so many layers? > > couldn't this just all be done inside blink? That was done initially but mkwst@ did not want site-specific behaviour inside Blink so we plumbed it to the chrome/ layer.
On 2016/08/05 at 12:31:25, mlamouri wrote: > On 2016/08/05 at 11:56:04, jochen wrote: > > is there a particular reason you plumb this through so many layers? > > > > couldn't this just all be done inside blink? > > That was done initially but mkwst@ did not want site-specific behaviour inside Blink so we plumbed it to the chrome/ layer. which patchset was that?
On 2016/08/05 at 12:32:20, jochen wrote: > On 2016/08/05 at 12:31:25, mlamouri wrote: > > On 2016/08/05 at 11:56:04, jochen wrote: > > > is there a particular reason you plumb this through so many layers? > > > > > > couldn't this just all be done inside blink? > > > > That was done initially but mkwst@ did not want site-specific behaviour inside Blink so we plumbed it to the chrome/ layer. > > which patchset was that? It's another CL: https://codereview.chromium.org/2132403003 (the CL change was not on purpose)
did you try using the same logic as the mobile youtube thing? I wonder why you need additional hooks over what that uses?
On 2016/08/05 at 13:42:59, jochen wrote: > did you try using the same logic as the mobile youtube thing? I wonder why you need additional hooks over what that uses? In my opinion, the only benefit in re-using the PluginPlaceholder code path would be to reduce plumbing which I guess is your goal here :) However, I think the current approach is closer to the metal: we very early change the request in a way that it is equivalent to embedding the HTML page via <embed>. If we go with the PluginPlaceholder approach, we would have to pretend there is a plugin and have it inject an iframe that would load the HTML page. In addition of the code overhead, using a plugin placeholder means that we would create an actual plugin on the page and the look and feel might be slightly different from a straight iframe. If you look at HTMLPlugInElement::requestObject you will see that there is a branch based on shouldUsePlugin. With our approach, the HTMLPluginElement will use the same loading path as the one used for a regular frame. With the PlaceholderPlugin approach, it would go with loadPlugin(). Regarding the overhead, more info here: https://cs.chromium.org/chromium/src/components/plugins/renderer/plugin_place... Finally, if we have compat in mind, Firefox has a similar approach than this CL while WebKit seems to be using a plugin replacement approach.
On 2016/08/05 14:31:05, Mounir Lamouri wrote: > On 2016/08/05 at 13:42:59, jochen wrote: > > did you try using the same logic as the mobile youtube thing? I wonder why you > need additional hooks over what that uses? > > In my opinion, the only benefit in re-using the PluginPlaceholder code path > would be to reduce plumbing which I guess is your goal here :) However, I think > the current approach is closer to the metal: we very early change the request in > a way that it is equivalent to embedding the HTML page via <embed>. If we go > with the PluginPlaceholder approach, we would have to pretend there is a plugin > and have it inject an iframe that would load the HTML page. In addition of the > code overhead, using a plugin placeholder means that we would create an actual > plugin on the page and the look and feel might be slightly different from a > straight iframe. > > If you look at HTMLPlugInElement::requestObject you will see that there is a > branch based on shouldUsePlugin. With our approach, the HTMLPluginElement will > use the same loading path as the one used for a regular frame. With the > PlaceholderPlugin approach, it would go with loadPlugin(). > > Regarding the overhead, more info here: > https://cs.chromium.org/chromium/src/components/plugins/renderer/plugin_place... > > Finally, if we have compat in mind, Firefox has a similar approach than this CL > while WebKit seems to be using a plugin replacement approach. I forgot about the mobile youtube plugin, but I agree with jochen: we should just reuse it rather than adding a one-off code path: the Blink public API already has too many specialized methods. It looks like we could just make the HTML data it loads a <iframe> that loads the youtube embed, right?
On 2016/08/05 at 15:46:22, dcheng wrote: > On 2016/08/05 14:31:05, Mounir Lamouri wrote: > > On 2016/08/05 at 13:42:59, jochen wrote: > > > did you try using the same logic as the mobile youtube thing? I wonder why you > > need additional hooks over what that uses? > > > > In my opinion, the only benefit in re-using the PluginPlaceholder code path > > would be to reduce plumbing which I guess is your goal here :) However, I think > > the current approach is closer to the metal: we very early change the request in > > a way that it is equivalent to embedding the HTML page via <embed>. If we go > > with the PluginPlaceholder approach, we would have to pretend there is a plugin > > and have it inject an iframe that would load the HTML page. In addition of the > > code overhead, using a plugin placeholder means that we would create an actual > > plugin on the page and the look and feel might be slightly different from a > > straight iframe. > > > > If you look at HTMLPlugInElement::requestObject you will see that there is a > > branch based on shouldUsePlugin. With our approach, the HTMLPluginElement will > > use the same loading path as the one used for a regular frame. With the > > PlaceholderPlugin approach, it would go with loadPlugin(). > > > > Regarding the overhead, more info here: > > https://cs.chromium.org/chromium/src/components/plugins/renderer/plugin_place... > > > > Finally, if we have compat in mind, Firefox has a similar approach than this CL > > while WebKit seems to be using a plugin replacement approach. > > I forgot about the mobile youtube plugin, but I agree with jochen: we should just reuse it rather than adding a one-off code path: the Blink public API already has too many specialized methods. It looks like we could just make the HTML data it loads a <iframe> that loads the youtube embed, right? It would load the <iframe> inside a plugin. It is semantically very different from loading an HTML page inside the original <embed>. For example, extensions have an API to override plugins. If we use a PlaceholderPlugin, such extensions would still apply.
On 2016/08/05 at 16:09:00, Mounir Lamouri wrote: > On 2016/08/05 at 15:46:22, dcheng wrote: > > On 2016/08/05 14:31:05, Mounir Lamouri wrote: > > > On 2016/08/05 at 13:42:59, jochen wrote: > > > > did you try using the same logic as the mobile youtube thing? I wonder why you > > > need additional hooks over what that uses? > > > > > > In my opinion, the only benefit in re-using the PluginPlaceholder code path > > > would be to reduce plumbing which I guess is your goal here :) However, I think > > > the current approach is closer to the metal: we very early change the request in > > > a way that it is equivalent to embedding the HTML page via <embed>. If we go > > > with the PluginPlaceholder approach, we would have to pretend there is a plugin > > > and have it inject an iframe that would load the HTML page. In addition of the > > > code overhead, using a plugin placeholder means that we would create an actual > > > plugin on the page and the look and feel might be slightly different from a > > > straight iframe. > > > > > > If you look at HTMLPlugInElement::requestObject you will see that there is a > > > branch based on shouldUsePlugin. With our approach, the HTMLPluginElement will > > > use the same loading path as the one used for a regular frame. With the > > > PlaceholderPlugin approach, it would go with loadPlugin(). > > > > > > Regarding the overhead, more info here: > > > https://cs.chromium.org/chromium/src/components/plugins/renderer/plugin_place... > > > > > > Finally, if we have compat in mind, Firefox has a similar approach than this CL > > > while WebKit seems to be using a plugin replacement approach. > > > > I forgot about the mobile youtube plugin, but I agree with jochen: we should just reuse it rather than adding a one-off code path: the Blink public API already has too many specialized methods. It looks like we could just make the HTML data it loads a <iframe> that loads the youtube embed, right? > > It would load the <iframe> inside a plugin. It is semantically very different from loading an HTML page inside the original <embed>. For example, extensions have an API to override plugins. If we use a PlaceholderPlugin, such extensions would still apply. It seems to me that the issue here boils down to whether we should have site-specific behaviour in Blink or should they be in the chrome/ layer. Having this behaviour in Blink would obviously reduce the plumbing but I don't think it's a great idea to shoehorn this feature into another type of plumbing because it should work. Can we consider keeping this approach and discuss whether we should move the code back to Blink later when the different stack holders are back?
I talked with mlamouri. I'm OK with not using the placeholder plugin approach, since this will allow us to remove the youtube placeholder plugin for mobile. However, it is a lot of plumbing and FrameLoaderClient / WebFrameClient already contain far too many methods... I do see some value in letting the embedder control this behavior though...
On 2016/08/05 16:29:10, dcheng (OOO Aug 1 - Aug 11) wrote: > I talked with mlamouri. I'm OK with not using the placeholder plugin approach, > since this will allow us to remove the youtube placeholder plugin for mobile. > > However, it is a lot of plumbing and FrameLoaderClient / WebFrameClient already > contain far too many methods... I do see some value in letting the embedder > control this behavior though... (Basically, I'm not opposed to this CL as-is, despite the WebFrameClient plumbing)
lgtm
kdsilva@google.com changed reviewers: + jam@chromium.org - jochen@chromium.org
kdsilva@google.com changed required reviewers: + jam@chromium.org - jochen@chromium.org
Hi John, Jochen, who was originally marked as a reviewer, is OOO for the next two weeks. Could you take a look? Elliott, who is CC'd, has already looked at it and was okay with it.
https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: if (!url.DomainIs("youtube.com") && !url.DomainIs("youtube-nocookie.com")) I can't find a unit test for youtube-nocookie.com. If I didn't miss it, can you add one? :) https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:478: for (auto data : test_data) const auto& https://codereview.chromium.org/2154233003/diff/600001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/600001/content/public/rendere... content/public/renderer/content_renderer_client.h:364: // A null URL is returned if the URL is not overriden. An empty URL? Same for the other place this comment is used.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... 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 wrote: > I can't find a unit test for http://youtube-nocookie.com. If I didn't miss it, can you > add one? :) Done. https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2154233003/diff/600001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:478: for (auto data : test_data) On 2016/08/08 12:45:17, Mounir Lamouri wrote: > const auto& Done. https://codereview.chromium.org/2154233003/diff/600001/content/public/rendere... File content/public/renderer/content_renderer_client.h (right): https://codereview.chromium.org/2154233003/diff/600001/content/public/rendere... content/public/renderer/content_renderer_client.h:364: // A null URL is returned if the URL is not overriden. On 2016/08/08 12:45:17, Mounir Lamouri wrote: > An empty URL? Same for the other place this comment is used. Done. Although I thought I had already updated these comments...
https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:706: // A null URL is returned if the URL is not overriden. I think you missed one :(
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/2154233003/diff/620001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrameClient.h (right): https://codereview.chromium.org/2154233003/diff/620001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrameClient.h:706: // A null URL is returned if the URL is not overriden. On 2016/08/08 19:43:07, Mounir Lamouri wrote: > I think you missed one :( Done.
The CQ bit was unchecked by kdsilva@google.com
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, mkwst@chromium.org, dcheng@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2154233003/#ps640001 (title: "Addressed comments")
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
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_androi...)
The CQ bit was checked by mlamouri@chromium.org
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
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_...)
The CQ bit was checked by mlamouri@chromium.org
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 ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #31 (id:640001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 31 (id:??) landed as https://crrev.com/e0135598f6bf547463bc943100da4574c5f89930 Cr-Commit-Position: refs/heads/master@{#410715} |
