Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(131)

Issue 10753019: Merge 144201 - More comprehensive handling of NOTIFICATION_NAV_ENTRY_PENDING for GoogleURLTracker. (Closed)

Created:
8 years, 5 months ago by Peter Kasting
Modified:
8 years, 5 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews
Visibility:
Public.

Description

Merge 144201 - More comprehensive handling of NOTIFICATION_NAV_ENTRY_PENDING for GoogleURLTracker. In particular, there are three cases that are improved: (1) We didn't deal well with further searches in the same tab that was already showing an infobar. Depending on ordering, various kinds of badness could occur, in particular trying to deref infobar_map_.end(). (2) If we caught a PENDING search, but before the load committed the user performed some other navigation, we'd show the infobar on the commit for the later navigation, leading to infobars on random pages. (3) In a generalization of (2), we now listen for PENDING in many more cases so that we know if the user is starting a new navigation, which may be the only way we get signalled that a previous navigation has been cancelled. (Note that if a pending navigation is simply stopped, and we were waiting for commit, we'll keep waiting, but this is harmless since eventually either the tab will be closed or the user will start another navigation.) These changes should hopefully make GoogleURLTracker as robust about navigation-handling as AlternateNavURLFetcher already was. Additionally, I reverted a previous change where a pending search would change the "search_url" of a visible infobar in that tab. Because we don't get notified when a load is stopped, we can't distinguish between "started a new search that hasn't committed" and "started but then cancelled a new search", and thus it's potentially wrong to search for the new URL (yet). We now once again wait until a search actually commits. In doing this I simplified things so that we only even pass in the search URL when a load commits. BUG=133108 TEST=Edit your preferences file so that you get prompted about changing Google search URLs. Do a Google search in one tab, wait for the infobar to appear, then do another search in the same tab. The infobar should stick around and interacting with it shouldn't cause crashes. Review URL: https://chromiumcodereview.appspot.com/10630022 TBR=pkasting@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145828

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+465 lines, -220 lines) Patch
M chrome/browser/google/google_url_tracker.h View 7 chunks +43 lines, -19 lines 0 comments Download
M chrome/browser/google/google_url_tracker.cc View 13 chunks +201 lines, -90 lines 0 comments Download
M chrome/browser/google/google_url_tracker_unittest.cc View 26 chunks +221 lines, -111 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
Peter Kasting
8 years, 5 months ago (2012-07-10 02:21:03 UTC) #1

          

Powered by Google App Engine
This is Rietveld 408576698