Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236403004:
Handling new frames and frame navigations with find-in-page during a find session. (Closed)
|
Created:
4 years, 4 months ago by paulmeyer Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_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. |
DescriptionHandling new frames and frame navigations with find-in-page during a find session.
This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar.
Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates).
Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKegg/view
BUG=2220, 621660
Committed: https://crrev.com/ec9c7f30eb10ab3fc9ab6707f7b736446ab89aa1
Cr-Commit-Position: refs/heads/master@{#420715}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Rebased and updated comment. #
Total comments: 1
Messages
Total messages: 38 (19 generated)
The CQ bit was checked by paulmeyer@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...
paulmeyer@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@ for review.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
On 2016/08/12 21:30:15, paulmeyer wrote: > +dcheng@ for review. Actually, if you'll wait a bit on the review, I'm going to write up a design doc to explain what's going on here.
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. BUG=2220,621660 ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Design doc: https://docs.google.com/a/google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxl... BUG=2220,621660 ==========
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Design doc: https://docs.google.com/a/google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxl... BUG=2220,621660 ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Design doc: https://docs.google.com/a/google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxl... BUG=2220,621660 ==========
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Design doc: https://docs.google.com/a/google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxl... BUG=2220,621660 ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 ==========
Patchset #1 (id:40001) has been deleted
On 2016/08/15 18:17:26, paulmeyer wrote: > On 2016/08/12 21:30:15, paulmeyer wrote: > > +dcheng@ for review. > > Actually, if you'll wait a bit on the review, I'm going to write up a design doc > to explain what's going on here. Okay, all ready.
Overall looks reasonable. Most of the comments are because I don't understand the code nearly well enough =) https://codereview.chromium.org/2236403004/diff/60001/chrome/browser/ui/find_... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/2236403004/diff/60001/chrome/browser/ui/find_... chrome/browser/ui/find_bar/find_bar_controller.cc:162: if (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && Please commenton this fixin the CL description too (I assume this is to keep the find bar from disappearing for subframe navigations) https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:159: matches_per_frame_it->second = number_of_matches; This used to get set to 0, now I'm not sure where that happens? https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:548: pending_find_next_reply_) { Can you help me understand the purpose of the next two changes? I don't know the find logic quite as well. https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:203: m_lastFindRequestCompletedWithNoMatches = false; Ditto: what is the significance behind these two changes? I just don't have enough of the find state machine in my head to understand the purpose. https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1943: if (!hasVisibleContent() && !options.force) { Why do we want to force a research if the frame's not visible? I don't think I understand the ordering of these two checks (above as well) https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFindOptions.h (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFindOptions.h:57: // useful when a dynamic content change is suspected. Let's be more specific: "For a re-search of the frame: typically used when forcing a re-search after the frame navigates."
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 ==========
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Thanks for getting to this. I know you're really swamped right now. PTAL https://codereview.chromium.org/2236403004/diff/60001/chrome/browser/ui/find_... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/2236403004/diff/60001/chrome/browser/ui/find_... chrome/browser/ui/find_bar/find_bar_controller.cc:162: if (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && On 2016/09/21 07:15:44, dcheng wrote: > Please commenton this fixin the CL description too (I assume this is to keep the > find bar from disappearing for subframe navigations) Done. https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:159: matches_per_frame_it->second = number_of_matches; On 2016/09/21 07:15:44, dcheng wrote: > This used to get set to 0, now I'm not sure where that happens? I'm not sure exactly what you mean. It is possible that |number_of_matches| is 0, which would set |matches_per_frame_it->second| to 0. Or, if you mean the initial state of each entry in |matches_per_frame_|, that is initialized to 0 in FindRequestManager::AddFrame(). https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:548: pending_find_next_reply_) { On 2016/09/21 07:15:44, dcheng wrote: > Can you help me understand the purpose of the next two changes? I don't know the > find logic quite as well. First change: So, really, this part of the check was always supposed to detect whether there would be another reply coming, and |request_id != current_request_.id| was a good approximation of that before |pending_find_next_reply_| existed (it is a more recent addition to the class that was needed for other reasons as well). Also, there were previously no cases where this would do the wrong thing. Now, this change is needed to allow the second part of this function to be reached in the case where a frame is added/navigated with matches that will come in a find reply that is not associated specifically with the current request. If after the frame change, there are matches in the page, but there is not yet an active match and no other replies expected, the second part of this function will ensure that an active match will still be found (there must always be an active match if there are any matches on the page). Second change: It was previously guaranteed that |request_id == current_request_.id| was true when reaching this part of the function, and that was relied upon. Now that it is not guaranteed, it is checked explicitly. https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:203: m_lastFindRequestCompletedWithNoMatches = false; On 2016/09/21 07:15:44, dcheng wrote: > Ditto: what is the significance behind these two changes? I just don't have > enough of the find state machine in my head to understand the purpose. First change: By this point in this function, at least one match has been found, so |m_lastFindRequestCompletedWithNoMatches| should definitely be false. Without this, there are some problems with TextFinder not searching certain frames when it really should. I actually ended up already landing this one line change separately in the mean time to fix these problems faster. This is no longer part of this patch. Second change: I'm not sure if this is actually specifically needed for this patch, but the active match should be reset if it is being cleared. I wrote this function myself in a previous patch, and I think I just missed doing this, so I figured I'd fix it. I could put this in a separate patch if you'd like though. https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1943: if (!hasVisibleContent() && !options.force) { On 2016/09/21 07:15:44, dcheng wrote: > Why do we want to force a research if the frame's not visible? I don't think I > understand the ordering of these two checks (above as well) This is because hasVisibleContent() is not perfect. It sometimes returns false when the frame does have visible content. I wasn't able to find an easy way to fix this function (and that's outside the scope of my patch), so I just bypassed it to make sure the frame gets searched if it is specified that it should be forcibly re-searched. I think that makes sense anyways that it shouldn't be held up by something like this if you're forcing it to be searched. https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFindOptions.h (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFindOptions.h:57: // useful when a dynamic content change is suspected. On 2016/09/21 07:15:44, dcheng wrote: > Let's be more specific: "For a re-search of the frame: typically used when > forcing a re-search after the frame navigates." Done.
Thanks for the detailed explanations https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:159: matches_per_frame_it->second = number_of_matches; On 2016/09/21 15:40:52, paulmeyer wrote: > On 2016/09/21 07:15:44, dcheng wrote: > > This used to get set to 0, now I'm not sure where that happens? > > I'm not sure exactly what you mean. It is possible that |number_of_matches| is > 0, which would set |matches_per_frame_it->second| to 0. Or, if you mean the > initial state of each entry in |matches_per_frame_|, that is initialized to 0 in > FindRequestManager::AddFrame(). if (int matches_delta = ...) If this evaluates to zero, this branch is never taken, and matches_per_frame_it->second is never set. https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:548: pending_find_next_reply_) { On 2016/09/21 15:40:52, paulmeyer wrote: > On 2016/09/21 07:15:44, dcheng wrote: > > Can you help me understand the purpose of the next two changes? I don't know > the > > find logic quite as well. > > First change: > > So, really, this part of the check was always supposed to detect whether there > would be another reply coming, and |request_id != current_request_.id| was a > good approximation of that before |pending_find_next_reply_| existed (it is a > more recent addition to the class that was needed for other reasons as well). > Also, there were previously no cases where this would do the wrong thing. Now, > this change is needed to allow the second part of this function to be reached in > the case where a frame is added/navigated with matches that will come in a find > reply that is not associated specifically with the current request. If after the > frame change, there are matches in the page, but there is not yet an active > match and no other replies expected, the second part of this function will > ensure that an active match will still be found (there must always be an active > match if there are any matches on the page). > > > Second change: > > It was previously guaranteed that |request_id == current_request_.id| was true > when reaching this part of the function, and that was relied upon. Now that it > is not guaranteed, it is checked explicitly. I think I only vaguely understand this, so I'll just assume it's right =) https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1943: if (!hasVisibleContent() && !options.force) { On 2016/09/21 15:40:52, paulmeyer wrote: > On 2016/09/21 07:15:44, dcheng wrote: > > Why do we want to force a research if the frame's not visible? I don't think I > > understand the ordering of these two checks (above as well) > > This is because hasVisibleContent() is not perfect. It sometimes returns false > when the frame does have visible content. I wasn't able to find an easy way to > fix this function (and that's outside the scope of my patch), so I just bypassed > it to make sure the frame gets searched if it is specified that it should be > forcibly re-searched. I think that makes sense anyways that it shouldn't be held > up by something like this if you're forcing it to be searched. Hmm, what are the cases where it's wrong? Would it make sense to file a bug?
https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2236403004/diff/60001/content/browser/find_re... content/browser/find_request_manager.cc:159: matches_per_frame_it->second = number_of_matches; On 2016/09/22 00:36:47, dcheng wrote: > On 2016/09/21 15:40:52, paulmeyer wrote: > > On 2016/09/21 07:15:44, dcheng wrote: > > > This used to get set to 0, now I'm not sure where that happens? > > > > I'm not sure exactly what you mean. It is possible that |number_of_matches| is > > 0, which would set |matches_per_frame_it->second| to 0. Or, if you mean the > > initial state of each entry in |matches_per_frame_|, that is initialized to 0 > in > > FindRequestManager::AddFrame(). > > if (int matches_delta = ...) > > If this evaluates to zero, this branch is never taken, and > matches_per_frame_it->second is never set. Oh, if that evaluates to 0, then |matches_per_frame_it->second| is already equal to |number_of_matches|, and doesn't need to be set. https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2236403004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1943: if (!hasVisibleContent() && !options.force) { On 2016/09/22 00:36:47, dcheng wrote: > On 2016/09/21 15:40:52, paulmeyer wrote: > > On 2016/09/21 07:15:44, dcheng wrote: > > > Why do we want to force a research if the frame's not visible? I don't think > I > > > understand the ordering of these two checks (above as well) > > > > This is because hasVisibleContent() is not perfect. It sometimes returns false > > when the frame does have visible content. I wasn't able to find an easy way to > > fix this function (and that's outside the scope of my patch), so I just > bypassed > > it to make sure the frame gets searched if it is specified that it should be > > forcibly re-searched. I think that makes sense anyways that it shouldn't be > held > > up by something like this if you're forcing it to be searched. > > Hmm, what are the cases where it's wrong? Would it make sense to file a bug? Okay, I'll try to file a bug. It's been a while since I originally looked into what was wrong with it though, so I'll have to do a bit more digging before I can say for sure which cases reproduce the bug.
Ok, LGTM (but let's file that bug, if the visibility check is busted, that's valuable information to know, since other code might be depending on that to be accurate-ish)
The CQ bit was checked by paulmeyer@chromium.org
The CQ bit was unchecked by paulmeyer@chromium.org
paulmeyer@chromium.org changed reviewers: + kenrb@chromium.org, pkasting@chromium.org
+pkasting@ for review of find_bar_controller.cc +kenrb@ for review of frame_messages.h
lgtm
LGTM https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... File chrome/browser/ui/find_bar/find_bar_controller.cc (right): https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... chrome/browser/ui/find_bar/find_bar_controller.cc:162: if (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && It seems a little surprising that we'd need this. I would expect the transition type to only be RELOAD for main frame navigations, and is_navigation_to_different_page() to only be set on main frame navigations as well. Is one of those not true?
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/23 00:12:34, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > chrome/browser/ui/find_bar/find_bar_controller.cc:162: if > (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && > It seems a little surprising that we'd need this. I would expect the transition > type to only be RELOAD for main frame navigations, and > is_navigation_to_different_page() to only be set on main frame navigations as > well. > > Is one of those not true? is_navigation_to_different_page() seems to still be true on subframe navigations (when to a different page).
On 2016/09/23 18:26:07, paulmeyer wrote: > On 2016/09/23 00:12:34, Peter Kasting wrote: > > LGTM > > > > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > > > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > > chrome/browser/ui/find_bar/find_bar_controller.cc:162: if > > (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && > > It seems a little surprising that we'd need this. I would expect the > transition > > type to only be RELOAD for main frame navigations, and > > is_navigation_to_different_page() to only be set on main frame navigations as > > well. > > > > Is one of those not true? > > is_navigation_to_different_page() seems to still be true on subframe navigations > (when to a different page). (Tiny note: try to reply on the code review tool rather than by email if possible, so your replies will be captured inline on each patch set.) OK. We might consider doing the is_main_frame check only in the is_navigation_to_different_page() case for clarity. That does make the conditional a little more awkwardly nested though.
On 2016/09/23 18:49:15, Peter Kasting wrote: > On 2016/09/23 18:26:07, paulmeyer wrote: > > On 2016/09/23 00:12:34, Peter Kasting wrote: > > > LGTM > > > > > > > > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > > > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > > > > > > > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > > > chrome/browser/ui/find_bar/find_bar_controller.cc:162: if > > > (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && > > > It seems a little surprising that we'd need this. I would expect the > > transition > > > type to only be RELOAD for main frame navigations, and > > > is_navigation_to_different_page() to only be set on main frame navigations > as > > > well. > > > > > > Is one of those not true? > > > > is_navigation_to_different_page() seems to still be true on subframe > navigations > > (when to a different page). > > (Tiny note: try to reply on the code review tool rather than by email if > possible, so your replies will be captured inline on each patch set.) > > OK. We might consider doing the is_main_frame check only in the > is_navigation_to_different_page() case for clarity. That does make the > conditional a little more awkwardly nested though. Actually, I just tested a bit more, and the RELOAD case can also occur not on the main frame (and inadvertently close the find bar). The case I found that reproduces that is: 1) Navigate to some page A that has two iframes. 2) Navigate the first iframe to page B (at a different site than page A). 3) Open the find bar and search for anything. 4) navigate the second iframe to page B. For whatever reason, the navigation in step (4) will have the transition type PAGE_TRANSITION_RELOAD, even though it is on a subframe. This also doesn't really seem like a reload to me at all, but maybe I'm not just not familiar with all the cases where a navigation should be considered a reload.
On 2016/09/23 19:31:29, paulmeyer wrote: > On 2016/09/23 18:49:15, Peter Kasting wrote: > > On 2016/09/23 18:26:07, paulmeyer wrote: > > > On 2016/09/23 00:12:34, Peter Kasting wrote: > > > > LGTM > > > > > > > > > > > > > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > > > > File chrome/browser/ui/find_bar/find_bar_controller.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2236403004/diff/120001/chrome/browser/ui/find... > > > > chrome/browser/ui/find_bar/find_bar_controller.cc:162: if > > > > (find_bar_->IsFindBarVisible() && commit_details->is_main_frame && > > > > It seems a little surprising that we'd need this. I would expect the > > > transition > > > > type to only be RELOAD for main frame navigations, and > > > > is_navigation_to_different_page() to only be set on main frame navigations > > as > > > > well. > > > > > > > > Is one of those not true? > > > > > > is_navigation_to_different_page() seems to still be true on subframe > > navigations > > > (when to a different page). > > > > (Tiny note: try to reply on the code review tool rather than by email if > > possible, so your replies will be captured inline on each patch set.) > > > > OK. We might consider doing the is_main_frame check only in the > > is_navigation_to_different_page() case for clarity. That does make the > > conditional a little more awkwardly nested though. > > Actually, I just tested a bit more, and the RELOAD case can also occur not on > the main frame (and inadvertently close the find bar). The case I found that > reproduces that is: > > 1) Navigate to some page A that has two iframes. > 2) Navigate the first iframe to page B (at a different site than page A). > 3) Open the find bar and search for anything. > 4) navigate the second iframe to page B. > > For whatever reason, the navigation in step (4) will have the transition type > PAGE_TRANSITION_RELOAD, even though it is on a subframe. This also doesn't > really seem like a reload to me at all, but maybe I'm not just not familiar with > all the cases where a navigation should be considered a reload. That sounds like a bug. Can you file that (bonus: with an easy testcase)? Hopefully creis or some other PlzNavigate folks can figure out what the right thing here is, as getting the transition type wrong can lead to issues like bad omnibox scoring or, in this case, potentially-confusing code.
Message was sent while issue was closed.
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 Committed: https://crrev.com/ec9c7f30eb10ab3fc9ab6707f7b736446ab89aa1 Cr-Commit-Position: refs/heads/master@{#420715} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ec9c7f30eb10ab3fc9ab6707f7b736446ab89aa1 Cr-Commit-Position: refs/heads/master@{#420715}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:120001) has been created in https://codereview.chromium.org/2363993003/ by gcasto@chromium.org. The reason for reverting is: This seems to be failing pretty consistently on some bots (e.g. https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...).
Message was sent while issue was closed.
Description was changed from ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 Committed: https://crrev.com/ec9c7f30eb10ab3fc9ab6707f7b736446ab89aa1 Cr-Commit-Position: refs/heads/master@{#420715} ========== to ========== Handling new frames and frame navigations with find-in-page during a find session. This patch implements new find-in-page functionality that allows for frames that are either newly added or navigated during a find session to be automatically searched so that matches in their new content are automatically highlighted and included in the results shown in the find bar. Also, there is a fix included in this patch to prevent the find-in-page bar from closing when a subframe navigates (the find session should only end if the main frame navigates). Design doc: https://docs.google.com/document/d/1r4F19FIKg4zPJCaSyJ9-T0sgFbxlCGKL3FjqQEAKe... BUG=2220,621660 Committed: https://crrev.com/ec9c7f30eb10ab3fc9ab6707f7b736446ab89aa1 Cr-Commit-Position: refs/heads/master@{#420715} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
