|
|
Created:
4 years, 4 months ago by Charlie Reis Modified:
4 years, 4 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNotify about PageState changes after an AUTO_SUBFRAME commit.
The session restore logic (specifically when exiting and restarting)
depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes.
In the old navigation logic, this happened for subframes during the
first UpdateState after an AUTO_SUBFRAME commit. In the new navigation
logic, we have already put the PageState on the FrameNavigationEntry
at commit time, so the UpdateState typically looks like a no-op and
doesn't generate a notification.
To fix this (and get the notification out as early as possible), this
CL notifies at commit time for AUTO_SUBFRAMEs.
BUG=638088
TEST=Visit chrome://help, then chrome://restart.
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c
Cr-Commit-Position: refs/heads/master@{#414207}
Patch Set 1 #Patch Set 2 : Fix unit test expectations. #Patch Set 3 : Add test. #Patch Set 4 : Add test file. #Patch Set 5 : Clean up. #
Total comments: 2
Patch Set 6 : Switch to ASSERT_TRUE. #
Messages
Total messages: 34 (25 generated)
Description was changed from ========== Notify about PageState changes after an AUTO_SUBFRAME commit. BUG=638088 TEST=Visit chrome://help, then chrome://restart. ========== to ========== Notify about PageState changes after an AUTO_SUBFRAME commit. BUG=638088 TEST=Visit chrome://help, then chrome://restart. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by creis@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by creis@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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by creis@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 checked by creis@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 ========== Notify about PageState changes after an AUTO_SUBFRAME commit. BUG=638088 TEST=Visit chrome://help, then chrome://restart. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
creis@chromium.org changed reviewers: + nasko@chromium.org, sky@chromium.org
nasko@, can you review? This is the fix you suggested, sending the notification at commit time instead of at UpdateState. I think that's a good approach. (It did require updating a few unit tests, which I did in a way that would still work if we later need to turn off UseSubframeNavigationEntries.) sky@, can you take a look at chrome/ for OWNERS on the test files? I can't easily test this in content/ because the bug only repros when we fully exit (not just closing/reopening a tab).
LGTM https://codereview.chromium.org/2275643003/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2275643003/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_browsertest.cc:1300: EXPECT_TRUE(content::ExecuteScript( ASSERT_TRUE? Otherwise if it fails the observer times out, so it seems better to fail early.
Thanks for putting this one together. LGTM!
The CQ bit was checked by creis@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2275643003/diff/80001/chrome/browser/sessions... File chrome/browser/sessions/session_restore_browsertest.cc (right): https://codereview.chromium.org/2275643003/diff/80001/chrome/browser/sessions... chrome/browser/sessions/session_restore_browsertest.cc:1300: EXPECT_TRUE(content::ExecuteScript( On 2016/08/24 20:25:57, sky wrote: > ASSERT_TRUE? Otherwise if it fails the observer times out, so it seems better to > fail early. Good idea. Done.
The CQ bit was unchecked by creis@chromium.org
The CQ bit was checked by creis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2275643003/#ps100001 (title: "Switch to ASSERT_TRUE.")
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
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_...)
On 2016/08/24 23:46:54, commit-bot: I haz the power wrote: > 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_...) These bots are flaky for everyone today. Same tests failing for someone else here: https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... I'm going to land anyway, since this is important to get fixed in Canary and all the other try jobs passed.
Description was changed from ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. NOTRY=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by creis@chromium.org
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 ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. NOTRY=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. NOTRY=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. NOTRY=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Notify about PageState changes after an AUTO_SUBFRAME commit. The session restore logic (specifically when exiting and restarting) depends on NOTIFICATION_NAV_ENTRY_CHANGED notifications for subframes. In the old navigation logic, this happened for subframes during the first UpdateState after an AUTO_SUBFRAME commit. In the new navigation logic, we have already put the PageState on the FrameNavigationEntry at commit time, so the UpdateState typically looks like a no-op and doesn't generate a notification. To fix this (and get the notification out as early as possible), this CL notifies at commit time for AUTO_SUBFRAMEs. BUG=638088 TEST=Visit chrome://help, then chrome://restart. NOTRY=true CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c Cr-Commit-Position: refs/heads/master@{#414207} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/59d5a47cb75df2754b71732c5581f2bb6be54c9c Cr-Commit-Position: refs/heads/master@{#414207} |