Merge SavePackageFilePicker{,ChromeOS}

Merge SavePackageFilePicker{,ChromeOS}

Move prefs::kSaveFileDefaultDirectory into DownloadPrefs.

Merge ChromeDownloadManagerDelegate::last_download_path_ with kSaveFileDefaultDirectory.

Add RegisterOverlayPref(kSaveFileDefaultDirectory) to PrefsTabHelper::InitIncognitoUserPrefStore in order to prevent leaking the path to the on-record prefs.

When the user sets kDownloadDefaultDirectory in chrome://settings, also set kSaveFileDefaultDirectory so that the save file picker shows that path.

bauerb: chrome/browser/browsing_data/ + chrome/browser/prefs/ + chrome/browser/ui/prefs/
achuith: chrome/browser/chromeos/
dbeam: chrome/browser/ui/webui/options/

BUG=179520
BUG=102782
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203140

[benjhayden, 2013-04-24 18:15:10]

It's starting to look like the SavePackageFilePickers need to be refactored. If
you have ideas for how they should be refactored, this might or might not be the
CL for it.

I should also write a test.

PTAL

[asanka, 2013-04-24 22:03:13]

The general approach makes sense.

I commented on the issue about some other considerations about which preferences
to use, though I'm not sure if this is the CL to address them.

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1235:
pref_service->SetFilePath(prefs::kSaveFileDefaultDirectory, path);
We should also call ChromeDownloadManagerDelegate::ClearLastDownloadPath() here
so that the new downloads folder will be picked up by regular Save As downloads.

[Randy Smith (Not in Mondays), 2013-04-25 17:58:55]

So the two thoughts that come to mind about refactoring are: a) the joking one I
made in person, of doing the SP unification, and b) that you make SPFPCO inherit
from SPFP, and make what you're currently passing in as a callback a protected
method on SPFP.  I'd like to encourage you to consider the second option, since
I don't like adding the extra callback to the interface--looking just at the
interface, it's really confusing why it's there (as I note below in asking for a
comment) and as best I can tell the reason you're adding it is to avoid
duplicating code.  There might also be other simplifications that could come
from doing the inheritance--the two classes are doing fairly similar things.

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:191: std::string
path_string = WideToUTF8(path.DirName().value());
Is this the usual method of persisting file paths on Windows?  I'm concerned
that WideToUTF8 could lose information that users on Windows actually care
about--should we find some way of encoding all the information into the
preference?

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker.h (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.h:24: const
content::SavePackagePathPickedCallback& callback);
Documentation somewhere explaining the difference between these two callbacks?

[asanka, 2013-04-25 19:01:39]

FYI There's some discussions on
https://code.google.com/p/chromium/issues/detail?id=179520 about the how the
last save directory should behave.

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:191: std::string
path_string = WideToUTF8(path.DirName().value());
On 2013/04/25 17:58:55, rdsmith wrote:
> Is this the usual method of persisting file paths on Windows?  I'm concerned
> that WideToUTF8 could lose information that users on Windows actually care
> about--should we find some way of encoding all the information into the
> preference?

kSaveFileDefaultDirectory is registered in CDMD as a FilePath pref.
save_file_path should be a FilePathPrefMember and no conversion should be done
when storing/retrieving the pref value.

[benjhayden, 2013-04-30 21:11:12]

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:191: std::string
path_string = WideToUTF8(path.DirName().value());
On 2013/04/25 19:01:39, asanka wrote:
> On 2013/04/25 17:58:55, rdsmith wrote:
> > Is this the usual method of persisting file paths on Windows?  I'm concerned
> > that WideToUTF8 could lose information that users on Windows actually care
> > about--should we find some way of encoding all the information into the
> > preference?
> 
> kSaveFileDefaultDirectory is registered in CDMD as a FilePath pref.
> save_file_path should be a FilePathPrefMember and no conversion should be done
> when storing/retrieving the pref value.

Done.

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:191: std::string
path_string = WideToUTF8(path.DirName().value());
On 2013/04/25 17:58:55, rdsmith wrote:
> Is this the usual method of persisting file paths on Windows?  I'm concerned
> that WideToUTF8 could lose information that users on Windows actually care
> about--should we find some way of encoding all the information into the
> preference?

Done.

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker.h (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.h:24: const
content::SavePackagePathPickedCallback& callback);
On 2013/04/25 17:58:55, rdsmith wrote:
> Documentation somewhere explaining the difference between these two callbacks?

Moot.

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12662032/diff/11001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:1235:
pref_service->SetFilePath(prefs::kSaveFileDefaultDirectory, path);
On 2013/04/24 22:03:13, asanka wrote:
> We should also call ChromeDownloadManagerDelegate::ClearLastDownloadPath()
here
> so that the new downloads folder will be picked up by regular Save As
downloads.

Moot -- merged CDMD::last_download_path_ with kSaveFileDefaultDirectory.

[Randy Smith (Not in Mondays), 2013-05-01 19:18:43]

Second round of comments.  Looks basically good.

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker.cc (right):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.cc:76:
ChromeDownloadManagerDelegate::DisableSafeBrowsing(download);
If I'm reading the code correctly, this hasn't historically been done under
ChromeOS.  Why start now?  (I don't object, I'm just asking for my own
education.)

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.cc:236: delete this;
Suggestion: I'm a bit uncomfortable with the number of different exit points
from this routine, all of them with a "delete this" in them.  Seems like an
invitation for drift leading to leaks.  It's not an idiom I've seen before, but
maybe setup a scoped_ptr<SavePackageFilePicker> delete_on_exit(this); at
function entry?

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.cc:266:
download_prefs_->SetSaveFilePath(path.DirName());
This doesn't look right to me, though I'm not sure of the right pattern.  I
think the *behavior* we want for incognito is for the path preference to sticky
inside the profile, and to disappear when the profile does.  If I was addressing
this from scratch, the way I'd do that is make sure that the download_prefs_
associated with an incognito profile were initialized from the corresponding
non-incognito profile but not synced out to disk when the profile was destroyed.
 I haven't checked, but I think it's worth at least a bit of code tracing to
find out if this is what's happening or not; if it is, you can skip the
conditional.  If it's not, I'd put at least a bit of thought into how you might
achieve the desired behavior before making it non-sticky within incognito
profiles.

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker.h (right):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.h:20: const
base::FilePath::StringType& default_extension_const,
nit: If it's a choice between making the interface a little confusing or making
the implementation a little confusing, I'd vote for the implementation :-}.  I
don't think the _const suffices make sense except in the context of the function
definition in the .cc file, so I'd leave them off here, and either a) accept the
difference in parameter naming between decl and defn, or b) rename things in the
implementation to make more sense (e.g. renaming the copies _nonconst).

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:29: #include
"chrome/browser/download/download_service_factory.h"
Why are all the extra includes needed now?

[benjhayden, 2013-05-06 20:05:18]

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker.cc (right):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.cc:76:
ChromeDownloadManagerDelegate::DisableSafeBrowsing(download);
On 2013/05/01 19:18:43, rdsmith wrote:
> If I'm reading the code correctly, this hasn't historically been done under
> ChromeOS.  Why start now?  (I don't object, I'm just asking for my own
> education.)

Code unification.
It will continue to do nothing if safe browsing is not enabled, and if/when safe
browsing is enabled on cros, then fewer things will need to be updated.

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.cc:236: delete this;
On 2013/05/01 19:18:43, rdsmith wrote:
> Suggestion: I'm a bit uncomfortable with the number of different exit points
> from this routine, all of them with a "delete this" in them.  Seems like an
> invitation for drift leading to leaks.  It's not an idiom I've seen before,
but
> maybe setup a scoped_ptr<SavePackageFilePicker> delete_on_exit(this); at
> function entry?

Done.

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.cc:266:
download_prefs_->SetSaveFilePath(path.DirName());
On 2013/05/01 19:18:43, rdsmith wrote:
> This doesn't look right to me, though I'm not sure of the right pattern.  I
> think the *behavior* we want for incognito is for the path preference to
sticky
> inside the profile, and to disappear when the profile does.  If I was
addressing
> this from scratch, the way I'd do that is make sure that the download_prefs_
> associated with an incognito profile were initialized from the corresponding
> non-incognito profile but not synced out to disk when the profile was
destroyed.
>  I haven't checked, but I think it's worth at least a bit of code tracing to
> find out if this is what's happening or not; if it is, you can skip the
> conditional.  If it's not, I'd put at least a bit of thought into how you
might
> achieve the desired behavior before making it non-sticky within incognito
> profiles.

This might be a can of worms.
Up until now, DownloadPrefs for incognito profiles leaked everything to the
on-record pref store.
I can see the privacy issue if directory names from incognito sessions were
leaked to the on-record pref store, so I added 'base::FilePath
DownloadPrefs::incognito_save_file_path_'. I can't quite see the same privacy
issue for the other fields there, so I only did this for save_file_path.
PTAL at download_prefs.cc.

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
File chrome/browser/download/save_package_file_picker.h (right):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/download/s...
chrome/browser/download/save_package_file_picker.h:20: const
base::FilePath::StringType& default_extension_const,
On 2013/05/01 19:18:43, rdsmith wrote:
> nit: If it's a choice between making the interface a little confusing or
making
> the implementation a little confusing, I'd vote for the implementation :-}.  I
> don't think the _const suffices make sense except in the context of the
function
> definition in the .cc file, so I'd leave them off here, and either a) accept
the
> difference in parameter naming between decl and defn, or b) rename things in
the
> implementation to make more sense (e.g. renaming the copies _nonconst).

Done.

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:29: #include
"chrome/browser/download/download_service_factory.h"
On 2013/05/01 19:18:43, rdsmith wrote:
> Why are all the extra includes needed now?

Done.

[Randy Smith (Not in Mondays), 2013-05-06 21:46:02]

Only remaining concern is around the prefs integration that we talked about
offline.  (Summary: Glancing at the code is looks like the incognito issues with
prefs are being taken care of a couple of layers below us and we shouldn't worry
about them, but if that's not what you're seeing in your experiments, it might
be worthwhile tracking down what's actually going on.)

[asanka, 2013-05-07 15:15:12]

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (left):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:326:
download_manager_delegate->ClearLastDownloadPath();
Why aren't you resetting kSaveFileDefaultDirectory ?

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:416: void
ChromeDownloadManagerDelegate::OnDownloadPathSelected(
Suggestion: This callback isn't required since setting the last download path is
something that DTD can do. However, you'll also need to redo the corresponding
test in d_t_d_unittests.cc.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_prefs.cc:154: download_path_.SetValue(path);
Suggestion: Should this also set save_file_path_ ? The new behavior of
save_file_path_ requires that it be changed whenever download_path_ changes.
Perhaps that responsibility should be internal to DownloadPrefs.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_prefs.cc:165: if (profile_->IsOffTheRecord()) {
Does this need to be handled this way? Isn't the incognito profile's
OverlayUserPrefStore effectively doing the same thing?

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:160: if (should_prompt_ &&
!download_prefs_->IsDownloadPathManaged()) {
The DCHECK() was to assert that we don't try to prompt for a managed path. I.e.
if should_prompt_ is true, then IsDownloadPathManaged() should be false. Having
this in a conditional doesn't make sense.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner_unittest.cc (left):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_target_determiner_unittest.cc:805:
SCOPED_TRACE(testing::Message()
Can you add a descriptive SCOPED_TRACE() here?

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_target_determiner_unittest.cc:1468:
set_last_selected_directory(GetPathInDownloadDir(
The purpose of this test is to verify that that when an extension overrides a
path, we use a one relative to the default download path instead of the last
selected path. If the last save path isn't being set, then this isn't testing
anything new.

The case where the last save path is the same as the default downloads path is
already covered in the TargetDeterminer_NotifyExtensionsSafe test.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/save_package_file_picker.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/save_package_file_picker.cc:146:
suggested_path_copy.Extension().compare(FILE_PATH_LITERAL("htm")) &&
FilePath::Extension() returns a leading period. I.e. it would return ".html" for
"foo.html".

It's probably easier to use FilePath::MatchesExtension() here instead of
.compare(). You'd need the leading period with that as well.

[benjhayden, 2013-05-17 20:31:53]

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (left):

https://codereview.chromium.org/12662032/diff/65001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:326:
download_manager_delegate->ClearLastDownloadPath();
On 2013/05/07 15:15:12, asanka wrote:
> Why aren't you resetting kSaveFileDefaultDirectory ?

See latest patchset.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:416: void
ChromeDownloadManagerDelegate::OnDownloadPathSelected(
On 2013/05/07 15:15:12, asanka wrote:
> Suggestion: This callback isn't required since setting the last download path
is
> something that DTD can do. However, you'll also need to redo the corresponding
> test in d_t_d_unittests.cc.

Done.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_prefs.cc:154: download_path_.SetValue(path);
On 2013/05/07 15:15:12, asanka wrote:
> Suggestion: Should this also set save_file_path_ ? The new behavior of
> save_file_path_ requires that it be changed whenever download_path_ changes.
> Perhaps that responsibility should be internal to DownloadPrefs.

Done.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_prefs.cc:165: if (profile_->IsOffTheRecord()) {
On 2013/05/07 15:15:12, asanka wrote:
> Does this need to be handled this way? Isn't the incognito profile's
> OverlayUserPrefStore effectively doing the same thing?

This is necessary. Without incognito_save_file_path_, saving a page to
secret/directory/ in incognito will cause the on-record save_file_path_ to be
secret/directory/.
I'm still investigating why OverlayUserPrefStore isn't doing this for us.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:160: if (should_prompt_ &&
!download_prefs_->IsDownloadPathManaged()) {
On 2013/05/07 15:15:12, asanka wrote:
> The DCHECK() was to assert that we don't try to prompt for a managed path.
I.e.
> if should_prompt_ is true, then IsDownloadPathManaged() should be false.
Having
> this in a conditional doesn't make sense.

Done.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner_unittest.cc (left):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_target_determiner_unittest.cc:805:
SCOPED_TRACE(testing::Message()
On 2013/05/07 15:15:12, asanka wrote:
> Can you add a descriptive SCOPED_TRACE() here?

Done.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/download_target_determiner_unittest.cc:1468:
set_last_selected_directory(GetPathInDownloadDir(
On 2013/05/07 15:15:12, asanka wrote:
> The purpose of this test is to verify that that when an extension overrides a
> path, we use a one relative to the default download path instead of the last
> selected path. If the last save path isn't being set, then this isn't testing
> anything new.
> 
> The case where the last save path is the same as the default downloads path is
> already covered in the TargetDeterminer_NotifyExtensionsSafe test.

Done.

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
File chrome/browser/download/save_package_file_picker.cc (right):

https://codereview.chromium.org/12662032/diff/177001/chrome/browser/download/...
chrome/browser/download/save_package_file_picker.cc:146:
suggested_path_copy.Extension().compare(FILE_PATH_LITERAL("htm")) &&
On 2013/05/07 15:15:12, asanka wrote:
> FilePath::Extension() returns a leading period. I.e. it would return ".html"
for
> "foo.html".
> 
> It's probably easier to use FilePath::MatchesExtension() here instead of
> .compare(). You'd need the leading period with that as well.

Done.

[benjhayden, 2013-05-17 21:23:52]

Figured out why the save file path was leaking.
SetValue forwards to the underlying (on-record) PrefStore when
ShallBeStoredInOverlay returns false. It only returns true if  the key was
passed to OverlayUserPrefStore::RegisterOverlayPref(), so I added
RegisterOverlayPref(kSaveFileDefaultDirectory) to
PrefsTabHelper::InitIncognitoUserPrefStore.

[Randy Smith (Not in Mondays), 2013-05-17 21:38:45]

LGTM.

[asanka, 2013-05-18 04:29:04]

LGTM

[commit-bot: I haz the power, 2013-05-29 13:42:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12662032/294001

[commit-bot: I haz the power, 2013-05-29 14:13:34]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-29 14:30:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12662032/294001

[commit-bot: I haz the power, 2013-05-29 14:50:44]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[benjhayden, 2013-05-29 15:34:50]

@r202870

[commit-bot: I haz the power, 2013-05-29 17:46:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12662032/321002

[commit-bot: I haz the power, 2013-05-29 18:01:16]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[benjhayden, 2013-05-29 18:14:14]

bauerb: chrome/browser/browsing_data/ + chrome/browser/prefs/ +
chrome/browser/ui/prefs/
achuith: chrome/browser/chromeos/
estade: chrome/browser/ui/webui/options/

[achuithb, 2013-05-29 18:15:40]

lgtm

[Bernhard Bauer, 2013-05-29 19:26:36]

chrome/browser/browsing_data/ + chrome/browser/ + chrome/browser/ui/prefs/ LGTM

[benjhayden, 2013-05-29 20:39:14]

dbeam: chrome/browser/ui/webui/options/

[Dan Beam, 2013-05-29 20:48:40]

rs lgtm

[commit-bot: I haz the power, 2013-05-29 20:56:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12662032/321002

[commit-bot: I haz the power, 2013-05-30 00:23:55]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-05-30 13:26:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/12662032/321002

[commit-bot: I haz the power, 2013-05-30 14:15:46]

Change committed as 203140