Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11775002:
Media Documents should disable View [Page] Source (Closed)
|
Created:
7 years, 11 months ago by wolenetz Modified:
7 years, 11 months ago CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, cbentzel+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, pam+watch_chromium.org, xhwang, Ami GONE FROM CHROMIUM, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMedia Documents should disable View [Page] Source
BUG=83714
TEST=ViewSourceTest.ViewSourceInMenuDisabledOnAMediaPage
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175639
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fix indentation #
Total comments: 2
Patch Set 3 : Removed extra string copy #
Messages
Total messages: 14 (0 generated)
Please review. Thanks, Matt https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... chrome/browser/tab_contents/view_source_browsertest.cc:24: const char kTestMedia[] = "files/media/pink_noise_140ms.wav"; @ jam@: Is referencing a test data file from media, not viewsource, OK or do we need to duplicate such content into the viewsource tree for use here? Lack of duplication could lead to test fragility if media test data changes later. https://codereview.chromium.org/11775002/diff/1/net/tools/testserver/testserv... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/11775002/diff/1/net/tools/testserver/testserv... net/tools/testserver/testserver.py:497: 'wav' : 'audio/wav', @ phajdan.jr@: This additional mime type is necessary to support the new test. I've included you as reviewer per OWNERS.
testserver LGTM
https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... chrome/browser/tab_contents/view_source_browsertest.cc:24: const char kTestMedia[] = "files/media/pink_noise_140ms.wav"; On 2013/01/04 22:58:37, wolenetz wrote: > @ jam@: Is referencing a test data file from media, not viewsource, OK or do we > need to duplicate such content into the viewsource tree for use here? Lack of > duplication could lead to test fragility if media test data changes later. I'm not sure what you mean? pink_noise_140ms.wav is in chrome/test/data/media/pink_noise_140ms.wav
https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... chrome/browser/tab_contents/view_source_browsertest.cc:24: const char kTestMedia[] = "files/media/pink_noise_140ms.wav"; On 2013/01/04 23:48:41, John Abd-El-Malek wrote: > On 2013/01/04 22:58:37, wolenetz wrote: > > @ jam@: Is referencing a test data file from media, not viewsource, OK or do > we > > need to duplicate such content into the viewsource tree for use here? Lack of > > duplication could lead to test fragility if media test data changes later. > > I'm not sure what you mean? pink_noise_140ms.wav is in > chrome/test/data/media/pink_noise_140ms.wav Yes (and the test passes). The question really is does this wav file need to be copied into chrome/test/data/viewsource/ and the value of kTestMedia updated to the viewsource location, or are references by this browsertest to chrome/test/data/media/ files OK?
On 2013/01/05 00:05:19, wolenetz wrote: > https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... > File chrome/browser/tab_contents/view_source_browsertest.cc (right): > > https://codereview.chromium.org/11775002/diff/1/chrome/browser/tab_contents/v... > chrome/browser/tab_contents/view_source_browsertest.cc:24: const char > kTestMedia[] = "files/media/pink_noise_140ms.wav"; > On 2013/01/04 23:48:41, John Abd-El-Malek wrote: > > On 2013/01/04 22:58:37, wolenetz wrote: > > > @ jam@: Is referencing a test data file from media, not viewsource, OK or do > > we > > > need to duplicate such content into the viewsource tree for use here? Lack > of > > > duplication could lead to test fragility if media test data changes later. > > > > I'm not sure what you mean? pink_noise_140ms.wav is in > > chrome/test/data/media/pink_noise_140ms.wav > > Yes (and the test passes). The question really is does this wav file need to be > copied into chrome/test/data/viewsource/ and the value of kTestMedia updated to > the viewsource location, or are references by this browsertest to > chrome/test/data/media/ files OK? yep that's fine as is
lgtm w/ nit jam: I believe you're on point for OWNERS review phajdan.jr: ditto for the .py stuff https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... content/browser/web_contents/navigation_controller_impl.cc:447: !net::IsSupportedMediaMimeType(mime_type); as a general FYI the one thing to sanity check here is that the list of supported media mime types here is the same as the list that's used to trigger the display of media (aka MediaDocument), otherwise we could end up in a situation where (for example) we trigger a MediaDocument but we still have view source enabled. I believe we're good here since the code that triggers MediaDocument based on MIME type... https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source... ... ends up checking the same function: https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/simple_webmim... https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... content/browser/web_contents/navigation_controller_impl.cc:450: is_viewable_mime_type && !web_contents_->GetInterstitialPage(); nit: this should have been a 4 space indent
Pending OWNERS review of PS2. Adding avi@ per OWNERS in src/chrome/browser/tab_contents https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... content/browser/web_contents/navigation_controller_impl.cc:447: !net::IsSupportedMediaMimeType(mime_type); On 2013/01/07 17:07:44, scherkus wrote: > as a general FYI the one thing to sanity check here is that the list of > supported media mime types here is the same as the list that's used to trigger > the display of media (aka MediaDocument), otherwise we could end up in a > situation where (for example) we trigger a MediaDocument but we still have view > source enabled. > > I believe we're good here since the code that triggers MediaDocument based on > MIME type... > > https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source... > > ... ends up checking the same function: > > https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/simple_webmim... Scherkus & I synced offline, and agree that risk of media mime_type support divergence exists. However, the net code is where we would change the set of supported media types, so risk is mitigated somewhat. We will also avoid possible layering violation and continue with the current approach of calling the net function directly. https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... content/browser/web_contents/navigation_controller_impl.cc:450: is_viewable_mime_type && !web_contents_->GetInterstitialPage(); On 2013/01/07 17:07:44, scherkus wrote: > nit: this should have been a 4 space indent Thanks. Fixed, I will upload another PS.
On 2013/01/07 19:28:26, wolenetz wrote: > Pending OWNERS review of PS2. Adding avi@ per OWNERS in > src/chrome/browser/tab_contents > > https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... > File content/browser/web_contents/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... > content/browser/web_contents/navigation_controller_impl.cc:447: > !net::IsSupportedMediaMimeType(mime_type); > On 2013/01/07 17:07:44, scherkus wrote: > > as a general FYI the one thing to sanity check here is that the list of > > supported media mime types here is the same as the list that's used to trigger > > the display of media (aka MediaDocument), otherwise we could end up in a > > situation where (for example) we trigger a MediaDocument but we still have > view > > source enabled. > > > > I believe we're good here since the code that triggers MediaDocument based on > > MIME type... > > > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source... > > > > ... ends up checking the same function: > > > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/simple_webmim... > > Scherkus & I synced offline, and agree that risk of media mime_type support > divergence exists. However, the net code is where we would change the set of > supported media types, so risk is mitigated somewhat. We will also avoid > possible layering violation and continue with the current approach of calling > the net function directly. > > https://codereview.chromium.org/11775002/diff/1/content/browser/web_contents/... > content/browser/web_contents/navigation_controller_impl.cc:450: > is_viewable_mime_type && !web_contents_->GetInterstitialPage(); > On 2013/01/07 17:07:44, scherkus wrote: > > nit: this should have been a 4 space indent > > Thanks. Fixed, I will upload another PS. Resending due to adding avi@ as reviewer.
lgtm
https://codereview.chromium.org/11775002/diff/5002/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11775002/diff/5002/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:445: const std::string& mime_type = web_contents_->GetContentsMimeType().c_str(); GetContentsMimeType() already returns a std::string; calling c_str() on it just makes an unnecessary copy.
https://codereview.chromium.org/11775002/diff/5002/content/browser/web_conten... File content/browser/web_contents/navigation_controller_impl.cc (right): https://codereview.chromium.org/11775002/diff/5002/content/browser/web_conten... content/browser/web_contents/navigation_controller_impl.cc:445: const std::string& mime_type = web_contents_->GetContentsMimeType().c_str(); On 2013/01/07 19:36:11, Avi wrote: > GetContentsMimeType() already returns a std::string; calling c_str() on it just > makes an unnecessary copy. Good point. I'll upload another PS with this fixed. Thank you.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wolenetz@chromium.org/11775002/1010
Message was sent while issue was closed.
Change committed as 175639 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
