Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734873002:
Fix ArcSessionManager state machine, part 2. (Closed)
|
Created:
3 years, 9 months ago by hidehiko Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org, victorhsieh, khmel_chromium.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ArcSessionManager state machine, part 2.
This CL fixes REMOVING_DATA_DIR part.
Conceptually, this splits RemoveArcData() into two methods.
- RequestArcDataRemoval(): Requests to remove the data when possible.
- StartArcDataRemoval(): Actually triggers to remove the data.
Note: the state transition cannot be simplified in this CL yet,
because OnSessionStopped(), it is unknown whether the stop is triggered
by RequestDisable() or not (existing race problem, but not regression).
When its state is fixed, the state transition related to REMOVING_DATA_DIR
can also be simplified.
cf) Part1: crrev.com/2737453003
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
Review-Url: https://codereview.chromium.org/2734873002
Cr-Commit-Position: refs/heads/master@{#455738}
Committed: https://chromium.googlesource.com/chromium/src/+/73c55143f033177c23833094fecdc51056063aab
Patch Set 1 #
Total comments: 21
Patch Set 2 : rebase #Patch Set 3 : Address comment. #
Total comments: 18
Patch Set 4 : rebase #Patch Set 5 : Address comments. #
Messages
Total messages: 22 (14 generated)
The CQ bit was checked by hidehiko@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.
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org, yusukes@chromium.org
PTAL. Note: I'm sending another CL, which introduces a new state STOPPING, to resolve the state machine incompleteness followed by this CL. Then, most of TODO in this CL should be resolved. Thank you for review in advance, - hidehiko
https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. In the CL description, can you add a link to the Part-1 CL? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:153: // Transition to the ARC data remove state. nit: move down the comment to L164? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:595: VLOG(1) << "ARC opt-out. Removing user data."; Maybe move this to L596 to follow the comment in the header more precisely? "A log statement with the removal reason must be added prior to calling this." https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:782: void ArcSessionManager::StartArcDataRemoval() { This is more like MaybeStartArcDataRemoval() since this is mostly no-op when the pref is not set. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:788: // TODO(hidehiko): DCHECK the previous state, when the state machine is What's still broken? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:797: OnArcDataRemoved(true); Changing the state to REMOVING_DATA_DIR, calling the callback synchronously (but skipping the observer_list_), then setting the state back to STOPPED synchronously, seems a bit difficult to read. Can't we move this up (to L787) and just do if (!the_pref_set) { MaybeReenableArc(); return; } ? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:820: if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) { Then can we remove this IF? Not notifying observer_list_ in OnArcDateRemoved depending on the pref seems confusing to me. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:824: LOG(ERROR) << "Request for ARC user data removal failed."; This is sort of a fatal error, right? It's a bit sad that only boolean is returned from session_manager and Chrome can't know the exact reason why it failed. Hopefully /var/log/message has enough error messages from session_manager to debug? (just saying, no fix needed.) https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:170: // Request to remove the ARC data. Requests
The CQ bit was checked by hidehiko@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...
Thank you for review. PTAL. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:153: // Transition to the ARC data remove state. On 2017/03/06 21:39:59, Yusuke Sato wrote: > nit: move down the comment to L164? It is intentional. The following if-stmt is workaround, and considered as a part of REMOVING_DATA_DIR, so set the state. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:595: VLOG(1) << "ARC opt-out. Removing user data."; On 2017/03/06 21:39:59, Yusuke Sato wrote: > Maybe move this to L596 to follow the comment in the header more precisely? "A > log statement with the removal reason must be added prior to calling this." Done. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:782: void ArcSessionManager::StartArcDataRemoval() { On 2017/03/06 21:39:59, Yusuke Sato wrote: > This is more like MaybeStartArcDataRemoval() since this is mostly no-op when the > pref is not set. I did this intentionally for consistency with StartArcTermsOfServiceNegotiation(). Or, should I add Maybe prefix for both cases? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:788: // TODO(hidehiko): DCHECK the previous state, when the state machine is On 2017/03/06 21:39:59, Yusuke Sato wrote: > What's still broken? OnSessionStopped() can be called regardless of state, unfortunately... https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:797: OnArcDataRemoved(true); On 2017/03/06 21:39:59, Yusuke Sato wrote: > Changing the state to REMOVING_DATA_DIR, calling the callback synchronously (but > skipping the observer_list_), then setting the state back to STOPPED > synchronously, seems a bit difficult to read. Can't we move this up (to L787) > and just do > > if (!the_pref_set) { > MaybeReenableArc(); > return; > } > > ? Changed to direct MaybeReenableArc() call. As for state, I still would like to keep it as is in this CL. As I wrote TODO, I'm planning to extract the data removal class. Then, kArcDataRemoveRequested is encapsulated in the class. Thus; ASM::StartArcDataRemoval() { SetState(REMOVING_DATA_DIR); data_deleter->MaybeStartArcDataRemoval( Bind(this.WeakPtr(), OnArcDataRemoved)); } ASM::OnArcDataRemoved() { // Current MaybeReenableArc(). Function name is TBD. } class DataDeleter { // TBD: name. public: void RequestArcDataRemoval(); void MaybeStartArcDataRemoval(callback) { if (!pref_has_flag) { return callback.Run(); } ... DBus Call ... } void OnDataRemoved() { ... // almost same. callback.Run(); } }; WDYT? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:820: if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) { On 2017/03/06 21:39:59, Yusuke Sato wrote: > Then can we remove this IF? Not notifying observer_list_ in OnArcDateRemoved > depending on the pref seems confusing to me. Done. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:824: LOG(ERROR) << "Request for ARC user data removal failed."; On 2017/03/06 21:39:59, Yusuke Sato wrote: > This is sort of a fatal error, right? It's a bit sad that only boolean is > returned from session_manager and Chrome can't know the exact reason why it > failed. Hopefully /var/log/message has enough error messages from > session_manager to debug? > > (just saying, no fix needed.) Acknowledged. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.h:170: // Request to remove the ARC data. On 2017/03/06 21:39:59, Yusuke Sato wrote: > Requests Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with style nits and a question. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/06 21:39:59, Yusuke Sato wrote: > In the CL description, can you add a link to the Part-1 CL? ping? https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:782: void ArcSessionManager::StartArcDataRemoval() { On 2017/03/07 18:51:01, hidehiko wrote: > On 2017/03/06 21:39:59, Yusuke Sato wrote: > > This is more like MaybeStartArcDataRemoval() since this is mostly no-op when > the > > pref is not set. > > I did this intentionally for consistency with > StartArcTermsOfServiceNegotiation(). Or, should I add Maybe prefix for both > cases? +1 for renaming both. Your call though. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:161: // synchronously calls OnArcDataRemoved(), which causes unexpected This line seems outdated now. StartArcDataRemoval no longer synchronously calls OnArcDataRemoved(). https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:823: DCHECK(profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)); Thanks, the cb function is much easier to follow now. BTW, this file has countless number of DCHECKs, but I'm wondering how many of them are automatically evaluated by tests. Do you have any numbers of this? https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:827: LOG(ERROR) << "Request for ARC user data removal failed."; nit: What about adding 'See session_manager logs for more details.' too? https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:838: void ArcSessionManager::MaybeReenableArc() { nit: why did you move this down? shouldn't we preserve git blame logs whenever possible? ..ah you followed the order in the header file. Then this is fine probably. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:279: // If not requested, OnArcDataRemoved() is called as if the data removal Please update the comment too.
comments, all minor, so lgtm. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:785: // Data removal cannot run, in parallel with ARC session. nit: remove comma. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:830: // Data removal runs (regardless of whether it is successfully done or This is not strictly true: there have been cases where session_manager becomes unresponsive via D-Bus and will *not* remove data. See crbug.com/631640#c63 for an instance where a similar thing happened. But in order to know here what part of the call failed is complicated. Given that the behavior is preserved, I'm fine with the code being left as-is, but the wording of the comment does need to change. (Or did I grossly misunderstood the comment and you only meant that the OnArcDataRemoved *event* runs regardless of ...?) https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:172: // remove the data on ARC stopping. nit: "after ARC stops" is a bit more precise (otherwise it can be interpreted as being leading-edge triggered). https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:281: void StartArcDataRemoval(); Another vote to call this "MaybeStartArcDataRemoval" since it does not always actually start the data removal.
Description was changed from ========== Fix ArcSessionManager state machine, part 2. This CL fixes REMOVING_DATA_DIR part. Conceptually, this splits RemoveArcData() into two methods. - RequestArcDataRemoval(): Requests to remove the data when possible. - StartArcDataRemoval(): Actually triggers to remove the data. Note: the state transition cannot be simplified in this CL yet, because OnSessionStopped(), it is unknown whether the stop is triggered by RequestDisable() or not (existing race problem, but not regression). When its state is fixed, the state transition related to REMOVING_DATA_DIR can also be simplified. BUG=657687 BUG=b/31079732 TEST=Ran trybots. ========== to ========== Fix ArcSessionManager state machine, part 2. This CL fixes REMOVING_DATA_DIR part. Conceptually, this splits RemoveArcData() into two methods. - RequestArcDataRemoval(): Requests to remove the data when possible. - StartArcDataRemoval(): Actually triggers to remove the data. Note: the state transition cannot be simplified in this CL yet, because OnSessionStopped(), it is unknown whether the stop is triggered by RequestDisable() or not (existing race problem, but not regression). When its state is fixed, the state transition related to REMOVING_DATA_DIR can also be simplified. cf) Part1: crrev.com/2737453003 BUG=657687 BUG=b/31079732 TEST=Ran trybots. ==========
Thank you for review. Submitting. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_play_store_enabled_preference_handler.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/07 21:03:08, Yusuke Sato wrote: > On 2017/03/06 21:39:59, Yusuke Sato wrote: > > In the CL description, can you add a link to the Part-1 CL? > > ping? Oops. Done. https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager.cc:782: void ArcSessionManager::StartArcDataRemoval() { On 2017/03/07 21:03:08, Yusuke Sato wrote: > On 2017/03/07 18:51:01, hidehiko wrote: > > On 2017/03/06 21:39:59, Yusuke Sato wrote: > > > This is more like MaybeStartArcDataRemoval() since this is mostly no-op when > > the > > > pref is not set. > > > > I did this intentionally for consistency with > > StartArcTermsOfServiceNegotiation(). Or, should I add Maybe prefix for both > > cases? > > +1 for renaming both. Your call though. Ok, done. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:161: // synchronously calls OnArcDataRemoved(), which causes unexpected On 2017/03/07 21:03:08, Yusuke Sato wrote: > This line seems outdated now. StartArcDataRemoval no longer synchronously calls > OnArcDataRemoved(). Done. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:785: // Data removal cannot run, in parallel with ARC session. On 2017/03/07 21:52:00, Luis Héctor Chávez wrote: > nit: remove comma. Done. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:823: DCHECK(profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)); On 2017/03/07 21:03:08, Yusuke Sato wrote: > Thanks, the cb function is much easier to follow now. > > BTW, this file has countless number of DCHECKs, but I'm wondering how many of > them are automatically evaluated by tests. Do you have any numbers of this? Could you explain a bit more details about your question? DCHECK causes fatal error on tests so passing tests means, at least in such scenarios, DCHECKs pass properly. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:827: LOG(ERROR) << "Request for ARC user data removal failed."; On 2017/03/07 21:03:08, Yusuke Sato wrote: > nit: What about adding 'See session_manager logs for more details.' too? Done. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:830: // Data removal runs (regardless of whether it is successfully done or On 2017/03/07 21:52:00, Luis Héctor Chávez wrote: > This is not strictly true: there have been cases where session_manager becomes > unresponsive via D-Bus and will *not* remove data. See crbug.com/631640#c63 for > an instance where a similar thing happened. > > But in order to know here what part of the call failed is complicated. Given > that the behavior is preserved, I'm fine with the code being left as-is, but the > wording of the comment does need to change. > > (Or did I grossly misunderstood the comment and you only meant that the > OnArcDataRemoved *event* runs regardless of ...?) I just wanted to say this is called regardless of data removal completed successfully or failed. Rephrased. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:838: void ArcSessionManager::MaybeReenableArc() { On 2017/03/07 21:03:08, Yusuke Sato wrote: > nit: why did you move this down? shouldn't we preserve git blame logs whenever > possible? > > ..ah you followed the order in the header file. Then this is fine probably. Yes, (and that's the order of the state transition). https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.h (right): https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:172: // remove the data on ARC stopping. On 2017/03/07 21:52:00, Luis Héctor Chávez wrote: > nit: "after ARC stops" is a bit more precise (otherwise it can be interpreted as > being leading-edge triggered). Done. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:279: // If not requested, OnArcDataRemoved() is called as if the data removal On 2017/03/07 21:03:08, Yusuke Sato wrote: > Please update the comment too. Done. https://codereview.chromium.org/2734873002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.h:281: void StartArcDataRemoval(); On 2017/03/07 21:52:00, Luis Héctor Chávez wrote: > Another vote to call this "MaybeStartArcDataRemoval" since it does not always > actually start the data removal. Done.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2734873002/#ps80001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489051558087420,
"parent_rev": "70960300c81513c83f1ea4b84af11effb6b1476f", "commit_rev":
"73c55143f033177c23833094fecdc51056063aab"}
Message was sent while issue was closed.
Description was changed from ========== Fix ArcSessionManager state machine, part 2. This CL fixes REMOVING_DATA_DIR part. Conceptually, this splits RemoveArcData() into two methods. - RequestArcDataRemoval(): Requests to remove the data when possible. - StartArcDataRemoval(): Actually triggers to remove the data. Note: the state transition cannot be simplified in this CL yet, because OnSessionStopped(), it is unknown whether the stop is triggered by RequestDisable() or not (existing race problem, but not regression). When its state is fixed, the state transition related to REMOVING_DATA_DIR can also be simplified. cf) Part1: crrev.com/2737453003 BUG=657687 BUG=b/31079732 TEST=Ran trybots. ========== to ========== Fix ArcSessionManager state machine, part 2. This CL fixes REMOVING_DATA_DIR part. Conceptually, this splits RemoveArcData() into two methods. - RequestArcDataRemoval(): Requests to remove the data when possible. - StartArcDataRemoval(): Actually triggers to remove the data. Note: the state transition cannot be simplified in this CL yet, because OnSessionStopped(), it is unknown whether the stop is triggered by RequestDisable() or not (existing race problem, but not regression). When its state is fixed, the state transition related to REMOVING_DATA_DIR can also be simplified. cf) Part1: crrev.com/2737453003 BUG=657687 BUG=b/31079732 TEST=Ran trybots. Review-Url: https://codereview.chromium.org/2734873002 Cr-Commit-Position: refs/heads/master@{#455738} Committed: https://chromium.googlesource.com/chromium/src/+/73c55143f033177c23833094fecd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/73c55143f033177c23833094fecd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
