Crash when taking screenshot onto Chrome OS Google Drive

Crash when taking screenshot onto Chrome OS Google Drive.
And, Don't stack screenshot notifications.

Changed profile handling. The profile is now retrieved everytime a screenshot is taken and
not only once in Ash init. The reason is that the profile can change, which caused this crash.

Changed behavior for a subsequent screenshot notification. It will replace the previous
notification (rather than add a new one).

When saving to Google Drive we have two path: screenshot path (used in notification) and
local path (for internal cash/work; used in writing). Add proper handling of both paths.


BUG=231707, 232876, 233574
R=jamescook@chromium.org
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195370

[sschmitz, 2013-04-18 17:13:14]

Added fix for second bug.

[sschmitz, 2013-04-18 23:11:41]

Changing reviewers since sky@ is OOO.

[James Cook, 2013-04-18 23:46:09]

One question

https://codereview.chromium.org/14027009/diff/5001/chrome/browser/ui/ash/scre...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/5001/chrome/browser/ui/ash/scre...
chrome/browser/ui/ash/screenshot_taker.cc:330:
g_browser_process->notification_ui_manager()->CancelById(notification_id_);
Why not just always give screenshots the same ID, like "screenshot"?  Would that
mess up some kind of notification history?

[sschmitz, 2013-04-19 00:51:28]

Done

https://codereview.chromium.org/14027009/diff/5001/chrome/browser/ui/ash/scre...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/5001/chrome/browser/ui/ash/scre...
chrome/browser/ui/ash/screenshot_taker.cc:330:
g_browser_process->notification_ui_manager()->CancelById(notification_id_);
On 2013/04/18 23:46:09, James Cook (Chromium) wrote:
> Why not just always give screenshots the same ID, like "screenshot"?  Would
that
> mess up some kind of notification history?

I talked to mukai@. And, yes, its ok and better to use the same notification id.
Done.

[sschmitz, 2013-04-19 17:07:49]

Add fix for additional issue re Google Drive.

[James Cook, 2013-04-19 17:25:34]

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:141: // |local_path| is a temporary
file in a hidden cash directory used for
cash -> cache

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:338:
g_browser_process->notification_ui_manager()->CancelById(notification_id);
Do you still need to cancel the old notification if you're showing one with the
same replace_id?  If not, don't do it.  If yes, this could use a comment.

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:357: new
ScreenshotTakerNotificationDelegate(notification_id,
Why not just make the notification ID string a constant and avoid passing it
around?

(It'll need to be a const char[] - you can't construct std::string as a
static/global.)

[sschmitz, 2013-04-19 17:53:14]

Done

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:141: // |local_path| is a temporary
file in a hidden cash directory used for
On 2013/04/19 17:25:34, James Cook (Chromium) wrote:
> cash -> cache

Done.

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:338:
g_browser_process->notification_ui_manager()->CancelById(notification_id);
On 2013/04/19 17:25:34, James Cook (Chromium) wrote:
> Do you still need to cancel the old notification if you're showing one with
the
> same replace_id?  If not, don't do it.  If yes, this could use a comment.

Currently yes for two reasons. This may or may not change in the future.
1. to avoid crash that mukai@ is fixing
2. to popup a notification. otherwise nothing happends. This may change in the
future

Added a comment.

https://codereview.chromium.org/14027009/diff/30001/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:357: new
ScreenshotTakerNotificationDelegate(notification_id,
On 2013/04/19 17:25:34, James Cook (Chromium) wrote:
> Why not just make the notification ID string a constant and avoid passing it
> around?
> 
> (It'll need to be a const char[] - you can't construct std::string as a
> static/global.)

Done.

[James Cook, 2013-04-19 17:59:11]

LGTM with nits

https://codereview.chromium.org/14027009/diff/32002/chrome/browser/ui/ash/scr...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/32002/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:47: const char kScreenshot[] =
"screenshot";
kScreenshotID?  Then the name would tell you something about how it is going to
be used.

https://codereview.chromium.org/14027009/diff/32002/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:337: // We cancel a previous
screenshot notificaiton, if any, to ensure we get
notificaiton -> notification

[sschmitz, 2013-04-19 19:26:54]

Thanks for your speedy review!

https://codereview.chromium.org/14027009/diff/32002/chrome/browser/ui/ash/scr...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/32002/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:47: const char kScreenshot[] =
"screenshot";
On 2013/04/19 17:59:11, James Cook (Chromium) wrote:
> kScreenshotID?  Then the name would tell you something about how it is going
to
> be used.
kScreenshotId -> kNotificationId
(fits nicely right before kNotificationOriginUrl)
Done.

https://codereview.chromium.org/14027009/diff/32002/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:337: // We cancel a previous
screenshot notificaiton, if any, to ensure we get
On 2013/04/19 17:59:11, James Cook (Chromium) wrote:
> notificaiton -> notification

Done.

[commit-bot: I haz the power, 2013-04-19 19:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/14027009/29020

[commit-bot: I haz the power, 2013-04-19 19:39:42]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-19 20:14:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/14027009/29020

[commit-bot: I haz the power, 2013-04-19 21:04:47]

Retried try job too often on mac_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-19 21:27:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/14027009/29020

[commit-bot: I haz the power, 2013-04-20 02:24:32]

Change committed as 195370

[sky, 2013-04-22 15:01:19]

https://codereview.chromium.org/14027009/diff/29020/chrome/browser/ui/ash/scr...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/14027009/diff/29020/chrome/browser/ui/ash/scr...
chrome/browser/ui/ash/screenshot_taker.cc:384: return
ProfileManager::GetDefaultProfileOrOffTheRecord();
The reason I wanted this code to take a Profile is to avoid use of this
deprecated method. I think we should make this code (and other code in ash) take
a ProfileGetter, which is a pure virtual interface that had a single method to
get the Profile. The chromeos version would use this method, where as the
non-chromeos version would take a hard coded profile.