Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961903003:
PageTransition: Remove unreached code. (Closed)
|
Created:
3 years, 5 months ago by arthursonzogni Modified:
3 years, 5 months ago CC:
chromium-reviews, nasko+codewatch_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPageTransition: Remove unreached code.
Remove a block of code that used to detect a incorrect PageTransition
and to replace it by its correct value.
Apparently, this block is no more reached. This CL removes it.
Furthermore, a test is added. It replays the steps one had to do to
reach this block of code.
BUG=736772, 740461
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2961903003
Cr-Commit-Position: refs/heads/master@{#485287}
Committed: https://chromium.googlesource.com/chromium/src/+/b757c63d2e76e4ea078e3ca4ca873d5b594073ea
Patch Set 1 : Check that the block of code is unreached. #Patch Set 2 : CHECK => DCHECK, Add test. #
Total comments: 4
Patch Set 3 : Add TODO (and rebase) #
Messages
Total messages: 33 (26 generated)
The CQ bit was checked by arthursonzogni@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.
Description was changed from ========== [TEST] Replace an unreachable block by a check. BUG=736772 ========== to ========== [TEST] Replace an unreachable block by a check. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by arthursonzogni@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by arthursonzogni@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 ========== [TEST] Replace an unreachable block by a check. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PageTransition: Remove unused code. Remove a block of code that used to detect a wrong PageTransition and to replace it with its correct value. This block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps that are described in the removed block of code. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PageTransition: Remove unused code. Remove a block of code that used to detect a wrong PageTransition and to replace it with its correct value. This block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps that are described in the removed block of code. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a wrong PageTransition and to replace it with its correct one. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
arthursonzogni@chromium.org changed reviewers: + avi@chromium.org, creis@chromium.org
Hi Avi and Charlie, Please take a look. https://codereview.chromium.org/2961903003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2961903003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6945: ui::PAGE_TRANSITION_FROM_ADDRESS_BAR))); That's interesting... In the past, I bet it used to be ui::PAGE_TRANSITION_MANUAL_SUBFRAME instead, isn't it?
Description was changed from ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a wrong PageTransition and to replace it with its correct one. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a incorrect PageTransition and to replace it by its correct value. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This lgtm; Charlie? https://codereview.chromium.org/2961903003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2961903003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6945: ui::PAGE_TRANSITION_FROM_ADDRESS_BAR))); On 2017/06/28 15:19:30, arthursonzogni wrote: > That's interesting... > In the past, I bet it used to be ui::PAGE_TRANSITION_MANUAL_SUBFRAME instead, > isn't it? I don't know. The history of Page Transitions is a mystery to me; I've always known them as unreliable. Maybe Charlie knows. https://codereview.chromium.org/2961903003/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2961903003/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4945: DCHECK(ui::PageTransitionIsMainFrame(params.transition)); Just wow. Hoping that this really does hold and we don't need that block.
Thanks for the review avi! Charlie, are you okay with this change?
Thanks for finding this. I'm a little wary about it since you may have actually found a bug, but I still think we should proceed now that we have a test. If we do have to fix the transition type to MANUAL_SUBFRAME, we should probably fix it in the browser process rather than in this RenderFrameImpl hack. In that sense, this LGTM whether the transition type thing is a bug or not. https://codereview.chromium.org/2961903003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2961903003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6945: ui::PAGE_TRANSITION_FROM_ADDRESS_BAR))); On 2017/06/28 17:03:17, Avi (ping after 24h) wrote: > On 2017/06/28 15:19:30, arthursonzogni wrote: > > That's interesting... > > In the past, I bet it used to be ui::PAGE_TRANSITION_MANUAL_SUBFRAME instead, > > isn't it? > > I don't know. The history of Page Transitions is a mystery to me; I've always > known them as unreliable. Maybe Charlie knows. Huh, this might explain why the code isn't reached anymore. I wonder if this is a bug, and if any code is miscounting subframe navigations now because we have the wrong transition type. My guess is that the switch to FrameNavigationEntries made it so that we don't update the transition type of the NavigationEntry anymore. Maybe we used to do it in RendererDidNavigateNewSubframe, but now we only do it in RendererDidNavigateToNewPage? We may be on thin ice until we understand that (in that we might have to fix the bug and then need to handle this case another way), but I'm glad you're adding the test to make sure the outcome is correct. Maybe it's sufficient for now to put a TODO on this EXPECT_TRUE that it's an open question whether this particular behavior is correct or if it should be MANUAL_SUBFRAME.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by arthursonzogni@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 for the reviews! I added a TODO and filled https://crbug.com/740461.
Description was changed from ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a incorrect PageTransition and to replace it by its correct value. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a incorrect PageTransition and to replace it by its correct value. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772,740461 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2961903003/#ps80001 (title: "Add TODO (and rebase)")
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": 1499703125035920,
"parent_rev": "e3a8263c6440700f8eb91b3c00b844784c3665cc", "commit_rev":
"b757c63d2e76e4ea078e3ca4ca873d5b594073ea"}
Message was sent while issue was closed.
Description was changed from ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a incorrect PageTransition and to replace it by its correct value. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772,740461 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PageTransition: Remove unreached code. Remove a block of code that used to detect a incorrect PageTransition and to replace it by its correct value. Apparently, this block is no more reached. This CL removes it. Furthermore, a test is added. It replays the steps one had to do to reach this block of code. BUG=736772,740461 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2961903003 Cr-Commit-Position: refs/heads/master@{#485287} Committed: https://chromium.googlesource.com/chromium/src/+/b757c63d2e76e4ea078e3ca4ca87... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/b757c63d2e76e4ea078e3ca4ca87... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
