Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727553006:
[sql] Convert thumbnails and top-sites databases to auto-recovery. (Closed)
|
Created:
3 years, 9 months ago by Scott Hess - ex-Googler Modified:
3 years, 8 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Convert thumbnails and top-sites databases to auto-recovery.
sql::Recovery::RecoverDatabase() seems to work pretty well for
Shortcuts, so backfill to use it for these two databases. Histograms
indicate that there's nothing really special about these databases which
demands special treatment.
BUG=240396, 597785
Review-Url: https://codereview.chromium.org/2727553006
Cr-Commit-Position: refs/heads/master@{#464213}
Committed: https://chromium.googlesource.com/chromium/src/+/5207e145f18d167484e8941c7fa4f6b38224bc76
Patch Set 1 #Patch Set 2 : git-cl-format #
Total comments: 22
Patch Set 3 : rebase #Patch Set 4 : tests and review comments #
Depends on Patchset: Messages
Total messages: 40 (21 generated)
git-cl-format
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Description was changed from ========== [sql] Convert thumbnails and top-sites databases to auto-recovery. sql::Recovery::RecoverDatabase() seems to work pretty well for Shortcuts, so backfill to use it for these two databases. Histograms indicate that there's nothing really special about these databases which demands special treatment. BUG=240396 ========== to ========== [sql] Convert thumbnails and top-sites databases to auto-recovery. sql::Recovery::RecoverDatabase() seems to work pretty well for Shortcuts, so backfill to use it for these two databases. Histograms indicate that there's nothing really special about these databases which demands special treatment. BUG=240396,597785 ==========
The CQ bit was checked by shess@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...
shess@chromium.org changed reviewers: + pwnall@chromium.org
Here's another sql::Recovery cl. I realize that most of the change is over in the history sub-system, but when I asked those OWNERS about whether they wanted the review, they threatened to add me to the OWNERS file. I think this is really more sql/-related than history/-related, so maybe better to concentrate effort from that end, first.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/03/02 00:54:29, Scott Hess wrote: > Here's another sql::Recovery cl. I realize that most of the change is over in > the history sub-system, but when I asked those OWNERS about whether they wanted > the review, they threatened to add me to the OWNERS file. I think this is > really more sql/-related than history/-related, so maybe better to concentrate > effort from that end, first. Do you want this in M58? I'm pretty hosed today, because I'm trying to get 2 CLs in M58, and then perf. Your answer will help me prioritize.
On 2017/03/02 13:05:22, pwnall wrote: > On 2017/03/02 00:54:29, Scott Hess wrote: > > Here's another sql::Recovery cl. I realize that most of the change is over in > > the history sub-system, but when I asked those OWNERS about whether they > wanted > > the review, they threatened to add me to the OWNERS file. I think this is > > really more sql/-related than history/-related, so maybe better to concentrate > > effort from that end, first. > > Do you want this in M58? > > I'm pretty hosed today, because I'm trying to get 2 CLs in M58, and then perf. > Your answer will help me prioritize. Nope! This is just deferred maintenance.
Can you please address the failing tests? https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:87: // Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20 (depr.) I think this change broke a test? https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:97: const int kDeprecatedVersionNumber = 6; // and earlier. I think this change broke a test. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... It makes sense to me that ThumbnailDatabaseTest.Version6 would fail, and we can remove it. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:237: DCHECK_EQ(8, kCurrentVersionNumber); Would it make sense to break this into a DCHECK_GE(8) that has the first sentence of the comment, and a DCHECK_LE(8) that'd have the 2nd comment, and possibly get bumped when the schema gets bumped? https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/top_sites_database.cc (left): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:306: UMA_HISTOGRAM_PERCENTAGE("History.TopSitesRecoveredPercentage", Sorry for my bad memory, but dose sql::Recovery::RecoverDatabase() track metrics equivalent to these? I think they're still a good idea in the general case. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/top_sites_database.cc (right): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:66: static const int kDeprecatedVersionNumber = 2; // and earlier. I think this breaks TopSitesDatabaseTest.Version2, same deal as above. Do we have a test that makes sure we can read from Version 3? (same issue about Version 7 before). https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:124: RECOVERY_EVENT_FAILED_SCOPER, // obsolete Would make sense to add OBSOLETE_ / DEPRECATED_ to these names, so any attempt to use them is obvious in review? https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:221: // seen for a few thousand database files. A percentage might give the reader a better intuition here. https://codereview.chromium.org/2727553006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2727553006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21100: + Deprecated as of March 2017. Can these say "Removed as of March 2017. No longer tracked." ? https://codereview.chromium.org/2727553006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:93718: + Obsolete March 2017. Why is this one not following the pattern above?
rebase
tests and review comments
The CQ bit was checked by shess@chromium.org to run a CQ dry run
I'm going to think on the "Should there be a histogram to track sizes, and where" while waiting for the trybots to process. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:87: // Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20 (depr.) On 2017/03/04 01:35:32, pwnall wrote: > I think this change broke a test? Yeah, brain fart, I tested the recovery case but not the general case. Bumping the deprecation forward isn't really necessary, but it's easier for this case than doing the work of making sure the recovery of old databases is handled right for something which histograms indicate essentially never happens. Thumbnails has about .01% of dbs at v6 at startup, Favicons about .01% at v2. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:237: DCHECK_EQ(8, kCurrentVersionNumber); On 2017/03/04 01:35:32, pwnall wrote: > Would it make sense to break this into a DCHECK_GE(8) that has the first > sentence of the comment, and a DCHECK_LE(8) that'd have the 2nd comment, and > possibly get bumped when the schema gets bumped? I'm trying to craft a revised comment, and running aground. Broadly, the issue is that there is "What databases can have existed?", which the code needs to handle and have tests for, versus "What is the current database version and does it work?", where I just want to call attention to an "if this then that" assertion. Like "If you change the schema version, here's something you need to think about." Of course, I just described a static assertion, which I think may not have been portably available when I originally wrote this code. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/top_sites_database.cc (left): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:306: UMA_HISTOGRAM_PERCENTAGE("History.TopSitesRecoveredPercentage", On 2017/03/04 01:35:32, pwnall wrote: > Sorry for my bad memory, but dose sql::Recovery::RecoverDatabase() track metrics > equivalent to these? I think they're still a good idea in the general case. The RecoverDatabase() code does not really have a comparable. When originally rolled out, I was worried that this code would show a big hit in a low-valued bucket, basically signalling "Routinely deleting everything". In the event, this histogram has 75% at 100/101, plus the other 25% of hits pretty evenly distributed to all the other buckets. I _think_ that's just reflecting un-vacuumed space in the file rather than rows being unexpectedly dropped by the recovery code. The Favicons version is all at 100/101. So I'm not sure what the action item would be for the histogram. The rows-recovered is easier to address, I don't think I've ever seen anything interesting in there. It's just a kinda normal distribution matching what I'd expect to see from a histogram tracking row counts in the wild. Again, having a big hit at 0 would have been a concern, but in the event it didn't show anything actionable. Hmm. I am thinking it might be nice to have a histogram for "Recovery was successful, but no data was recovered." High occurrence of that may indicate a problem with the recovery code. But setting the threshold to 0 for the entire database may not make sense, because tables like [meta] may always provide enough to get over that threshold. And yet tracking something more subtle like "Empty tables were seen" also concerns me, because there's nothing wrong with empty tables. As things stand, there's not a strong way to expose something like "rows dropped" or "pages dropped" from the virtual table itself, though I could perhaps add a table-specific function to do it. Percentage isn't great because the ratio will differ across users, which would smear the signal out. I'll think on this for a bit, if there was an obvious out in the above paragraph, hit me with a clue stick. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/top_sites_database.cc (right): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:66: static const int kDeprecatedVersionNumber = 2; // and earlier. On 2017/03/04 01:35:32, pwnall wrote: > I think this breaks TopSitesDatabaseTest.Version2, same deal as above. Do we > have a test that makes sure we can read from Version 3? (same issue about > Version 7 before). Current-version tests are under Version/Recovery rather than VersionN/RecoveryN. If a new version comes out, my expectation would be to copy/paste those tests, rename one to add the right N, dupe the golden files, etc. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:124: RECOVERY_EVENT_FAILED_SCOPER, // obsolete On 2017/03/04 01:35:32, pwnall wrote: > Would make sense to add OBSOLETE_ / DEPRECATED_ to these names, so any attempt > to use them is obvious in review? Done. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:221: // seen for a few thousand database files. On 2017/03/04 01:35:32, pwnall wrote: > A percentage might give the reader a better intuition here. The databases in this state were previously persistent, meaning they accumulate in this bucket over time (but only once per launch due handle poisoning). The databases fixed fall will hit once under SUCCESS, but then off the radar, so I'm not sure there's a comparable denominator. Over the past 28 days, there have been 125k users who have recovered this database 170k times (um), but 37k users failed over 900k attempts, so the max is like 30% stuck here. The number-recovered was much higher originally (due to accumulated corruption which has been recovered), so I think the likely real value is in the neighborhood of 5%. Which is on top of being a fraction of a percent of the total userbase (I mean "users with newly-corrupted databases" because "users who ever saw corruption in any database" is more like a few percent last time I tried to quantify it). So my goal in the comment is to suggest that the aggregate total number is less than some "not worth bothering with" epsilon. But I also don't want to write a learned tome in the comment. Percentage-wise, even the top-line number is maybe not worth the effort once the overhang is knocked down, but once dealt with it's also not complicated to keep dealing with it. https://codereview.chromium.org/2727553006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2727553006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21100: + Deprecated as of March 2017. On 2017/03/04 01:35:32, pwnall wrote: > Can these say "Removed as of March 2017. No longer tracked." ? Done, though "No longer tracked as of March 2017." https://codereview.chromium.org/2727553006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:93718: + Obsolete March 2017. On 2017/03/04 01:35:32, pwnall wrote: > Why is this one not following the pattern above? Random, I was copy-pasting from some other enum (in case the format was different, it wasn't). Made it regular.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shess@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.
https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:87: // Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20 (depr.) On 2017/03/07 01:34:31, Scott Hess wrote: > On 2017/03/04 01:35:32, pwnall wrote: > > I think this change broke a test? > > Yeah, brain fart, I tested the recovery case but not the general case. > > Bumping the deprecation forward isn't really necessary, but it's easier for this > case than doing the work of making sure the recovery of old databases is handled > right for something which histograms indicate essentially never happens. > Thumbnails has about .01% of dbs at v6 at startup, Favicons about .01% at v2. Sorry for being unclear. I didn't mean to say you shouldn't deprecate! If anything, the comments above suggest that it should be fine to deprecate v7 and remove any recovery code associated with it. Remove all the old things :) I just meant the test should be removed, assuming its name describes what it does correctly. If we deprecated v6, we presumably don't care about being able to recover from v6 correctly. The only alternative I could think of is to make the v6 test assert that we correctly reject the old db, but I assume we already have a test covering that code path. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:237: DCHECK_EQ(8, kCurrentVersionNumber); On 2017/03/07 01:34:31, Scott Hess wrote: > On 2017/03/04 01:35:32, pwnall wrote: > > Would it make sense to break this into a DCHECK_GE(8) that has the first > > sentence of the comment, and a DCHECK_LE(8) that'd have the 2nd comment, and > > possibly get bumped when the schema gets bumped? > > I'm trying to craft a revised comment, and running aground. > > Broadly, the issue is that there is "What databases can have existed?", which > the code needs to handle and have tests for, versus "What is the current > database version and does it work?", where I just want to call attention to an > "if this then that" assertion. Like "If you change the schema version, here's > something you need to think about." > > Of course, I just described a static assertion, which I think may not have been > portably available when I originally wrote this code. Thanks for explaining! IIUC, you want to force a diff here when the current schema changes. Would it make more sense to have a comment near the schema version number describing the issues here, or pointing here? Alternatively, you could consider having a markdown describing the current schema and all the considerations needed to revise it, and having a brief comment near the kCurrentVersionNumber definition pointing to the doc. Whichever path you choose is fine with me. static_assert works for me too, but it seems like it's adding an extra step to the discovery process. (change version number, compile, discover the comments here; vs change version number, discover comments right next to it) https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/top_sites_database.cc (left): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:306: UMA_HISTOGRAM_PERCENTAGE("History.TopSitesRecoveredPercentage", On 2017/03/07 01:34:31, Scott Hess wrote: > On 2017/03/04 01:35:32, pwnall wrote: > > Sorry for my bad memory, but dose sql::Recovery::RecoverDatabase() track > metrics > > equivalent to these? I think they're still a good idea in the general case. > > The RecoverDatabase() code does not really have a comparable. > > When originally rolled out, I was worried that this code would show a big hit in > a low-valued bucket, basically signalling "Routinely deleting everything". In > the event, this histogram has 75% at 100/101, plus the other 25% of hits pretty > evenly distributed to all the other buckets. I _think_ that's just reflecting > un-vacuumed space in the file rather than rows being unexpectedly dropped by the > recovery code. The Favicons version is all at 100/101. So I'm not sure what > the action item would be for the histogram. > > The rows-recovered is easier to address, I don't think I've ever seen anything > interesting in there. It's just a kinda normal distribution matching what I'd > expect to see from a histogram tracking row counts in the wild. Again, having a > big hit at 0 would have been a concern, but in the event it didn't show anything > actionable. > > Hmm. I am thinking it might be nice to have a histogram for "Recovery was > successful, but no data was recovered." High occurrence of that may indicate a > problem with the recovery code. But setting the threshold to 0 for the entire > database may not make sense, because tables like [meta] may always provide > enough to get over that threshold. And yet tracking something more subtle like > "Empty tables were seen" also concerns me, because there's nothing wrong with > empty tables. As things stand, there's not a strong way to expose something > like "rows dropped" or "pages dropped" from the virtual table itself, though I > could perhaps add a table-specific function to do it. Percentage isn't great > because the ratio will differ across users, which would smear the signal out. > > I'll think on this for a bit, if there was an obvious out in the above > paragraph, hit me with a clue stick. Yeah, I don't see anything obvious either. Let's table this issue then, and assume that our tests are comprehensive enough. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... File components/history/core/browser/top_sites_database.cc (right): https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:66: static const int kDeprecatedVersionNumber = 2; // and earlier. On 2017/03/07 01:34:31, Scott Hess wrote: > On 2017/03/04 01:35:32, pwnall wrote: > > I think this breaks TopSitesDatabaseTest.Version2, same deal as above. Do we > > have a test that makes sure we can read from Version 3? (same issue about > > Version 7 before). > > Current-version tests are under Version/Recovery rather than VersionN/RecoveryN. > If a new version comes out, my expectation would be to copy/paste those tests, > rename one to add the right N, dupe the golden files, etc. Fair enough! I think you'd get less churn if all the tests would use a VersionN schema, because adding a new version would only cause a diff of add VersionN, instead of add VersionN-1 and change Version. At the same time, I don't think it's worth changing things, given that the schemas seems to have settled completely. https://codereview.chromium.org/2727553006/diff/20001/components/history/core... components/history/core/browser/top_sites_database.cc:221: // seen for a few thousand database files. On 2017/03/07 01:34:31, Scott Hess wrote: > On 2017/03/04 01:35:32, pwnall wrote: > > A percentage might give the reader a better intuition here. > > The databases in this state were previously persistent, meaning they accumulate > in this bucket over time (but only once per launch due handle poisoning). The > databases fixed fall will hit once under SUCCESS, but then off the radar, so I'm > not sure there's a comparable denominator. Over the past 28 days, there have > been 125k users who have recovered this database 170k times (um), but 37k users > failed over 900k attempts, so the max is like 30% stuck here. The > number-recovered was much higher originally (due to accumulated corruption which > has been recovered), so I think the likely real value is in the neighborhood of > 5%. > > Which is on top of being a fraction of a percent of the total userbase (I mean > "users with newly-corrupted databases" because "users who ever saw corruption in > any database" is more like a few percent last time I tried to quantify it). > > So my goal in the comment is to suggest that the aggregate total number is less > than some "not worth bothering with" epsilon. But I also don't want to write a > learned tome in the comment. Percentage-wise, even the top-line number is maybe > not worth the effort once the overhang is knocked down, but once dealt with it's > also not complicated to keep dealing with it. Seems like we can use the 1bn+ users, which is a public number, to say this case hits < 0.0037% of our users? I do agree that it's negligible.
On 2017/03/07 01:34:31, Scott Hess wrote: > I'm going to think on the "Should there be a histogram to track sizes, and > where" while waiting for the trybots to process. LGTM. I assume the pause means you didn't get any new insight past what was said in the comments. I agree that there doesn't seem to be a solution that works for the general case, so let's punt on that and get this cleanup committed :)
On 2017/03/16 18:57:31, pwnall wrote: > On 2017/03/07 01:34:31, Scott Hess wrote: > > I'm going to think on the "Should there be a histogram to track sizes, and > > where" while waiting for the trybots to process. > > LGTM. > > I assume the pause means you didn't get any new insight past what was said in > the comments. I agree that there doesn't seem to be a solution that works for > the general case, so let's punt on that and get this cleanup committed :) I think it meant I got distracted, and every time I've come back I've been like "I want to think about something else". I'll circle back and push through, and if nothing major comes up I'll land it.
On 2017/03/31 22:43:44, Scott Hess wrote: > On 2017/03/16 18:57:31, pwnall wrote: > > On 2017/03/07 01:34:31, Scott Hess wrote: > > > I'm going to think on the "Should there be a histogram to track sizes, and > > > where" while waiting for the trybots to process. > > > > LGTM. > > > > I assume the pause means you didn't get any new insight past what was said in > > the comments. I agree that there doesn't seem to be a solution that works for > > the general case, so let's punt on that and get this cleanup committed :) > > I think it meant I got distracted, and every time I've come back I've been like > "I want to think about something else". I'll circle back and push through, and > if nothing major comes up I'll land it. I think that landing this and potentially following up with a CL for histograms would be slightly better for both of our mental caches :)
The CQ bit was checked by shess@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
shess@chromium.org changed reviewers: + isherman@chromium.org, sky@chromium.org
On 2017/04/12 18:12:29, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, Oh, duh, that makes sense. sky@ for history, isherman@ for histograms, thanks!
LGTM
histograms.xml lgtm, thanks for the cleanup
The CQ bit was checked by shess@chromium.org
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": 60001, "attempt_start_ts": 1492029099866680,
"parent_rev": "7f507c20c1fb15aea209777169f831019aaf6874", "commit_rev":
"5207e145f18d167484e8941c7fa4f6b38224bc76"}
Message was sent while issue was closed.
Description was changed from ========== [sql] Convert thumbnails and top-sites databases to auto-recovery. sql::Recovery::RecoverDatabase() seems to work pretty well for Shortcuts, so backfill to use it for these two databases. Histograms indicate that there's nothing really special about these databases which demands special treatment. BUG=240396,597785 ========== to ========== [sql] Convert thumbnails and top-sites databases to auto-recovery. sql::Recovery::RecoverDatabase() seems to work pretty well for Shortcuts, so backfill to use it for these two databases. Histograms indicate that there's nothing really special about these databases which demands special treatment. BUG=240396,597785 Review-Url: https://codereview.chromium.org/2727553006 Cr-Commit-Position: refs/heads/master@{#464213} Committed: https://chromium.googlesource.com/chromium/src/+/5207e145f18d167484e8941c7fa4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5207e145f18d167484e8941c7fa4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
