Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561163002:
Fix snapshots from Downloads being deleted by last_n. (Closed)
|
Created:
4 years ago by carlosk Modified:
4 years ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org, fgorski, dewittj Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix snapshots from Downloads being deleted by last_n.
Partially reverts the change that fixed good snapshots being deleted by
failed ones in the context of last_n
(https://codereview.chromium.org/2542833003/) because it created a new bug
were pages offline-d through Downloas were being incorrectly erased
(https://crbug.com/671955). The reverted fix will be solved again later using a
different approach.
The reason to make this revert only partial is to keep the test
improvements made which are still useful and valid. One test that had
to be disabled because of the fix being removed. A new test was also
added to confirm the incorrect snapshot deletion issue and avoid future
occurrences.
BUG=655697, 671955
Committed: https://crrev.com/3d768b7fc9d17d2df1f6d6d2d46758944e3bf09a
Cr-Commit-Position: refs/heads/master@{#438314}
Patch Set 1 #Patch Set 2 : Reverted only the runtime code; kept test improvements; added new test for new bug. #Patch Set 3 : Added missing test comments. #Patch Set 4 : Disabled RequestCoordinator factory to fix SQL errors #
Total comments: 16
Patch Set 5 : RequestCoordinatorFactory is disabled by reusing existing mocking code (and a bit more). #
Total comments: 3
Patch Set 6 : Minor test simplification. #
Dependent Patchsets: Messages
Total messages: 39 (22 generated)
The CQ bit was checked by carlosk@chromium.org
Created Revert of Failed offline snapshots won't erase a successful one from the same page load.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Revert of Failed offline snapshots won't erase a successful one from the same page load. (patchset #2 id:20001 of https://codereview.chromium.org/2542833003/ ) Reason for revert: This created a new issue: https://crbug.com/671955. And the approach to fix this will be different: - The problem this CL fixes will decrease in importance once MHTML generation stops failing because of missing iframes: https://crbug.com/655708 - We intend to soon refactor RecentTabHelper and that should finally fix this issue. Original issue's description: > Failed offline snapshots won't erase a successful one from the same page load. > > If a second offline snapshot starts for the same page load where a > previous one was saved, it should not immediately delete it: as it might > fail the user would end up with no offline copy of the page. This change > fixes this issue. > > Three new tests are added: one to cover this precise case and a couple > more to check for related situations. Also many comments were added or > updated. > > BUG=655697 > > Committed: https://crrev.com/96de1925bdf5c051bb903581a427d7f9795c0032 > Cr-Commit-Position: refs/heads/master@{#436086} TBR=fgorski@chromium.org,dewittj@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=655697 ========== to ========== Revert of Failed offline snapshots won't erase a successful one from the same page load. (patchset #2 id:20001 of https://codereview.chromium.org/2542833003/ ) Reason for revert: This created a new issue: https://crbug.com/671955. And the approach to fix this will be different: - The problem this CL fixes will decrease in importance once MHTML generation stops failing because of missing iframes: https://crbug.com/655708 - We intend to soon refactor RecentTabHelper and that should finally fix this issue. Original issue's description: > Failed offline snapshots won't erase a successful one from the same page load. > > If a second offline snapshot starts for the same page load where a > previous one was saved, it should not immediately delete it: as it might > fail the user would end up with no offline copy of the page. This change > fixes this issue. > > Three new tests are added: one to cover this precise case and a couple > more to check for related situations. Also many comments were added or > updated. > > BUG=655697 > > Committed: https://crrev.com/96de1925bdf5c051bb903581a427d7f9795c0032 > Cr-Commit-Position: refs/heads/master@{#436086} TBR=fgorski@chromium.org,dewittj@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=655697,671955 ==========
carlosk: looks like this never landed?
On 2016/12/09 22:32:47, dewittj wrote: > carlosk: looks like this never landed? Yes, I am aware and in fact glad it didn't as it would revery much more than needed. I'm working with the patch locally so to keep the test improvements and other stuff in place and only revert the changes to recent_tab_helper.*. I'm also adding a new test that covers the very bug the reverted change introduced.
The CQ bit was checked by carlosk@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...
Description was changed from ========== Revert of Failed offline snapshots won't erase a successful one from the same page load. (patchset #2 id:20001 of https://codereview.chromium.org/2542833003/ ) Reason for revert: This created a new issue: https://crbug.com/671955. And the approach to fix this will be different: - The problem this CL fixes will decrease in importance once MHTML generation stops failing because of missing iframes: https://crbug.com/655708 - We intend to soon refactor RecentTabHelper and that should finally fix this issue. Original issue's description: > Failed offline snapshots won't erase a successful one from the same page load. > > If a second offline snapshot starts for the same page load where a > previous one was saved, it should not immediately delete it: as it might > fail the user would end up with no offline copy of the page. This change > fixes this issue. > > Three new tests are added: one to cover this precise case and a couple > more to check for related situations. Also many comments were added or > updated. > > BUG=655697 > > Committed: https://crrev.com/96de1925bdf5c051bb903581a427d7f9795c0032 > Cr-Commit-Position: refs/heads/master@{#436086} TBR=fgorski@chromium.org,dewittj@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=655697,671955 ========== to ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 ==========
Description was changed from ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 ========== to ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 ==========
carlosk@chromium.org changed reviewers: + dimich@chromium.org - dewittj@chromium.org, fgorski@chromium.org
I converted this revert to be more of a regular patch. And that's why one should not mix different issues into a single patch. :) dimich@: PTAL fgorski@, dewittj@: FYI.
I added a new patch because I forgot to add some comments in the test file. I also noticed that when running locally the newly added test is somewhat flaky. This is what I see in the logs: I 89.271s run_tests_on_device(00b12f19f4314037) [ RUN ] RecentTabHelperTest.TwoDownloadCapturesInARowSamePage I 89.271s run_tests_on_device(00b12f19f4314037) [ERROR:connection.cc(1947)] BackgroundRequestQueue sqlite error 1802, errno 0: disk I/O error, sql: CREATE TABLE IF NOT EXISTS request_queue_v1 (request_id INTE GER PRIMARY KEY NOT NULL, creation_time INTEGER NOT NULL, activation_time INTEGER NOT NULL DEFAULT 0, last_attempt_time INTEGER NOT NULL DEFAULT 0, started_attempt_count INTEGER NOT NULL, completed_attempt_count INTEGER NOT NULL, state INTEGER NOT NULL DEFAULT 0, url VARCHAR NOT NULL, client_namespace VARCHAR NOT NULL, client_id VARCHAR NOT NULL) I 89.271s run_tests_on_device(00b12f19f4314037) [FATAL:connection.cc(1962)] disk I/O error I 89.271s run_tests_on_device(00b12f19f4314037) #00 0xdf5921a7 /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x0009a1a7 I 89.272s run_tests_on_device(00b12f19f4314037) #01 0xdf15358f /data/app/org.chromium.native_test-2/lib/arm/libsql.cr.so+0x0000b58f I 89.272s run_tests_on_device(00b12f19f4314037) #02 0xdf153655 /data/app/org.chromium.native_test-2/lib/arm/libsql.cr.so+0x0000b655 I 89.272s run_tests_on_device(00b12f19f4314037) #03 0xd72ed90d /data/app/org.chromium.native_test-2/lib/arm/lib_unit_tests__library.cr.so+0x0109f90d I 89.272s run_tests_on_device(00b12f19f4314037) #04 0xd72ede1f /data/app/org.chromium.native_test-2/lib/arm/lib_unit_tests__library.cr.so+0x0109fe1f I 89.272s run_tests_on_device(00b12f19f4314037) #05 0xd72dad65 /data/app/org.chromium.native_test-2/lib/arm/lib_unit_tests__library.cr.so+0x0108cd65 I 89.272s run_tests_on_device(00b12f19f4314037) #06 0xdf5d48a9 /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000dc8a9 I 89.272s run_tests_on_device(00b12f19f4314037) #07 0xdf5d4aeb /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000dcaeb I 89.272s run_tests_on_device(00b12f19f4314037) #08 0xdf5d52e1 /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000dd2e1 I 89.272s run_tests_on_device(00b12f19f4314037) #09 0xdf5d16dd /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000d96dd I 89.272s run_tests_on_device(00b12f19f4314037) #10 0xf7146883 /system/lib/libc.so+0x0003f883 I 89.272s run_tests_on_device(00b12f19f4314037) #11 0xf7120f75 /system/lib/libc.so+0x00019f75 I 89.272s run_tests_on_device(00b12f19f4314037) #12 0xffffffff <unknown> I 89.272s run_tests_on_device(00b12f19f4314037) I 89.272s run_tests_on_device(00b12f19f4314037) [ERROR:test_suite.cc(295)] Currently running: RecentTabHelperTest.TwoDownloadCapturesInARowSamePage Any ideas on why?
I think it fails because the ObserveAndDownloadCurrentPage creates an instance of RequestCoordinator and that one tries to create/open SQLite DB which is not working in unittests because of the threading setup I think. For those tests to work, the one good way is to mock out RequestCoordinator, just like OfflinePageModel is mocked out already. I think this was the reason the TEST_DownloadRequest was disabled before... On Mon, Dec 12, 2016 at 4:40 PM <carlosk@chromium.org> wrote: > I added a new patch because I forgot to add some comments in the test file. > > I also noticed that when running locally the newly added test is somewhat > flaky. > This is what I see in the logs: > > I 89.271s run_tests_on_device(00b12f19f4314037) [ RUN ] > RecentTabHelperTest.TwoDownloadCapturesInARowSamePage > I 89.271s run_tests_on_device(00b12f19f4314037) [ERROR:connection.cc(1947)] > BackgroundRequestQueue sqlite error 1802, errno 0: disk I/O error, sql: > CREATE > TABLE IF NOT EXISTS request_queue_v1 (request_id INTE > GER PRIMARY KEY NOT NULL, creation_time INTEGER NOT NULL, activation_time > INTEGER NOT NULL DEFAULT 0, last_attempt_time INTEGER NOT NULL DEFAULT 0, > started_attempt_count INTEGER NOT NULL, completed_attempt_count > INTEGER NOT NULL, state INTEGER NOT NULL DEFAULT 0, url VARCHAR NOT NULL, > client_namespace VARCHAR NOT NULL, client_id VARCHAR NOT NULL) > I 89.271s run_tests_on_device(00b12f19f4314037) [FATAL:connection.cc(1962)] > disk I/O error > I 89.271s run_tests_on_device(00b12f19f4314037) #00 0xdf5921a7 > /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x0009a1a7 > I 89.272s run_tests_on_device(00b12f19f4314037) #01 0xdf15358f > /data/app/org.chromium.native_test-2/lib/arm/libsql.cr.so+0x0000b58f > I 89.272s run_tests_on_device(00b12f19f4314037) #02 0xdf153655 > /data/app/org.chromium.native_test-2/lib/arm/libsql.cr.so+0x0000b655 > I 89.272s run_tests_on_device(00b12f19f4314037) #03 0xd72ed90d > /data/app/org.chromium.native_test-2/lib/arm/lib_unit_tests__library.cr.so > +0x0109f90d > I 89.272s run_tests_on_device(00b12f19f4314037) #04 0xd72ede1f > /data/app/org.chromium.native_test-2/lib/arm/lib_unit_tests__library.cr.so > +0x0109fe1f > I 89.272s run_tests_on_device(00b12f19f4314037) #05 0xd72dad65 > /data/app/org.chromium.native_test-2/lib/arm/lib_unit_tests__library.cr.so > +0x0108cd65 > I 89.272s run_tests_on_device(00b12f19f4314037) #06 0xdf5d48a9 > /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000dc8a9 > I 89.272s run_tests_on_device(00b12f19f4314037) #07 0xdf5d4aeb > /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000dcaeb > I 89.272s run_tests_on_device(00b12f19f4314037) #08 0xdf5d52e1 > /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000dd2e1 > I 89.272s run_tests_on_device(00b12f19f4314037) #09 0xdf5d16dd > /data/app/org.chromium.native_test-2/lib/arm/libbase.cr.so+0x000d96dd > I 89.272s run_tests_on_device(00b12f19f4314037) #10 0xf7146883 > /system/lib/libc.so+0x0003f883 > I 89.272s run_tests_on_device(00b12f19f4314037) #11 0xf7120f75 > /system/lib/libc.so+0x00019f75 > I 89.272s run_tests_on_device(00b12f19f4314037) #12 0xffffffff <unknown> > I 89.272s run_tests_on_device(00b12f19f4314037) > I 89.272s run_tests_on_device(00b12f19f4314037) [ERROR:test_suite.cc(295)] > Currently running: RecentTabHelperTest.TwoDownloadCapturesInARowSamePage > > Any ideas on why? > > https://codereview.chromium.org/2561163002/ > -- 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.
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
That was the problem indeed. Fixed in the latest patchset.
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.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:201: ChromeRenderViewHostTestHarness::TearDown(); per my other comment you don't need this code, but if you did, I think this isn't the right order for a tear down operation. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:480: const int64_t second_offline_id = 123l; nit: 123L https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:481: ASSERT_NE(second_offline_id, first_offline_id); how is the first offline id assigned? https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:489: { nit: I am not sure why you are using {} here. That don't seem to match out code anywhere. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:498: const int64_t third_offline_id = 456l; ditto https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:500: ASSERT_NE(third_offline_id, second_offline_id); you control both IDs. I am not sure this line makes sense. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:511: int third_index = FindPageIndexForOfflineId(third_offline_id); given how you use the index below, wouldn't it make more sense to return iterator from the FindPage... method? https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... File chrome/browser/android/offline_pages/request_coordinator_factory.h (right): https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.h:39: void ReturnNullptrForTesting(bool enabled) { always_nullptr_ = enabled; } I don't think changing anything about request coordinator should be necessary here. Did you try using: SetTestingFactoryAndUse And passing in a method that returns nullptr? As an alternative you can use this: https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/tes... in your test. This method was build for what you are trying to do. Example usage is in offline_page_utils_unittest.cc https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off...
The CQ bit was checked by carlosk@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...
Thanks! https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:201: ChromeRenderViewHostTestHarness::TearDown(); On 2016/12/13 17:17:38, fgorski wrote: > per my other comment you don't need this code, but if you did, I think this > isn't the right order for a tear down operation. Removed. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:480: const int64_t second_offline_id = 123l; On 2016/12/13 17:17:38, fgorski wrote: > nit: 123L Done here and in the last test in this file where I based this from. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:481: ASSERT_NE(second_offline_id, first_offline_id); On 2016/12/13 17:17:38, fgorski wrote: > how is the first offline id assigned? The first entry is a last_n one and its offline ID is automatically set by the offline model. Downloads do "suggest" their own IDs, hence the difference. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:489: { On 2016/12/13 17:17:38, fgorski wrote: > nit: I am not sure why you are using {} here. > That don't seem to match out code anywhere. I use them to isolate the internal scope and avoid wrong copy/pasted code that incorrectly reuses variables. In this case I didn't want |second_index| to be reused by mistake in the similar section below. But I agree it kinds looks weird here so I'm removing it. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:498: const int64_t third_offline_id = 456l; On 2016/12/13 17:17:38, fgorski wrote: > ditto Done. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:500: ASSERT_NE(third_offline_id, second_offline_id); On 2016/12/13 17:17:38, fgorski wrote: > you control both IDs. I am not sure this line makes sense. Done. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:511: int third_index = FindPageIndexForOfflineId(third_offline_id); On 2016/12/13 17:17:38, fgorski wrote: > given how you use the index below, wouldn't it make more sense to return > iterator from the FindPage... method? I agree with what you're saying given the usage below but I find iterator comparisons more complicated so I switched the method to returning a pointer. https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... File chrome/browser/android/offline_pages/request_coordinator_factory.h (right): https://codereview.chromium.org/2561163002/diff/150001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.h:39: void ReturnNullptrForTesting(bool enabled) { always_nullptr_ = enabled; } On 2016/12/13 17:17:38, fgorski wrote: > I don't think changing anything about request coordinator should be necessary > here. > > Did you try using: SetTestingFactoryAndUse > And passing in a method that returns nullptr? > > As an alternative you can use this: > https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/tes... > in your test. This method was build for what you are trying to do. > Example usage is in offline_page_utils_unittest.cc > https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off... Done. Thanks for pointing it out. I did not know about SetTestingFactoryAndUse. https://codereview.chromium.org/2561163002/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2561163002/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:180: ChromeRenderViewHostTestHarness::SetUp(); I left this change because it makes the code more future proof.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm with one more change suggestion. https://codereview.chromium.org/2561163002/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2561163002/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:478: ASSERT_NE(second_offline_id, first_offline_id); Since you know the first_offline_id, and you can specify the second and third, why not simply increment the first one to do so and never risk (I know chance is slim) a random failure of this test.
https://codereview.chromium.org/2561163002/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2561163002/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:478: ASSERT_NE(second_offline_id, first_offline_id); On 2016/12/13 21:24:17, fgorski wrote: > Since you know the first_offline_id, and you can specify the second and third, > why not simply increment the first one to do so and never risk (I know chance is > slim) a random failure of this test. Done.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2561163002/#ps190001 (title: "Minor test simplification.")
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": 190001, "attempt_start_ts": 1481665869831820,
"parent_rev": "9488a25e9ef0b8e237bbcbae5c203f685d7a3346", "commit_rev":
"c2417a52c4f582ecb5b154128fad72e09ba2292f"}
Message was sent while issue was closed.
Description was changed from ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 ========== to ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 Review-Url: https://codereview.chromium.org/2561163002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 Review-Url: https://codereview.chromium.org/2561163002 ========== to ========== Fix snapshots from Downloads being deleted by last_n. Partially reverts the change that fixed good snapshots being deleted by failed ones in the context of last_n (https://codereview.chromium.org/2542833003/) because it created a new bug were pages offline-d through Downloas were being incorrectly erased (https://crbug.com/671955). The reverted fix will be solved again later using a different approach. The reason to make this revert only partial is to keep the test improvements made which are still useful and valid. One test that had to be disabled because of the fix being removed. A new test was also added to confirm the incorrect snapshot deletion issue and avoid future occurrences. BUG=655697,671955 Committed: https://crrev.com/3d768b7fc9d17d2df1f6d6d2d46758944e3bf09a Cr-Commit-Position: refs/heads/master@{#438314} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3d768b7fc9d17d2df1f6d6d2d46758944e3bf09a Cr-Commit-Position: refs/heads/master@{#438314} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
