Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
720163003:
Safe Browsing: Add Omnibox Interaction Incident Reporter (Closed)
DescriptionSafe Browsing: Add Omnibox Interaction Incident Reporter
Tested. Seems to work (records the right histogram values when I expect it to trigger; doesn't record them when I don't).
TBR=noelutz@google.com
BUG=
Committed: https://crrev.com/55360114e1008cb26e9c38ff720f06a6fef7529c
Cr-Commit-Position: refs/heads/master@{#307774}
Patch Set 1 #Patch Set 2 : fix typo #
Total comments: 8
Patch Set 3 : grt's comments #Patch Set 4 : full implementation #Patch Set 5 : formatting #
Total comments: 22
Patch Set 6 : greg's suggestions #Patch Set 7 : spacing #
Total comments: 2
Patch Set 8 : remove explicit #
Total comments: 4
Patch Set 9 : remove unnecessary namespaces #Messages
Total messages: 35 (8 generated)
mpearson@chromium.org changed reviewers: + grt@chromium.org
Greg, Please take a look. Obviously, some details are still being hashed out. This is the general structure however. I have not yet figured out the best place to instantiate the OmniboxWatcher class. Maybe somewhere in the initialization of IncidentReportingService? What do you think? thanks, mark
How about making the watcher profile-specific and hanging it off of ProfileImpl? This will allow the watcher to accept an AddIncidentCallback associated with the profile it's watching, which is better that using a process-wide callback since each omnibox is associated with exactly one profile. https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc (right): https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:30: const OmniboxLog log(*content::Details<OmniboxLog>(details).ptr()); do you need to copy the log here? how about: const OmniboxLog* log = content::Details<OmniboxLog>(details).ptr()? https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h (right): https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:27: const AddIncidentCallback& incident_callback_; AddIncidentCallback incident_callback_; https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:31: }; #include "base/macros.h" DISALLOW_COPY_AND_ASSIGN(OmniboxWatcher); https://codereview.chromium.org/720163003/diff/20001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/720163003/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:372: optional OmniboxIncident omnibox_interaction = 5; omnibox_interaction -> omnibox for consistency's sake (or change the type to OmniboxInteractionIncident).
"How about making the watcher profile-specific and hanging it off of ProfileImpl? This will allow the watcher to accept an AddIncidentCallback associated with the profile it's watching, which is better that using a process-wide callback since each omnibox is associated with exactly one profile." It looks like there's a new world order: // STOP!!!! DO NOT ADD ANY MORE ITEMS HERE!!!! // // Instead, make your Service/Manager/whatever object you're hanging off the // Profile use our new BrowserContextKeyedServiceFactory system instead. // You can find the design document here: // // https://sites.google.com/a/chromium.org/dev/developers/design-documents/profi... // // ... I'll think about how to use that architecture. The key question is: where are AddIncidentCallback instances available? Where-ever I add code to instantiate my class needs access to one. Tomorrow I'll hunt through the code to figure this out (or you can answer it for me ;-) ). --mark https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc (right): https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:30: const OmniboxLog log(*content::Details<OmniboxLog>(details).ptr()); On 2014/11/15 17:56:15, grt wrote: > do you need to copy the log here? how about: > const OmniboxLog* log = content::Details<OmniboxLog>(details).ptr()? Done. https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h (right): https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:27: const AddIncidentCallback& incident_callback_; On 2014/11/15 17:56:15, grt wrote: > AddIncidentCallback incident_callback_; Done. https://codereview.chromium.org/720163003/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:31: }; On 2014/11/15 17:56:15, grt wrote: > #include "base/macros.h" > DISALLOW_COPY_AND_ASSIGN(OmniboxWatcher); Done. https://codereview.chromium.org/720163003/diff/20001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/720163003/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:372: optional OmniboxIncident omnibox_interaction = 5; On 2014/11/15 17:56:15, grt wrote: > omnibox_interaction -> omnibox for consistency's sake (or change the type to > OmniboxInteractionIncident). Did the latter.
On 2014/11/18 00:54:44, Mark P wrote: > "How about making the watcher profile-specific and hanging it off of > ProfileImpl? > This will allow the watcher to accept an AddIncidentCallback associated with the > profile it's watching, which is better that using a process-wide callback since > each omnibox is associated with exactly one profile." > > It looks like there's a new world order: > // STOP!!!! DO NOT ADD ANY MORE ITEMS HERE!!!! > // > // Instead, make your Service/Manager/whatever object you're hanging off the > // Profile use our new BrowserContextKeyedServiceFactory system instead. > // You can find the design document here: > // > // > https://sites.google.com/a/chromium.org/dev/developers/design-documents/profi... > // > // ... > > I'll think about how to use that architecture. > > The key question is: where are AddIncidentCallback instances available? > Where-ever I add code to instantiate my class needs access to one. Tomorrow > I'll hunt through the code to figure this out (or you can answer it for me ;-) > ). I added the pref_validation_delegate_ member to ProfileImpl not too long ago, so adding new members isn't banned outright. Take a look at it and SafeBrowsingService::CreatePreferenceValidationDelegate, which returns a validation delegate that holds on to a profile-specific AddIncidentCallback.
On 2014/11/19 19:59:11, grt wrote: > On 2014/11/18 00:54:44, Mark P wrote: > > "How about making the watcher profile-specific and hanging it off of > > ProfileImpl? > > This will allow the watcher to accept an AddIncidentCallback associated with > the > > profile it's watching, which is better that using a process-wide callback > since > > each omnibox is associated with exactly one profile." > > > > It looks like there's a new world order: > > // STOP!!!! DO NOT ADD ANY MORE ITEMS HERE!!!! > > // > > // Instead, make your Service/Manager/whatever object you're hanging off the > > // Profile use our new BrowserContextKeyedServiceFactory system instead. > > // You can find the design document here: > > // > > // > > > https://sites.google.com/a/chromium.org/dev/developers/design-documents/profi... > > // > > // ... > > > > I'll think about how to use that architecture. > > > > The key question is: where are AddIncidentCallback instances available? > > Where-ever I add code to instantiate my class needs access to one. Tomorrow > > I'll hunt through the code to figure this out (or you can answer it for me ;-) > > ). > > I added the pref_validation_delegate_ member to ProfileImpl not too long ago, so > adding new members isn't banned outright. Take a look at it and > SafeBrowsingService::CreatePreferenceValidationDelegate, which returns a > validation delegate that holds on to a profile-specific AddIncidentCallback. I spent some time looking at the code. To create an OmniboxWatcher, I need access to an instance of IncidentReportingService to be able to call GetAddIncidentCallback(). This limits where I can put the code (unless I want to add a setter for the incident callback variable later). I'm planning to add the code to chrome/browser/safe_browsing/safe_browsing_service.cc instead, as that creates the IncidentReportingService and also knows what profiles are open. What do you think of this idea? By the way, what benefits do I get by associating this incident report with a particular profile (as opposed to passing in NULL as the profile to GetAddIncidentCallback())? Also, some code in safe_browsing_service doesn't create certain things in incognito mode. Should I be creating an omnibox interaction watcher in incognito mode? If you don't feel comfortable answering this questions in this code review thread, feel free to reply to me privately. thanks, mark
This is ready for review. thanks, mark
https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc:8: #include "chrome/browser/safe_browsing/incident_reporting/incident_handler_util.h" unused https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc:16: // We want to only return the first suspicious omnibox interaction so we use There's been some discussion on chromium-dev about using "we" like this (https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...). I think this and other comments could be made a bit more precise without the pronoun and discussing desires ("we want..."). For example: // Return a constant, as only one kind of suspicious interaction is detected. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc:24: // We use a constant digest because don't need to report additional // Return a constant (independent of the incident's payload) so that only the // first suspicious interaction is reported. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:20: content::NotificationService::AllSources()); pass in the Profile* and use content::Source<Profile>(profile) here so that each instance only monitors the profile its callback is associated with. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:37: // such typing is--but we don't monitor queries for now.) consider replacing the parenthetical remark with: // TODO(mpearson): Add support for suspicious queries. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:44: const GURL& origin = selected_suggestion.destination_url.GetOrigin(); gurl.h says that GetOrigin() may return an empty URL in some circumstances. is there any possibility of that happening here? if so, should this skip adding the incident if origin.is_empty()? https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:16: class OmniboxWatcher : public content::NotificationObserver { // Observes all URLs opened via the Omnibox popup, adding an incident to the // incident reporting service for those that are typed impossibly quickly. or something like that https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:18: OmniboxWatcher(const AddIncidentCallback& callback); explicit OmniboxWatcher(const AddIncidentCallback& callback); https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; how about putting a scoped_ptr<OmniboxWatcher> member on IncidentReportingService::ProfileContext instead of having a new collection here? i have some reservations about mixing incident handlers in there, but it seems like such a no-brainer to have IncidentReportingService::OnProfileAdded create the watcher and let it die when the ProfileContext is deleted in IncidentReportingService::OnProfileDestroyed.
> https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... > chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, > safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; > how about putting a scoped_ptr<OmniboxWatcher> member on > IncidentReportingService::ProfileContext instead of having a new collection > here? i have some reservations about mixing incident handlers in there, but it > seems like such a no-brainer to have IncidentReportingService::OnProfileAdded > create the watcher and let it die when the ProfileContext is deleted in > IncidentReportingService::OnProfileDestroyed. I'm still working on this one. Right now I'm trying to sort out how creating a profile context (which now creates an OmniboxWatcher, which needs the result of GetAddIncidentCallback()) can work, given that the function GetAddIncidentCallback() calls GetOrCreateProfileContext(), which is what creates a ProfileContext. In other words, I have an instantiation loop that I need to figure out how to sort out. Perhaps tomorrow. --mark
On 2014/12/09 00:38:37, Mark P wrote: > > > https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... > > chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, > > safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; > > how about putting a scoped_ptr<OmniboxWatcher> member on > > IncidentReportingService::ProfileContext instead of having a new collection > > here? i have some reservations about mixing incident handlers in there, but it > > seems like such a no-brainer to have IncidentReportingService::OnProfileAdded > > create the watcher and let it die when the ProfileContext is deleted in > > IncidentReportingService::OnProfileDestroyed. > > I'm still working on this one. Right now I'm trying to sort out how creating a > profile context (which now creates an OmniboxWatcher, which needs the result of > GetAddIncidentCallback()) can work, given that the function > GetAddIncidentCallback() calls GetOrCreateProfileContext(), which is what > creates a ProfileContext. In other words, I have an instantiation loop that I > need to figure out how to sort out. Perhaps tomorrow. > > --mark so much for the no-brainer theory. :-) will this work: - add a scoped_ptr<OmniboxWatcher> member to ProfileContext - ProfileContext::ProfileContext() does not create the watcher - OnProfileAdded() creates the watcher and stuffs it in the context (maybe near the top just after "context->added = true;")
Please take a look, thanks. Also, when you're done looking, who do you suggest I get as an approver? mattm? noelutz? --mark https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc:8: #include "chrome/browser/safe_browsing/incident_reporting/incident_handler_util.h" On 2014/12/05 18:40:56, grt wrote: > unused Done. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc:16: // We want to only return the first suspicious omnibox interaction so we use On 2014/12/05 18:40:56, grt wrote: > There's been some discussion on chromium-dev about using "we" like this > (https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...). > I think this and other comments could be made a bit more precise without the > pronoun and discussing desires ("we want..."). For example: > // Return a constant, as only one kind of suspicious interaction is detected. Thanks for the improved comment. The original comment was icky mostly because I didn't clearly understand (or need to understand) what key and digest meant. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_incident_handlers.cc:24: // We use a constant digest because don't need to report additional On 2014/12/05 18:40:56, grt wrote: > // Return a constant (independent of the incident's payload) so that only the > // first suspicious interaction is reported. Done. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:20: content::NotificationService::AllSources()); On 2014/12/05 18:40:56, grt wrote: > pass in the Profile* and use content::Source<Profile>(profile) here so that each > instance only monitors the profile its callback is associated with. Good idea. Done. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:37: // such typing is--but we don't monitor queries for now.) On 2014/12/05 18:40:56, grt wrote: > consider replacing the parenthetical remark with: > // TODO(mpearson): Add support for suspicious queries. Done. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.cc:44: const GURL& origin = selected_suggestion.destination_url.GetOrigin(); On 2014/12/05 18:40:56, grt wrote: > gurl.h says that GetOrigin() may return an empty URL in some circumstances. is > there any possibility of that happening here? I don't believe so. The omnibox suggestions (which technically include the various types of what-you-typed navigations) all are supposed to have valid destination URLs. It's that destination URL that gets broadcast. I'm pretty sure we'd have seen other issues in Chrome is sometimes destination URLs are not valid. > if so, should this skip adding the > incident if origin.is_empty()? https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:16: class OmniboxWatcher : public content::NotificationObserver { On 2014/12/05 18:40:56, grt wrote: > // Observes all URLs opened via the Omnibox popup, adding an incident to the > // incident reporting service for those that are typed impossibly quickly. > > or something like that Done. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:18: OmniboxWatcher(const AddIncidentCallback& callback); On 2014/12/05 18:40:56, grt wrote: > explicit OmniboxWatcher(const AddIncidentCallback& callback); Done. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; On 2014/12/05 18:40:56, grt wrote: > how about putting a scoped_ptr<OmniboxWatcher> member on > IncidentReportingService::ProfileContext instead of having a new collection > here? i have some reservations about mixing incident handlers in there, but it > seems like such a no-brainer to have IncidentReportingService::OnProfileAdded > create the watcher and let it die when the ProfileContext is deleted in > IncidentReportingService::OnProfileDestroyed. Done. It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens for NOTIFICATION_PROFILE_CREATED? I don't think these are equivalent.
Oops, forgot to revise one code review comment before I hit publish. --mark https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; On 2014/12/05 18:40:56, grt wrote: > how about putting a scoped_ptr<OmniboxWatcher> member on > IncidentReportingService::ProfileContext instead of having a new collection > here? i have some reservations about mixing incident handlers in there, but it > seems like such a no-brainer to have IncidentReportingService::OnProfileAdded > create the watcher and let it die when the ProfileContext is deleted in > IncidentReportingService::OnProfileDestroyed. Done. I love how I can offhandedly mention any issue that I'm having when I leave work on day, and, before I spend much time thinking about it, I have the solution in my inbox in the morning. :-) It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens for NOTIFICATION_PROFILE_CREATED? I don't think these are equivalent.
grt@chromium.org changed reviewers: + asvitkine@chromium.org, noelutz@chromium.org
CC+noms: Could you answer the question below regarding PROFILE_CREATED vs PROFILE_ADDED? R+noelutz: Please perform OWNERS review of csd.proto here. R+asvitkine: Please preforme OWNERS review of histograms.xml here. Thanks. https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; On 2014/12/09 19:24:48, Mark P wrote: > On 2014/12/05 18:40:56, grt wrote: > > how about putting a scoped_ptr<OmniboxWatcher> member on > > IncidentReportingService::ProfileContext instead of having a new collection > > here? i have some reservations about mixing incident handlers in there, but it > > seems like such a no-brainer to have IncidentReportingService::OnProfileAdded > > create the watcher and let it die when the ProfileContext is deleted in > > IncidentReportingService::OnProfileDestroyed. > > Done. > > I love how I can offhandedly mention any issue that I'm having when I leave work > on day, and, before I spend much time thinking about it, I have the solution in > my inbox in the morning. :-) > > It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens for > NOTIFICATION_PROFILE_CREATED? I don't think these are equivalent. I think it's okay. PROFILE_ADDED happens when the Profile is added to the ProfileManager. It likely happens a little later than _CREATED, but still before the omnibox can be interacted with. noms: Are there any cases where NOTIFICATION_PROFILE_CREATED can be sent for a profile, but not NOTIFICATION_PROFILE_ADDED? More to the point, is it possible for a user to interact with the omnibox before PROFILE_ADDED is called? https://codereview.chromium.org/720163003/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h (right): https://codereview.chromium.org/720163003/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:22: explicit OmniboxWatcher(Profile* profile, ah, now explicit isn't needed since there are two args.
histograms lgtm
https://codereview.chromium.org/720163003/diff/120001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h (right): https://codereview.chromium.org/720163003/diff/120001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/omnibox_watcher.h:22: explicit OmniboxWatcher(Profile* profile, On 2014/12/09 20:15:52, grt wrote: > ah, now explicit isn't needed since there are two args. Done.
noms@chromium.org changed reviewers: + noms@chromium.org
https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; On 2014/12/09 20:15:52, grt wrote: > On 2014/12/09 19:24:48, Mark P wrote: > > On 2014/12/05 18:40:56, grt wrote: > > > how about putting a scoped_ptr<OmniboxWatcher> member on > > > IncidentReportingService::ProfileContext instead of having a new collection > > > here? i have some reservations about mixing incident handlers in there, but > it > > > seems like such a no-brainer to have > IncidentReportingService::OnProfileAdded > > > create the watcher and let it die when the ProfileContext is deleted in > > > IncidentReportingService::OnProfileDestroyed. > > > > Done. > > > > I love how I can offhandedly mention any issue that I'm having when I leave > work > > on day, and, before I spend much time thinking about it, I have the solution > in > > my inbox in the morning. :-) > > > > It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens for > > NOTIFICATION_PROFILE_CREATED? I don't think these are equivalent. > > I think it's okay. PROFILE_ADDED happens when the Profile is added to the > ProfileManager. It likely happens a little later than _CREATED, but still before > the omnibox can be interacted with. > > noms: Are there any cases where NOTIFICATION_PROFILE_CREATED can be sent for a > profile, but not NOTIFICATION_PROFILE_ADDED? More to the point, is it possible > for a user to interact with the omnibox before PROFILE_ADDED is called? Hmmm, so I think that this can happen for sure in tests (you manually create a Profile, but you don't add it to the ProfileManager). In a regular Chrome run, I think it would only happen if something's gone horribly wrong: Chrome crashes after the Profile was created, the ProfileManager gets itself corrupted...all situations when all bets are off anyway. I didn't check who listens to PROFILE_CREATED, but keep in mind at that point all PKSes and stuff like that aren't initialized for the Profile yet -- those get done after PROFILE_CREATED and before PROFILE_ADDED.
lgtm https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/720163003/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.h:250: std::map<Profile*, safe_browsing::OmniboxWatcher*> omnibox_watcher_map_; On 2014/12/09 20:35:52, Monica Dinculescu wrote: > On 2014/12/09 20:15:52, grt wrote: > > On 2014/12/09 19:24:48, Mark P wrote: > > > On 2014/12/05 18:40:56, grt wrote: > > > > how about putting a scoped_ptr<OmniboxWatcher> member on > > > > IncidentReportingService::ProfileContext instead of having a new > collection > > > > here? i have some reservations about mixing incident handlers in there, > but > > it > > > > seems like such a no-brainer to have > > IncidentReportingService::OnProfileAdded > > > > create the watcher and let it die when the ProfileContext is deleted in > > > > IncidentReportingService::OnProfileDestroyed. > > > > > > Done. > > > > > > I love how I can offhandedly mention any issue that I'm having when I leave > > work > > > on day, and, before I spend much time thinking about it, I have the solution > > in > > > my inbox in the morning. :-) > > > > > > It is okay that one listens for NOTIFICATION_PROFILE_ADDED and one listens > for > > > NOTIFICATION_PROFILE_CREATED? I don't think these are equivalent. > > > > I think it's okay. PROFILE_ADDED happens when the Profile is added to the > > ProfileManager. It likely happens a little later than _CREATED, but still > before > > the omnibox can be interacted with. > > > > noms: Are there any cases where NOTIFICATION_PROFILE_CREATED can be sent for a > > profile, but not NOTIFICATION_PROFILE_ADDED? More to the point, is it possible > > for a user to interact with the omnibox before PROFILE_ADDED is called? > > Hmmm, so I think that this can happen for sure in tests (you manually create a > Profile, but you don't add it to the ProfileManager). In a regular Chrome run, I > think it would only happen if something's gone horribly wrong: Chrome crashes > after the Profile was created, the ProfileManager gets itself corrupted...all > situations when all bets are off anyway. > > I didn't check who listens to PROFILE_CREATED, but keep in mind at that point > all PKSes and stuff like that aren't initialized for the Profile yet -- those > get done after PROFILE_CREATED and before PROFILE_ADDED. Thanks for the info. I think this is okay, then.
Okay, now I'm just waiting for noelutz's OWNERS review of csd.proto. --mark
lgtm
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/720163003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/720163003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/720163003/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/720163003/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:234: scoped_ptr<safe_browsing::OmniboxWatcher> omnibox_watcher; no need for safe_browsing:: here https://codereview.chromium.org/720163003/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:443: new OmniboxWatcher::OmniboxWatcher(profile, new OmniboxWatcher(profile, GetAddIncidentCallback(profile))
https://codereview.chromium.org/720163003/diff/140001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc (right): https://codereview.chromium.org/720163003/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:234: scoped_ptr<safe_browsing::OmniboxWatcher> omnibox_watcher; On 2014/12/10 20:02:22, grt wrote: > no need for safe_browsing:: here Done. https://codereview.chromium.org/720163003/diff/140001/chrome/browser/safe_bro... chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:443: new OmniboxWatcher::OmniboxWatcher(profile, On 2014/12/10 20:02:22, grt wrote: > new OmniboxWatcher(profile, GetAddIncidentCallback(profile)) Done.
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/720163003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/55360114e1008cb26e9c38ff720f06a6fef7529c Cr-Commit-Position: refs/heads/master@{#307774} |
