Content settings clean-up: Clarify resource identifier & get rid of NO_RESOURCE_IDENTIFIER.

Content settings clean-up: Clarify resource identifier & get rid of NO_RESOURCE_IDENTIFIER.

current NO_RESOURCE_IDENTIFIER usage is aganinst style guidelines.
Made a class in content_settings.h and using function to get std::string()
To get rid of NO_RESOURCE_IDENTIFIER.
Now no need to add content_settings_provider.h in places like
desktop_notification_profile_util.cc and other places.

BUG=399592
Committed: https://crrev.com/76a6893ca1e3ef72d9d371c50c20c2600cca0e60
Cr-Commit-Position: refs/heads/master@{#324362}

[Deepak, 2015-03-26 07:13:52]

deepak.m1@samsung.com changed reviewers:
+ markusheintz@chromium.org

[Deepak, 2015-03-26 07:13:53]

This is a small clean up for removing NO_RESOURCE_IDENTIFIER macro. That is
against style guidelines.
PTAL
Thanks

[markusheintz_, 2015-03-27 10:27:34]

Thanks a lot Deepak for looking into this.

Martin, Raymes do you have any preferences how we should represent "no resources
identifier"?

An empty string is treated as "no resources identifier" but it is more readable
to be explicit.

https://codereview.chromium.org/1034143002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1034143002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:180:
NoResourceIdentifier::GetResourceIdentifier());
nit: Please put each param back into a single line to stay consistent with the
rest of the file.

https://codereview.chromium.org/1034143002/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings.h (right):

https://codereview.chromium.org/1034143002/diff/1/components/content_settings...
components/content_settings/core/common/content_settings.h:62: static const
std::string GetResourceIdentifier() { return std::string(); }
Add

 private:
  DISALLOW_IMPLICIT_CONSTRUCTORS(NoResourceIdentifier);

[Deepak, 2015-03-27 11:24:06]

deepak.m1@samsung.com changed reviewers:
+ mukai@chromium.org

[Deepak, 2015-03-27 11:25:18]

deepak.m1@samsung.com changed reviewers:
+ xhwang@chromium.org

[Deepak, 2015-03-27 11:40:04]

Thanks for review.
Changes done as suggested.
PTAL
Thanks.

https://codereview.chromium.org/1034143002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1034143002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:180:
NoResourceIdentifier::GetResourceIdentifier());
On 2015/03/27 10:27:34, markusheintz_ wrote:
> nit: Please put each param back into a single line to stay consistent with the
> rest of the file.

Done.

https://codereview.chromium.org/1034143002/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings.h (right):

https://codereview.chromium.org/1034143002/diff/1/components/content_settings...
components/content_settings/core/common/content_settings.h:62: static const
std::string GetResourceIdentifier() { return std::string(); }
On 2015/03/27 10:27:34, markusheintz_ wrote:
> Add
> 
>  private:
>   DISALLOW_IMPLICIT_CONSTRUCTORS(NoResourceIdentifier);

Done.

[msramek, 2015-03-27 12:30:45]

msramek@chromium.org changed reviewers:
+ msramek@chromium.org

[msramek, 2015-03-27 12:30:46]

I agree that all those occurrences should not include provider.h just to use
this macro, and moving it to content_settings.h makes more sense.

Ironically, most of the code I saw in provider classes doesn't even use the
macro. They just use ResourceIdentifier().

Anyway, now we ended up with:

content_settings_provider.h:
typedef std::string ResourceIdentifier;

content_settings.h:
class NoResourceIdentifier;

Instead of having two separate classes for empty and non-empty identifier,
shouldn't we rather replace 

NoResourceIdentifier::GetResourceIdentifier()

with

ResourceIdentifier()

? Or to be more explicit, something along the lines of

content_settings.h:
class ResourceIdentifier(string) {
  static const ResourceIdentifier GetEmpty();
}

[raymes, 2015-03-30 00:54:13]

> Instead of having two separate classes for empty and non-empty identifier,
> shouldn't we rather replace 
> 
> NoResourceIdentifier::GetResourceIdentifier()
> 
> with
> 
> ResourceIdentifier()
> 
> ? Or to be more explicit, something along the lines of
> 
> content_settings.h:
> class ResourceIdentifier(string) {
>   static const ResourceIdentifier GetEmpty();
> }

Either of those sounds good :)

[Deepak, 2015-04-07 06:23:06]

@msramek & @raymes
Thanks for review .
I have replaced NoResourceIdentifier::GetResourceIdentifier() with
content_settings::ResourceIdentifier().
removed NoResourceIdentifier class.
PTAL.
Thanks

[msramek, 2015-04-07 12:01:40]

https://codereview.chromium.org/1034143002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/1034143002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_stream_devices_controller.cc:20: #include
"components/content_settings/core/browser/content_settings_provider.h"
Remove this inclusion?

https://codereview.chromium.org/1034143002/diff/60001/components/content_sett...
File components/content_settings/core/browser/content_settings_provider.h
(left):

https://codereview.chromium.org/1034143002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_provider.h:24: typedef
std::string ResourceIdentifier;
ResourceIdentifier is still defined here, so all those files still need to
include provider.h. Please move this to content_settings.h.

Please consider also removing this shadowing definition:
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...

[Deepak, 2015-04-07 13:08:14]

Changes done as suggested.
PTAL.

https://codereview.chromium.org/1034143002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/1034143002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_stream_devices_controller.cc:20: #include
"components/content_settings/core/browser/content_settings_provider.h"
On 2015/04/07 12:01:40, msramek wrote:

Done.

https://codereview.chromium.org/1034143002/diff/60001/components/content_sett...
File components/content_settings/core/browser/content_settings_provider.h
(left):

https://codereview.chromium.org/1034143002/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_provider.h:24: typedef
std::string ResourceIdentifier;
On 2015/04/07 12:01:40, msramek wrote:

Done.

[msramek, 2015-04-07 13:17:42]

LGTM (non-owner).

[Deepak, 2015-04-07 13:22:49]

On 2015/04/07 13:17:42, msramek wrote:

xhwang for 
chrome/browser/media/media_capture_devices_dispatcher.cc 
chrome/browser/media/media_stream_devices_controller.cc 

Jun Mukai for
desktop_notification_profile_util.cc 
platform_notification_service_impl.cc 

Bernhard Bauer for 
components/content_settings/XXX

PTAL

[Deepak, 2015-04-07 13:23:06]

deepak.m1@samsung.com changed reviewers:
+ bauerb@chromium.org

[xhwang, 2015-04-07 16:14:29]

xhwang@chromium.org changed reviewers:
+ perkj@chromium.org
- xhwang@chromium.org

[xhwang, 2015-04-07 16:15:19]

s/xhwang/perkj

perj: Please OWNERS review changes in
chrome/browser/media/media_capture_devices_dispatcher.cc 
chrome/browser/media/media_stream_devices_controller.cc

[Jun Mukai, 2015-04-07 17:54:15]

c/b/notifications lgtm

[Deepak, 2015-04-08 02:56:22]

Friendly Ping to 
perkj for 
chrome/browser/media/media_capture_devices_dispatcher.cc 
chrome/browser/media/media_stream_devices_controller.cc 

Bernhard Bauer for 
components/content_settings/XXX

[Bernhard Bauer, 2015-04-08 08:25:22]

lgtm

[perkj_chrome, 2015-04-08 10:51:15]

lgtm

https://codereview.chromium.org/1034143002/diff/100001/chrome/browser/media/m...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/1034143002/diff/100001/chrome/browser/media/m...
chrome/browser/media/media_capture_devices_dispatcher.cc:426:
CONTENT_SETTING_ALLOW) {
is it clang format that does this? git cl format? It looks wrong to me to not
have this indentet.

[Deepak, 2015-04-08 11:12:08]

On 2015/04/08 10:51:15, perkj wrote:
> lgtm
> 
>
https://codereview.chromium.org/1034143002/diff/100001/chrome/browser/media/m...
> File chrome/browser/media/media_capture_devices_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/1034143002/diff/100001/chrome/browser/media/m...
> chrome/browser/media/media_capture_devices_dispatcher.cc:426:
> CONTENT_SETTING_ALLOW) {
> is it clang format that does this? git cl format? It looks wrong to me to not
> have this indentet.

nit Addressed.
Thanks for review.

[Deepak, 2015-04-08 11:12:19]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-04-08 11:12:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, bauerb@chromium.org, mukai@chromium.org,
perkj@chromium.org
Link to the patchset: https://codereview.chromium.org/1034143002/#ps140001
(title: "Addressing nit.")

[commit-bot: I haz the power, 2015-04-08 11:13:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1034143002/140001

[commit-bot: I haz the power, 2015-04-08 11:23:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-08 11:23:57]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Deepak, 2015-04-08 11:26:38]

deepak.m1@samsung.com changed reviewers:
+ sergeyu@chromium.org

[Deepak, 2015-04-08 11:26:39]

@sergeyu
Please give approval for 
chrome/browser/media/media_capture_devices_dispatcher.cc
chrome/browser/media/media_stream_devices_controller.cc

Thanks

[Deepak, 2015-04-08 11:32:24]

On 2015/04/08 11:26:39, Deepak wrote:
@xhwang
Please give approval for 
chrome/browser/media/media_capture_devices_dispatcher.cc
chrome/browser/media/media_stream_devices_controller.cc

unfortunately with @perkj approval it is still giving presubmit error.

Thanks

[Deepak, 2015-04-08 12:11:47]

deepak.m1@samsung.com changed reviewers:
+ xhwang@chromium.org

[Sergey Ulanov, 2015-04-08 17:04:43]

chrome/browser/media - LGTM

[Deepak, 2015-04-09 02:42:09]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-04-09 02:44:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1034143002/140001

[commit-bot: I haz the power, 2015-04-09 04:06:06]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-09 04:06:48]

Patchset 8 (id:??) landed as
https://crrev.com/76a6893ca1e3ef72d9d371c50c20c2600cca0e60
Cr-Commit-Position: refs/heads/master@{#324362}