Chromium Code Reviews Chromium Code Reviews
Issue
4880003:
Fix a number of problems with the GoogleURLTracker (Closed)
DescriptionFix a number of problems with the GoogleURLTracker, mostly around the multiple-searches-simultaneously use case:
* The old code used members to track things like "what's the search URL". But since there's one GoogleURLTracker and potentially many simultaneously open tabs doing searches, this caused problems with searches overwriting the data for previous searches, leading to infobars that would redo the wrong search. The new code stuffs this sort of data into the infobar delegate so it's safe to have many simultaneous infobars, which are now tracked in a map.
* Related to the above issue, we'd call registrar_.RemoveAll() any time an infobar was closed or we got a committed or closed notification. But since multiple tabs could in theory be listening for notifications, this was wrong. This could cause infobars to sometimes not appear when doing searches in multiple tabs. Now we are more surgical with our notifications.
* Made accepting one infobar close all of them and trigger all of them to re-search. Similarly, canceling any one infobar now cancels them all.
* The unittest code has been made more robust, if a bit confusing. I changed the way the helper code provided mock search domain responses to make it safe to provide a response whether or not a query had actually happened. This way if something goes wrong and we don't query at the places we think we should, we won't have hangs or data corruption or anything else. The downside here is that in order to avoid actually needing a WebContents*, NavigationController*, InfoBarTabHelper*, etc. to be constructed, I instead use simple ints that are reinterpret_cast<>()ed to the various types to serve as unique pointers. The GoogleURLTracker itself doesn't actually deref these pointers, so this is safe, and it neatly sidesteps having to mock out a bunch of different things, but it's fairly unorthodox.
* More unittests for cases that the original code didn't cover, e.g. multiple simultaneous infobars.
This does NOT yet deal with a major cause of bug 110158 that the GoogleURLTracker does not correctly handle instant (and maybe also prerendering). I wanted to make the underlying framework more sane and robust before trying to fix that.
BUG=54274, 110158
TEST=Moving to different countries causes a "you moved" infobar to trigger when the user does a search. No matter how many tabs have infobars, accepting or rejecting the new Google TLD should close them all, and if the TLD was accepted, redo all their searches on the new TLD.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133598
Patch Set 1 #
Total comments: 3
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : #
Total comments: 15
Messages
Total messages: 11 (0 generated)
|
|||||||||||||||||||||||||||||||||||||
