Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
591063003:
Split ChromeContentBrowserClient into smaller parts. (Closed)
|
Created:
6 years, 3 months ago by Jitu( very slow this week) Modified:
6 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSplit ChromeContentBrowserClient into smaller parts.
In this patch refactor the code and move some plugin related code to ChromeContentBrowserClientPluginPart.
BUG=414061
Committed: https://crrev.com/42e889aec29b5b68cf87b17e58fb1cb0303f110c
Cr-Commit-Position: refs/heads/master@{#307003}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved to new file #Patch Set 3 : Fix build errors #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 15
Patch Set 6 : fix review comment #
Total comments: 1
Patch Set 7 : fix comments #
Total comments: 3
Patch Set 8 : Moved headers inside flag #
Total comments: 1
Patch Set 9 : #
Messages
Total messages: 26 (5 generated)
jitendra.ks@samsung.com changed reviewers: + rockot@chromium.org, thestig@chromium.org
still fill some of the code can be moved inside ChromeContentBrowserClientExtensionsPart, so added. PTAL...
On 2014/09/22 14:14:02, Jitu wrote: > still feel some of the code can be moved inside > ChromeContentBrowserClientExtensionsPart, so added. > > PTAL...
Thank you for taking this on. Looking good, but below is the reason why I didn't do this initially when I created ChromeContentBrowserClientExtensionsPart. https://codereview.chromium.org/591063003/diff/1/chrome/browser/chrome_conten... File chrome/browser/chrome_content_browser_client.cc (left): https://codereview.chromium.org/591063003/diff/1/chrome/browser/chrome_conten... chrome/browser/chrome_content_browser_client.cc:2309: if (IsExtensionOrSharedModuleWhitelisted(url, extension_set, BTW, this is only declared if defined(ENABLE_PLUGINS) is true as well. (ENABLE_PLUGINS refers to PPAPI plugins) Given all the code you are trying to move really should be covered under (defined(ENABLE_PLUGINS) && defined(ENABLE_EXTENSIONS)), would you consider in a separate file altogether and set the build rules accordingly? https://codereview.chromium.org/591063003/diff/1/chrome/browser/extensions/ch... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/591063003/diff/1/chrome/browser/extensions/ch... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:404: Profile* profile = Profile::FromBrowserContext(browser_context); I know you are just cutting+pasting code here, but ExtensionSystem::Get() actually takes a BrowserContext*, so you don't need this conversion to a Profile* here.
On 2014/09/23 01:15:11, Lei Zhang wrote: > Thank you for taking this on. Looking good, but below is the reason why I didn't > do this initially when I created ChromeContentBrowserClientExtensionsPart. > > https://codereview.chromium.org/591063003/diff/1/chrome/browser/chrome_conten... > File chrome/browser/chrome_content_browser_client.cc (left): > > https://codereview.chromium.org/591063003/diff/1/chrome/browser/chrome_conten... > chrome/browser/chrome_content_browser_client.cc:2309: if > (IsExtensionOrSharedModuleWhitelisted(url, extension_set, > BTW, this is only declared if defined(ENABLE_PLUGINS) is true as well. > (ENABLE_PLUGINS refers to PPAPI plugins) > > Given all the code you are trying to move really should be covered under > (defined(ENABLE_PLUGINS) && defined(ENABLE_EXTENSIONS)), would you consider in a > separate file altogether and set the build rules accordingly? > > https://codereview.chromium.org/591063003/diff/1/chrome/browser/extensions/ch... > File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc > (right): > > https://codereview.chromium.org/591063003/diff/1/chrome/browser/extensions/ch... > chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:404: > Profile* profile = Profile::FromBrowserContext(browser_context); > I know you are just cutting+pasting code here, but ExtensionSystem::Get() > actually takes a BrowserContext*, so you don't need this conversion to a > Profile* here. Thanks... I ll create a separate file and do the changes.
Patchset #2 (id:20001) has been deleted
PTAL
On 2014/09/26 14:16:44, Jitu (slow this week) wrote: > PTAL Patch set 2 either doesn't compile or fails many browser_tests.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Dear Lei Zhang, I just tried to add the same code in different files. This is compiling fine. But failing in browser_tests. For ex its failing giving below logs 18200:18200:1013/192238:INFO:CONSOLE(275)] "19:22:38.600 - org.chromium.externalclearkey.initializefail is not a known key system", source: http://127.0.0.1:60852/files/eme_player_js/utils.js (275) [18200:18200:1013/192238:INFO:CONSOLE(275)] "19:22:38.602 - Using PrefixedClearKeyPlayer", source: http://127.0.0.1:60852/files/eme_player_js/utils.js (275) [18200:18200:1013/192238:INFO:CONSOLE(275)] "19:22:38.602 - Registering video event handlers.", source: http://127.0.0.1:60852/files/eme_player_js/utils.js (275) [18200:18200:1013/192238:INFO:CONSOLE(275)] "19:22:38.603 - Loading media using src.", source: http://127.0.0.1:60852/files/eme_player_js/utils.js (275) [18200:18200:1013/192238:INFO:CONSOLE(275)] "19:22:38.634 - org.chromium.externalclearkey.initializefail Generate key request, initData: 30313233343536373839303132333435", source: http://127.0.0.1:60852/files/eme_player_js/utils.js (275). Can you please help me out in this. Just guide me if i am doing anything wrong here.
(Whoops, this CL fell through the cracks! Sorry for the late reply.) It's not immediately obvious what's wrong, but if you are just moving things over, it *should* work because it's effectively the same code, just re-organized. https://codereview.chromium.org/591063003/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/591063003/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:776: plugin_parts_->RenderProcessWillLaunch(host); The goal is to get rid of the ifdefs in this file, so this will "naturally" happen below with: for (size_t i = 0; i < extra_parts_.size(); ++i) extra_parts_[i]->RenderProcessWillLaunch(host); https://codereview.chromium.org/591063003/diff/160001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.h (right): https://codereview.chromium.org/591063003/diff/160001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.h:345: plugins::ChromeContentBrowserClientPluginsPart* plugin_parts_; ChromeContentBrowserClientPluginsPart should be a subclass of ChromeContentBrowserClientParts, and added to |extra_parts_| instead of being in a separate |plugin_parts_|.
sorry for late response. PTAL
looking good... https://codereview.chromium.org/591063003/diff/200001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2580: if (CommandLine::ForCurrentProcess()->HasSwitch( Move this into chrome_content_browser_client_plugins_part.cc as well? https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. ditto, no (c) https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:21: using namespace extensions; Please don't do this, it's against the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces -> search for "forbidden" https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:34: int id = host->GetID(); You can just put this in line 36, since the various isn't being reused like it was before. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:45: const extensions::ExtensionSet* extension_set = NULL; I don't think there's any standard build configurations that has plugin enabled but not extensions, though this can change in the future. I recommend putting the extensions bits in this file inside #defines. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.h (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.h:14: // Implements the extensions portion of ChromeContentBrowserClient. comment needs to be updated
Fixed the comments PTAL https://codereview.chromium.org/591063003/diff/200001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2580: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2014/12/04 22:59:43, Lei Zhang wrote: > Move this into chrome_content_browser_client_plugins_part.cc as well? Done. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/12/04 22:59:43, Lei Zhang wrote: > ditto, no (c) Done. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:21: using namespace extensions; On 2014/12/04 22:59:43, Lei Zhang wrote: > Please don't do this, it's against the style guide. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces -> > search for "forbidden" Thanks Done https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:34: int id = host->GetID(); On 2014/12/04 22:59:43, Lei Zhang wrote: > You can just put this in line 36, since the various isn't being reused like it > was before. Done. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:45: const extensions::ExtensionSet* extension_set = NULL; On 2014/12/04 22:59:43, Lei Zhang wrote: > I don't think there's any standard build configurations that has plugin enabled > but not extensions, though this can change in the future. I recommend putting > the extensions bits in this file inside #defines. So for in this function you mean to say if extensions is not defined it should return false. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.h (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/12/04 22:59:43, Lei Zhang wrote: > nit: no (c) Done. https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.h:14: // Implements the extensions portion of ChromeContentBrowserClient. On 2014/12/04 22:59:43, Lei Zhang wrote: > comment needs to be updated Done.
https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/200001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:45: const extensions::ExtensionSet* extension_set = NULL; On 2014/12/05 05:39:31, Jitu wrote: > On 2014/12/04 22:59:43, Lei Zhang wrote: > > I don't think there's any standard build configurations that has plugin > enabled > > but not extensions, though this can change in the future. I recommend putting > > the extensions bits in this file inside #defines. > > So for in this function you mean to say if extensions is not defined it should > return false. Sure, and you need to look through the other functions, and the #includes too. https://codereview.chromium.org/591063003/diff/220001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/220001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:55: #endif #else return false; #endif Otherwise I worry this might confuse some tools.
PTAL https://codereview.chromium.org/591063003/diff/240001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/240001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:135: #endif While i have tried like here #else chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); // Allow dev channel APIs to be used on "Canary", "Dev", and "Unknown" // releases of Chrome. Permitting "Unknown" allows these APIs to be used on // Chromium builds as well. return channel <= chrome::VersionInfo::CHANNEL_DEV; #endif It gives error chrome_content_browser_client_plugins_part.cc:142:1: error: control may reach end of non-void function [-Werror,-Wreturn-type] so i have changed like above. Anyway this function is called only when ENABLE_PLUGINS and ENABLE_EXTENSIONS are enabled. So above piece of code after #endif will trigger for ENABLE_PLUGINS.
lgtm https://codereview.chromium.org/591063003/diff/240001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/240001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:8: #include "chrome/browser/extensions/extension_service.h" Can you put all the extensions includes inside #if defined(ENABLE_EXTENSIONS) as well?
PTAL https://codereview.chromium.org/591063003/diff/240001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/240001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:8: #include "chrome/browser/extensions/extension_service.h" On 2014/12/05 06:58:51, Lei Zhang wrote: > Can you put all the extensions includes inside #if defined(ENABLE_EXTENSIONS) as > well? Done.
https://codereview.chromium.org/591063003/diff/260001/chrome/browser/plugins/... File chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc (right): https://codereview.chromium.org/591063003/diff/260001/chrome/browser/plugins/... chrome/browser/plugins/chrome_content_browser_client_plugins_part.cc:8: #include "chrome/browser/extensions/extension_service.h" You forgot this guy
sorry i overlooked it :) PTAL
lgtm++
The CQ bit was checked by jitendra.ks@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/591063003/280001
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/42e889aec29b5b68cf87b17e58fb1cb0303f110c Cr-Commit-Position: refs/heads/master@{#307003} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
