Set root resource ID upon full feed update.

Set root resource ID upon full feed update.

We used to set the resource ID at DriveResourceMetadata's
constructor as for WAPI the resource ID for root is
a fixed string, but for Drive API it is not fixed and
was set when About resource is obtained.

This caused a trouble for maintaining the code and
this CL is trying to consolidate setting root resource
ID at one place.

Although for Drive API 'root' can be used for a convenient
alias for queries, Drive API usually returns a reference
to root directory (e.g. reference to parent folder)
as its real root directory resource ID rather than 'root'
alias, therefore we don't use it here.

BUG=152230, 152288, 157314
TEST=compiles, pass all existing tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164876

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165219

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=165303

[kochi, 2012-10-22 07:58:05]

Hi Satoru,

Could you review this?
Thanks,

[satorux1, 2012-10-22 08:03:30]

LGTM

[commit-bot: I haz the power, 2012-10-22 08:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/11227020/1

[achuithb, 2012-10-22 09:27:56]

https://chromiumcodereview.appspot.com/11227020/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

https://chromiumcodereview.appspot.com/11227020/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/drive/drive_resource_metadata.h:64: const char
kDriveRootDirectoryResourceId[] = "folder:root";
We should probably move this to the cc file in anonymous scope, and also add a
comment that this is only used for v1 api.

[kochi, 2012-10-23 06:08:46]

I found that InitFromDB()'s usage of root resource ID is
problematic with the old change and worked on more complete
change.

Now you have to call InitializeRootEntry() after
an DriveResourceMetadata instance is created to pass the
root ID, then the class remembers it in |saved_root_resource_id_| to reuse in
InitFromDB() to
lookup the saved root directory.

Satoru, Achuith,
Could you take another look?

Thanks,

http://codereview.chromium.org/11227020/diff/1/chrome/browser/chromeos/drive/...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11227020/diff/1/chrome/browser/chromeos/drive/...
chrome/browser/chromeos/drive/drive_resource_metadata.h:64: const char
kDriveRootDirectoryResourceId[] = "folder:root";
On 2012/10/22 09:27:56, achuith.bhandarkar wrote:
> We should probably move this to the cc file in anonymous scope, and also add a
> comment that this is only used for v1 api.

Done.

[satorux1, 2012-10-23 06:54:50]

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:54: const char
kWAPIRootDirectoryResourceId[] = "folder:root";
I think it's better to keep the constant in drive_resource_metadata.h, as the
constant is used in two different files.

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:222:
saved_root_resource_id_ = root_id;
do we need this? I think we can get the same value by root_->resource_id().

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:712: } else if
(entry->resource_id() == saved_root_resource_id_) {
why not root_->resource_id() ?

[kochi, 2012-10-23 07:44:35]

Thanks for the review!

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_file_system_unittest.cc (right):

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_file_system_unittest.cc:54: const char
kWAPIRootDirectoryResourceId[] = "folder:root";
On 2012/10/23 06:54:50, satorux1 wrote:
> I think it's better to keep the constant in drive_resource_metadata.h, as the
> constant is used in two different files.

Done.

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:222:
saved_root_resource_id_ = root_id;
On 2012/10/23 06:54:50, satorux1 wrote:
> do we need this? I think we can get the same value by root_->resource_id().

This was introduced to keep the root resource ID for
InitFromDB(), as it calls ClearRoot() once to reset the
|root_| then restores |root_| from saved serialized data.
But now I realized that I can just save the ID before
ClearRoot() and use that ID in InitResourceMap().

Probably I'll have to handle offline case for Drive API,
when we can't get the root resource ID from About metadata
and has to restore metadata cache from saved DB.

http://codereview.chromium.org/11227020/diff/2009/chrome/browser/chromeos/dri...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:712: } else if
(entry->resource_id() == saved_root_resource_id_) {
On 2012/10/23 06:54:50, satorux1 wrote:
> why not root_->resource_id() ?

As mentioned in the previous comment.
We already called ClearRoot().

[satorux1, 2012-10-23 08:06:26]

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:269: const std::string
root_resource_id() const;
Drop the first const.

Besides, this seems to be only used for testing. If so, please rename this to
GetRootResourceIdForTesting()

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right):

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:30: const char
kTestRootDirectoryResourceId[] = "folder:fakeroot";
I guess we don't need this. What's wrong with using
kWAPIRootDirectoryResourceId?

[kochi, 2012-10-23 08:21:21]

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:269: const std::string
root_resource_id() const;
On 2012/10/23 08:06:26, satorux1 wrote:
> Drop the first const.
Done.

> Besides, this seems to be only used for testing. If so, please rename this to
> GetRootResourceIdForTesting()

No, this is used for
http://codereview.chromium.org/11230025/

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right):

http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:30: const char
kTestRootDirectoryResourceId[] = "folder:fakeroot";
On 2012/10/23 08:06:26, satorux1 wrote:
> I guess we don't need this. What's wrong with using
> kWAPIRootDirectoryResourceId?

The idea is that we should not assume that the root
resource id is constant "folder:fakeroot".

If we give "folder:root", some tests ran just fine, while
if I turn on --enable-drive-v2-api it failed immediately.
So using a different ID is useful.

I would also like to change drive_file_system_unittest.cc
as well to use the different constant.

[satorux1, 2012-10-23 08:28:28]

On 2012/10/23 08:21:21, Takayoshi Kochi wrote:
>
http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
> File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):
> 
>
http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
> chrome/browser/chromeos/drive/drive_resource_metadata.h:269: const std::string
> root_resource_id() const;
> On 2012/10/23 08:06:26, satorux1 wrote:
> > Drop the first const.
> Done.
> 
> > Besides, this seems to be only used for testing. If so, please rename this
to
> > GetRootResourceIdForTesting()
> 
> No, this is used for
> http://codereview.chromium.org/11230025/

I see.


> 
>
http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
> File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc
(right):
> 
>
http://codereview.chromium.org/11227020/diff/12008/chrome/browser/chromeos/dr...
> chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:30: const
char
> kTestRootDirectoryResourceId[] = "folder:fakeroot";
> On 2012/10/23 08:06:26, satorux1 wrote:
> > I guess we don't need this. What's wrong with using
> > kWAPIRootDirectoryResourceId?
> 
> The idea is that we should not assume that the root
> resource id is constant "folder:fakeroot".
> 
> If we give "folder:root", some tests ran just fine, while
> if I turn on --enable-drive-v2-api it failed immediately.
> So using a different ID is useful.
> 
> I would also like to change drive_file_system_unittest.cc
> as well to use the different constant.

I think you can do it in this patch?

[satorux1, 2012-10-23 09:04:41]

I rethought about the root ID business, and am becoming to think that the
current approach is convoluted. 

1) Calling InitializeRootEntry() in two very different places for old and new
APIs is tricky
2) Calling InitializeRootEntry() in the middle of parsing AboutResource is
awkward (for new API)
3) Passing a fake root ID like "orphanedroot" in drive_feed_processor.cc looks
like a hack
4) Exposing root_id() from DriveResourceMetadata seems to be unnecessary (used
for http://codereview.chromium.org/11230025/)


That said, I think the following approach would be cleaner. What do you think?

1) Remove root_id parameter from InitializeRootEntry()
2) Call InitializeRootEntry() always from DriveResourceMetadata's constructor
3) Make InitializeRootEntry() private
4) Add DriveResourceMetadata::SetRootResourceId()
5) Call SetRootResourceId() when the initial loading from the server finishes
for both old and new APIs

5) is tricky but I think we can do it.

[kochi, 2012-10-23 16:14:00]

On 2012/10/23 09:04:41, satorux1 wrote:
> I rethought about the root ID business, and am becoming to think that the
> current approach is convoluted. 
> 
> 1) Calling InitializeRootEntry() in two very different places for old and new
> APIs is tricky
> 2) Calling InitializeRootEntry() in the middle of parsing AboutResource is
> awkward (for new API)
> 3) Passing a fake root ID like "orphanedroot" in drive_feed_processor.cc looks
> like a hack
> 4) Exposing root_id() from DriveResourceMetadata seems to be unnecessary (used
> for http://codereview.chromium.org/11230025/)
> 
> 
> That said, I think the following approach would be cleaner. What do you think?
> 
> 1) Remove root_id parameter from InitializeRootEntry()
> 2) Call InitializeRootEntry() always from DriveResourceMetadata's constructor
> 3) Make InitializeRootEntry() private
> 4) Add DriveResourceMetadata::SetRootResourceId()
> 5) Call SetRootResourceId() when the initial loading from the server finishes
> for both old and new APIs
> 
> 5) is tricky but I think we can do it.

I was also thinking that 1),  we call InitializeRootEntry() in very different
place
for WAPI/Drive API, is problematic.  So I'm for changing the code to align these
calls so that one won't break the other.

I'll work on this.  Please wait for a while.

[satorux1, 2012-10-24 01:30:25]

On 2012/10/23 16:14:00, Takayoshi Kochi wrote:
> On 2012/10/23 09:04:41, satorux1 wrote:
> > I rethought about the root ID business, and am becoming to think that the
> > current approach is convoluted. 
> > 
> > 1) Calling InitializeRootEntry() in two very different places for old and
new
> > APIs is tricky
> > 2) Calling InitializeRootEntry() in the middle of parsing AboutResource is
> > awkward (for new API)
> > 3) Passing a fake root ID like "orphanedroot" in drive_feed_processor.cc
looks
> > like a hack
> > 4) Exposing root_id() from DriveResourceMetadata seems to be unnecessary
(used
> > for http://codereview.chromium.org/11230025/)
> > 
> > 
> > That said, I think the following approach would be cleaner. What do you
think?
> > 
> > 1) Remove root_id parameter from InitializeRootEntry()
> > 2) Call InitializeRootEntry() always from DriveResourceMetadata's
constructor
> > 3) Make InitializeRootEntry() private
> > 4) Add DriveResourceMetadata::SetRootResourceId()
> > 5) Call SetRootResourceId() when the initial loading from the server
finishes
> > for both old and new APIs
> > 
> > 5) is tricky but I think we can do it.
> 
> I was also thinking that 1),  we call InitializeRootEntry() in very different
> place
> for WAPI/Drive API, is problematic.  So I'm for changing the code to align
these
> calls so that one won't break the other.
> 
> I'll work on this.  Please wait for a while.

I took a look at the code, and I think a good place to set the root ID would be
DriveFeedLoader::UpdateFromFeed()

You can check if start_changestamp == 0 to see if it's a full fetch rather than
a delta fetch.

[kochi, 2012-10-26 03:44:17]

Revised the patch.
Could you take another look?

[satorux1, 2012-10-26 06:14:36]

http://codereview.chromium.org/11227020/diff/23001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_feed_loader.cc (right):

http://codereview.chromium.org/11227020/diff/23001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_feed_loader.cc:901: if (start_changestamp ==
0) {
{  // full fetch

http://codereview.chromium.org/11227020/diff/23001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11227020/diff/23001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:246: if
(!root_->resource_id().empty()) {
Do we need to check? Can we just always run the same code regardless of the
resource id being empty or not?

http://codereview.chromium.org/11227020/diff/23001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/11227020/diff/23001/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:385: extern const char kEnableDriveV2Api[];
This shouldn't be part of the patch?

[kochi, 2012-10-26 07:35:42]

Thanks for comments.

The change for addressing comments for ClearRoot()
revealed that I should have set root resource ID for
|orphaned_resource| in DriveFeedProcessor::ApplyEntryProtoMap().

https://chromiumcodereview.appspot.com/11227020/diff/23001/chrome/browser/chr...
File chrome/browser/chromeos/drive/drive_feed_loader.cc (right):

https://chromiumcodereview.appspot.com/11227020/diff/23001/chrome/browser/chr...
chrome/browser/chromeos/drive/drive_feed_loader.cc:901: if (start_changestamp ==
0) {
On 2012/10/26 06:14:36, satorux1 wrote:
> {  // full fetch

Added a bit more descriptive comment.
Please take a look.

https://chromiumcodereview.appspot.com/11227020/diff/23001/chrome/browser/chr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://chromiumcodereview.appspot.com/11227020/diff/23001/chrome/browser/chr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:246: if
(!root_->resource_id().empty()) {
On 2012/10/26 06:14:36, satorux1 wrote:
> Do we need to check? Can we just always run the same code regardless of the
> resource id being empty or not?

No. RemoveEntryFromResourceMap DCHECK()'s empty resource ID.
In some unittests DriveResourceMetadata is created and destroyed without setting
the root resource ID.

In these cases when the destructor calls ClearRoot(),
root_->resource_id().empty() is true.

That said,
251 DCHECK(resource_map_.empty());
252 resource_map_.clear();
should be out of this if() and I moved out.

https://chromiumcodereview.appspot.com/11227020/diff/23001/chrome/common/chro...
File chrome/common/chrome_switches.h (right):

https://chromiumcodereview.appspot.com/11227020/diff/23001/chrome/common/chro...
chrome/common/chrome_switches.h:385: extern const char kEnableDriveV2Api[];
On 2012/10/26 06:14:36, satorux1 wrote:
> This shouldn't be part of the patch?

Right, removed.

[satorux1, 2012-10-26 07:52:43]

http://codereview.chromium.org/11227020/diff/23001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11227020/diff/23001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:246: if
(!root_->resource_id().empty()) {
On 2012/10/26 07:35:42, Takayoshi Kochi wrote:
> On 2012/10/26 06:14:36, satorux1 wrote:
> > Do we need to check? Can we just always run the same code regardless of the
> > resource id being empty or not?
> 
> No. RemoveEntryFromResourceMap DCHECK()'s empty resource ID.
> In some unittests DriveResourceMetadata is created and destroyed without
setting
> the root resource ID.
> 
> In these cases when the destructor calls ClearRoot(),
> root_->resource_id().empty() is true.
> 
> That said,
> 251 DCHECK(resource_map_.empty());
> 252 resource_map_.clear();
> should be out of this if() and I moved out.

I think we can change ClearRoot() to do something like

// The root is not yet initialized.
if (root_->resource_id())
  return;

http://codereview.chromium.org/11227020/diff/38010/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

http://codereview.chromium.org/11227020/diff/38010/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:172: void
SetRootResourceId(const std::string& id);
Let's rename this back to InitializeRootEntry()

I forgot that this function also adds the root entry to the resource map.

[kochi, 2012-10-26 09:01:23]

ClearRoot() is also modified.

https://chromiumcodereview.appspot.com/11227020/diff/38010/chrome/browser/chr...
File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):

https://chromiumcodereview.appspot.com/11227020/diff/38010/chrome/browser/chr...
chrome/browser/chromeos/drive/drive_resource_metadata.h:172: void
SetRootResourceId(const std::string& id);
On 2012/10/26 07:52:44, satorux1 wrote:
> Let's rename this back to InitializeRootEntry()
> 
> I forgot that this function also adds the root entry to the resource map.

Done.

[kochi, 2012-10-26 09:24:58]

On 2012/10/26 09:01:23, Takayoshi Kochi wrote:
> ClearRoot() is also modified.
> 
>
https://chromiumcodereview.appspot.com/11227020/diff/38010/chrome/browser/chr...
> File chrome/browser/chromeos/drive/drive_resource_metadata.h (right):
> 
>
https://chromiumcodereview.appspot.com/11227020/diff/38010/chrome/browser/chr...
> chrome/browser/chromeos/drive/drive_resource_metadata.h:172: void
> SetRootResourceId(const std::string& id);
> On 2012/10/26 07:52:44, satorux1 wrote:
> > Let's rename this back to InitializeRootEntry()
> > 
> > I forgot that this function also adds the root entry to the resource map.
> 
> Done.

Updated subject and description as well, to match the change.

[satorux1, 2012-10-29 05:28:35]

http://codereview.chromium.org/11227020/diff/48001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_feed_processor.cc (right):

http://codereview.chromium.org/11227020/diff/48001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_feed_processor.cc:97:
orphaned_resources->InitializeRootEntry("orphan");
This code should be unnecessary. orphaned resource stuff was removed by achuith.

http://codereview.chromium.org/11227020/diff/48001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

http://codereview.chromium.org/11227020/diff/48001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:812: // Root resource
ID is already set from proto.
Maybe:

// Call AddEntryToResourceMap() instead of InitializeRootEntry() as the root
resource ID is already set from proto.

[kochi, 2012-10-29 05:52:39]

Fixed for comments.

This still fails unit_tests for me.
Please hold on review.

https://chromiumcodereview.appspot.com/11227020/diff/48001/chrome/browser/chr...
File chrome/browser/chromeos/drive/drive_feed_processor.cc (right):

https://chromiumcodereview.appspot.com/11227020/diff/48001/chrome/browser/chr...
chrome/browser/chromeos/drive/drive_feed_processor.cc:97:
orphaned_resources->InitializeRootEntry("orphan");
On 2012/10/29 05:28:35, satorux1 wrote:
> This code should be unnecessary. orphaned resource stuff was removed by
achuith.

Removed.

https://chromiumcodereview.appspot.com/11227020/diff/48001/chrome/browser/chr...
File chrome/browser/chromeos/drive/drive_resource_metadata.cc (right):

https://chromiumcodereview.appspot.com/11227020/diff/48001/chrome/browser/chr...
chrome/browser/chromeos/drive/drive_resource_metadata.cc:812: // Root resource
ID is already set from proto.
On 2012/10/29 05:28:35, satorux1 wrote:
> Maybe:
> 
> // Call AddEntryToResourceMap() instead of InitializeRootEntry() as the root
> resource ID is already set from proto.

Thanks for clarification.  Applied.

[satorux1, 2012-10-29 06:06:37]

LGTM!

[kochi, 2012-10-29 10:28:44]

On 2012/10/29 06:06:37, satorux1 wrote:
> LGTM!

Thanks for the review!

The unit_test failure was resolved by another CL:
https://chromiumcodereview.appspot.com/11346006/

[commit-bot: I haz the power, 2012-10-30 05:59:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/11227020/50002

[commit-bot: I haz the power, 2012-10-30 08:35:45]

Change committed as 164876

[kochi, 2012-10-30 09:22:32]

On 2012/10/30 08:35:45, I haz the power (commit-bot) wrote:
> Change committed as 164876

This caused unit_tests failure for StaleCacheRemover.
http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C...

Reverted at r164883.

[kochi, 2012-10-31 09:12:30]

On 2012/10/30 09:22:32, Takayoshi Kochi wrote:
> On 2012/10/30 08:35:45, I haz the power (commit-bot) wrote:
> > Change committed as 164876
> 
> This caused unit_tests failure for StaleCacheRemover.
>
http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%252...
> 
> Reverted at r164883.

Hopefully, if I fix the StaleCacheFilesRemover unittest by
https://chromiumcodereview.appspot.com/11368002/
I can revert the revert.

[commit-bot: I haz the power, 2012-10-31 17:02:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/11227020/50002

[commit-bot: I haz the power, 2012-10-31 20:47:54]

Change committed as 165219

[kochi, 2012-11-01 04:09:20]

On 2012/10/31 20:47:54, I haz the power (commit-bot) wrote:
> Change committed as 165219

This failed again and reverted at r165236,
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

[kochi, 2012-11-01 06:45:14]

On 2012/11/01 04:09:20, Takayoshi Kochi wrote:
> On 2012/10/31 20:47:54, I haz the power (commit-bot) wrote:
> > Change committed as 165219
> 
> This failed again and reverted at r165236,
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%2520ChromiumOS...

I don't see any reason for DriveileSystemTest.ContentSearchWithNewEntry failure.
Cannot repro locally.

Re-committing...

[phoglund_chromium, 2012-11-05 13:44:07]

I'm sorry, but the ChromeOS tests still flake (as you can see in
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...),
and it's the DCHECK you added which is blowing up.  You will have to fix either
the code or the tests here. 

It's possible that the old version of code ignored an empty root resource id
(for bad or worse) and that there's some race condition that causes the code to
end up in that bad state. Either way, I'll roll this back for now. I understand
it's frustrating since I can see you have committed this a few times, but we
need to solve the problem with the flaky tests. It's very likely this race
condition can happen in the production code too and in that case something bad
will likely happen.

[kochi, 2012-11-06 02:20:36]

On 2012/11/05 13:44:07, phoglund wrote:
> I'm sorry, but the ChromeOS tests still flake (as you can see in
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%2520ChromiumOS...),
> and it's the DCHECK you added which is blowing up.  You will have to fix
either
> the code or the tests here. 
> 
> It's possible that the old version of code ignored an empty root resource id
> (for bad or worse) and that there's some race condition that causes the code
to
> end up in that bad state. Either way, I'll roll this back for now. I
understand
> it's frustrating since I can see you have committed this a few times, but we
> need to solve the problem with the flaky tests. It's very likely this race
> condition can happen in the production code too and in that case something bad
> will likely happen.

OK, I'll investigate and try again.

[kochi, 2012-11-06 06:35:58]

On 2012/11/06 02:20:36, Takayoshi Kochi wrote:
> On 2012/11/05 13:44:07, phoglund wrote:
> > I'm sorry, but the ChromeOS tests still flake (as you can see in
> >
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%252520Chromium...),
> > and it's the DCHECK you added which is blowing up.  You will have to fix
> either
> > the code or the tests here. 
> > 
> > It's possible that the old version of code ignored an empty root resource id
> > (for bad or worse) and that there's some race condition that causes the code
> to
> > end up in that bad state. Either way, I'll roll this back for now. I
> understand
> > it's frustrating since I can see you have committed this a few times, but we
> > need to solve the problem with the flaky tests. It's very likely this race
> > condition can happen in the production code too and in that case something
bad
> > will likely happen.
> 
> OK, I'll investigate and try again.

FYI filed a bug for this
http://code.google.com/p/chromium/issues/detail?id=159562