Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
499733002:
Fixed javascript_dialog_manager to compile when extensions are disabled. (Closed)
DescriptionFixed javascript_dialog_manager to compile when extensions are disabled.
BUG=None
TEST= trybots
Committed: https://crrev.com/950d9641877a2e2b0e692a26634e12f574e45eb5
Cr-Commit-Position: refs/heads/master@{#291575}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Completely #if guarded entire GetExtensionsProcessManager function #
Total comments: 7
Patch Set 3 : Fixed up extension macros even more #
Total comments: 1
Patch Set 4 : Converted Increment/DecrementLazyKeepaliveCount() functions to noop #Messages
Total messages: 17 (0 generated)
I ran into this when compiling for android, please take a look.
lgtm with an optional nit. https://codereview.chromium.org/499733002/diff/1/chrome/browser/ui/app_modal_... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/1/chrome/browser/ui/app_modal_... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:67: #endif // defined(ENABLE_EXTENSIONS) nit: "// ENABLE_EXTENSIONS" would follow the more common general commenting pattern for pre-processor block termination. That said, apparently this particular pre-processor definition more commonly follows your pattern here.
https://codereview.chromium.org/499733002/diff/1/chrome/browser/ui/app_modal_... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/1/chrome/browser/ui/app_modal_... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:67: #endif // defined(ENABLE_EXTENSIONS) On 2014/08/22 22:21:50, msw wrote: > nit: "// ENABLE_EXTENSIONS" would follow the more common general commenting > pattern for pre-processor block termination. That said, apparently this > particular pre-processor definition more commonly follows your pattern here. Done. I went ahead and fixed all the other #endif // defined(ENABLE_EXTENSIONS)" in this file to match the general pattern.
+thestig FYI, he has been cleaning up Android compile for extensions
https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:59: #if defined(ENABLE_EXTENSIONS) I'm actually not sure what build errors you'd see by omitting these guards. Further, these shoudn't even be called if |GetExtensionForWebContents| is returning NULL and blocking the calls below. What am I missing?
There's a similar CL here BTW: https://codereview.chromium.org/478473003/
https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:21: #include "extensions/browser/extension_system.h" You should #ifdef these two files to flush out any other extension code in the file with compile errors. https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:51: #endif // ENABLE_EXTENSIONS nit: I think most people do // defined(FOO) https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:186: const Extension* extension = GetExtensionForWebContents(web_contents); As I mentioned on the other CL, maybe pull all the GetExtensionForWebContents() calls into {Incre,Decre}mentLazyKeepaliveCount() ?
On 2014/08/22 22:36:52, Lei Zhang wrote: > There's a similar CL here BTW: https://codereview.chromium.org/478473003/ Ah, it is as you say. I will close this issue.
Message was sent while issue was closed.
On 2014/08/22 22:40:47, David Yen wrote: > On 2014/08/22 22:36:52, Lei Zhang wrote: > > There's a similar CL here BTW: https://codereview.chromium.org/478473003/ > > Ah, it is as you say. I will close this issue. I wasn't saying close this issue. I think you are slightly ahead of azarchs@ and you have the time zone difference advantage.
re-opened and applied Lei Zhang's suggestions. GetExtensionForWebContents() stopped compiling after the headers were taken out so I just #ifdef'ed out all 4 helper functions for the extensions. https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:51: #endif // ENABLE_EXTENSIONS On 2014/08/22 22:39:50, Lei Zhang wrote: > nit: I think most people do // defined(FOO) Done. https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:186: const Extension* extension = GetExtensionForWebContents(web_contents); On 2014/08/22 22:39:50, Lei Zhang wrote: > As I mentioned on the other CL, maybe pull all the GetExtensionForWebContents() > calls into {Incre,Decre}mentLazyKeepaliveCount() ? Done.
https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/20001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:59: #if defined(ENABLE_EXTENSIONS) On 2014/08/22 22:36:10, msw wrote: > I'm actually not sure what build errors you'd see by omitting these guards. > Further, these shoudn't even be called if |GetExtensionForWebContents| is > returning NULL and blocking the calls below. What am I missing? Even though technically it can never be called, there is a linker error on pm->IncrementalLazyKeepaliveCount() and pm->DecrementLazyKeepaliveCount().
https://codereview.chromium.org/499733002/diff/40001/chrome/browser/ui/app_mo... File chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc (right): https://codereview.chromium.org/499733002/diff/40001/chrome/browser/ui/app_mo... chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.cc:58: void IncrementLazyKeepaliveCount(WebContents* web_contents) { This can be a no-op function when extensions are disabled, but it needs to be defined. Then you don't need to wrap all the callers in an #ifdef.
Otherwise LGTM
The CQ bit was checked by dyen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dyen@chromium.org/499733002/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as b187cbeb83889a31f0ba5b634dcc0a4268cbf662
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/950d9641877a2e2b0e692a26634e12f574e45eb5 Cr-Commit-Position: refs/heads/master@{#291575} |
