Fix saved screenshots for feedback.

Fix saved screenshots for feedback.
Fixed the location where the saved screenshots are picked from. Also fixed the issue with saved screenshots blocking the UI.


R=zelidrag@chromium.org,derat@chromium.org
BUG=chromium-os:18227, chromium:73180
TEST=Tested with sending feedback using a saved screenshot.

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

[zel, 2011-08-12 13:01:36]

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.cc:23: int request_id, const
std::vector<unsigned char> bytes) {
const std::vector<unsigned char>& bytes ?

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.cc:28: std::vector<unsigned char>
ScreenshotSource::GetCachedScreenshot(
can you avoid data copy here? can't you just return
unsigned char*

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:38: const std::vector<unsigned char>
bytes);
const std::vector<unsigned char>& bytes

[Daniel Erat, 2011-08-12 14:43:40]

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.cc:36: // change it's address 0x0;
look into this eventually.
nit: "its", not "it's"

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:9: #include <map>
nit: alphabetize

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:21: 
nit: remove extra blank line

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:37: void
CacheAndSendScreenshot(const std::string& path, int request_id,
nit: add comment and only put one param on each line unless they all fit on a
single line

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:45: std::map<std::string,
std::vector<unsigned char> > cached_screenshots_;
don't store things that are expensive to copy inside STL containers.  you should
wrap the vectors in linked_ptrs (see base/memory/linked_ptr.h) or just use
RefCountedBytes, which i think will handle all of this for you, instead of
bytes.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:45: std::map<std::string,
std::vector<unsigned char> > cached_screenshots_;
is this code caching screenshots indefinitely?  if so, this is a poor use of
memory.

[rkc, 2011-08-22 13:23:52]

Review comments incorporated.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.cc:23: int request_id, const
std::vector<unsigned char> bytes) {
On 2011/08/12 13:01:37, zel wrote:
> const std::vector<unsigned char>& bytes ?

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.cc:28: std::vector<unsigned char>
ScreenshotSource::GetCachedScreenshot(
On 2011/08/12 13:01:37, zel wrote:
> can you avoid data copy here? can't you just return
> unsigned char* 

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.cc:36: // change it's address 0x0;
look into this eventually.
On 2011/08/12 14:43:40, Daniel Erat wrote:
> nit: "its", not "it's"

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:9: #include <map>
On 2011/08/12 14:43:40, Daniel Erat wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:21: 
On 2011/08/12 14:43:40, Daniel Erat wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:37: void
CacheAndSendScreenshot(const std::string& path, int request_id,
On 2011/08/12 14:43:40, Daniel Erat wrote:
> nit: add comment and only put one param on each line unless they all fit on a
> single line

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:38: const std::vector<unsigned char>
bytes);
On 2011/08/12 13:01:37, zel wrote:
> const std::vector<unsigned char>& bytes

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:45: std::map<std::string,
std::vector<unsigned char> > cached_screenshots_;
On 2011/08/12 14:43:40, Daniel Erat wrote:
> don't store things that are expensive to copy inside STL containers.  you
should
> wrap the vectors in linked_ptrs (see base/memory/linked_ptr.h) or just use
> RefCountedBytes, which i think will handle all of this for you, instead of
> bytes.

Done.

http://codereview.chromium.org/7635017/diff/1/chrome/browser/ui/webui/screens...
chrome/browser/ui/webui/screenshot_source.h:45: std::map<std::string,
std::vector<unsigned char> > cached_screenshots_;
On 2011/08/12 14:43:40, Daniel Erat wrote:
> is this code caching screenshots indefinitely?  if so, this is a poor use of
> memory.

No, this gets put in the data sources delete queue once we clobber the
screenshots source in the bug_report destructor; all cached data should be
deleted then.

[Daniel Erat, 2011-08-22 16:42:02]

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:17: static const char
kCurrentScreenshot[] = "current";
nit: it'd be better to give this variable and kSavedScreeshots more accurate
names; this looks like it's a portion of a filename, and kSavedScreenshots looks
like it's a portion of a path.  kCurrentScreenshotFilename and
kSavedScreenshotsDirectory?

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:30: const std::string& full_path) {
make the parameter name match the name from the header (|path| there,
|full_path| here)

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:32: if
(cached_screenshots_.find(path) != cached_screenshots_.end()) {
nit: it's a better practice to save find()'s returned iterator in a local
variable and then test it against end() and use it to access the value. 
otherwise, you're doing a second lookup with operator[].

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:40: void
ScreenshotSource::SendSavedScreenshot(const std::string& path,
make function definition order match the declaration order from the header

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:61: read_bytes->clear();
shouldn't you abort without sending the screenshot if you fail to read it from
disk?

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.h:46: void SendSavedScreenshot(const
std::string& filename, int request_id);
headers should have comments describing what functions do -- i don't understand
the difference between SendScreenshot() and SendSavedScreenshot() when looking
at this file.

also don't understand why SendScreenshot()'s parameter is named |path| and
SendSavedScreenshot()'s is named |filename|.  is the former a full path and the
latter just a basename?  if there's no difference, please give them the same
names.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.h:50: std::map<std::string,
ScreenshotDataPtr> cached_screenshots_;
add a comment here describing what you're using as the map's key

[rkc, 2011-08-23 10:25:19]

Review comments incorporated.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
File chrome/browser/ui/webui/screenshot_source.cc (right):

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:17: static const char
kCurrentScreenshot[] = "current";
On 2011/08/22 16:42:02, Daniel Erat wrote:
> nit: it'd be better to give this variable and kSavedScreeshots more accurate
> names; this looks like it's a portion of a filename, and kSavedScreenshots
looks
> like it's a portion of a path.  kCurrentScreenshotFilename and
> kSavedScreenshotsDirectory?

Done.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:30: const std::string& full_path) {
On 2011/08/22 16:42:02, Daniel Erat wrote:
> make the parameter name match the name from the header (|path| there,
> |full_path| here)
Changed header to full_path.
Done.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:32: if
(cached_screenshots_.find(path) != cached_screenshots_.end()) {
On 2011/08/22 16:42:02, Daniel Erat wrote:
> nit: it's a better practice to save find()'s returned iterator in a local
> variable and then test it against end() and use it to access the value. 
> otherwise, you're doing a second lookup with operator[].

Done.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:40: void
ScreenshotSource::SendSavedScreenshot(const std::string& path,
On 2011/08/22 16:42:02, Daniel Erat wrote:
> make function definition order match the declaration order from the header

Done.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.cc:61: read_bytes->clear();
On 2011/08/22 16:42:02, Daniel Erat wrote:
> shouldn't you abort without sending the screenshot if you fail to read it from
> disk?

This accomplishes the same thing, while avoiding the need for various null-ptr
checks.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.h:46: void SendSavedScreenshot(const
std::string& filename, int request_id);
On 2011/08/22 16:42:02, Daniel Erat wrote:
> headers should have comments describing what functions do -- i don't
understand
> the difference between SendScreenshot() and SendSavedScreenshot() when looking
> at this file.
> 
> also don't understand why SendScreenshot()'s parameter is named |path| and
> SendSavedScreenshot()'s is named |filename|.  is the former a full path and
the
> latter just a basename?  if there's no difference, please give them the same
> names.

Done.

http://codereview.chromium.org/7635017/diff/7001/chrome/browser/ui/webui/scre...
chrome/browser/ui/webui/screenshot_source.h:50: std::map<std::string,
ScreenshotDataPtr> cached_screenshots_;
On 2011/08/22 16:42:02, Daniel Erat wrote:
> add a comment here describing what you're using as the map's key

Done.

[Daniel Erat, 2011-08-23 16:58:37]

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

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:106: read_bytes->clear();
i don't follow your previous comment about how this handles read errors.  it
looks to me like the following call to CacheAndSendScreenshot() caches an empty
array.  shouldn't it at least check that the array isn't empty before caching
it?

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:34: // Get the screenshot specified
by the given relative path that we've cached
this is confusing to me.  the comment says "relative path" but the parameter is
named "full_path" (which i interpret as synonymous with "absolute path").

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:35: // from a previous request to
the screenshots source
nit: add period at end of sentence

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:36: // Note: Query string will be
stripped from the full_path
nit: add period

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:48: // in case of a saved
screenshot) to the requestor
nit: add period

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:52: void SendSavedScreenshot(const
std::string& filename, int request_id);
why do these methods still all have subtly different parameter names (full_path
vs. path vs. filename)?  also, the header still doesn't match the
implementation.  SendSavedScreenshot()'s parameter is "filename" here but "path"
in the .cc file.

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:56: ScreenshotDataPtr
current_screenshot_;
nit: add blank line after this one

[rkc, 2011-08-25 13:09:33]

Comments fixed.

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

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:106: read_bytes->clear();
On 2011/08/23 16:58:37, Daniel Erat wrote:
> i don't follow your previous comment about how this handles read errors.  it
> looks to me like the following call to CacheAndSendScreenshot() caches an
empty
> array.  shouldn't it at least check that the array isn't empty before caching
> it?

So in case we can't read the saved screenshot, there are three expected outcomes
we'd want,

a.) The screenshot is not sent with the report
b.) On the webpage, the screenshot is 'unavailable'
c.) Behavior for a.) and b.) should remain the same if using 
GetCachedScreenshot to get the screenshot

All three behaviors are identical if we set the size of read_bytes to 0; there
is code in bug_report_util.cc to ensure that a 0 byte screenshot is simply not
attached to the report. Sending a 0 sized screenshot to the requestor shows up
as image not available on the webpage. Caching the same 0 size screenshot
ensures we have the same behavior on the cached versus original screenshot
request.


Plus it saves us the trouble of null pointer checks :)

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
File chrome/browser/ui/webui/screenshot_source.h (right):

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:34: // Get the screenshot specified
by the given relative path that we've cached
On 2011/08/23 16:58:37, Daniel Erat wrote:
> this is confusing to me.  the comment says "relative path" but the parameter
is
> named "full_path" (which i interpret as synonymous with "absolute path").

I've redone all the 'path' parameter names in both the files. The naming should
be accurate and make much more sense now.
Done.

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:35: // from a previous request to
the screenshots source
On 2011/08/23 16:58:37, Daniel Erat wrote:
> nit: add period at end of sentence

Done.

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:36: // Note: Query string will be
stripped from the full_path
On 2011/08/23 16:58:37, Daniel Erat wrote:
> nit: add period

Done.

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:48: // in case of a saved
screenshot) to the requestor
On 2011/08/23 16:58:37, Daniel Erat wrote:
> nit: add period

Done.

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:52: void SendSavedScreenshot(const
std::string& filename, int request_id);
On 2011/08/23 16:58:37, Daniel Erat wrote:
> why do these methods still all have subtly different parameter names
(full_path
> vs. path vs. filename)?  also, the header still doesn't match the
> implementation.  SendSavedScreenshot()'s parameter is "filename" here but
"path"
> in the .cc file.

Changed the names and comments to make sense all the functions.
Done.

http://codereview.chromium.org/7635017/diff/10003/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.h:56: ScreenshotDataPtr
current_screenshot_;
On 2011/08/23 16:58:37, Daniel Erat wrote:
> nit: add blank line after this one

Done.

[Daniel Erat, 2011-08-25 14:03:52]

Thanks!  LGTM.

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

http://codereview.chromium.org/7635017/diff/18001/chrome/browser/ui/webui/scr...
chrome/browser/ui/webui/screenshot_source.cc:84:
strlen(kSavedScreenshotsBasePath));
nit: as a way to ensure that someone else's future changes don't pass an
incorrect path, it'd be good to add a DCHECK() before this that screenshot_path
actually starts with kSavedScreenshotsBasePath