Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
287653004:
Schedule posttask to decide Uma optin option offering (Closed)
|
Created:
6 years, 7 months ago by yao Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSchedule a task to run ShouldOfferMetricsReporting() on FILE thread. The reason of doing this is that GoogleUpdateSettings::GetCollectStatsConsent() can't be called from UI thread, but the bubble class function might be called from UI thread.
BUG=372060
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272300
Patch Set 1 #Patch Set 2 : Update comments #
Total comments: 18
Patch Set 3 : Address comments #Patch Set 4 : Fix #include #Patch Set 5 : Rebase. #Patch Set 6 : Update comments #Patch Set 7 : Remove debug bits. #
Total comments: 8
Patch Set 8 : Address comments #
Total comments: 2
Patch Set 9 : Check browser's existance. #
Total comments: 6
Patch Set 10 : Add browser destruction observer. #Patch Set 11 : Nit #Patch Set 12 : Nit #
Total comments: 8
Patch Set 13 : Address Comments #Patch Set 14 : Nit #Patch Set 15 : Nit #
Total comments: 10
Patch Set 16 : Address review comments. #
Total comments: 10
Patch Set 17 : Address comments #Patch Set 18 : Nit #Patch Set 19 : Rebase #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_impl.cc:848: if (!CommandLine::ForCurrentProcess()->HasSwitch( Is there a reason you're changing this part? https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:11: #include "base/memory/ref_counted.h" Is this include needed? https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:47: #define FROM_HERE FROM_HERE_WITH_EXPLICIT_FUNCTION(__FUNCTION__) Why is this needed? Can you include a header that defines this instead? https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:83: // can't be called from UI thread, but current function might be called from Nit: "might be" -> "is" https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:88: &ShouldOfferMetricsReporting); Nit: The code would be less verbose if you inline these into the PostTask(...) invocation. (Plus you're not using the right naming convention for them anyway...) https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:111: crash_bubble->offer_uma_optin_ = offer_uma_optin; Make it a ctor parameter instead. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.h:48: static void ShowForReal(Browser* browser, bool buffer); Rename |buffer| to a more meaningful name and make the comment explain the purpose of this function (vs. Show()).
https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_impl.cc:848: if (!CommandLine::ForCurrentProcess()->HasSwitch( On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Is there a reason you're changing this part? yes, In the situation where there are multiple profiles (say 3) and but only 2 profiles were open when the browser crashes. Then when the browser is opened again, only those 2 profiles are opened automatically. At this time, if switching to the 3rd profile, the info bar is shown in stead of the bubble. The reason for this was, command_line_.HasSwitch() returns false but CommandLine::ForCurrentProcess()->HasSwitch returns True on the same key. I didn't find the exact reason for this, but I know this solves the problem. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:11: #include "base/memory/ref_counted.h" On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Is this include needed? Done. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:47: #define FROM_HERE FROM_HERE_WITH_EXPLICIT_FUNCTION(__FUNCTION__) On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Why is this needed? Can you include a header that defines this instead? Done. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:83: // can't be called from UI thread, but current function might be called from On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Nit: "might be" -> "is" When the browser starts up, it's not called by UI thread. Only when the user switches profiles, it's called by UI thread. (Based on when the crash is observed) https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:88: &ShouldOfferMetricsReporting); On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Nit: The code would be less verbose if you inline these into the PostTask(...) > invocation. (Plus you're not using the right naming convention for them > anyway...) Done. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:111: crash_bubble->offer_uma_optin_ = offer_uma_optin; On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Make it a ctor parameter instead. Done. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.h:48: static void ShowForReal(Browser* browser, bool buffer); On 2014/05/14 14:09:57, Alexei Svitkine wrote: > Rename |buffer| to a more meaningful name and make the comment explain the > purpose of this function (vs. Show()). Done.
https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_impl.cc:848: if (!CommandLine::ForCurrentProcess()->HasSwitch( On 2014/05/14 14:53:28, yao wrote: > On 2014/05/14 14:09:57, Alexei Svitkine wrote: > > Is there a reason you're changing this part? > > yes, > In the situation where there are multiple profiles (say 3) and but only 2 > profiles were open when the browser crashes. Then when the browser is opened > again, only those 2 profiles are opened automatically. At this time, if > switching to the 3rd profile, the info bar is shown in stead of the bubble. > > The reason for this was, command_line_.HasSwitch() returns false but > CommandLine::ForCurrentProcess()->HasSwitch returns True on the same key. > > I didn't find the exact reason for this, but I know this solves the problem. That's very surprising. Can you do a little investigating as to why they're different? (i.e. where does command_line_ come from?) If your change is indeed what we want to do to fix this, then it should have a comment explaining why |command_line_| isn't being used, else someone will just "clean up the code" and break it again. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:83: // can't be called from UI thread, but current function might be called from On 2014/05/14 14:53:28, yao wrote: > On 2014/05/14 14:09:57, Alexei Svitkine wrote: > > Nit: "might be" -> "is" > > When the browser starts up, it's not called by UI thread. Only when the user > switches profiles, it's called by UI thread. > > (Based on when the crash is observed) It should always be called on the UI thread. If it's not, then there's another bug. (However, it's quite possible that during browser start up the IO checks are not turned on yet.) To confirm, you can add the following at the top of the function (and probably good to have in general): DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator_impl.cc:848: if (!CommandLine::ForCurrentProcess()->HasSwitch( On 2014/05/14 15:02:54, Alexei Svitkine wrote: > On 2014/05/14 14:53:28, yao wrote: > > On 2014/05/14 14:09:57, Alexei Svitkine wrote: > > > Is there a reason you're changing this part? > > > > yes, > > In the situation where there are multiple profiles (say 3) and but only 2 > > profiles were open when the browser crashes. Then when the browser is opened > > again, only those 2 profiles are opened automatically. At this time, if > > switching to the 3rd profile, the info bar is shown in stead of the bubble. > > > > The reason for this was, command_line_.HasSwitch() returns false but > > CommandLine::ForCurrentProcess()->HasSwitch returns True on the same key. > > > > I didn't find the exact reason for this, but I know this solves the problem. > > That's very surprising. Can you do a little investigating as to why they're > different? (i.e. where does command_line_ come from?) > > If your change is indeed what we want to do to fix this, then it should have a > comment explaining why |command_line_| isn't being used, else someone will just > "clean up the code" and break it again. I believe here is the cause: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... It seems to me that they just pass in an empty CommandLine object. I added a comment to state that command_line_ can't be used here. https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:83: // can't be called from UI thread, but current function might be called from On 2014/05/14 15:02:54, Alexei Svitkine wrote: > On 2014/05/14 14:53:28, yao wrote: > > On 2014/05/14 14:09:57, Alexei Svitkine wrote: > > > Nit: "might be" -> "is" > > > > When the browser starts up, it's not called by UI thread. Only when the user > > switches profiles, it's called by UI thread. > > > > (Based on when the crash is observed) > > It should always be called on the UI thread. If it's not, then there's another > bug. > > (However, it's quite possible that during browser start up the IO checks are not > turned on yet.) > > To confirm, you can add the following at the top of the function (and probably > good to have in general): > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > Done.
lgtm % nits https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:88: // parameter to create and show the bubble. Nit: This comment is too verbose for what it's describing, how about just: // Schedule a task to run ShouldOfferMetricsReporting() on FILE thread, since // GoogleUpdateSettings::GetCollectStatsConsent() does IO. Then, call // SessionCrashedBubbleView::ShowForReal with the result. https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:42: // offered. After it finishes, calls ShowForReal to show the bubble. Nit: This comment is talking about an implementation detail of the function, so shouldn't be in the docs on the public function. https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:53: // option should be offered, it creates and shows the session crashed bubble. Nit: Seems like you have two sentences jammed together - i.e. "Called after x" and "After x, it creates." how about: // Creates and shows the session crashed bubble, with |offer_uma_optin| // indicating whether the UMA opt-in checkbox should be shown. Called // by Show() after checking whether the UMA option should be presented. https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:122: // Whether or not should offer UMA opt-in option. Nit: "Whether or not the UMA opt-in option should be shown."
+sky for owner's review. Thanks! https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:88: // parameter to create and show the bubble. On 2014/05/14 21:17:02, Alexei Svitkine wrote: > Nit: This comment is too verbose for what it's describing, how about just: > > // Schedule a task to run ShouldOfferMetricsReporting() on FILE thread, since > // GoogleUpdateSettings::GetCollectStatsConsent() does IO. Then, call > // SessionCrashedBubbleView::ShowForReal with the result. Done. https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:42: // offered. After it finishes, calls ShowForReal to show the bubble. On 2014/05/14 21:17:02, Alexei Svitkine wrote: > Nit: This comment is talking about an implementation detail of the function, so > shouldn't be in the docs on the public function. Done. https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:53: // option should be offered, it creates and shows the session crashed bubble. On 2014/05/14 21:17:02, Alexei Svitkine wrote: > Nit: Seems like you have two sentences jammed together - i.e. "Called after x" > and "After x, it creates." > > how about: > > // Creates and shows the session crashed bubble, with |offer_uma_optin| > // indicating whether the UMA opt-in checkbox should be shown. Called > // by Show() after checking whether the UMA option should be presented. Done. https://codereview.chromium.org/287653004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:122: // Whether or not should offer UMA opt-in option. On 2014/05/14 21:17:02, Alexei Svitkine wrote: > Nit: "Whether or not the UMA opt-in option should be shown." Done.
https://codereview.chromium.org/287653004/diff/150001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/150001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:90: base::Bind(&SessionCrashedBubbleView::ShowForReal, browser)); Won't you crash if browser is destroyed by the time you get the response?
https://codereview.chromium.org/287653004/diff/150001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/150001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:90: base::Bind(&SessionCrashedBubbleView::ShowForReal, browser)); On 2014/05/14 23:22:58, sky wrote: > Won't you crash if browser is destroyed by the time you get the response? Added the check to make sure browser is still live before using it.
https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:102: it != browser_list->end(); ++it ) { Can you use std:find() on browser_list->begin() and browser_list->end() instead and do an early return otherwise? https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:54: static void ShowForReal(Browser* browser, chrome::HostDesktopType type, Nit: 1 param per line.
https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:100: BrowserList* browser_list = BrowserList::GetInstance(type); This is not a good way to check if the browser is still valid. For example, tcmalloc makes it easy for deletes rapidly followed by a new to get the same address. Additionally the browser may be in the process of being deleted so that if you attempt to use it weird things may result. Instead of passing a raw browser* pass around an object that contains the browser and listens for destruction. That way if it gets here and the browser is no longer valid you'll know.
https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:100: BrowserList* browser_list = BrowserList::GetInstance(type); On 2014/05/15 22:57:38, sky wrote: > This is not a good way to check if the browser is still valid. For example, > tcmalloc makes it easy for deletes rapidly followed by a new to get the same > address. Additionally the browser may be in the process of being deleted so that > if you attempt to use it weird things may result. > > Instead of passing a raw browser* pass around an object that contains the > browser and listens for destruction. That way if it gets here and the browser is > no longer valid you'll know. Done. https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:102: it != browser_list->end(); ++it ) { On 2014/05/15 22:50:49, Alexei Svitkine wrote: > Can you use std:find() on browser_list->begin() and browser_list->end() instead > and do an early return otherwise? Done. https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/170001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:54: static void ShowForReal(Browser* browser, chrome::HostDesktopType type, On 2014/05/15 22:50:49, Alexei Svitkine wrote: > Nit: 1 param per line. Done.
https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:82: virtual void OnBrowserRemoved(Browser* browser) OVERRIDE; Nit: Separate by blank lines. https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:85: Browser* browser_; Nit: DISALLOW_COPY_AND_ASSIGN(). https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:131: Browser* browser, I think you can remove this parameter and instead expose a browser() method on |browser_observer| and do the return if browser_observer->browser() is NULL. https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:29: class BrowserRemovalObserver; I think you can make it an inner class inside SessionCrashedBubbleView and then just declare it (i.e. "class BrowserRemovalObserver;") in the private section in the header, while still keeping the definition in the .cc.
https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:82: virtual void OnBrowserRemoved(Browser* browser) OVERRIDE; On 2014/05/20 15:52:42, Alexei Svitkine wrote: > Nit: Separate by blank lines. Done. https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:85: Browser* browser_; On 2014/05/20 15:52:42, Alexei Svitkine wrote: > Nit: DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:131: Browser* browser, On 2014/05/20 15:52:42, Alexei Svitkine wrote: > I think you can remove this parameter and instead expose a browser() method on > |browser_observer| and do the return if browser_observer->browser() is NULL. I was thinking doing a browser==browser_ is safer, but, right, it doesn't make a difference I guess. Done. https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/230001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:29: class BrowserRemovalObserver; On 2014/05/20 15:52:42, Alexei Svitkine wrote: > I think you can make it an inner class inside SessionCrashedBubbleView and then > just declare it (i.e. "class BrowserRemovalObserver;") in the private section in > the header, while still keeping the definition in the .cc. Done.
LGTM % nits https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:91: DISALLOW_COPY_AND_ASSIGN(BrowserRemovalObserver); Nit: Add an empty line before this. https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:95: Browser* browser) : browser_(browser) { Nit: Add DCHECK(browser_); https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:119: BrowserRemovalObserver* browser_observer = Nit: Add a comment and mention that this will be deallocated in ShowForReal(). https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:141: delete browser_observer; Nit: You can put this right above the if, so you don't need to have it in two places. Then you can remove the {}'s on the if as well. https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:55: // by Show() after checking whether the UMA option should be presented. Nit: Expand comment to mention that it takes ownership of |browser_observer|.
https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:91: DISALLOW_COPY_AND_ASSIGN(BrowserRemovalObserver); On 2014/05/21 07:23:00, Alexei Svitkine wrote: > Nit: Add an empty line before this. Done. https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:95: Browser* browser) : browser_(browser) { On 2014/05/21 07:23:00, Alexei Svitkine wrote: > Nit: Add DCHECK(browser_); Done. https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:119: BrowserRemovalObserver* browser_observer = On 2014/05/21 07:23:00, Alexei Svitkine wrote: > Nit: Add a comment and mention that this will be deallocated in ShowForReal(). Done. https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:141: delete browser_observer; On 2014/05/21 07:23:00, Alexei Svitkine wrote: > Nit: You can put this right above the if, so you don't need to have it in two > places. Then you can remove the {}'s on the if as well. Done. https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/287653004/diff/290001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:55: // by Show() after checking whether the UMA option should be presented. On 2014/05/21 07:23:00, Alexei Svitkine wrote: > Nit: Expand comment to mention that it takes ownership of |browser_observer|. Done.
https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:78: class SessionCrashedBubbleView::BrowserRemovalObserver Add description. https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:82: ~BrowserRemovalObserver(); virtual (because BrowserRemovalObserver subclasses BrowserListObserver). https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:96: Browser* browser) : browser_(browser) { nit: ':' on next line (see style guide). https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:132: base::Bind(&SessionCrashedBubbleView::ShowForReal, browser_observer)); Pass a scoped_ptr to make ownership clear. 122 should be in a scoped_ptr too.
https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:132: base::Bind(&SessionCrashedBubbleView::ShowForReal, browser_observer)); On 2014/05/21 16:13:09, sky wrote: > Pass a scoped_ptr to make ownership clear. 122 should be in a scoped_ptr too. Neat, I didn't realise you could do this, so I didn't suggest this. But looks like it's indeed a thing - you just need to use base::Passed(&browser_observer) (with browser_observer being a scoped_ptr).
https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:78: class SessionCrashedBubbleView::BrowserRemovalObserver On 2014/05/21 16:13:09, sky wrote: > Add description. Done. https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:82: ~BrowserRemovalObserver(); On 2014/05/21 16:13:09, sky wrote: > virtual (because BrowserRemovalObserver subclasses BrowserListObserver). Done. https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:96: Browser* browser) : browser_(browser) { On 2014/05/21 16:13:09, sky wrote: > nit: ':' on next line (see style guide). Done. https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:132: base::Bind(&SessionCrashedBubbleView::ShowForReal, browser_observer)); On 2014/05/21 16:13:09, sky wrote: > Pass a scoped_ptr to make ownership clear. 122 should be in a scoped_ptr too. Done. https://codereview.chromium.org/287653004/diff/310001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:132: base::Bind(&SessionCrashedBubbleView::ShowForReal, browser_observer)); On 2014/05/21 16:25:27, Alexei Svitkine wrote: > On 2014/05/21 16:13:09, sky wrote: > > Pass a scoped_ptr to make ownership clear. 122 should be in a scoped_ptr too. > > Neat, I didn't realise you could do this, so I didn't suggest this. > > But looks like it's indeed a thing - you just need to use > base::Passed(&browser_observer) (with browser_observer being a scoped_ptr). Done.
LGTM
The CQ bit was checked by yiyaoliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/287653004/370001
Message was sent while issue was closed.
Change committed as 272300 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
