Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
953793003:
Ensuring interception of stream get determined by plugin path before checking mime type. (Closed)
DescriptionEnsuring interception of stream get determined by plugin path before checking mime type.
Earlier we are checking whether a extension with url that have mime type
should be intercepted as stream by checking plugin availablity.
Now we are passing plugin path and then checking whether their is
MimehandlerView Plugin available with that path and then we are allowing
intercepted as stream. So now interception is not depends upon mimetype.
BUG=443466
Committed: https://crrev.com/d7d1167121bfe4b80d2cdf7559171aeff25c02c6
Cr-Commit-Position: refs/heads/master@{#324187}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Changes as per review comments. #
Total comments: 9
Patch Set 3 : Changes as per review comments #Patch Set 4 : Added Test Case. #Patch Set 5 : Adding Test Case. #
Total comments: 10
Patch Set 6 : Changing Test Cases. #
Total comments: 10
Patch Set 7 : Changes as per review comments. #Patch Set 8 : Fixing Build error. #Patch Set 9 : #
Total comments: 29
Patch Set 10 : Changes as per review comments. #Patch Set 11 : Resolving Build Errors. #
Total comments: 13
Patch Set 12 : Changed as per review comments. #Patch Set 13 : Changes as per review comments. #
Total comments: 8
Patch Set 14 : Changes as per review comments. #
Total comments: 15
Patch Set 15 : Addressing nits. #
Total comments: 4
Patch Set 16 : Addressing nits. #Messages
Total messages: 65 (9 generated)
deepak.m1@samsung.com changed reviewers: + jochen@chromium.org, raymes@chromium.org
PTAL
deepak.m1@samsung.com changed reviewers: + mmenke@chromium.org, scottmg@chromium.org
+ mmenke@chromium.org for content/browser/loader/* + scottmg@chromium.org for resource_dispatcher_host_delegate.cc/h PTAL
deepak.m1@samsung.com changed reviewers: + jam@chromium.org
Hey Deepak. Thanks for taking a look, I will get to this tomorrow or Wednesday. mmenke,scottmg: this isn't ready for OWNERS review just yet. Thanks!
On 2015/03/02 06:10:00, raymes wrote: > Hey Deepak. Thanks for taking a look, I will get to this tomorrow or Wednesday. > > mmenke,scottmg: this isn't ready for OWNERS review just yet. > > Thanks! ok!
i'm not owners anyway, removing myself.
Not a review, but this says it fixes something, but there are no tests... Can we add one?
On 2015/03/03 18:38:00, mmenke wrote: > Not a review, but this says it fixes something, but there are no tests... Can > we add one? Actually this is follow up for https://codereview.chromium.org/933093002/. and an improvement in current code as earlier when we are Ensuring streams aren't intercepted unless their associated MimeHandleView plugin is enabled, we are checking plugin enabled part by fetching all the plugin's from plugin service and checking wheather plugin path is same as extenison's url, and then checking the availablity of the plugin. With this patch no need to fetch all the plugin's from plugin service for compairing plugin path with extension url like earlier. we are passing the plugin path as file_path, In the whitelist for loop we can compare the file_path with the extension url, and if it matches then we will check the plugin availablity. so if plugin is not available then stream will not be intercepted. Thanks
PTAL Thanks.
PTAL please. Thanks.
PTAL Thanks.
On 2015/03/11 08:16:27, Deepak wrote: > PTAL > Thanks. It's generally best to be clear who you're pinging. Raymes: I believe that's for you?
On 2015/03/11 14:31:26, mmenke wrote: > On 2015/03/11 08:16:27, Deepak wrote: > > PTAL > > Thanks. > > It's generally best to be clear who you're pinging. Raymes: I believe that's > for you? @Raymes PTAL
https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:601: const base::FilePath& file_path, plugin_path https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:616: content::PluginServiceFilter* filter = service->GetFilter(); We can remove the above 2 lines (see below) https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:638: // Check that the plugin is actually enabled. We don't need to check if it's enabled. This check will already be performed before we get here. We can delete this. https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:646: } I think we can add a DCHECK in this case: DCHECK(!handler->handler_url().empty()); https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:657: if (!handler->handler_url().empty()) { In this case we should be able to remove this if-statement and turn it into the opposite DCHECK: DCHECK(handler->handler_url().empty()); https://codereview.chromium.org/953793003/diff/1/chrome/browser/renderer_host... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:659: *payload = target_info.view_id; We can remove the above 2 lines here, they should never be hit. https://codereview.chromium.org/953793003/diff/1/content/browser/loader/buffe... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/1/content/browser/loader/buffe... content/browser/loader/buffered_resource_handler_unittest.cc:102: const base::FilePath& file_path, plugin_path https://codereview.chromium.org/953793003/diff/1/content/browser/loader/buffe... content/browser/loader/buffered_resource_handler_unittest.cc:222: TEST_F(BufferedResourceHandlerTest, StreamHandling) { Does this test pass now? I would have expected this file to need some changes? https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... content/browser/loader/resource_dispatcher_host_impl.cc:731: const base::FilePath& file_path, plugin_path https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... content/browser/loader/resource_dispatcher_host_impl.h:245: const base::FilePath& file_path, plugin_path https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... File content/public/browser/resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... content/public/browser/resource_dispatcher_host_delegate.cc:58: const base::FilePath& file_path, plugin_path https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... content/public/browser/resource_dispatcher_host_delegate.h:96: virtual bool ShouldInterceptResourceAsStream(const base::FilePath& file_path, plugin_path
@Raymes, Thanks for review. Changes done as suggested. PTAL. https://codereview.chromium.org/953793003/diff/1/content/browser/loader/buffe... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/1/content/browser/loader/buffe... content/browser/loader/buffered_resource_handler_unittest.cc:102: const base::FilePath& file_path, On 2015/03/12 05:21:03, raymes wrote: > plugin_path Done. https://codereview.chromium.org/953793003/diff/1/content/browser/loader/buffe... content/browser/loader/buffered_resource_handler_unittest.cc:222: TEST_F(BufferedResourceHandlerTest, StreamHandling) { On 2015/03/12 05:21:03, raymes wrote: > Does this test pass now? I would have expected this file to need some changes? I have run all content_unittests, All the test cases are getting passed. https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... content/browser/loader/resource_dispatcher_host_impl.cc:731: const base::FilePath& file_path, On 2015/03/12 05:21:03, raymes wrote: > plugin_path Done. https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/953793003/diff/1/content/browser/loader/resou... content/browser/loader/resource_dispatcher_host_impl.h:245: const base::FilePath& file_path, On 2015/03/12 05:21:03, raymes wrote: > plugin_path Done. https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... File content/public/browser/resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... content/public/browser/resource_dispatcher_host_delegate.cc:58: const base::FilePath& file_path, On 2015/03/12 05:21:03, raymes wrote: > plugin_path Done. https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/953793003/diff/1/content/public/browser/resou... content/public/browser/resource_dispatcher_host_delegate.h:96: virtual bool ShouldInterceptResourceAsStream(const base::FilePath& file_path, On 2015/03/12 05:21:04, raymes wrote: > plugin_path Done.
@Raymes, Thanks for review. Changes done as suggested. PTAL.
https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:612: nit: don't need a newline https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:627: if (plugin_path == We should add a comment above this line: // If the MimeHandlerView plugin to be loaded matches the extension, intercept the stream for that extension. https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:637: } else { Actually we should do this only if the plugin path is empty. Also add a comment here // If no plugin path is provided, then we are trying to intercept the stream for the streamsPrivate API. https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... content/browser/loader/buffered_resource_handler.cc:317: if (plugin.type == WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) { We should add a unittest that hits this line by implementing a fake PluginService that returns PluginInfo that matches.
@Raymes Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:612: On 2015/03/16 05:36:50, raymes wrote: > nit: don't need a newline Done. https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:627: if (plugin_path == On 2015/03/16 05:36:50, raymes wrote: > We should add a comment above this line: > // If the MimeHandlerView plugin to be loaded matches the extension, intercept > the stream for that extension. Done. https://codereview.chromium.org/953793003/diff/20001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:637: } else { On 2015/03/16 05:36:49, raymes wrote: > Actually we should do this only if the plugin path is empty. > Also add a comment here > // If no plugin path is provided, then we are trying to intercept the stream for > the streamsPrivate API. Done. https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... content/browser/loader/buffered_resource_handler.cc:317: if (plugin.type == WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) { On 2015/03/16 05:36:50, raymes wrote: > We should add a unittest that hits this line by implementing a fake > PluginService that returns PluginInfo that matches. > With the current test cases we can check above condition. I have modified fake_plugin_service.cc by making the plugin info of PLUGIN_TYPE_BROWSER_PLUGIN type,and making has_plugin as true, so above condition will get hit when we have resource type as RESOURCE_TYPE_OBJECT. When 'stream_has_handler_' is true then we are returning true from this function and when 'stream_has_handler_' is false then we will return false from this function.
https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... content/browser/loader/buffered_resource_handler.cc:317: if (plugin.type == WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) { Can we add a separate test though (copying one of the previous tests). There should be a test that exercises this condition and one that exercises the condition where has_plugin is false.
On 2015/03/18 03:45:52, raymes wrote: > https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... > File content/browser/loader/buffered_resource_handler.cc (right): > > https://codereview.chromium.org/953793003/diff/20001/content/browser/loader/b... > content/browser/loader/buffered_resource_handler.cc:317: if (plugin.type == > WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) { > Can we add a separate test though (copying one of the previous tests). There > should be a test that exercises this condition and one that exercises the > condition where has_plugin is false. @Raymes, I have added test case as suggested. PTAL Thanks.
https://codereview.chromium.org/953793003/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:611: if (plugin_path.empty() && handler && Please merge this if-statement into the else above so that it's } else if (....) https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... content/browser/loader/buffered_resource_handler_unittest.cc:294: TEST_F(BufferedResourceHandlerTest, StreamHandlingWithPluginPath) { I think it would be sufficient to add a single additional case to the test above which hits that new code path rather than adding this whole function.
https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... content/browser/loader/buffered_resource_handler_unittest.cc:233: set_plugin_available(false); Let's set the plugin as available for most of the tests and just add a single additional test where it isn't available but the stream is still intercepted. This should test the legacy codepath (for the streamsPrivate API). https://codereview.chromium.org/953793003/diff/80001/content/test/fake_plugin... File content/test/fake_plugin_service.cc (right): https://codereview.chromium.org/953793003/diff/80001/content/test/fake_plugin... content/test/fake_plugin_service.cc:50: info->path = base::FilePath(); If a plugin is available, shouldn't we return a valid plugin path? Otherwise we won't hit that codepath right? https://codereview.chromium.org/953793003/diff/80001/content/test/fake_plugin... content/test/fake_plugin_service.cc:51: return true; Rather than adding this stuff here, I think we should have a separate class defined inside the unittest which inherits from FakePluginService and overrides this method.
@Raymes Changes done as suggested. PTAL https://codereview.chromium.org/953793003/diff/80001/chrome/browser/renderer_... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/953793003/diff/80001/chrome/browser/renderer_... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:611: if (plugin_path.empty() && handler && On 2015/03/19 00:39:22, raymes wrote: > Please merge this if-statement into the else above so that it's > } else if (....) Done. https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... content/browser/loader/buffered_resource_handler_unittest.cc:233: set_plugin_available(false); On 2015/03/19 00:45:51, raymes wrote: > Let's set the plugin as available for most of the tests and just add a single > additional test where it isn't available but the stream is still intercepted. > This should test the legacy codepath (for the streamsPrivate API). Done. https://codereview.chromium.org/953793003/diff/80001/content/browser/loader/b... content/browser/loader/buffered_resource_handler_unittest.cc:294: TEST_F(BufferedResourceHandlerTest, StreamHandlingWithPluginPath) { On 2015/03/19 00:39:22, raymes wrote: > I think it would be sufficient to add a single additional case to the test above > which hits that new code path rather than adding this whole function. Done. https://codereview.chromium.org/953793003/diff/80001/content/test/fake_plugin... File content/test/fake_plugin_service.cc (right): https://codereview.chromium.org/953793003/diff/80001/content/test/fake_plugin... content/test/fake_plugin_service.cc:50: info->path = base::FilePath(); On 2015/03/19 00:45:51, raymes wrote: > If a plugin is available, shouldn't we return a valid plugin path? Otherwise we > won't hit that codepath right? Even when we give the proper plugin path then also, TestResourceDispatcherHost::MaybeInterceptAsStream() in buffered_resource_handler_unittest.cc get called not the actual ResourceDispatcherHostImpl::MaybeInterceptAsStream(). As this is just simulation that MaybeInterceptAsStream() calling. For completeness I am assigning proper plugin path when plugin available as suggested. https://codereview.chromium.org/953793003/diff/80001/content/test/fake_plugin... content/test/fake_plugin_service.cc:51: return true; On 2015/03/19 00:45:51, raymes wrote: > Rather than adding this stuff here, I think we should have a separate class > defined inside the unittest which inherits from FakePluginService and overrides > this method. Done.
@Raymes Friendly Ping ...
raymes@chromium.org changed reviewers: - scottmg@chromium.org
lgtm https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:158: TestFakePluginService(bool fake_plugin_available) nit: plugin_available is fine https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:331: // with plugin not available. nit: add to the comment: "This is the case when intercepting streams for the streamsPrivate extensions API."
https://codereview.chromium.org/953793003/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/953793003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:61: net::URLRequest* request, URLRequest* should be the first parameter https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:158: TestFakePluginService(bool fake_plugin_available) explicit https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:173: }; DISALLOW_COPY_AND_ASSIGN()
@Raymes and @Jochen Thanks for review and providing valuable comments. Changes done as suggested. PTAL https://codereview.chromium.org/953793003/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/953793003/diff/100001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h:61: net::URLRequest* request, On 2015/03/24 09:20:54, jochen (slow) wrote: > URLRequest* should be the first parameter Done. https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:158: TestFakePluginService(bool fake_plugin_available) On 2015/03/24 03:01:49, raymes wrote: > nit: plugin_available is fine Done. https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:158: TestFakePluginService(bool fake_plugin_available) On 2015/03/24 09:20:54, jochen (slow) wrote: > explicit Done. https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:173: }; On 2015/03/24 09:20:54, jochen (slow) wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/953793003/diff/100001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:331: // with plugin not available. On 2015/03/24 03:01:49, raymes wrote: > nit: add to the comment: "This is the case when intercepting streams for the > streamsPrivate extensions API." Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/953793003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/953793003/160001
I'm deferring to raymes on correctness of the code. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:334: // away. nit: Don't use "we" in comments. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:503: #if defined(ENABLE_PLUGINS) Can we just wrap this method in this ifdef? We never call it if enable plugins is false, and I'm not a big fan of code that's never run and never tested. Looks fine now, but can easily imagine this method getting more complicated, and then a future refactor unearthing a bug in the never used case. Could either move the ifdef up here and add it in the header, or move it to an anonymous namespace up top in this file (That does mean you'll have to pass in a bunch of parameter, but it also makes clear that it has no side effects on the BufferedResourceHandler - that's my preferred option, but feel free to disagree). Alternatively, could just inline it in your new method, as it's not a lot of code. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:59: bool IsHandledByPlugin(bool* defer, bool* result); This should be documented. We return a boolean, and yet we also take a pointer to a "result". Unclear what either means. Also not clear how we call back in the case "defer" is set to true. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:68: bool GetSupportingPlugin(WebPluginInfo* plugin, bool* stale); Mind documenting this? Unclear what the return value means, or stale, for that matter. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:159: : plugin_available_(plugin_available) {} nit: Blank line after constructor. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:172: bool plugin_available_; nit: const? https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:173: DISALLOW_COPY_AND_ASSIGN(TestFakePluginService); nit: Line break before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:185: std::string* actual_mime_type) { optional: May want to just inline this method, given how small its body is. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:190: info->path = base::FilePath::FromUTF8Unsafe(std::string( include base::FilePath https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:340: TestStreamIsIntercepted(allow_download, must_download, resource_type)); Should we also have a similar test where is_stale is initially true? https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:245: const base::FilePath& plugin_path, Need to forward declare this (Admittedly, should have been forward declared in this file before, but doesn't look like it was)
The CQ bit was unchecked by mmenke@chromium.org
Thanks mmenke! https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:334: // away. if execution reaches here https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:339: *result = UseAlternateNextHandler(handler.Pass(), payload); cancel_request = !UseAlternateNextHandler... https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:365: return result; Probably we should change this to: bool cancel_request = false; bool handled_by_plugin = IsHandledByPlugin(defer, cancel_request); if (cancel_request) return false; if (handled_by_plugin) return true; mmenke: Does this sound reasonable to you? https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:59: bool IsHandledByPlugin(bool* defer, bool* result); On 2015/03/24 14:44:32, mmenke wrote: > This should be documented. We return a boolean, and yet we also take a pointer > to a "result". Unclear what either means. Also not clear how we call back in > the case "defer" is set to true. It was never clear how we call back in the case defer is set to true - the documentation in this class isn't great :) I think that's orthogonal to this change though. |result| is the return value of SelectNextHandler and again there is no documentation here, so it's unclear. But tracing the code through it looks like it is whether the request should be cancelled. So we can rename |result| to |cancel_request| // Returns true if a plugin will handle the current request. |defer| // is set to true if plugin data is stale and needs to be refreshed // before the request can be handled. |cancel_request| is true if // the request should be cancelled and false otherwise. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:68: bool GetSupportingPlugin(WebPluginInfo* plugin, bool* stale); On 2015/03/24 14:44:32, mmenke wrote: > Mind documenting this? Unclear what the return value means, or stale, for that > matter. As you mentioned in your comment, this function can (and probably should) just be inlined into IsHandledByPlugin. But for the sake of explanation: // Returns true if there is a plugin that can handle the current // request. If there is such a plugin, it is returned in |plugin|. If // the plugin data is out of date and needs to be refreshed from disk, // |stale| will be set to true and the result will not be valid.
@Raymes & @mmenke Thanks for review, Changes done as suggested. PTAL https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:334: // away. On 2015/03/25 00:26:32, raymes wrote: > if execution reaches here Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:339: *result = UseAlternateNextHandler(handler.Pass(), payload); On 2015/03/25 00:26:32, raymes wrote: > cancel_request = !UseAlternateNextHandler... Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:365: return result; On 2015/03/25 00:26:32, raymes wrote: > Probably we should change this to: > > bool cancel_request = false; > bool handled_by_plugin = IsHandledByPlugin(defer, cancel_request); > if (cancel_request) > return false; > if (handled_by_plugin) > return true; > > mmenke: Does this sound reasonable to you? Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:503: #if defined(ENABLE_PLUGINS) On 2015/03/24 14:44:32, mmenke wrote: > Can we just wrap this method in this ifdef? We never call it if enable plugins > is false, and I'm not a big fan of code that's never run and never tested. > Looks fine now, but can easily imagine this method getting more complicated, and > then a future refactor unearthing a bug in the never used case. > > Could either move the ifdef up here and add it in the header, or move it to an > anonymous namespace up top in this file (That does mean you'll have to pass in a > bunch of parameter, but it also makes clear that it has no side effects on the > BufferedResourceHandler - that's my preferred option, but feel free to > disagree). > > Alternatively, could just inline it in your new method, as it's not a lot of > code. Have made the function inline. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:59: bool IsHandledByPlugin(bool* defer, bool* result); On 2015/03/25 00:26:32, raymes wrote: > On 2015/03/24 14:44:32, mmenke wrote: > > This should be documented. We return a boolean, and yet we also take a > pointer > > to a "result". Unclear what either means. Also not clear how we call back in > > the case "defer" is set to true. > > It was never clear how we call back in the case defer is set to true - the > documentation in this class isn't great :) I think that's orthogonal to this > change though. > > |result| is the return value of SelectNextHandler and again there is no > documentation here, so it's unclear. But tracing the code through it looks like > it is whether the request should be cancelled. > > So we can rename |result| to |cancel_request| > > // Returns true if a plugin will handle the current request. |defer| > // is set to true if plugin data is stale and needs to be refreshed > // before the request can be handled. |cancel_request| is true if > // the request should be cancelled and false otherwise. Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:68: bool GetSupportingPlugin(WebPluginInfo* plugin, bool* stale); On 2015/03/25 00:26:32, raymes wrote: > On 2015/03/24 14:44:32, mmenke wrote: > > Mind documenting this? Unclear what the return value means, or stale, for > that > > matter. > > As you mentioned in your comment, this function can (and probably should) just > be inlined into IsHandledByPlugin. But for the sake of explanation: > > // Returns true if there is a plugin that can handle the current > // request. If there is such a plugin, it is returned in |plugin|. If > // the plugin data is out of date and needs to be refreshed from disk, > // |stale| will be set to true and the result will not be valid. Have made this function inline as it is not getting used anywhere else except in IsHandledByPlugin(). https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:159: : plugin_available_(plugin_available) {} On 2015/03/24 14:44:32, mmenke wrote: > nit: Blank line after constructor. Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:172: bool plugin_available_; On 2015/03/24 14:44:32, mmenke wrote: > nit: const? Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:173: DISALLOW_COPY_AND_ASSIGN(TestFakePluginService); On 2015/03/24 14:44:32, mmenke wrote: > nit: Line break before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:185: std::string* actual_mime_type) { On 2015/03/24 14:44:32, mmenke wrote: > optional: May want to just inline this method, given how small its body is. Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:190: info->path = base::FilePath::FromUTF8Unsafe(std::string( On 2015/03/24 14:44:32, mmenke wrote: > include base::FilePath Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:340: TestStreamIsIntercepted(allow_download, must_download, resource_type)); On 2015/03/24 14:44:32, mmenke wrote: > Should we also have a similar test where is_stale is initially true? Done. https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/953793003/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:245: const base::FilePath& plugin_path, On 2015/03/24 14:44:32, mmenke wrote: > Need to forward declare this (Admittedly, should have been forward declared in > this file before, but doesn't look like it was) Done.
Looks good, thanks for the cleanups! Other than nits, just one question about the test. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:303: // cancelled and true otherwise. Method-level docs should go before the declaration, rather than the definition. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:159: explicit TestFakePluginService(bool plugin_available, bool is_plugin_stale) nit: Explicit no longer needed. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:185: DISALLOW_COPY_AND_ASSIGN(TestFakePluginService); On 2015/03/24 14:44:32, mmenke wrote: > nit: Line break before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:276: set_plugin_stale(false); optional nit: This is the default, so no need to set it. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:352: TestStreamIsIntercepted(allow_download, must_download, resource_type)); I'm not familiar with this test fixture, but is there a way we can make sure it's intercepted asynchronously, once the plugin list is "updated"?
https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:352: TestStreamIsIntercepted(allow_download, must_download, resource_type)); On 2015/03/25 14:11:44, mmenke wrote: > I'm not familiar with this test fixture, but is there a way we can make sure > it's intercepted asynchronously, once the plugin list is "updated"? If we understood how the resume request process worked we could probably write a test for it, but I don't know what function gets called into to resume the request. Possibly OnResponseStarted just gets called again but I'm not completely sure.
Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.cc (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler.cc:303: // cancelled and true otherwise. On 2015/03/25 14:11:44, mmenke wrote: > Method-level docs should go before the declaration, rather than the definition. Done. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:159: explicit TestFakePluginService(bool plugin_available, bool is_plugin_stale) On 2015/03/25 14:11:44, mmenke wrote: > nit: Explicit no longer needed. Done. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:185: DISALLOW_COPY_AND_ASSIGN(TestFakePluginService); On 2015/03/25 14:11:44, mmenke wrote: > On 2015/03/24 14:44:32, mmenke wrote: > > nit: Line break before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:276: set_plugin_stale(false); On 2015/03/25 14:11:44, mmenke wrote: > optional nit: This is the default, so no need to set it. Done. https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:352: TestStreamIsIntercepted(allow_download, must_download, resource_type)); On 2015/03/25 23:32:08, raymes wrote: > On 2015/03/25 14:11:44, mmenke wrote: > > I'm not familiar with this test fixture, but is there a way we can make sure > > it's intercepted asynchronously, once the plugin list is "updated"? > > If we understood how the resume request process worked we could probably write a > test for it, but I don't know what function gets called into to resume the > request. Possibly OnResponseStarted just gets called again but I'm not > completely sure. It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc binds GetPluginInternal() which actually calls callback function in OnPluginLoaded(). But In Fake_plugin_service.cc GetPlugin() is empty and does not register any callback. as it does not have GetPluginInternal() also. so no callback is getting called.But I am not sure.
https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:352: TestStreamIsIntercepted(allow_download, must_download, resource_type)); On 2015/03/26 04:55:11, Deepak wrote: > On 2015/03/25 23:32:08, raymes wrote: > > On 2015/03/25 14:11:44, mmenke wrote: > > > I'm not familiar with this test fixture, but is there a way we can make sure > > > it's intercepted asynchronously, once the plugin list is "updated"? > > > > If we understood how the resume request process worked we could probably write > a > > test for it, but I don't know what function gets called into to resume the > > request. Possibly OnResponseStarted just gets called again but I'm not > > completely sure. > > It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc binds > GetPluginInternal() which actually calls > callback function in OnPluginLoaded(). > But In Fake_plugin_service.cc GetPlugin() is empty and does not register any > callback. as it does not have GetPluginInternal() also. > so no callback is getting called.But I am not sure. So we can just override FakePluginService::GetPlugins and have it asynchronously invoke the callback passed to it, and set is_plugin_state_ to false. May also need to call content::RunAllPendingInMessageLoop twice instead of once, or find some other more aggressive way to run tasks. Or it may just work. It's unclear. Sound reasonable? Know not testing this isn't a result of your new code, but I feel the current testing is pretty inadequate, and it a case your CL affects.
On 2015/03/26 14:59:28, mmenke wrote: > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > File content/browser/loader/buffered_resource_handler_unittest.cc (right): > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > content/browser/loader/buffered_resource_handler_unittest.cc:352: > TestStreamIsIntercepted(allow_download, must_download, resource_type)); > On 2015/03/26 04:55:11, Deepak wrote: > > On 2015/03/25 23:32:08, raymes wrote: > > > On 2015/03/25 14:11:44, mmenke wrote: > > > > I'm not familiar with this test fixture, but is there a way we can make > sure > > > > it's intercepted asynchronously, once the plugin list is "updated"? > > > > > > If we understood how the resume request process worked we could probably > write > > a > > > test for it, but I don't know what function gets called into to resume the > > > request. Possibly OnResponseStarted just gets called again but I'm not > > > completely sure. > > > > It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc binds > > GetPluginInternal() which actually calls > > callback function in OnPluginLoaded(). > > But In Fake_plugin_service.cc GetPlugin() is empty and does not register any > > callback. as it does not have GetPluginInternal() also. > > so no callback is getting called.But I am not sure. > > So we can just override FakePluginService::GetPlugins and have it asynchronously > invoke the callback passed to it, and set is_plugin_state_ to false. > > May also need to call content::RunAllPendingInMessageLoop twice instead of once, > or find some other more aggressive way to run tasks. Or it may just work. It's > unclear. > > Sound reasonable? > > Know not testing this isn't a result of your new code, but I feel the current > testing is pretty inadequate, and it a case your CL affects. I understood that we will override GetPlugins() and it will get callback function BufferedResourceHandler::OnPluginsLoaded() here it will check LoadPluginListInProcess() But we don't have any plugin list. and in GetPlugins() internally binds PluginServiceImpl::GetPluginsInternal() also, that we don't have in fake_plugin_service file. and secondly it does PostSequencedWorkerTaskWithShutdownBehavior(), and we don't have plugin_list_token_ binded in our fake_plugin_service. Can you please elaborate about intended test case. As above things are unclear to me.
On 2015/03/26 15:28:58, Deepak wrote: > On 2015/03/26 14:59:28, mmenke wrote: > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > File content/browser/loader/buffered_resource_handler_unittest.cc (right): > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > content/browser/loader/buffered_resource_handler_unittest.cc:352: > > TestStreamIsIntercepted(allow_download, must_download, resource_type)); > > On 2015/03/26 04:55:11, Deepak wrote: > > > On 2015/03/25 23:32:08, raymes wrote: > > > > On 2015/03/25 14:11:44, mmenke wrote: > > > > > I'm not familiar with this test fixture, but is there a way we can make > > sure > > > > > it's intercepted asynchronously, once the plugin list is "updated"? > > > > > > > > If we understood how the resume request process worked we could probably > > write > > > a > > > > test for it, but I don't know what function gets called into to resume the > > > > request. Possibly OnResponseStarted just gets called again but I'm not > > > > completely sure. > > > > > > It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc binds > > > GetPluginInternal() which actually calls > > > callback function in OnPluginLoaded(). > > > But In Fake_plugin_service.cc GetPlugin() is empty and does not register any > > > callback. as it does not have GetPluginInternal() also. > > > so no callback is getting called.But I am not sure. > > > > So we can just override FakePluginService::GetPlugins and have it > asynchronously > > invoke the callback passed to it, and set is_plugin_state_ to false. > > > > May also need to call content::RunAllPendingInMessageLoop twice instead of > once, > > or find some other more aggressive way to run tasks. Or it may just work. > It's > > unclear. > > > > Sound reasonable? > > > > Know not testing this isn't a result of your new code, but I feel the current > > testing is pretty inadequate, and it a case your CL affects. > > I understood that we will override GetPlugins() and it will get callback > function > BufferedResourceHandler::OnPluginsLoaded() here it will check > LoadPluginListInProcess() > But we don't have any plugin list. > and in GetPlugins() internally binds PluginServiceImpl::GetPluginsInternal() > also, that we don't have in fake_plugin_service file. > and secondly it does PostSequencedWorkerTaskWithShutdownBehavior(), and we don't > have plugin_list_token_ binded in > our fake_plugin_service. > Can you please elaborate about intended test case. > As above things are unclear to me. I don't see why GetPluginsInternal matters. We're subclassing FakePluginService already. We can trivially add a method to override GetPlugins, and make it post a task to run the callback with an empty plugin list asynchronous. BufferedResourceHandler::OnPluginsLoaded ignores the passed in list, anyways, so we don't actually need a non-empty list. I'm just concerned that we have no tests that go through the path where we have to defer the request until we load a fresh list of plugins.
On 2015/03/26 15:36:24, mmenke wrote: > On 2015/03/26 15:28:58, Deepak wrote: > > On 2015/03/26 14:59:28, mmenke wrote: > > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > > File content/browser/loader/buffered_resource_handler_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > > content/browser/loader/buffered_resource_handler_unittest.cc:352: > > > TestStreamIsIntercepted(allow_download, must_download, resource_type)); > > > On 2015/03/26 04:55:11, Deepak wrote: > > > > On 2015/03/25 23:32:08, raymes wrote: > > > > > On 2015/03/25 14:11:44, mmenke wrote: > > > > > > I'm not familiar with this test fixture, but is there a way we can > make > > > sure > > > > > > it's intercepted asynchronously, once the plugin list is "updated"? > > > > > > > > > > If we understood how the resume request process worked we could probably > > > write > > > > a > > > > > test for it, but I don't know what function gets called into to resume > the > > > > > request. Possibly OnResponseStarted just gets called again but I'm not > > > > > completely sure. > > > > > > > > It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc binds > > > > GetPluginInternal() which actually calls > > > > callback function in OnPluginLoaded(). > > > > But In Fake_plugin_service.cc GetPlugin() is empty and does not register > any > > > > callback. as it does not have GetPluginInternal() also. > > > > so no callback is getting called.But I am not sure. > > > > > > So we can just override FakePluginService::GetPlugins and have it > > asynchronously > > > invoke the callback passed to it, and set is_plugin_state_ to false. > > > > > > May also need to call content::RunAllPendingInMessageLoop twice instead of > > once, > > > or find some other more aggressive way to run tasks. Or it may just work. > > It's > > > unclear. > > > > > > Sound reasonable? > > > > > > Know not testing this isn't a result of your new code, but I feel the > current > > > testing is pretty inadequate, and it a case your CL affects. > > > > I understood that we will override GetPlugins() and it will get callback > > function > > BufferedResourceHandler::OnPluginsLoaded() here it will check > > LoadPluginListInProcess() > > But we don't have any plugin list. > > and in GetPlugins() internally binds PluginServiceImpl::GetPluginsInternal() > > also, that we don't have in fake_plugin_service file. > > and secondly it does PostSequencedWorkerTaskWithShutdownBehavior(), and we > don't > > have plugin_list_token_ binded in > > our fake_plugin_service. > > Can you please elaborate about intended test case. > > As above things are unclear to me. > > I don't see why GetPluginsInternal matters. We're subclassing FakePluginService > already. We can trivially add a method to override GetPlugins, and make it post > a task to run the callback with an empty plugin list asynchronous. > BufferedResourceHandler::OnPluginsLoaded ignores the passed in list, anyways, so > we don't actually need a non-empty list. > > I'm just concerned that we have no tests that go through the path where we have > to defer the request until we load a fresh list of plugins. ok, I got some understanding. I will add a function like void GetPlugins(const GetPluginsCallback& callback) override { scoped_refptr<base::MessageLoopProxy> target_loop( base::MessageLoop::current()->message_loop_proxy()); std::vector<WebPluginInfo> plugins; target_loop->PostTask(FROM_HERE, base::Bind(callback, plugins)); } in TestFakePluginService class, so it will call BufferedResourceHandler::OnPluginsLoaded()with empty plugin list. Then should we just check that OnPluginsLoaded() get called.? Please correct me if I misunderstood something.
On 2015/03/26 16:30:03, Deepak wrote: > On 2015/03/26 15:36:24, mmenke wrote: > > On 2015/03/26 15:28:58, Deepak wrote: > > > On 2015/03/26 14:59:28, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > > > File content/browser/loader/buffered_resource_handler_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > > > content/browser/loader/buffered_resource_handler_unittest.cc:352: > > > > TestStreamIsIntercepted(allow_download, must_download, resource_type)); > > > > On 2015/03/26 04:55:11, Deepak wrote: > > > > > On 2015/03/25 23:32:08, raymes wrote: > > > > > > On 2015/03/25 14:11:44, mmenke wrote: > > > > > > > I'm not familiar with this test fixture, but is there a way we can > > make > > > > sure > > > > > > > it's intercepted asynchronously, once the plugin list is "updated"? > > > > > > > > > > > > If we understood how the resume request process worked we could > probably > > > > write > > > > > a > > > > > > test for it, but I don't know what function gets called into to resume > > the > > > > > > request. Possibly OnResponseStarted just gets called again but I'm not > > > > > > completely sure. > > > > > > > > > > It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc > binds > > > > > GetPluginInternal() which actually calls > > > > > callback function in OnPluginLoaded(). > > > > > But In Fake_plugin_service.cc GetPlugin() is empty and does not register > > any > > > > > callback. as it does not have GetPluginInternal() also. > > > > > so no callback is getting called.But I am not sure. > > > > > > > > So we can just override FakePluginService::GetPlugins and have it > > > asynchronously > > > > invoke the callback passed to it, and set is_plugin_state_ to false. > > > > > > > > May also need to call content::RunAllPendingInMessageLoop twice instead of > > > once, > > > > or find some other more aggressive way to run tasks. Or it may just work. > > > > It's > > > > unclear. > > > > > > > > Sound reasonable? > > > > > > > > Know not testing this isn't a result of your new code, but I feel the > > current > > > > testing is pretty inadequate, and it a case your CL affects. > > > > > > I understood that we will override GetPlugins() and it will get callback > > > function > > > BufferedResourceHandler::OnPluginsLoaded() here it will check > > > LoadPluginListInProcess() > > > But we don't have any plugin list. > > > and in GetPlugins() internally binds PluginServiceImpl::GetPluginsInternal() > > > also, that we don't have in fake_plugin_service file. > > > and secondly it does PostSequencedWorkerTaskWithShutdownBehavior(), and we > > don't > > > have plugin_list_token_ binded in > > > our fake_plugin_service. > > > Can you please elaborate about intended test case. > > > As above things are unclear to me. > > > > I don't see why GetPluginsInternal matters. We're subclassing > FakePluginService > > already. We can trivially add a method to override GetPlugins, and make it > post > > a task to run the callback with an empty plugin list asynchronous. > > BufferedResourceHandler::OnPluginsLoaded ignores the passed in list, anyways, > so > > we don't actually need a non-empty list. > > > > I'm just concerned that we have no tests that go through the path where we > have > > to defer the request until we load a fresh list of plugins. > > ok, I got some understanding. > I will add a function like > > void GetPlugins(const GetPluginsCallback& callback) override { > scoped_refptr<base::MessageLoopProxy> target_loop( > base::MessageLoop::current()->message_loop_proxy()); > std::vector<WebPluginInfo> plugins; > target_loop->PostTask(FROM_HERE, base::Bind(callback, plugins)); > } > > in TestFakePluginService class, so it will call > BufferedResourceHandler::OnPluginsLoaded()with empty plugin list. > Then should we just check that OnPluginsLoaded() get called.? > > Please correct me if I misunderstood something. Rather than check that OnPluginsLoaded is called, should check that host.intercepted_as_stream() is true... Fortunately, TestStreamIsIntercepted already does this, so you just need to switch the "EXPECT_FALSE" at the end of the method to "EXPECT_TRUE". Also, in BufferedResourceHandlerTest::TestStreamIsIntercepted, the "content::RunAllPendingInMessageLoop();" call may not run the message loop long enough. If the changed EXPECT_TRUE is failing, try calling "RunAllPendingInMessageLoop" 2 or three times in a row instead of once. If that fixes the problem, ideally we'd find a single call that waits long enough. Haven't really thought about it.
On 2015/03/26 16:35:15, mmenke wrote: > On 2015/03/26 16:30:03, Deepak wrote: > > On 2015/03/26 15:36:24, mmenke wrote: > > > On 2015/03/26 15:28:58, Deepak wrote: > > > > On 2015/03/26 14:59:28, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > > > > File content/browser/loader/buffered_resource_handler_unittest.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... > > > > > content/browser/loader/buffered_resource_handler_unittest.cc:352: > > > > > TestStreamIsIntercepted(allow_download, must_download, resource_type)); > > > > > On 2015/03/26 04:55:11, Deepak wrote: > > > > > > On 2015/03/25 23:32:08, raymes wrote: > > > > > > > On 2015/03/25 14:11:44, mmenke wrote: > > > > > > > > I'm not familiar with this test fixture, but is there a way we can > > > make > > > > > sure > > > > > > > > it's intercepted asynchronously, once the plugin list is > "updated"? > > > > > > > > > > > > > > If we understood how the resume request process worked we could > > probably > > > > > write > > > > > > a > > > > > > > test for it, but I don't know what function gets called into to > resume > > > the > > > > > > > request. Possibly OnResponseStarted just gets called again but I'm > not > > > > > > > completely sure. > > > > > > > > > > > > It appears,in actual scenerio GetPlugins() in plugin_service_impl.cc > > binds > > > > > > GetPluginInternal() which actually calls > > > > > > callback function in OnPluginLoaded(). > > > > > > But In Fake_plugin_service.cc GetPlugin() is empty and does not > register > > > any > > > > > > callback. as it does not have GetPluginInternal() also. > > > > > > so no callback is getting called.But I am not sure. > > > > > > > > > > So we can just override FakePluginService::GetPlugins and have it > > > > asynchronously > > > > > invoke the callback passed to it, and set is_plugin_state_ to false. > > > > > > > > > > May also need to call content::RunAllPendingInMessageLoop twice instead > of > > > > once, > > > > > or find some other more aggressive way to run tasks. Or it may just > work. > > > > > > It's > > > > > unclear. > > > > > > > > > > Sound reasonable? > > > > > > > > > > Know not testing this isn't a result of your new code, but I feel the > > > current > > > > > testing is pretty inadequate, and it a case your CL affects. > > > > > > > > I understood that we will override GetPlugins() and it will get callback > > > > function > > > > BufferedResourceHandler::OnPluginsLoaded() here it will check > > > > LoadPluginListInProcess() > > > > But we don't have any plugin list. > > > > and in GetPlugins() internally binds > PluginServiceImpl::GetPluginsInternal() > > > > also, that we don't have in fake_plugin_service file. > > > > and secondly it does PostSequencedWorkerTaskWithShutdownBehavior(), and we > > > don't > > > > have plugin_list_token_ binded in > > > > our fake_plugin_service. > > > > Can you please elaborate about intended test case. > > > > As above things are unclear to me. > > > > > > I don't see why GetPluginsInternal matters. We're subclassing > > FakePluginService > > > already. We can trivially add a method to override GetPlugins, and make it > > post > > > a task to run the callback with an empty plugin list asynchronous. > > > BufferedResourceHandler::OnPluginsLoaded ignores the passed in list, > anyways, > > so > > > we don't actually need a non-empty list. > > > > > > I'm just concerned that we have no tests that go through the path where we > > have > > > to defer the request until we load a fresh list of plugins. > > > > ok, I got some understanding. > > I will add a function like > > > > void GetPlugins(const GetPluginsCallback& callback) override { > > scoped_refptr<base::MessageLoopProxy> target_loop( > > base::MessageLoop::current()->message_loop_proxy()); > > std::vector<WebPluginInfo> plugins; > > target_loop->PostTask(FROM_HERE, base::Bind(callback, plugins)); > > } > > > > in TestFakePluginService class, so it will call > > BufferedResourceHandler::OnPluginsLoaded()with empty plugin list. > > Then should we just check that OnPluginsLoaded() get called.? > > > > Please correct me if I misunderstood something. > > Rather than check that OnPluginsLoaded is called, should check that > host.intercepted_as_stream() is true... Fortunately, TestStreamIsIntercepted > already does this, so you just need to switch the "EXPECT_FALSE" at the end of > the method to "EXPECT_TRUE". > > Also, in BufferedResourceHandlerTest::TestStreamIsIntercepted, the > "content::RunAllPendingInMessageLoop();" call may not run the message loop long > enough. If the changed EXPECT_TRUE is failing, try calling > "RunAllPendingInMessageLoop" 2 or three times in a row instead of once. If that > fixes the problem, ideally we'd find a single call that waits long enough. > Haven't really thought about it. @mmenke Thanks for guidance & suggestions. Changes done and working as expected. PTAL
Sorry, forgot to get back to this one today. I'll take a look on Monday.
https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/953793003/diff/200001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:59: bool IsHandledByPlugin(bool* defer, bool* cancel_request); this should be request_handled now then https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:173: *is_stale = is_plugin_stale_; nit: if it's stale, we should return false https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:186: std::vector<WebPluginInfo> plugins; We should add a valid plugin here to make sure the test works. https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:187: target_loop->PostTask(FROM_HERE, base::Bind(callback, plugins)); it should be sufficient to use base::MessageLoop::current()->PostTask
https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:107: std::string* payload) override { Could you add an EXPECT that this method is only invoked once? Just concerned that the new test wouldn't catch the case of us incorrectly calling this method twice. This shouldn't be called more than once in any existing test (and if a test every wanted to do so, intercepted_as_stream_ being a bool and not a count would be a little weird).
On 2015/03/30 01:17:38, mmenke wrote: @mmenke Thanks for review. Changes done as suggested. PTAL.
https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:107: std::string* payload) override { On 2015/03/30 01:17:38, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:173: *is_stale = is_plugin_stale_; On 2015/03/30 01:00:07, raymes wrote: Done. https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:186: std::vector<WebPluginInfo> plugins; On 2015/03/30 01:00:07, raymes wrote: Actually onPluginLoad() does not use Plugin vector, mmenke asked me to pass empty vector list. https://codereview.chromium.org/953793003/diff/240001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:187: target_loop->PostTask(FROM_HERE, base::Bind(callback, plugins)); On 2015/03/30 01:00:07, raymes wrote: Done.
@mmenke Thanks for review. Changes done as suggested. PTAL.
Just nits. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:118: int GetInterceptAsStream() { return intercepted_as_stream_count; } GetInterceptAsStream -> intercepted_as_stream_count https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:118: int GetInterceptAsStream() { return intercepted_as_stream_count; } nit: const https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:129: int intercepted_as_stream_count; nit: intercepted_as_stream_count_ https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:167: TestFakePluginService(bool plugin_available, bool is_plugin_stale) Should comment about is_plugin_stale behavior. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:193: base::MessageLoop::current()->message_loop_proxy()); No longer needed. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:278: EXPECT_TRUE(host.GetInterceptAsStream() < 2); Prefer EXPECT_LT, since it prints out more useful information on failure. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:284: // always returns false. nit: "as plugins are disabled" https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:285: #if !defined(OS_ANDROID) Can we use "#if defined(ENABLE_PLUGINS)" instead (And update the comment?). Seems much better to gate on the feature than the platform.
Oh, and LGTM - guess you're in a rather different timezone than me, so holding off signoff for a quick re-review of your responses may be heavier weight than I was thinking. Thanks for the tests! And be sure to wait for raymes to sign off again.
@mmenke Thanks for review. yes, I am in Indian Time zone. Changes done as suggested. Thanks https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:129: int intercepted_as_stream_count; On 2015/04/07 14:52:51, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:167: TestFakePluginService(bool plugin_available, bool is_plugin_stale) On 2015/04/07 14:52:51, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:193: base::MessageLoop::current()->message_loop_proxy()); On 2015/04/07 14:52:51, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:278: EXPECT_TRUE(host.GetInterceptAsStream() < 2); On 2015/04/07 14:52:51, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:278: EXPECT_TRUE(host.GetInterceptAsStream() < 2); On 2015/04/07 14:52:51, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:284: // always returns false. On 2015/04/07 14:52:51, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/260001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:285: #if !defined(OS_ANDROID) On 2015/04/07 14:52:51, mmenke wrote: Done.
(Still LGTM, just 2 nits) https://codereview.chromium.org/953793003/diff/280001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/280001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:118: const int intercepted_as_stream_count() { nit: const in wrong place. Line should be: int intercepted_as_stream_count() const { https://codereview.chromium.org/953793003/diff/280001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:202: // plugin is ready to use. I don't think this is clear, and documentation should generally be above public methods for consumers, rather than private member variables. Instead of this suggest, above constructor, something like: "If |is_plugin_stale| is true, GetPluginInfo will indicate the plugins are stale until GetPlugins is called."
Changes done. Thanks https://codereview.chromium.org/953793003/diff/280001/content/browser/loader/... File content/browser/loader/buffered_resource_handler_unittest.cc (right): https://codereview.chromium.org/953793003/diff/280001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:118: const int intercepted_as_stream_count() { On 2015/04/07 15:33:24, mmenke wrote: Done. https://codereview.chromium.org/953793003/diff/280001/content/browser/loader/... content/browser/loader/buffered_resource_handler_unittest.cc:202: // plugin is ready to use. On 2015/04/07 15:33:24, mmenke wrote: Done.
The CQ bit was checked by deepak.m1@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, raymes@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/953793003/#ps300001 (title: "Addressing nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/953793003/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/d7d1167121bfe4b80d2cdf7559171aeff25c02c6 Cr-Commit-Position: refs/heads/master@{#324187}
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1081013002/ by deepak.m1@samsung.com. The reason for reverting is: Need to fix some more logic for r475390. Once that will get fixed , I will re land this patch again.. |
