Fixing variable name typo ('Parition' -> Partition).

Fixing variable name typo ('Parition' -> Partition).

BUG=

Committed: https://crrev.com/ffab7376b57e032cd311b4893b9c764b2c0e441c
Cr-Commit-Position: refs/heads/master@{#343550}

[Savago, 2015-08-14 19:14:04]

a.cavalcanti@samsung.com changed reviewers:
+ avi@chromium.org, davidben@chromium.org, jam@chromium.org

[davidben, 2015-08-14 20:24:39]

Why did you do it this way? Could you switch it back to three variables and only
fix the typo? This version is much more error-prone. Imagine if we added another
one after MOUNT_POINTS. You have to #ifdef the index one too.

And we still need both a string name and variable name, just it's an index this
time, so it doesn't actually save on repetition.


I left some comments re style just FYI, but the old version is better, so please
don't fix them and instead leave the variables as they are, typo aside.

https://codereview.chromium.org/1284403002/diff/1/content/browser/browser_con...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1284403002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:38: const char *KeyNames[] {
"download_manager", "content_storage_partition_map"
Style: * hugs the type

https://codereview.chromium.org/1284403002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:38: const char *KeyNames[] {
"download_manager", "content_storage_partition_map"
Style: kKeyNames

https://codereview.chromium.org/1284403002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:40: , "mount_points"
No reason to do the comma like this. Trailing commas are legal in arrays.

https://codereview.chromium.org/1284403002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:158: KeyNames[MOUNT_POINTS]);
Style: 80 column limit.

[Savago, 2015-08-14 21:12:55]

David

First, thanks for the review. I will revert the code to have the 3 variables.

I noticed that kMountPointsKey is used *only* in a single function, unlike the
others. Perhaps we could just make it a local variable at the call site (thus
removing the #ifdef preprocessor statement at the top of file)?

I uploaded this change, let me know if you think it is ok (or if you prefer it
to be global).a

[davidben, 2015-08-14 21:18:34]

Please also fix the CL description.

https://codereview.chromium.org/1284403002/diff/20001/content/browser/browser...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1284403002/diff/20001/content/browser/browser...
content/browser/browser_context.cc:144: const char kMountPointsKey[] =
"mount_points";
This miiiiight accidentally work, but only because the compiler will likely
allocate the string literal in .rodata rather than put it on stack or something.

GetUserData requires that the value be the same each time. In fact, the string
is irrelevant and is mostly a debugging aid I think? (Actually I think we
usually use a slightly different pattern these days since the compiler might try
to globally deduplicate string pointers across compilation units?)

Anyway, it's the value of the pointer that's significant, not the string, and
that value needs to be the same across calls. Let's just put that back. If we
want it revised further, we'll do a separate CL.

[Savago, 2015-08-14 23:10:28]

Coolio, should be ok now.


On 2015/08/14 21:18:34, David Benjamin wrote:
> Please also fix the CL description.
> 
>
https://codereview.chromium.org/1284403002/diff/20001/content/browser/browser...
> File content/browser/browser_context.cc (right):
> 
>
https://codereview.chromium.org/1284403002/diff/20001/content/browser/browser...
> content/browser/browser_context.cc:144: const char kMountPointsKey[] =
> "mount_points";
> This miiiiight accidentally work, but only because the compiler will likely
> allocate the string literal in .rodata rather than put it on stack or
something.
> 
> GetUserData requires that the value be the same each time. In fact, the string
> is irrelevant and is mostly a debugging aid I think? (Actually I think we
> usually use a slightly different pattern these days since the compiler might
try
> to globally deduplicate string pointers across compilation units?)
> 
> Anyway, it's the value of the pointer that's significant, not the string, and
> that value needs to be the same across calls. Let's just put that back. If we
> want it revised further, we'll do a separate CL.

[davidben, 2015-08-14 23:13:07]

lgtm

[Confirmed a.cavalcanti@samsung.com is in AUTHORS and there is a CLA on file.]

[Savago, 2015-08-14 23:27:45]

The CQ bit was checked by a.cavalcanti@samsung.com

[commit-bot: I haz the power, 2015-08-14 23:28:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1284403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1284403002/40001

[commit-bot: I haz the power, 2015-08-15 02:55:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-15 02:56:49]

Patchset 3 (id:??) landed as
https://crrev.com/ffab7376b57e032cd311b4893b9c764b2c0e441c
Cr-Commit-Position: refs/heads/master@{#343550}