Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272123002:
[HBD] Intercept navigation to Flash download page (Closed)
Description[HBD] Intercept navigation to Flash download page
Intercepts navigation to get.adobe.com/flashplayer
Here is more information about HTML5 by Default: go/hbd-design-doc
BUG=626728
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/8ddaeb7e8317409c88cbb9f1bc65e4351f539239
Cr-Commit-Position: refs/heads/master@{#416113}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Changes per tommycli@'s comments #
Total comments: 13
Patch Set 3 : Changes per Lei Zhang's comments #Patch Set 4 : Unit test #
Total comments: 6
Patch Set 5 : Changes per tommycli@'s comments #Patch Set 6 : EXPECT_EQ to ASSERT_EQ for WillStartRequest() checks #
Total comments: 2
Patch Set 7 : Rebase and build fix #Patch Set 8 : Changes per clamy@'s comment #
Total comments: 4
Patch Set 9 : Changes per Lei Zhang's comments #
Messages
Total messages: 42 (19 generated)
trizzofo@google.com changed reviewers: + tommycli@chromium.org
tommycli, ptal
tomas: thanks - some comments: https://codereview.chromium.org/2272123002/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2272123002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:62: #include "chrome/browser/plugins/flash_download_interception.h" These new includes should also be in the ENABLE_PLUGINS ifdef, see line 303 Otherwise i think it won't compile on some platforms https://codereview.chromium.org/2272123002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:3020: if (base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)) { This should only happen if it's a main frame navigation, so please add an IsInMainFrame block on this if statement. And it should also be in an ENABLE_PLUGINS ifdef. Also I think the FeatureList test should be pushed to be within the FlashDownloadInterception::MaybeCreateThrottleFor body. https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:35: FlashDownloadInterception::MaybeCreateThrottleFor( nit: You ran git cl format right? https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.h (right): https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.h:21: class FlashDownloadInterception { class comment should be updated. something similar to: // Redirects Flash Download URL navigations, since Chrome already ships with Flash. Use your imagination :) Also do you think it should be named similarly to the AppUrlRedirector? i.e. FlashDownloadUrlRedirector? I'm not sure, but think about it.
https://codereview.chromium.org/2272123002/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2272123002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:62: #include "chrome/browser/plugins/flash_download_interception.h" On 2016/08/24 19:11:30, tommycli wrote: > These new includes should also be in the ENABLE_PLUGINS ifdef, see line 303 > > Otherwise i think it won't compile on some platforms Done. https://codereview.chromium.org/2272123002/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:3020: if (base::FeatureList::IsEnabled(features::kPreferHtmlOverPlugins)) { On 2016/08/24 19:11:30, tommycli wrote: > This should only happen if it's a main frame navigation, so please add an > IsInMainFrame block on this if statement. > And it should also be in an ENABLE_PLUGINS ifdef. > > Also I think the FeatureList test should be pushed to be within the > FlashDownloadInterception::MaybeCreateThrottleFor body. Done. https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.cc:35: FlashDownloadInterception::MaybeCreateThrottleFor( On 2016/08/24 19:11:30, tommycli wrote: > nit: You ran git cl format right? Yes, I ran it. I've now added a "using content::NavigationHandle" so that it fits in 2 lines instead of 3. https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... File chrome/browser/plugins/flash_download_interception.h (right): https://codereview.chromium.org/2272123002/diff/1/chrome/browser/plugins/flas... chrome/browser/plugins/flash_download_interception.h:21: class FlashDownloadInterception { On 2016/08/24 19:11:30, tommycli wrote: > class comment should be updated. something similar to: > > // Redirects Flash Download URL navigations, since Chrome already ships with > Flash. > > Use your imagination :) > > Also do you think it should be named similarly to the AppUrlRedirector? i.e. > FlashDownloadUrlRedirector? > > I'm not sure, but think about it. Changed the comment. ptal. I decided to name the class FlashDownloadInterception because I believe that what we're doing here isn't exactly a redirection. Is it? Correct me if I'm wrong. Maybe the name can be interpreted by some as an actual Flash download intercept, and that's not the case. Maybe I should rename it to FlashDownloadUrlInterception? Let me know what you think.
LGTM, though you will need another reviewer.
On 2016/08/25 00:42:19, tommycli wrote: > LGTM, though you will need another reviewer. I know another reviewer! (BTW, CC bauerb as well?)
thestig@chromium.org changed reviewers: + thestig@chromium.org
https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3019: #if defined(ENABLE_PLUGINS) Merge into the if block below? if (handle->IsInMainFrame()) { #if defined(ENABLE_PLUGINS) foo #endif bar } https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:19: using content::WebContents; not needed https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:27: bool ShouldInterceptNavigation( Does this need a comment or a TODO? https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:47: if (handle->GetURL().GetContent().compare(0, strlen(kFlashDownloadURL), Use base::StartsWith() ? https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.h (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.h:15: class NavigationThrottle; Either this is not needed, or the navigation_throttle.h include is not. https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.h:23: static std::unique_ptr<content::NavigationThrottle> MaybeCreateThrottleFor( Would unit tests be useful?
+cc bauerb This is happening. Not required to review, but FYI
https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:3019: #if defined(ENABLE_PLUGINS) On 2016/08/25 00:55:06, Lei Zhang wrote: > Merge into the if block below? > > if (handle->IsInMainFrame()) { > #if defined(ENABLE_PLUGINS) > foo > #endif > > bar > } Done. https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.cc (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:19: using content::WebContents; On 2016/08/25 00:55:06, Lei Zhang wrote: > not needed Done. https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:27: bool ShouldInterceptNavigation( On 2016/08/25 00:55:06, Lei Zhang wrote: > Does this need a comment or a TODO? Added a TODO to body. https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.cc:47: if (handle->GetURL().GetContent().compare(0, strlen(kFlashDownloadURL), On 2016/08/25 00:55:07, Lei Zhang wrote: > Use base::StartsWith() ? Done. https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.h (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.h:15: class NavigationThrottle; On 2016/08/25 00:55:07, Lei Zhang wrote: > Either this is not needed, or the navigation_throttle.h include is not. Done. https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.h:23: static std::unique_ptr<content::NavigationThrottle> MaybeCreateThrottleFor( On 2016/08/25 00:55:07, Lei Zhang wrote: > Would unit tests be useful? Apparently, AppUrlRedirector (similar class) is tested with browser tests. I don't know which is better. I'll take a better look and talk with tommycli@ about it.
https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... File chrome/browser/plugins/flash_download_interception.h (right): https://codereview.chromium.org/2272123002/diff/20001/chrome/browser/plugins/... chrome/browser/plugins/flash_download_interception.h:23: static std::unique_ptr<content::NavigationThrottle> MaybeCreateThrottleFor( On 2016/08/25 01:50:36, trizzofo wrote: > On 2016/08/25 00:55:07, Lei Zhang wrote: > > Would unit tests be useful? > > Apparently, AppUrlRedirector (similar class) is tested with browser tests. I > don't know which is better. I'll take a better look and talk with tommycli@ > about it. Since this CL is blocking some UI work (permission prompt), it would be good to commit it asap and take care of tests later.
On 2016/08/25 20:46:52, trizzofo wrote: > Since this CL is blocking some UI work (permission prompt), it would be good to > commit it asap and take care of tests later. Is there no way to do the following in a unit_test? - create arbitrary NavigationHandles - pass them to MaybeCreateThrottleFor() - see if throttles are (not) created as expected
Description was changed from ========== [HBD] Intercept navigation to Flash download page Intercepts navigation to get.adobe.com/flashplayer Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 ========== to ========== [HBD] Intercept navigation to Flash download page Intercepts navigation to get.adobe.com/flashplayer Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
trizzofo@google.com changed reviewers: + clamy@chromium.org
clamy, ptal at content/browser/frame_host. Thanks!
https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:23: std::unique_ptr<NavigationThrottle> throttle; If you split up the two cases below, these common setup things can be put into the SetUp() function of your test class https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:40: { These scoped portions suggest to me it should be divided into two tests: TEST_F(FlashDownloadInterceptionTest, PreferHtmlOverPluginsOff) and TEST_F(FlashDownloadInterceptionTest, PreferHtmlOverPluginsOn) https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:62: EXPECT_NE(nullptr, throttle); To give this test a bit more teeth in the non-nullptr case, also test the result of throttle->WillStartRequest(); and potentially... WillRedirectRequest, though I'm not sure what's supposed to happen in that case.
https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:23: std::unique_ptr<NavigationThrottle> throttle; On 2016/09/01 01:08:20, tommycli wrote: > If you split up the two cases below, these common setup things can be put into > the SetUp() function of your test class Done. https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:40: { On 2016/09/01 01:08:20, tommycli wrote: > These scoped portions suggest to me it should be divided into two tests: > > TEST_F(FlashDownloadInterceptionTest, PreferHtmlOverPluginsOff) > > and > > TEST_F(FlashDownloadInterceptionTest, PreferHtmlOverPluginsOn) Done.
https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2272123002/diff/100001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:62: EXPECT_NE(nullptr, throttle); On 2016/09/01 01:08:20, tommycli wrote: > To give this test a bit more teeth in the non-nullptr case, also test the result > of throttle->WillStartRequest(); and potentially... WillRedirectRequest, though > I'm not sure what's supposed to happen in that case. Done.
lgtm thanks
The CQ bit was checked by tommycli@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I don't think the content/ changes are necessary. We already have one function that will set has_user_gesture to true. https://codereview.chromium.org/2272123002/diff/140001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2272123002/diff/140001/content/public/browser... content/public/browser/navigation_handle.h:210: virtual void SetHasUserGestureForTesting(bool has_user_gesture) = 0; This doesn't seem necessary. Just call CallWillStartRequestForTesting with has_user_gesture set to true.
The CQ bit was checked by trizzofo@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.
Lei Zhang, ptal. https://codereview.chromium.org/2272123002/diff/140001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2272123002/diff/140001/content/public/browser... content/public/browser/navigation_handle.h:210: virtual void SetHasUserGestureForTesting(bool has_user_gesture) = 0; On 2016/09/01 18:26:42, clamy wrote: > This doesn't seem necessary. Just call CallWillStartRequestForTesting with > has_user_gesture set to true. Done. Thanks!
The CQ bit was checked by trizzofo@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...
lgtm https://codereview.chromium.org/2272123002/diff/180001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2272123002/diff/180001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:28: GURL("https://get.adobe.com/flashplayer/"), main_rfh()); Maybe also try: http://get.adobe.com/flashplayer http://example.com/get.adobe.com/flashplayer https://codereview.chromium.org/2272123002/diff/180001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:43: std::unique_ptr<NavigationHandle> flash_slash_navigation_handle_; Can the two that are only used in a single test be local variables instead?
https://codereview.chromium.org/2272123002/diff/180001/chrome/browser/plugins... File chrome/browser/plugins/flash_download_interception_unittest.cc (right): https://codereview.chromium.org/2272123002/diff/180001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:28: GURL("https://get.adobe.com/flashplayer/"), main_rfh()); On 2016/09/01 21:08:11, Lei Zhang wrote: > Maybe also try: > > http://get.adobe.com/flashplayer > http://example.com/get.adobe.com/flashplayer Done. https://codereview.chromium.org/2272123002/diff/180001/chrome/browser/plugins... chrome/browser/plugins/flash_download_interception_unittest.cc:43: std::unique_ptr<NavigationHandle> flash_slash_navigation_handle_; On 2016/09/01 21:08:11, Lei Zhang wrote: > Can the two that are only used in a single test be local variables instead? Done.
The CQ bit was checked by trizzofo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2272123002/#ps200001 (title: "Changes per Lei Zhang's comments")
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.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [HBD] Intercept navigation to Flash download page Intercepts navigation to get.adobe.com/flashplayer Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [HBD] Intercept navigation to Flash download page Intercepts navigation to get.adobe.com/flashplayer Here is more information about HTML5 by Default: go/hbd-design-doc BUG=626728 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/8ddaeb7e8317409c88cbb9f1bc65e4351f539239 Cr-Commit-Position: refs/heads/master@{#416113} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8ddaeb7e8317409c88cbb9f1bc65e4351f539239 Cr-Commit-Position: refs/heads/master@{#416113} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
