[chromium-reviews] For system-level installs, if the spellcheck dictionary fails to get download...

For system-level installs, if the spellcheck dictionary fails to get downloaded in the profile Application directory, download it in the User Data directory instead.
BUG=http://www.crbug.com/9650
TEST=Launch Chrome, and navigate to translate.google.com. Before typing anything in the text box, change name of folder Chrome/Application/Dictionaries to something else. Then, start typing something. The spellcheck dictionary, unable to get the folder Dictionaries, will fail to download, which will trigger its download in Chrome/User Data/Dictionaries. After it has downloaded, spellchecker will start working automatically.

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

[brettw, 2009-07-13 17:04:02]

http://codereview.chromium.org/155394/diff/4/1006
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/155394/diff/4/1006#newcode465
Line 465: if (!file_util::PathExists(bdict_file_name_)) {
I don't think I understand how this works. Doesn't the bdict_file_name already
include the app filename? It looks to me like you will download to the
user-data-dir every time.

http://codereview.chromium.org/155394/diff/4/1006#newcode470
Line 470: Task* dic_task =
dic_download_state_changer_factory_.NewRunnableMethod(
This function seems like it's getting excessively complicated, and there are two
separate calls to PostTask(StartDownload). Can this be refactored into separate
functions that do the setup and have only one PostTask call?

http://codereview.chromium.org/155394/diff/4/1006#newcode507
Line 507: } else { // There is no dictionary, despite trying to donwload twice.
End-of-line comments should have two spaces before them.

[sidchat (Google), 2009-07-13 18:06:58]

Made changes as per your comments - please have a look again.

-Sid

http://codereview.chromium.org/155394/diff/4/1006
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/155394/diff/4/1006#newcode465
Line 465: if (!file_util::PathExists(bdict_file_name_)) {
Explanations have been added as a comment.
On 2009/07/13 17:04:02, brettw wrote:
> I don't think I understand how this works. Doesn't the bdict_file_name already
> include the app filename? It looks to me like you will download to the
> user-data-dir every time.

http://codereview.chromium.org/155394/diff/4/1006#newcode470
Line 470: Task* dic_task =
dic_download_state_changer_factory_.NewRunnableMethod(
Moved the whole download process in a separate method.

On 2009/07/13 17:04:02, brettw wrote:
> This function seems like it's getting excessively complicated, and there are
two
> separate calls to PostTask(StartDownload). Can this be refactored into
separate
> functions that do the setup and have only one PostTask call?

http://codereview.chromium.org/155394/diff/4/1006#newcode507
Line 507: } else { // There is no dictionary, despite trying to donwload twice.
On 2009/07/13 17:04:02, brettw wrote:
> End-of-line comments should have two spaces before them.

Done.

[brettw, 2009-07-13 20:09:30]

I don't see how you addressed my comment. What happens when the dictionary has
been downloaded to the user data dir and we start again with it already there?
Will we use the already downloaded one?

[sidchat (Google), 2009-07-13 20:41:16]

> I don't see how you addressed my comment. What happens when the dictionary has
> been downloaded to the user data dir and we start again with it already there?
> Will we use the already downloaded one?
When we try to use Spellchecker again, it looks at the application directory,
cannot find, tries to download, fails, looks at the user data directory, finds
it and uses it.

I detect an issue here this time - every time the spell-checker is initialized,
it goes through the loop above. There should be someway for the SpellChecker to
remember the path set last time (if it is user data directory) so that it does
not go through the loop above again and again.

Let me update the CL accordingly.

[sidchat (Google), 2009-07-13 21:51:28]

OK, so now both directories are checked before attempting to download the file.
Please have a look again.

-Sid

On 2009/07/13 20:41:16, sidchat (Google) wrote:
> > I don't see how you addressed my comment. What happens when the dictionary
has
> > been downloaded to the user data dir and we start again with it already
there?
> > Will we use the already downloaded one?
> When we try to use Spellchecker again, it looks at the application directory,
> cannot find, tries to download, fails, looks at the user data directory, finds
> it and uses it.
> 
> I detect an issue here this time - every time the spell-checker is
initialized,
> it goes through the loop above. There should be someway for the SpellChecker
to
> remember the path set last time (if it is user data directory) so that it does
> not go through the loop above again and again.
> 
> Let me update the CL accordingly.

[brettw, 2009-07-13 21:55:43]

Okay, I think that bug is addressed. I'm now concerned that you will have to
download it twice when it needs to go in the user profile dir. It looks to me
like the sequence of events is:

1. First word to spellcheck -> kick off a download to app dir
2. That fails sometime in the future
3. Then we spellcheck again -> kick off a download to the user dir
4. We download again.

Is that correct? It seems wrong to me to have to do all that. Do you agree?

[sidchat (Google), 2009-07-13 22:31:46]

> Okay, I think that bug is addressed. I'm now concerned that you will have to
> download it twice when it needs to go in the user profile dir. It looks to me
> like the sequence of events is:
> 
> 1. First word to spellcheck -> kick off a download to app dir
> 2. That fails sometime in the future
> 3. Then we spellcheck again -> kick off a download to the user dir
> 4. We download again.
> 
> Is that correct? It seems wrong to me to have to do all that. Do you agree?
Yes, downloading twice sounds redundant, although I think this will occur only
once (for each dictionary language) - so should not be an issue in general.
Nevertheless, I can think of the following ways to solve this:

- Is there a way to know the permission of a directory? Then we can download
only once and point it directly to the directory having the permissions.

- Download in the user directory by default. Some dictionaries are already there
in the apps directory. Let them remain, and we will do the check as we are doing
now to check both the directories.

[sidchat (Google), 2009-07-13 22:44:38]

Hmm OK. Offline chat with Rahulk reveals that "user data" folder is always
available for writing, for system-wide installs or otherwise. So, I will
download the dictionary to user data/Dictionaries by default, and then try to
move it to application folder. If it fails, let it remain in user data.

On 2009/07/13 22:31:46, sidchat (Google) wrote:
> > Okay, I think that bug is addressed. I'm now concerned that you will have to
> > download it twice when it needs to go in the user profile dir. It looks to
me
> > like the sequence of events is:
> > 
> > 1. First word to spellcheck -> kick off a download to app dir
> > 2. That fails sometime in the future
> > 3. Then we spellcheck again -> kick off a download to the user dir
> > 4. We download again.
> > 
> > Is that correct? It seems wrong to me to have to do all that. Do you agree?
> Yes, downloading twice sounds redundant, although I think this will occur only
> once (for each dictionary language) - so should not be an issue in general.
> Nevertheless, I can think of the following ways to solve this:
> 
> - Is there a way to know the permission of a directory? Then we can download
> only once and point it directly to the directory having the permissions.
> 
> - Download in the user directory by default. Some dictionaries are already
there
> in the apps directory. Let them remain, and we will do the check as we are
doing
> now to check both the directories.

[sidchat (Google), 2009-07-13 23:33:40]

OK, so now the download occurs only once. From the memory, check to see whether
it can be saved in Applications folder - if not, save it in user data folder.
Please have a look again.

-Sid

On 2009/07/13 22:44:38, sidchat (Google) wrote:
> Hmm OK. Offline chat with Rahulk reveals that "user data" folder is always
> available for writing, for system-wide installs or otherwise. So, I will
> download the dictionary to user data/Dictionaries by default, and then try to
> move it to application folder. If it fails, let it remain in user data.
> 
> On 2009/07/13 22:31:46, sidchat (Google) wrote:
> > > Okay, I think that bug is addressed. I'm now concerned that you will have
to
> > > download it twice when it needs to go in the user profile dir. It looks to
> me
> > > like the sequence of events is:
> > > 
> > > 1. First word to spellcheck -> kick off a download to app dir
> > > 2. That fails sometime in the future
> > > 3. Then we spellcheck again -> kick off a download to the user dir
> > > 4. We download again.
> > > 
> > > Is that correct? It seems wrong to me to have to do all that. Do you
agree?
> > Yes, downloading twice sounds redundant, although I think this will occur
only
> > once (for each dictionary language) - so should not be an issue in general.
> > Nevertheless, I can think of the following ways to solve this:
> > 
> > - Is there a way to know the permission of a directory? Then we can download
> > only once and point it directly to the directory having the permissions.
> > 
> > - Download in the user directory by default. Some dictionaries are already
> there
> > in the apps directory. Let them remain, and we will do the check as we are
> doing
> > now to check both the directories.

[brettw, 2009-07-15 20:33:08]

http://codereview.chromium.org/155394/diff/1032/18
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/155394/diff/1032/18#newcode297
Line 297: if (!SaveBufferToFile(data)) {
This seems weird. You call a function called SaveBufferToFile, and when that
fails, the code from that function is copied into this function and run. Can you
refactor SaveBufferToFile such that it's appropriate for both cases?

http://codereview.chromium.org/155394/diff/1032/18#newcode302
Line 302: dict_dir_userdata = dict_dir_userdata.AppendASCII("Dictionaries");
This code for computing the directory name is duplicated twice (and the constant
is duplicated as well. Seems like it should be a separate function.

http://codereview.chromium.org/155394/diff/1032/18#newcode519
Line 519: tried_to_init_ = true;
This has to be duplicated for every return path. How about moving it up to right
below where it's checked above?

[sidchat (Google), 2009-07-16 21:47:35]

Please have a look again

-Sid

http://codereview.chromium.org/155394/diff/1032/18
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/155394/diff/1032/18#newcode297
Line 297: if (!SaveBufferToFile(data)) {
On 2009/07/15 20:33:08, brettw wrote:
> This seems weird. You call a function called SaveBufferToFile, and when that
> fails, the code from that function is copied into this function and run. Can
you
> refactor SaveBufferToFile such that it's appropriate for both cases?

Done.

http://codereview.chromium.org/155394/diff/1032/18#newcode302
Line 302: dict_dir_userdata = dict_dir_userdata.AppendASCII("Dictionaries");
On 2009/07/15 20:33:08, brettw wrote:
> This code for computing the directory name is duplicated twice (and the
constant
> is duplicated as well. Seems like it should be a separate function.

Done.

http://codereview.chromium.org/155394/diff/1032/18#newcode519
Line 519: tried_to_init_ = true;
That won't work because tried_to_init_ = true can be set only when dictionary
download was tried and failed (thats when it comes to this part of the code), or
dictionary is there but could not initiate hunspell due to some reason (end of
this function). Anywhere else would make a wrong return.
On 2009/07/15 20:33:08, brettw wrote:
> This has to be duplicated for every return path. How about moving it up to
right
> below where it's checked above?

[brettw, 2009-07-17 17:43:03]

LGTM