Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217413002:
arc: Fix sync related issue with old container. (Closed)
|
Created:
4 years, 4 months ago by lgcheng Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, Matt Giuca Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix sync related issue with old container.
Sync related issue when the user is running a version of Chrome that
contains the ARC package sync implementation, but is also running an older
version of the Android container.
BUG=637057
Test= Manual test
Committed: https://crrev.com/2aea7674d6287edb686adb4c91519b04f3af7ac6
Cr-Commit-Position: refs/heads/master@{#411423}
Patch Set 1 #
Total comments: 4
Patch Set 2 : arc: Sync related issue. #Messages
Total messages: 22 (13 generated)
jhorwich@chromium.org changed reviewers: + jhorwich@chromium.org
https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:26: constexpr int64_t kUnsupportedAndroidID = 0; Perhaps call this "kEmptyAndroidID" or "kNoAndroidId" instead? https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:278: // service from recording the user's installation correctly. I'd phrase this a bit differently but generally it's a good description. How about something like: If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container that sends a 0 AndroidID for package update events, and the user already has some packages installed, the sync service will send these packages with the 0 AndroidID. Then if the user later has a new device, package installation will fail and the package will stay in 'pending install'. Furthermore, even if the user manually installs the package, the existing 'pending install' entry will prevent the sync service from recording the user's new installation correctly.
Description was changed from ========== arc: Sync related issue. BUG= ========== to ========== arc: Sync related issue. BUG= http://b/30710474 ==========
Description was changed from ========== arc: Sync related issue. BUG= http://b/30710474 ========== to ========== Sync related issue If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container BUG= http://b/30710474 ==========
Description was changed from ========== Sync related issue If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container BUG= http://b/30710474 ========== to ========== Sync related issue If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ==========
https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:26: constexpr int64_t kUnsupportedAndroidID = 0; On 2016/08/06 00:00:35, Josh Horwich wrote: > Perhaps call this "kEmptyAndroidID" or "kNoAndroidId" instead? Done. https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:278: // service from recording the user's installation correctly. On 2016/08/06 00:00:35, Josh Horwich wrote: > I'd phrase this a bit differently but generally it's a good description. How > about something like: > > If the user is running a version of Chrome that contains the ARC package sync > implementation, but is also running an older version of the Android container > that sends a 0 AndroidID for package update events, and the user already has > some packages installed, the sync service will send these packages with the 0 > AndroidID. Then if the user later has a new device, package installation will > fail and the package will stay in 'pending install'. Furthermore, even if the > user manually installs the package, the existing 'pending install' entry will > prevent the sync service from recording the user's new installation correctly. Done.
Description was changed from ========== Sync related issue If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ========== to ========== Sync related issue. If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ==========
Description was changed from ========== Sync related issue. If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ========== to ========== Sync related issue. If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ==========
Description was changed from ========== Sync related issue. If the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ========== to ========== Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ==========
On 2016/08/06 00:33:51, lgcheng wrote: > https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... > File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): > > https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:26: constexpr > int64_t kUnsupportedAndroidID = 0; > On 2016/08/06 00:00:35, Josh Horwich wrote: > > Perhaps call this "kEmptyAndroidID" or "kNoAndroidId" instead? > > Done. > > https://codereview.chromium.org/2217413002/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:278: // service > from recording the user's installation correctly. > On 2016/08/06 00:00:35, Josh Horwich wrote: > > I'd phrase this a bit differently but generally it's a good description. How > > about something like: > > > > If the user is running a version of Chrome that contains the ARC package sync > > implementation, but is also running an older version of the Android container > > that sends a 0 AndroidID for package update events, and the user already has > > some packages installed, the sync service will send these packages with the 0 > > AndroidID. Then if the user later has a new device, package installation will > > fail and the package will stay in 'pending install'. Furthermore, even if the > > user manually installs the package, the existing 'pending install' entry will > > prevent the sync service from recording the user's new installation correctly. > > Done. Also, can you change the commit title/first line of commit to describe what this change does instead of what it's fixing? "arc: Fix sync for older containers". If you want to merge this to M53, you must also create a crbug.com bug and use it instead of the b/ bug.
Description was changed from ========== Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ========== to ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ==========
lgcheng@google.com changed reviewers: + stevenjb@chromium.org
Hi Steven, PTAL this patch when you have a moment. Thanks!
Description was changed from ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG= http://b/30710474 Test= Manual test ========== to ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG=637057 Test= Manual test ==========
lgtm
The CQ bit was checked by lgcheng@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/11 20:53:12, stevenjb wrote: > lgtm Thanks for review!
Message was sent while issue was closed.
Description was changed from ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG=637057 Test= Manual test ========== to ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG=637057 Test= Manual test ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG=637057 Test= Manual test ========== to ========== Fix sync related issue with old container. Sync related issue when the user is running a version of Chrome that contains the ARC package sync implementation, but is also running an older version of the Android container. BUG=637057 Test= Manual test Committed: https://crrev.com/2aea7674d6287edb686adb4c91519b04f3af7ac6 Cr-Commit-Position: refs/heads/master@{#411423} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2aea7674d6287edb686adb4c91519b04f3af7ac6 Cr-Commit-Position: refs/heads/master@{#411423} |
