Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
582673002:
Fixed Build breaks while notification is enabled and extension is disabled. (Closed)
|
Created:
6 years, 3 months ago by Jitu( very slow this week) Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFixed Build breaks while notification is enabled and extension is disabled.
This patch will fixed the build errors which is due to while notification flag is
enabled and extension flag is disabled.
BUG=None
Committed: https://crrev.com/7a629442652c4df0bdf71c9d7a23f446a0418886
Cr-Commit-Position: refs/heads/master@{#296352}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fix review comments #
Total comments: 2
Patch Set 3 : fixed the comments #
Total comments: 2
Patch Set 4 : fix comments #
Total comments: 1
Patch Set 5 : Modified as chromium style #Patch Set 6 : Changed header alphabetically #Patch Set 7 : rebased #
Messages
Total messages: 28 (6 generated)
jitendra.ks@samsung.com changed reviewers: + stevenjb@chromium.org
PTAL
stevenjb@chromium.org changed reviewers: + mukai@chromium.org
+mukai@ who is more recently familiar with this code. https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:64: public extensions::ExtensionRegistryObserver Ugh, this sort of #ifdef tends to be error prone. I notice that we are still including #include "extensions/browser/extension_registry_observer.h". Can we leave the class dependency and just #ifdef out the code inside the implementation in the .cc file?
mukai@chromium.org changed reviewers: + dewittj@chromium.org
+dewittj, peter https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:64: public extensions::ExtensionRegistryObserver On 2014/09/18 17:56:08, stevenjb wrote: > Ugh, this sort of #ifdef tends to be error prone. > I notice that we are still including #include > "extensions/browser/extension_registry_observer.h". > Can we leave the class dependency and just #ifdef out the code inside the > implementation in the .cc file? I am okay with this. Factoring-out the observer would be cleaner for the coding though.
https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:64: public extensions::ExtensionRegistryObserver On 2014/09/18 18:07:13, Jun Mukai wrote: > On 2014/09/18 17:56:08, stevenjb wrote: > > Ugh, this sort of #ifdef tends to be error prone. > > I notice that we are still including #include > > "extensions/browser/extension_registry_observer.h". > > Can we leave the class dependency and just #ifdef out the code inside the > > implementation in the .cc file? > > I am okay with this. Factoring-out the observer would be cleaner for the coding > though. Seems to me that this is a temporary state of affairs anyway, since Peter is planning to move the extension code into a separate service.
https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.cc:101: #if defined(ENABLE_EXTENSION) this should be ENABLE_EXTENSIONS https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.cc:126: #if defined(ENABLE_EXTENSION) Same. https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:62: #if defined(ENABLE_EXTENSION) This should be ENABLE_EXTENSIONS https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:136: #if defined(ENABLED_EXTENSIONS) This should be ENABLE_EXTENSIONS https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:159: #if defined(ENABLED_EXTENSIONS) This should be ENABLE_EXTENSIONS
This seems fine to me % existing comments. Assuming you are doing this for Android, can you enable the compile-time flag (but don't yet offer a way to enable the Web exposed API)? We're OK with taking a ~10 kb binary size hit, and this would allow the bots to verify that this continues to work.
PTAL... https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.cc (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.cc:101: #if defined(ENABLE_EXTENSION) On 2014/09/19 17:33:05, dewittj wrote: > this should be ENABLE_EXTENSIONS Done. https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.cc:126: #if defined(ENABLE_EXTENSION) On 2014/09/19 17:33:05, dewittj wrote: > Same. Done. https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:62: #if defined(ENABLE_EXTENSION) On 2014/09/19 17:33:05, dewittj wrote: > This should be ENABLE_EXTENSIONS Done. https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:136: #if defined(ENABLED_EXTENSIONS) On 2014/09/19 17:33:05, dewittj wrote: > This should be ENABLE_EXTENSIONS Done. https://codereview.chromium.org/582673002/diff/1/chrome/browser/notifications... chrome/browser/notifications/desktop_notification_service.h:159: #if defined(ENABLED_EXTENSIONS) On 2014/09/19 17:33:05, dewittj wrote: > This should be ENABLE_EXTENSIONS Done.
https://codereview.chromium.org/582673002/diff/20001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/582673002/diff/20001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:1868: #if defined(ENABLED_EXTENSIONS) ENABLE_EXTENSIONS not ENABLED
PTAL ... Thanks https://codereview.chromium.org/582673002/diff/20001/chrome/browser/chrome_co... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/582673002/diff/20001/chrome/browser/chrome_co... chrome/browser/chrome_content_browser_client.cc:1868: #if defined(ENABLED_EXTENSIONS) On 2014/09/22 05:53:16, Jun Mukai wrote: > ENABLE_EXTENSIONS > not ENABLED Done. Thanks
sorry for commenting one by one... :-/ https://codereview.chromium.org/582673002/diff/40001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/40001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.h:25: #include "extensions/browser/extension_registry_observer.h" I think you should exclude this line by ENABLE_EXTENSIONS right? https://codereview.chromium.org/582673002/diff/40001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.h:44: class ExtensionRegistry; Also better to exclude this as well just for safety.
also it would be ideal to cleanup the #include in chrome_content_browser_client.cc, but it could be a bit tough due to other parts of that file. In that case I'm okay to leave it as is.
Hi Mukai, Fixed the comments by you. PTAL. I will be doing clean up of for the file chrome_content_browser_client.cc as part of the below bug. Which i have started working. https://code.google.com/p/chromium/issues/detail?id=414061
https://codereview.chromium.org/582673002/diff/60001/chrome/browser/notificat... File chrome/browser/notifications/desktop_notification_service.h (right): https://codereview.chromium.org/582673002/diff/60001/chrome/browser/notificat... chrome/browser/notifications/desktop_notification_service.h:25: #if defined(ENABLE_EXTENSIONS) Please move this below. Our style is: #include ... #include ... #if defined(...) #include ... #endif refer desktop_notification_service.cc which has this section.
PTAL
No, that is not what I said. Please see desktop_notification_service.cc The file should have the include sections in the order of: a. the list of #include, alphabetical order b. the list of #include guarded by a #if defined(), alphabetical order inside of #if .. #endif a and b should be separated by a blank space. Therefore, the #if defined(ENABLE_EXTENSIONS) section itself has to move below gurl.h
Thanks 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/582673002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/59384)
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/582673002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 0de4a347430ce7f39422510139d5b013f7b240ff
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7a629442652c4df0bdf71c9d7a23f446a0418886 Cr-Commit-Position: refs/heads/master@{#296352} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
