Allow /drive files in "Report Issue" page.

Allow /drive files in "Report Issue" page.


BUG=140622
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151876

[kinaba, 2012-08-15 08:19:56]

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:117: bool ScreenshotGDataComp(const
gdata::GDataEntryProto& entry1,
Looks nicer to include "Timestamp" in the name of the function.

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:115:
file_system->GetFileByResourceId(
The method is DCHECKED to run on UI or IO thread, not FILE thread.

I think this file name resolution task must be done before posting
SendSavedScreenshot, and we can keep SendSavedScreenshot unchanged.

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:13: #include "base/file_path.h"
Forward declaration looks sufficient.

[Jun Mukai, 2012-08-15 08:30:42]

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:117: bool ScreenshotGDataComp(const
gdata::GDataEntryProto& entry1,
On 2012/08/15 08:19:57, kinaba wrote:
> Looks nicer to include "Timestamp" in the name of the function.

Done.

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.cc:115:
file_system->GetFileByResourceId(
On 2012/08/15 08:19:57, kinaba wrote:
> The method is DCHECKED to run on UI or IO thread, not FILE thread.
> 
> I think this file name resolution task must be done before posting
> SendSavedScreenshot, and we can keep SendSavedScreenshot unchanged.

Aagh, right.  Modified to do that.

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/10837253/diff/11001/chrome/browser/ui/webui/sc...
chrome/browser/ui/webui/screenshot_source.h:13: #include "base/file_path.h"
On 2012/08/15 08:19:57, kinaba wrote:
> Forward declaration looks sufficient.

Done.

[kinaba, 2012-08-15 09:01:55]

http://codereview.chromium.org/10837253/diff/9003/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10837253/diff/9003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:104:
base::Bind(&ScreenshotSource::SendSavedScreenshot,
This time, SendSavedScreenshot is called back on the same thread as the caller
(which I guess UI thread). It violates file_util::ReadFile's thread restriction
to FILE thread. How about the following structure?

GetFileByResourceId(..., &WhatEverTheNameIs);

void WhatEverTheNameIs(path, error, file_type) {
  // On UI thread.
  if(error is error || file_type is not good)
    CacheAndSendScreenshot();
  else
    PostTask(FILE, SendSavedScreenshot, path, request_id);
}

void SendSavedScreenshot(...) {
  // On FILE thread. Stays exactly the same as before this CL.
}

[kinaba, 2012-08-15 09:06:11]

A note that may reduce confusion: the temp local file prepared by
GetFileByResourceId is on GData cache, and unlikely to be evicted soon after you
get out the callback. So I don't think we need to afraid much for the case.

[Jun Mukai, 2012-08-15 09:18:45]

http://codereview.chromium.org/10837253/diff/9003/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/10837253/diff/9003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:104:
base::Bind(&ScreenshotSource::SendSavedScreenshot,
On 2012/08/15 09:01:55, kinaba wrote:
> This time, SendSavedScreenshot is called back on the same thread as the caller
> (which I guess UI thread). It violates file_util::ReadFile's thread
restriction
> to FILE thread. How about the following structure?
> 
> GetFileByResourceId(..., &WhatEverTheNameIs);
> 
> void WhatEverTheNameIs(path, error, file_type) {
>   // On UI thread.
>   if(error is error || file_type is not good)
>     CacheAndSendScreenshot();
>   else
>     PostTask(FILE, SendSavedScreenshot, path, request_id);
> }
> 
> void SendSavedScreenshot(...) {
>   // On FILE thread. Stays exactly the same as before this CL.
> }

Done.

[kinaba, 2012-08-15 13:59:35]

lgtm

[rkc, 2012-08-15 22:32:01]

LGTM provided the comment is fixed.

http://codereview.chromium.org/10837253/diff/14001/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10837253/diff/14001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:563: base::Unretained(this),
base::Owned(saved_screenshots));
This should use a WeakPtr.

[Jun Mukai, 2012-08-16 00:03:18]

Evan, could you take a look at this CL as the OWNER?

http://codereview.chromium.org/10837253/diff/14001/chrome/browser/ui/webui/fe...
File chrome/browser/ui/webui/feedback_ui.cc (right):

http://codereview.chromium.org/10837253/diff/14001/chrome/browser/ui/webui/fe...
chrome/browser/ui/webui/feedback_ui.cc:563: base::Unretained(this),
base::Owned(saved_screenshots));
On 2012/08/15 22:32:01, Rahul Chaturvedi wrote:
> This should use a WeakPtr.

Done.

[Evan Stade, 2012-08-16 00:31:10]

lgtm

[commit-bot: I haz the power, 2012-08-16 02:20:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10837253/10006

[commit-bot: I haz the power, 2012-08-16 02:59:54]

Try job failure for 10837253-10006 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-16 05:14:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10837253/1007

[commit-bot: I haz the power, 2012-08-16 10:41:08]

Change committed as 151876