Added 'starred' to EntryProperty in FileManagerPrivateAPI.

Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show 'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeStarredPropertyInitialized

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.

Committed: https://crrev.com/68666d15026e666c18587cf73b560cf1076b818a
Cr-Commit-Position: refs/heads/master@{#421802}

[harukam, 2016-09-16 11:40:48]

harukam@google.com changed reviewers:
+ fukino@chromium.org

[harukam, 2016-09-16 11:40:49]

fukino-san, PTAL.

[fukino, 2016-09-27 06:24:23]

https://codereview.chromium.org/2346873004/diff/1/components/drive/drive.proto
File components/drive/drive.proto (right):

https://codereview.chromium.org/2346873004/diff/1/components/drive/drive.prot...
components/drive/drive.proto:113: optional bool starred = 12;
I guess tag number 12 was used before, so reusing it may problematic.
Let's use 20 just in case.

[harukam, 2016-09-27 08:12:08]

fukino-san, I changed the 'starred' tag number to a fresh one. Please take
another look.

https://codereview.chromium.org/2346873004/diff/1/components/drive/drive.proto
File components/drive/drive.proto (right):

https://codereview.chromium.org/2346873004/diff/1/components/drive/drive.prot...
components/drive/drive.proto:113: optional bool starred = 12;
On 2016/09/27 06:24:23, fukino wrote:
> I guess tag number 12 was used before, so reusing it may problematic.
> Let's use 20 just in case.

Done.

[fukino, 2016-09-27 08:20:15]

+hashimoto@ 

looking good so far, but EntryProperties.starred will always be false if
metadata are cached in DB.
Let's find a way to re-fetch the metadata after we add starred property.

[hashimoto, 2016-09-27 08:35:04]

On 2016/09/27 08:20:15, fukino wrote:
> +hashimoto@ 
> 
> looking good so far, but EntryProperties.starred will always be false if
> metadata are cached in DB.
> Let's find a way to re-fetch the metadata after we add starred property.

FYI you can use ResourceEntry::has_starred() to know whether an entry has the
property (i.e. starred==false) or not (i.e. starred is not stored for an entry
in DB).

[harukam, 2016-09-29 05:16:34]

Description was changed from

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*
==========

to

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once, so that we can
avoid holding local metadata which has no 'starred' property. We realized it by
using 'has_starred' flag.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeHasStarred
==========

[harukam, 2016-09-29 05:42:17]

harukam@google.com changed reviewers:
+ hashimoto@chromium.org
- fukino@chromium.org

[harukam, 2016-09-29 05:42:18]

Hashimoto-san,
thank you for advice about Drive metadata.

Could you take a look at files under components/drive?

https://codereview.chromium.org/2346873004/diff/80001/components/drive/resour...
File components/drive/resource_metadata_storage.cc (right):

https://codereview.chromium.org/2346873004/diff/80001/components/drive/resour...
components/drive/resource_metadata_storage.cc:634: if (resource_map_) {
if (resource_map_) { ... } is needed to work correctly even in the case such as
ResourceMetadataStorageTest.WrongPath
Is this an appropriate way?

[hashimoto, 2016-09-29 08:43:43]

Looking good.
Thank you for doing this!

Also, please make sure that the intended behavior actually happens and the
change list is reloaded for an old DB.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
File components/drive/resource_metadata_storage.cc (right):

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage.cc:637: if (GetHeader(&header) ==
FILE_ERROR_OK)
GetHeader() shouldn't fail here.
If it fails, it's better to return false to abort.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage.cc:641: FileError error =
SetLargestChangestamp(0);
Instead of SetLargestChangestamp() & PutHeader(), you can do the same thing with
a single PutHeader() (SetLargestChangestamp is just a wrapper of PutHeader).

header.largest_changestamp(0);
header.set_starred_proeprty_initialized(true);
error = PutHeader(header);

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage.cc:644: PutHeader(header);
Please check for an error here.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
File components/drive/resource_metadata_storage_unittest.cc (right):

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage_unittest.cc:55: void
GetStarredPropertyInitialized(bool* value) {
How about returning the value?
  bool GetStarredPropertyInitialized()

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage_unittest.cc:663:
EXPECT_TRUE(starred_property_initialized);
Please check that the largest changestamp becomes 0.

[harukam, 2016-09-29 09:39:46]

Hashimoto-san, thanks for reviewing.
Please take another look.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
File components/drive/resource_metadata_storage.cc (right):

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage.cc:637: if (GetHeader(&header) ==
FILE_ERROR_OK)
On 2016/09/29 08:43:43, hashimoto wrote:
> GetHeader() shouldn't fail here.
> If it fails, it's better to return false to abort.

Acknowledged.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage.cc:641: FileError error =
SetLargestChangestamp(0);
On 2016/09/29 08:43:43, hashimoto wrote:
> Instead of SetLargestChangestamp() & PutHeader(), you can do the same thing
with
> a single PutHeader() (SetLargestChangestamp is just a wrapper of PutHeader).
> 
> header.largest_changestamp(0);
> header.set_starred_proeprty_initialized(true);
> error = PutHeader(header);

Thanks.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage.cc:644: PutHeader(header);
On 2016/09/29 08:43:43, hashimoto wrote:
> Please check for an error here.

Done.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
File components/drive/resource_metadata_storage_unittest.cc (right):

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage_unittest.cc:55: void
GetStarredPropertyInitialized(bool* value) {
On 2016/09/29 08:43:43, hashimoto wrote:
> How about returning the value?
>   bool GetStarredPropertyInitialized()

I chose passe-by-pointer like as GetLargestChangestamp(int64_t* val), but
passing-by-value seems to be simpler in this case. Acknowledged.

https://codereview.chromium.org/2346873004/diff/120001/components/drive/resou...
components/drive/resource_metadata_storage_unittest.cc:663:
EXPECT_TRUE(starred_property_initialized);
On 2016/09/29 08:43:43, hashimoto wrote:
> Please check that the largest changestamp becomes 0.

Done.

[harukam, 2016-09-29 09:40:21]

Description was changed from

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once, so that we can
avoid holding local metadata which has no 'starred' property. We realized it by
using 'has_starred' flag.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeHasStarred
==========

to

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once, so that we can
avoid holding local metadata which has no 'starred' property. We realized it by
using 'starred_property_initialized' flag.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeHasStarred
==========

[hashimoto, 2016-09-29 09:46:06]

lgtm with nits

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
File components/drive/resource_metadata_storage.cc (right):

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
components/drive/resource_metadata_storage.cc:640:
header.set_largest_changestamp(0);
Please add a comment to describe how it works (i.e. largest changestamp == 0
means data in DB is obsolete. So data for all entries will be reloaded).

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
File components/drive/resource_metadata_storage_unittest.cc (right):

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
components/drive/resource_metadata_storage_unittest.cc:54: // Return
|storage_|'s starred_property_initialized.
nit: Returns

[fukino, 2016-09-29 10:55:16]

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

[fukino, 2016-09-29 10:55:17]

lgtm for private_api_drive.cc

[harukam, 2016-09-29 11:08:20]

harukam@google.com changed reviewers:
+ mtomasz@chromium.org

[harukam, 2016-09-29 11:08:22]

Thanks for reviewing, hashimoto-san, fukino-san.

mtomasz@, please take a look at
chrome/common/extensions/api/file_manager_private.idl

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
File components/drive/resource_metadata_storage.cc (right):

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
components/drive/resource_metadata_storage.cc:640:
header.set_largest_changestamp(0);
On 2016/09/29 09:46:06, hashimoto wrote:
> Please add a comment to describe how it works (i.e. largest changestamp == 0
> means data in DB is obsolete. So data for all entries will be reloaded).

Done.

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
File components/drive/resource_metadata_storage_unittest.cc (right):

https://codereview.chromium.org/2346873004/diff/140001/components/drive/resou...
components/drive/resource_metadata_storage_unittest.cc:54: // Return
|storage_|'s starred_property_initialized.
On 2016/09/29 09:46:06, hashimoto wrote:
> nit: Returns

Acknowledged.

[harukam, 2016-09-29 11:16:32]

Description was changed from

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once, so that we can
avoid holding local metadata which has no 'starred' property. We realized it by
using 'starred_property_initialized' flag.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeHasStarred
==========

to

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeHasStarred

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.
==========

[mtomasz, 2016-09-29 11:24:22]

lgtm!

[harukam, 2016-09-29 11:26:03]

Description was changed from

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests --gtest_filter=ResourceMetadataStorageTest.ChangeHasStarred

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.
==========

to

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests
--gtest_filter=ResourceMetadataStorageTest.ChangeStarredPropertyInitialized

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.
==========

[harukam, 2016-09-29 11:27:21]

The CQ bit was checked by harukam@google.com

[harukam, 2016-09-29 11:27:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
hashimoto@chromium.org, fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2346873004/#ps160001
(title: "Added and modified comments.")

[commit-bot: I haz the power, 2016-09-29 11:27:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 12:00:22]

Description was changed from

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests
--gtest_filter=ResourceMetadataStorageTest.ChangeStarredPropertyInitialized

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.
==========

to

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests
--gtest_filter=ResourceMetadataStorageTest.ChangeStarredPropertyInitialized

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.
==========

[commit-bot: I haz the power, 2016-09-29 12:00:27]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-29 12:02:17]

Description was changed from

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests
--gtest_filter=ResourceMetadataStorageTest.ChangeStarredPropertyInitialized

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.
==========

to

==========
Added 'starred' to EntryProperty in FileManagerPrivateAPI.

We need to get 'starred' attribute of Drive file/directory in order to show
'starred' icon in Files app.

BUG=646839
TEST=unit_tests --gtest_filter=ResourceEntryConversionTest*

There has been no 'starred' property in Drive API on chromeOS until before.
It's necessary to update all resources from server just once,
so that we can avoid holding an old DB which has no 'starred' property.
We realized it by using 'starred_property_initialized' flag.
The below test ensures that the flag correctly changes.

TEST=unit_tests
--gtest_filter=ResourceMetadataStorageTest.ChangeStarredPropertyInitialized

It is manually tested that the change list is reloaded for an old DB,
and an updated DB gets right 'starred' values.

Committed: https://crrev.com/68666d15026e666c18587cf73b560cf1076b818a
Cr-Commit-Position: refs/heads/master@{#421802}
==========

[commit-bot: I haz the power, 2016-09-29 12:02:19]

Patchset 9 (id:??) landed as
https://crrev.com/68666d15026e666c18587cf73b560cf1076b818a
Cr-Commit-Position: refs/heads/master@{#421802}