Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2185973003:
[Offline Pages] Delete associated page along with bookmark. (Closed)
|
Created:
4 years, 4 months ago by romax Modified:
4 years, 3 months ago CC:
chromium-reviews, noyau+watch_chromium.org, browser-components-watch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Delete associated page along with bookmark.
When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by BookmarkModel and get initialized as part of BookmarkModel.
BUG=630796
Committed: https://crrev.com/c02075fe34677d1d194b65e6b2536a3f2204640a
Cr-Commit-Position: refs/heads/master@{#416343}
Patch Set 1 #Patch Set 2 : Use BookmarkModelObserver #
Total comments: 2
Patch Set 3 : Moving to c++. #
Total comments: 2
Patch Set 4 : Adding offline_pages::ExternalListener. #
Total comments: 10
Patch Set 5 : Move changes back to offline_pages. #Patch Set 6 : Remove unintended changes. #Patch Set 7 : Remove unintended changes. #
Total comments: 1
Patch Set 8 : Patch without dependency. #Patch Set 9 : Move expiring logic to observer, remove changes in offline_pages. #Patch Set 10 : Removing debug lines. #
Total comments: 14
Patch Set 11 : Rename Owned* to OfflinePageBookmarkObserver and Impl classes. #
Total comments: 2
Patch Set 12 : Moving into bookmark_client. #Patch Set 13 : Unnecessary changes removed. #Patch Set 14 : Fixing conflicts. #
Messages
Total messages: 54 (29 generated)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
romax@chromium.org changed reviewers: + dimich@chromium.org, ianwen@chromium.org
PTAL, this cl would delete associated offline page when deleting a bookmark.
According to the bug, an offline page will be added when the user bookmarks a page. Where is this done? Instead of letting bookmark model caching an offline page bridge, I am inclined to make some offline page classes to be a bookmark model observer and do this kind of clean up in that observer.
On 2016/07/27 21:04:01, Ian Wen wrote: > According to the bug, an offline page will be added when the user bookmarks a > page. Where is this done? > > Instead of letting bookmark model caching an offline page bridge, I am inclined > to make some offline page classes to be a bookmark model observer and do this > kind of clean up in that observer. the adding part was done in ChromeActivity#AddOrEditBookmark. I think it would be more reasonable to remove pages which doesn't have a bookmark associated. I guess I'll have another way to implement this. Thanks!
Maybe even do this on C++ side? There is BookmarkModelObserver there as well.
PTAL. I went back to use the observer, however I haven't done it for undo (we don't care about undoing deleting a page cache..) dimich@ sorry I didn't see your comments earlier, I tried but seems we don't pass profile into our model...
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2185973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2185973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:57: private BookmarkModel mBookmarkModel; It seems weird for the OfflinePagesBridge to keep a ref to BookmarkModel. It should not know about bookmark model, it's should be a more or less transparent class for accessing C++ functionality of OfflinePages. Having a separate class that depends on both bookmark model and offline page model, observes one and deletes pages from another makes more sense. To observe and coordinate both models is a task worthy of a class... If you want to only use observer, then it seems it has to be created together with BookmarkModel service on C++ side, to ensure that we get all observer notifications - somewhat like BookmarkUndoService is created here: https://cs.chromium.org/chromium/src/chrome/browser/bookmarks/bookmark_model_... You don't need profile in OffllinePageModel, you have profile at the factory when OfflinePageModel is created for profile. Also those factories can declare dependencies of services if needed - see ProfileSyncServiceFactory for example. Happy to discuss offline. https://codereview.chromium.org/2185973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:137: mBookmarkModel.addObserver(mBookmarkModelObserver); I'm curious, what guarantees that OfflinePageBridge is created before any bookmarks are removed? If the bookmarks can be removed before the OfflinePageBridge is created, then we could miss some notifications.
https://codereview.chromium.org/2185973003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_model_factory.cc (right): https://codereview.chromium.org/2185973003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_model_factory.cc:27: DependsOn(BookmarkModelFactory::GetInstance()); Hmm, I wonder if it's a right direction of the dependency. What will guarantee that every time a BookmarkModel is removing a bookmark there is an OfflinePageModel around to observe this? Should the BookmarksModel depend on OfflinePageModel? When BookmarkModel is constructed, it can create an OfflineModel and make it observer. This will guarantee the OfflinePageModel is there. WDYT? https://codereview.chromium.org/2185973003/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2185973003/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.h:64: public bookmarks::BookmarkModelObserver { If you use BaseBookmarkModelObserver as base class, you will be able to reduce the number of empty overrides below...
Sorry for delay! Some (hopefully final) comments: https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_model_factory.cc (right): https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_model_factory.cc:77: offline_page_listener = new offline_pages::ExternalListenerImpl(profile); It's better to not use WrapUnique, instead: auto offline_page_listener = base::MakeUnique<offline_pages::ExternalListenerImpl>(profile); ... new BookamrkModel(..., offline_page_listener) https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... File chrome/browser/bookmarks/chrome_bookmark_client.h (right): https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... chrome/browser/bookmarks/chrome_bookmark_client.h:30: namespace offline_pages { are there changes in this class? https://codereview.chromium.org/2185973003/diff/60001/components/bookmarks/br... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/2185973003/diff/60001/components/bookmarks/br... components/bookmarks/browser/bookmark_model.h:77: explicit BookmarkModel( when ctor takes more then one arg, it should not be explicit anymore. https://codereview.chromium.org/2185973003/diff/60001/components/offline_page... File components/offline_pages/external_listener.h (right): https://codereview.chromium.org/2185973003/diff/60001/components/offline_page... components/offline_pages/external_listener.h:17: class ExternalListener : public bookmarks::BaseBookmarkModelObserver { Lets name it somehow with "Bookmark" word, something like OfflinePagesBookmarksObserver. Then the name of file/class will give some idea what it is about. Same about "impl" one. https://codereview.chromium.org/2185973003/diff/60001/components/offline_page... components/offline_pages/external_listener.h:25: const std::set<GURL>& removed_urls) override {} I think this can be made abstract method, since it's the one ytou want to override in Impl.
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
Description was changed from ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. BUG=630796 ========== to ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by OfflinePageModel when BookmarkModel is created. BUG=630796 ==========
romax@chromium.org changed reviewers: + thestig@chromium.org - ianwen@chromium.org
PTAL. Thanks! dimich@, Since anyways we'll do a load on Chrome start, I'd like to pull the Load method out of constructor. We can discuss later. thestig@ PTAL c/b/bookmarks https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... File chrome/browser/bookmarks/bookmark_model_factory.cc (right): https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... chrome/browser/bookmarks/bookmark_model_factory.cc:77: offline_page_listener = new offline_pages::ExternalListenerImpl(profile); On 2016/08/12 05:43:43, Dmitry Titov wrote: > It's better to not use WrapUnique, instead: > > auto offline_page_listener = > base::MakeUnique<offline_pages::ExternalListenerImpl>(profile); > ... new BookamrkModel(..., offline_page_listener) Done. https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... File chrome/browser/bookmarks/chrome_bookmark_client.h (right): https://codereview.chromium.org/2185973003/diff/60001/chrome/browser/bookmark... chrome/browser/bookmarks/chrome_bookmark_client.h:30: namespace offline_pages { On 2016/08/12 05:43:43, Dmitry Titov wrote: > are there changes in this class? i think this thing was left behind. deleted. https://codereview.chromium.org/2185973003/diff/60001/components/bookmarks/br... File components/bookmarks/browser/bookmark_model.h (right): https://codereview.chromium.org/2185973003/diff/60001/components/bookmarks/br... components/bookmarks/browser/bookmark_model.h:77: explicit BookmarkModel( On 2016/08/12 05:43:43, Dmitry Titov wrote: > when ctor takes more then one arg, it should not be explicit anymore. Done. https://codereview.chromium.org/2185973003/diff/60001/components/offline_page... File components/offline_pages/external_listener.h (right): https://codereview.chromium.org/2185973003/diff/60001/components/offline_page... components/offline_pages/external_listener.h:17: class ExternalListener : public bookmarks::BaseBookmarkModelObserver { On 2016/08/12 05:43:43, Dmitry Titov wrote: > Lets name it somehow with "Bookmark" word, something like > OfflinePagesBookmarksObserver. Then the name of file/class will give some idea > what it is about. Same about "impl" one. Done. https://codereview.chromium.org/2185973003/diff/60001/components/offline_page... components/offline_pages/external_listener.h:25: const std::set<GURL>& removed_urls) override {} On 2016/08/12 05:43:43, Dmitry Titov wrote: > I think this can be made abstract method, since it's the one ytou want to > override in Impl. Done.
LGTM with dimich's approval.
https://codereview.chromium.org/2185973003/diff/160001/chrome/browser/bookmar... File chrome/browser/bookmarks/bookmark_model_factory.cc (right): https://codereview.chromium.org/2185973003/diff/160001/chrome/browser/bookmar... chrome/browser/bookmarks/bookmark_model_factory.cc:84: offline_pages::OfflinePageModelFactory::GetForBrowserContext(context) But, doesn't it remove the benefit of having OfflinePageBookmarkObserver - that it can be created w/o creating (and loading) of the OfflinePageModel?
Patchset #10 (id:220001) has been deleted
https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc:15: OfflinePageBookmarkObserver::OfflinePageBookmarkObserver(Profile* profile) I'd use BrowserContext* instead of Profile* here, it is a simpler base class which is more portable across codebase. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc:32: base::Unretained(this))); Using unretained seems dangerous. What if this instance is destroyed while async op is going on? Typically, a class would have a base::WeakPtrFactory member to automatically cancel callbacks once it is destroyed. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc:40: base::Unretained(this))); Same here (unretained) https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bookmark_observer.h (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.h:30: void BookmarkModelChanged() override {} no need to override this. Also, we typically group the overrides of each interface together with a comment saying which overrides those are. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.h:45: OfflinePageModel* offline_page_model_; What is a lifetime of that? What if the OPM is destroyed before BookmarkModel that controls lifetime of this class? https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/bookmar... File chrome/browser/bookmarks/bookmark_model_factory.cc (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/bookmar... chrome/browser/bookmarks/bookmark_model_factory.cc:31: #include "chrome/browser/android/offline_pages/offline_page_model_factory.h" I think you don't use factory here. https://codereview.chromium.org/2185973003/diff/240001/chrome/test/base/testi... File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/test/base/testi... chrome/test/base/testing_profile.cc:228: base::WrapUnique(new ChromeBookmarkClient( Since you touch this code, the style guide now says prefer MakeUnique<Foo>() instead of WrapUnique(new Foo()).
Patchset #11 (id:260001) has been deleted
Patchset #11 (id:280001) has been deleted
Patchset #11 (id:300001) has been deleted
Description was changed from ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by OfflinePageModel when BookmarkModel is created. BUG=630796 ========== to ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by BookmarkModel and get initialized as part of BookmarkModel. BUG=630796 ==========
romax@chromium.org changed reviewers: - thestig@chromium.org
romax@chromium.org changed reviewers: + sky@chromium.org, zea@chromium.org
zea@chromium.org: Please review changes in c/browser_sync/ and sync_bookmarks/ sky@chromium.org: Please review changes in chrome/ and components/bookmarks/ PTAL, thanks! https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc:15: OfflinePageBookmarkObserver::OfflinePageBookmarkObserver(Profile* profile) On 2016/08/26 23:08:58, Dmitry Titov wrote: > I'd use BrowserContext* instead of Profile* here, it is a simpler base class > which is more portable across codebase. Done. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc:32: base::Unretained(this))); On 2016/08/26 23:08:59, Dmitry Titov wrote: > Using unretained seems dangerous. What if this instance is destroyed while async > op is going on? Typically, a class would have a base::WeakPtrFactory member to > automatically cancel callbacks once it is destroyed. Done. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.cc:40: base::Unretained(this))); On 2016/08/26 23:08:59, Dmitry Titov wrote: > Same here (unretained) Done. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_bookmark_observer.h (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.h:30: void BookmarkModelChanged() override {} On 2016/08/26 23:08:59, Dmitry Titov wrote: > no need to override this. > Also, we typically group the overrides of each interface together with a comment > saying which overrides those are. Done. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_bookmark_observer.h:45: OfflinePageModel* offline_page_model_; On 2016/08/26 23:08:59, Dmitry Titov wrote: > What is a lifetime of that? What if the OPM is destroyed before BookmarkModel > that controls lifetime of this class? The OPM itself should be independent, and if we destroyed the OPM, then next time if we would like to call the callback it would create an OPM again since the pointer is nullptr? It's more like a weak_ptr. https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/bookmar... File chrome/browser/bookmarks/bookmark_model_factory.cc (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/browser/bookmar... chrome/browser/bookmarks/bookmark_model_factory.cc:31: #include "chrome/browser/android/offline_pages/offline_page_model_factory.h" On 2016/08/26 23:08:59, Dmitry Titov wrote: > I think you don't use factory here. Done. https://codereview.chromium.org/2185973003/diff/240001/chrome/test/base/testi... File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/2185973003/diff/240001/chrome/test/base/testi... chrome/test/base/testing_profile.cc:228: base::WrapUnique(new ChromeBookmarkClient( On 2016/08/26 23:08:59, Dmitry Titov wrote: > Since you touch this code, the style guide now says prefer MakeUnique<Foo>() > instead of WrapUnique(new Foo()). Done.
https://codereview.chromium.org/2185973003/diff/320001/components/bookmarks/b... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/2185973003/diff/320001/components/bookmarks/b... components/bookmarks/browser/bookmark_model.cc:115: offline_page_observer_(std::move(offline_page_observer)), Why do you need OfflinePageBookmarkObserver bake into the BookmarkModel? Can't you create the OfflinePageBookmarkObserver as necessary once BookmarkModel is created?
https://codereview.chromium.org/2185973003/diff/320001/components/bookmarks/b... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/2185973003/diff/320001/components/bookmarks/b... components/bookmarks/browser/bookmark_model.cc:115: offline_page_observer_(std::move(offline_page_observer)), On 2016/08/29 23:02:43, sky wrote: > Why do you need OfflinePageBookmarkObserver bake into the BookmarkModel? Can't > you create the OfflinePageBookmarkObserver as necessary once BookmarkModel is > created? Because I was thinking that it's better for the BookmarkModel to own this Observer, so that no events would be missed and the OfflinePageModel can load lazily. Sorry if I misunderstood your questions. But is it your main concern that the BookmarkModel owns the observer or it is part of the constructor? I guess I can use a setter here to avoid changes to the constructor...
It's better to keep the bookmarkmodel separate from code that depends upon it. To do otherwise means we end up with big monolithic systems. BookmarkModel doesn't need to know about OfflinePageBookmarksObservers to do it's job. BookmarkModel communicates by way of BookmarkModelObserver interface and shouldn't need to treat the offline case specially. Can you add the logic you need to BookmarkClient? -Scott On Mon, Aug 29, 2016 at 4:21 PM, <romax@chromium.org> wrote: > > https://codereview.chromium.org/2185973003/diff/320001/components/bookmarks/b... > File components/bookmarks/browser/bookmark_model.cc (right): > > https://codereview.chromium.org/2185973003/diff/320001/components/bookmarks/b... > components/bookmarks/browser/bookmark_model.cc:115: > offline_page_observer_(std::move(offline_page_observer)), > On 2016/08/29 23:02:43, sky wrote: >> Why do you need OfflinePageBookmarkObserver bake into the > BookmarkModel? Can't >> you create the OfflinePageBookmarkObserver as necessary once > BookmarkModel is >> created? > > Because I was thinking that it's better for the BookmarkModel to own > this Observer, so that no events would be missed and the > OfflinePageModel can load lazily. > Sorry if I misunderstood your questions. But is it your main concern > that the BookmarkModel owns the observer or it is part of the > constructor? I guess I can use a setter here to avoid changes to the > constructor... > > https://codereview.chromium.org/2185973003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
romax@chromium.org changed reviewers: - zea@chromium.org
Thanks sky@! I think this one looks much better :) PTAL, thanks!
Thanks sky@! I think this one looks much better :) PTAL, thanks!
Thanks! LGTM
Awesome progress on this patch, glad to see! LGTM
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2185973003/#ps360001 (title: "Unnecessary changes removed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #14 (id:380001) has been deleted
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, dimich@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2185973003/#ps400001 (title: "Fixing conflicts.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by BookmarkModel and get initialized as part of BookmarkModel. BUG=630796 ========== to ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by BookmarkModel and get initialized as part of BookmarkModel. BUG=630796 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by BookmarkModel and get initialized as part of BookmarkModel. BUG=630796 ========== to ========== [Offline Pages] Delete associated page along with bookmark. When deleting bookmarks we should also delete the associated offline page cache. Doing this by adding an observer owned by BookmarkModel and get initialized as part of BookmarkModel. BUG=630796 Committed: https://crrev.com/c02075fe34677d1d194b65e6b2536a3f2204640a Cr-Commit-Position: refs/heads/master@{#416343} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c02075fe34677d1d194b65e6b2536a3f2204640a Cr-Commit-Position: refs/heads/master@{#416343} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
