Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
290453005:
Remove Infobars notifications from GoogleURLTrackerMapEntry (Closed)
|
Created:
6 years, 7 months ago by droger Modified:
6 years, 6 months ago CC:
chromium-reviews, blundell Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove Infobars notifications from GoogleURLTrackerMapEntry
The InfoBarManager::Observer() is used instead.
GoogleURLTrackerMapEntry now makes actual calls on the InfoBarManager (to
register and unregister as an observer). This has some consequences:
- the |infobar_manager_| pointer is no longer const
- the unittest has to create actual instances of InfoBarManager instead of
using placeholder integers.
BUG=373243
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274510
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : code comments in the unit test #
Total comments: 4
Patch Set 4 : scoped hash map #Patch Set 5 : format #
Total comments: 3
Patch Set 6 : review comments #
Total comments: 7
Patch Set 7 : Rebase + review comments #
Total comments: 9
Patch Set 8 : Remove infobar creator #Patch Set 9 : Remove unique IDs #Patch Set 10 : Removed unused code #
Total comments: 6
Patch Set 11 : Review comments #
Messages
Total messages: 26 (0 generated)
CC blundell: this is intersecting with your componentization work. As discussed offline, let me know if you think we should drop this CL and wait for google/ componentization to be more advanced before removing the notifications.
If GoogleUrlTracker no longer uses InfoBarService in the future, but InfoBarManager instead, all you will be able to simplify the test by replacing the vector of WebContents by a vector of InfoBarManagers. You will have to revert the changes related to the introduction of ChromeRenderViewHostTestHarness though.
https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:461: if ((size_t)unique_id >= web_contents_.size()) oof. maybe a map instead of a vector?
https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:461: if ((size_t)unique_id >= web_contents_.size()) On 2014/05/15 15:52:47, blundell wrote: > oof. maybe a map instead of a vector? That was my first idea too, but: Unfortunately there is no ScopedMap, and this is far from trivial to write :( A simpler alternative is to use a map of linked_ptr, but it seems that it is discouraged. (see https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/6i8vKfmHnJQ...)
https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:461: if ((size_t)unique_id >= web_contents_.size()) On 2014/05/15 15:57:40, droger wrote: > On 2014/05/15 15:52:47, blundell wrote: > > oof. maybe a map instead of a vector? > > That was my first idea too, but: > Unfortunately there is no ScopedMap, and this is far from trivial to write :( > A simpler alternative is to use a map of linked_ptr, but it seems that it is > discouraged. > > (see > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/6i8vKfmHnJQ...) As the test only actually needs 4 instances, maybe I can get rid of the data structure entirely and always instantiate 4 instances.
That would be fine I think. FYI, I also have https://codereview.chromium.org/293503003/ waiting to be reviewed by pkasting now, so you could also wait for that to land.
https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/40001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:461: if ((size_t)unique_id >= web_contents_.size()) On 2014/05/15 15:52:47, blundell wrote: > oof. maybe a map instead of a vector? I found a base::ScopedPtrHashMap that I didn't know about. I updated the CL to use this, which is much cleaner.
Could you add to the CL description why the real InfoBarService objects have to be created now?
On 2014/05/26 15:26:55, blundell wrote: > Could you add to the CL description why the real InfoBarService objects have to > be created now? Done.
LGTM with nits https://codereview.chromium.org/290453005/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/80001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:42: // the caller doesn't own the returned object, we rely on |test_harness| Add a TODO(blundell) in here to simplify this test once InfoBarManager can be used instead of InfoBarService. You can reference crbug.com/373233. https://codereview.chromium.org/290453005/diff/80001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:488: web_contents_.find(unique_id); nit: I would call this web_contents_map_.
+pkasting as OWNER https://codereview.chromium.org/290453005/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/80001/chrome/browser/google/go... chrome/browser/google/google_url_tracker_unittest.cc:488: web_contents_.find(unique_id); On 2014/05/26 15:45:36, blundell wrote: > nit: I would call this web_contents_map_. Done.
I will try to review this tomorrow.
https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_map_entry.cc (right): https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_map_entry.cc:49: // infobars::InfoBarManager::Observer implementation. Nit: Don't add this comment. https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_map_entry.h (right): https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_map_entry.h:41: // infobars::InfoBarManager::Observer implementation. Nit: Preserve the ":" (instead of " implementation.") end-of-comment from the previous code. https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:278: // cleaned up in OnInfoBarClosed(). If you're going to have a real infobar service, why not go ahead and really add infobars to it? https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:318: ChromeRenderViewHostTestHarness::SetUp(); If we're going to have both constructor/destructor and SetUp()/TearDown() functions, there should be comments about why the code in each place can't go in the other. https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:320: ->RegisterUserPrefsOnBrowserContextForTest(profile()); Nit: I know not everyone agrees with this, but I think the previous "place -> on end of first line" wrapping was more correct. https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:347: web_contents_map_.clear(); Is this necessary?
Note: https://codereview.chromium.org/293503003/ just landed, changing the map entry from needing an InfoBarService to just needing an InfoBarManager. You should be able to simplify the unittest based on that I think.
I've rebased the change now that the code uses InfoBarManager instead of InfoBarService. I also removed test code which was dealing with the management of the fake infobar service. We can actually use real infobar managers instead and get rid of that management. https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/100001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:278: // cleaned up in OnInfoBarClosed(). On 2014/05/28 22:27:19, Peter Kasting wrote: > If you're going to have a real infobar service, why not go ahead and really add > infobars to it? Done.
https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:222: // Lazily creates WebContents instances, adds them in |infobar_manager_map_| This comment is stale now, right? https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:234: base::ScopedPtrHashMap<int, infobars::InfoBarManager> infobar_manager_map_; This is probably a big change, but do you even need this map anymore, and to be passing around the ints to index into it? Couldn't you just have several InfoBarManagers and then use GoogleURLTracker's |entry_map_| to get at the MapEntries (and NavHelpers, etc) associated with those InfoBarManagers? https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:258: base::Bind(&GoogleURLTrackerInfoBarDelegate::Create); This is just calling the production creation function now, right? If so, you can eliminate the |infobar_creator_| ivar entirely.
https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:222: // Lazily creates WebContents instances, adds them in |infobar_manager_map_| On 2014/06/02 15:02:40, blundell wrote: > This comment is stale now, right? No. https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:234: base::ScopedPtrHashMap<int, infobars::InfoBarManager> infobar_manager_map_; On 2014/06/02 15:02:40, blundell wrote: > This is probably a big change, but do you even need this map anymore, and to be > passing around the ints to index into it? Couldn't you just have several > InfoBarManagers and then use GoogleURLTracker's |entry_map_| to get at the > MapEntries (and NavHelpers, etc) associated with those InfoBarManagers? I need something to own the InfoBarManagers, as the URL Tracker code does not own them. I could use the map entries to get at them (I considered doing it at some point), but I don't really see benefit since this does not allow to get rid of the map. https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:258: base::Bind(&GoogleURLTrackerInfoBarDelegate::Create); On 2014/06/02 15:02:40, blundell wrote: > This is just calling the production creation function now, right? If so, you can > eliminate the |infobar_creator_| ivar entirely. Done.
https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:222: // Lazily creates WebContents instances, adds them in |infobar_manager_map_| But you're not creating WebContents instances anymore... On 2014/06/02 15:56:29, droger wrote: > On 2014/06/02 15:02:40, blundell wrote: > > This comment is stale now, right? > > No. https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:234: base::ScopedPtrHashMap<int, infobars::InfoBarManager> infobar_manager_map_; The "something" could just be like 3 (or however many you need) TestInfoBarManager ivars. It would enable you to get rid of the map that's being maintained in this test in parallel to the map that GoogleURLTracker is maintaining for the same purpose, and get rid of these unique_ids which don't correspond to anything in the production code at this point. It's possible that I'm missing some complication though. On 2014/06/02 15:56:29, droger wrote: > On 2014/06/02 15:02:40, blundell wrote: > > This is probably a big change, but do you even need this map anymore, and to > be > > passing around the ints to index into it? Couldn't you just have several > > InfoBarManagers and then use GoogleURLTracker's |entry_map_| to get at the > > MapEntries (and NavHelpers, etc) associated with those InfoBarManagers? > > I need something to own the InfoBarManagers, as the URL Tracker code does not > own them. > I could use the map entries to get at them (I considered doing it at some > point), but I don't really see benefit since this does not allow to get rid of > the map.
https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/120001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:234: base::ScopedPtrHashMap<int, infobars::InfoBarManager> infobar_manager_map_; On 2014/06/02 16:00:09, blundell wrote: > The "something" could just be like 3 (or however many you need) > TestInfoBarManager ivars. It would enable you to get rid of the map that's being > maintained in this test in parallel to the map that GoogleURLTracker is > maintaining for the same purpose, and get rid of these unique_ids which don't > correspond to anything in the production code at this point. It's possible that > I'm missing some complication though. > > On 2014/06/02 15:56:29, droger wrote: > > On 2014/06/02 15:02:40, blundell wrote: > > > This is probably a big change, but do you even need this map anymore, and to > > be > > > passing around the ints to index into it? Couldn't you just have several > > > InfoBarManagers and then use GoogleURLTracker's |entry_map_| to get at the > > > MapEntries (and NavHelpers, etc) associated with those InfoBarManagers? > > > > I need something to own the InfoBarManagers, as the URL Tracker code does not > > own them. > > I could use the map entries to get at them (I considered doing it at some > > point), but I don't really see benefit since this does not allow to get rid of > > the map. > Done. Although SetNavigationPending still requires some unique ID, but it ends up working relatively well.
LGTM https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_map_entry.h (right): https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_map_entry.h:49: // infobars::InfoBarManager::Observer implementation: Nit: Don't add "implementation" https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:170: }; Nit: Technically, all classes should have DISALLOW_COPY_AND_ASSIGN
LGTM! https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:316: int unique_id) { Tiny nit: If you wanted to you could pass an active entry ID to TestInfoBarManager, return that from GetActiveEntryID, and call GetActiveEntryID in this function rather than taking in the unique_id.
https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_map_entry.h (right): https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_map_entry.h:49: // infobars::InfoBarManager::Observer implementation: On 2014/06/02 18:46:44, Peter Kasting wrote: > Nit: Don't add "implementation" Done. https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... File chrome/browser/google/google_url_tracker_unittest.cc (right): https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:170: }; On 2014/06/02 18:46:44, Peter Kasting wrote: > Nit: Technically, all classes should have DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/290453005/diff/180001/chrome/browser/google/g... chrome/browser/google/google_url_tracker_unittest.cc:316: int unique_id) { On 2014/06/02 18:54:29, blundell wrote: > Tiny nit: If you wanted to you could pass an active entry ID to > TestInfoBarManager, return that from GetActiveEntryID, and call GetActiveEntryID > in this function rather than taking in the unique_id. Done.
The CQ bit was checked by droger@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/droger@chromium.org/290453005/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 274510 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
