New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 833690 link

Starred by 1 user
Status: Fixed
Owner:
slangley@chromium.org
Closed: May 2018
Cc:
█ sashab@chromium.org
fukino@chromium.org
yamaguchi@chromium.org
hashimoto@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 715355



Sign in to add a comment

Start using start page token for fetching drive changes, not startChangeId

Project Member Reported by slangley@chromium.org, Apr 17 2018 
Today the drive engine uses the largestChangeId to track when a users corpus has changed and we need to fetch updates from the server.

However, this is problematic because

a) It doesn't work with team drive changes, which rely on a start page token, and
b) It's deprecated.

As it is desirable to have the same code flow to retrieve changes to the users corpus as well as team drives we should move to the new method.

This involves replacing "AboutResourceLoader" with something that works for multiple team drive ID's and caches the start page token as well.

Comment 1 by sashab@chromium.org, Apr 19 2018

Labels: -Pri-3 Pri-1
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0

commit ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0
Author: Stuart Langley <slangley@chromium.org>
Date: Thu Apr 19 04:05:56 2018

Introduce StartPageTokenLoader, which will replace AboutResourceLoader.

Currently AboutResourceLoader is used to retrieve and cache the largest
change id from the server for the users corpus. To imlpement TeamDrvies we
need to move to using start page tokens instead.

This CL introduces StartPageTokenLoader which performs the loading and caching
of the start page token for a give team drive.

As AboutResourceLoader is going to be removed from ChangeListLoader I pulled
it out and put it into it's own file, and created a unittest specifically for
it.

There is some duplication in the unittest setup code, which I can eliminate in a
followup CL. As it's possible we'll remove AboutResourceLoader entirely I wanted
to wait and see before refactoring the unit tests.


Bug:  833690 
Change-Id: I079cdb898b28427e07109913a74f1dd4b2071ef1
Reviewed-on: https://chromium-review.googlesource.com/1016216
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Stuart Langley <slangley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551941}
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/BUILD.gn
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/DEPS
[add] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/about_resource_loader_unittest.cc
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/change_list_loader_unittest.cc
[add] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/about_resource_loader.cc
[add] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/about_resource_loader.h
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/change_list_loader.cc
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/change_list_loader.h
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/directory_loader.cc
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/file_system.cc
[add] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/start_page_token_loader.cc
[add] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/chromeos/start_page_token_loader.h
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/directory_loader_unittest.cc
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/file_system/operation_test_base.cc
[add] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/start_page_token_loader_unittest.cc
[modify] https://crrev.com/ad8728cb4a4a9aae5e3218161aed2fdd10ab97b0/components/drive/sync_client_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 25 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba44adf8f3fed92efac18747fee8554ff9654b72

commit ba44adf8f3fed92efac18747fee8554ff9654b72
Author: Stuart Langley <slangley@chromium.org>
Date: Wed Apr 25 10:58:26 2018

Provide a method in JobScheduler to get a changelist using a start page token.

This CL introduces an overload GetChangeList method in JobScheduler that accepts
a team_drive_id and a start_page_token as parameters to retrieve the changes.

This is the next step in being able to use start tokens to retrieve changes
rather then the deprecated largest change id, which does not work for team
drives.

In DriveServiceInterface I did not overload GetChangeList but rather
introduced a new method name that accepts team_drive_id and start_page_token as
parameters. This is because base::Bind cannot resolve overloads and I would
have had to add static_casts throughout the code to tell base::Bind which method
to resolve to, and it's just not worth it.

Bug:  833690 
Change-Id: I5598cfe9d5a53a47ae8e8482585ae617f0851765
Reviewed-on: https://chromium-review.googlesource.com/1016181
Commit-Queue: Stuart Langley <slangley@chromium.org>
Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553508}
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/chrome/browser/sync_file_system/drive_backend/drive_service_on_worker.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/chrome/browser/sync_file_system/drive_backend/drive_service_on_worker.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/chrome/browser/sync_file_system/drive_backend/drive_service_wrapper.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/chrome/browser/sync_file_system/drive_backend/drive_service_wrapper.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/file_change.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/job_scheduler.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/job_scheduler.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/job_scheduler_unittest.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/drive_api_service.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/drive_api_service.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/drive_service_interface.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/dummy_drive_service.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/dummy_drive_service.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/fake_drive_service.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/components/drive/service/fake_drive_service.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/google_apis/drive/drive_api_parser.cc
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/google_apis/drive/drive_api_parser.h
[modify] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/google_apis/drive/drive_api_parser_unittest.cc
[add] https://crrev.com/ba44adf8f3fed92efac18747fee8554ff9654b72/google_apis/test/data/drive/changelist_with_new_start_page_token.json

Comment 4 by slangley@chromium.org, May 10 2018

Labels: -M-68 M-69
Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95c215051866fcfdcfb2d7705a61b33e5197df4e

commit 95c215051866fcfdcfb2d7705a61b33e5197df4e
Author: Stuart Langley <slangley@chromium.org>
Date: Fri May 25 04:21:29 2018

Pass the team drive id and root path to loaders so the can support team drives.

Currently change_list_loader and directory_loader only work for the users
default corpus. We can fix this by passing the team_drive_id and the
root_entry_path as parameters during construction rather then just assuming
the users default corpus root.

With this change we can not use directory_loader and change_list_loader to also
processes changes from team drives by passing the appropriate team_drive_id and
root_entry_path.

Bug:  835703 ,  833690 

Change-Id: I5ba3ed8fa3323dfd520c9e1837934eeef79a30a7
Reviewed-on: https://chromium-review.googlesource.com/1068642
Commit-Queue: Stuart Langley <slangley@chromium.org>
Reviewed-by: Sasha Morrissey <sashab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561767}
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/change_list_loader_unittest.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/change_list_processor_unittest.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/change_list_loader.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/change_list_loader.h
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/change_list_processor.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/change_list_processor.h
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/default_corpus_change_list_loader.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/directory_loader.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/chromeos/directory_loader.h
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/directory_loader_unittest.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/file_system/operation_test_base.cc
[modify] https://crrev.com/95c215051866fcfdcfb2d7705a61b33e5197df4e/components/drive/sync_client_unittest.cc

Comment 6 by slangley@chromium.org, May 25 2018

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, May 29 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e51b56a86e37d3a8950f645075bc9694386894cc

commit e51b56a86e37d3a8950f645075bc9694386894cc
Author: Stuart Langley <slangley@chromium.org>
Date: Tue May 29 12:32:00 2018

Fix element tag used to display start page token in drive-internals.

This change was not propagated when changing in the original
change list to convert from largest change id to start page token.

Bug:  833690 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ifbedaac2523469d6e2ad41ac08da600860464c2b
Reviewed-on: https://chromium-review.googlesource.com/1075830
Reviewed-by: Sasha Morrissey <sashab@chromium.org>
Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org>
Commit-Queue: Stuart Langley <slangley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562392}
[modify] https://crrev.com/e51b56a86e37d3a8950f645075bc9694386894cc/chrome/browser/resources/chromeos/drive_internals.js
►

Sign in to add a comment