Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
404803004:
Cleaned up the code for suppressed messages (Closed)
DescriptionCleaned up the code for suppressed messages
Permissions warning messages are suprressed around the code which make it hard to know which supress what. Put them in one DS the suppression is done in a single loop. This help visualize which suppress what and a neater place to add ones later.
BUG=384487
R=kalman, meacer
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284771
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressing kalman comments #
Total comments: 4
Patch Set 3 : Additional kalman comments addressed #
Total comments: 4
Patch Set 4 : Addressing meacer comments #Patch Set 5 : Fixing the android failure #Messages
Total messages: 19 (0 generated)
meacer@ can you please review this for me.
looks fine, a bunch of nits https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (left): https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:107: // Both tabs and history already allow reading favicons. but maybe you should retain these comments on that object? https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:139: SuppressMessage(messages, this change isn't obvious, and it doesn't seem related to this CL. what is the effect? https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:84: PermissionMessage::ID> SuppressList = { should be kSuppressList I suppose? https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:85: // Some warnings are more generic and/or powerful and superseed other this comment applies to the whole variable right, not to the Bookmarks suppression specifically? could you move it out... and actually describe what the mapping means? does the former suppress the latter, or do they suppress each other, or...? https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:98: PermissionMessage::kFileSystemDirectory}}; can you sort this list?
kalman@ hopefully things are better this round. https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (left): https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:107: // Both tabs and history already allow reading favicons. On 2014/07/18 20:17:26, kalman wrote: > but maybe you should retain these comments on that object? Done. https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:139: SuppressMessage(messages, On 2014/07/18 20:17:25, kalman wrote: > this change isn't obvious, and it doesn't seem related to this CL. what is the > effect? This was added in another CL when showing the messages for devices access (bug 384353). https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:84: PermissionMessage::ID> SuppressList = { On 2014/07/18 20:17:25, kalman wrote: > should be kSuppressList I suppose? Done. https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:85: // Some warnings are more generic and/or powerful and superseed other On 2014/07/18 20:17:25, kalman wrote: > this comment applies to the whole variable right, not to the Bookmarks > suppression specifically? could you move it out... and actually describe what > the mapping means? does the former suppress the latter, or do they suppress each > other, or...? Done. https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:98: PermissionMessage::kFileSystemDirectory}}; On 2014/07/18 20:17:25, kalman wrote: > can you sort this list? Done.
lgtm https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (left): https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:139: SuppressMessage(messages, On 2014/07/18 21:29:12, mhm wrote: > On 2014/07/18 20:17:25, kalman wrote: > > this change isn't obvious, and it doesn't seem related to this CL. what is the > > effect? > > This was added in another CL when showing the messages for devices access (bug > 384353). I mean that the other changes you've made are in GetPermissionMessages and are clearly a no-op logic wise, but this isn't clearly a no-op because it's in a different function. I don't really know why it's here in the first place though, seems like all the other SuppressMessage calls are in GetPermissionMessages. https://codereview.chromium.org/404803004/diff/20001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/404803004/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:94: // DeclarativeWebRequest. comment is in the wrong place https://codereview.chromium.org/404803004/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:96: // kFileSystemDirectory as the write directory message implies it. comment could be re-worded to be more consistent with the others
> https://codereview.chromium.org/404803004/diff/1/chrome/common/extensions/per... > chrome/common/extensions/permissions/chrome_permission_message_provider.cc:139: > SuppressMessage(messages, > On 2014/07/18 21:29:12, mhm wrote: > > On 2014/07/18 20:17:25, kalman wrote: > > > this change isn't obvious, and it doesn't seem related to this CL. what is > the > > > effect? > > > > This was added in another CL when showing the messages for devices access (bug > > 384353). > > I mean that the other changes you've made are in GetPermissionMessages and are > clearly a no-op logic wise, but this isn't clearly a no-op because it's in a > different function. I don't really know why it's here in the first place though, > seems like all the other SuppressMessage calls are in GetPermissionMessages. All of them except two. I ran the unit tests to make sure their related warnings tests pass and found not issues.
https://codereview.chromium.org/404803004/diff/20001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/404803004/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:94: // DeclarativeWebRequest. On 2014/07/18 21:36:33, kalman wrote: > comment is in the wrong place Done. https://codereview.chromium.org/404803004/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:96: // kFileSystemDirectory as the write directory message implies it. On 2014/07/18 21:36:33, kalman wrote: > comment could be re-worded to be more consistent with the others Done.
The CQ bit was checked by mohammed@chromium.org
The CQ bit was unchecked by mohammed@chromium.org
On 2014/07/18 21:49:57, mhm wrote: > The CQ bit was unchecked by mailto:mohammed@chromium.org meacer@ any comments before I commit?
Lgtm with two comments https://codereview.chromium.org/404803004/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/404803004/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:95: // TODO(sammc): Remove this. See http://crbug.com/284849. Would it make sense to make this comment similar to the others and close this bug now? https://codereview.chromium.org/404803004/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:128: it++) { nit: ++it instead of it++
Thanks for the comments meacer@ https://codereview.chromium.org/404803004/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/404803004/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:95: // TODO(sammc): Remove this. See http://crbug.com/284849. On 2014/07/18 22:19:49, Mustafa Emre Acer wrote: > Would it make sense to make this comment similar to the others and close this > bug now? I am not sure what is needed for that bug. https://codereview.chromium.org/404803004/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:128: it++) { On 2014/07/18 22:19:49, Mustafa Emre Acer wrote: > nit: ++it instead of it++ Done.
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/404803004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30662)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
The CQ bit was checked by mohammed@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohammed@chromium.org/404803004/80001
Message was sent while issue was closed.
Change committed as 284771 |
