Refactor GDataWapiFeedLoader::LoadFromServer() parameters

Refactor GDataWapiFeedLoader::LoadFromServer() parameters

All the parameters for LoadFromServer() is consolidated in 
struct LoadFeedParams, and the function is made private in
GDataWapiFeedLoader class. Instead, SearchFromServer()
and LoadDirectoryFromServer() public methods are added to the class.
|should_fetch_multiple_feeds| parameter was never used with
false value, so it is removed.



BUG=141359
TEST=all tests pass, manually check file browser works.

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

[kochi, 2012-08-16 10:36:30]

Hi Satoru,

Could you review this?
Thanks,

[satorux1, 2012-08-16 10:56:29]

http://codereview.chromium.org/10836285/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h (right):

http://codereview.chromium.org/10836285/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:107: };
hmm, rather than just putting a bunch of parameters in a struct, can we do a
better job?

Would it be possible to redesign the class so we don't need to deal with this
many parameters in LoadFromServer()?

For instance, Seems to me that |search_query| is awkward. We might want to have
a separate function for search, like SearchFromServer().

[kochi, 2012-08-17 03:30:41]

Thanks for the review!

Made 2 public interface (SearchFromServer and LoadDirectoryFromServer)
and made LoadFromServer private.

Does this look better?

[satorux1, 2012-08-17 11:01:31]

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2034: file_path));
wow, six parameters are gone! Nice.

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2437:
base::Bind(&GDataFileSystem::OnSearch, ui_weak_ptr_, callback));
nice.

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (left):

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:157: bool
should_fetch_multiple_feeds,
so the parameter was unsed in the existing code?

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:316:
weak_ptr_factory_.GetWeakPtr()));
glad that the large copy-paste is gone.

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h (right):

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:121: // Starts retrieving
feed for a specific directory from the server.
please write more function comments. please describe parameters. we also need:

 // |callback| must not be null.

http://codereview.chromium.org/10836285/diff/6/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:127: // Starts retrieving
search results from the server.
more function comment.

[kochi, 2012-08-17 15:56:37]

Thanks for the review!

CL updated for the comments.

http://chromiumcodereview.appspot.com/10836285/diff/6/chrome/browser/chromeos...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (left):

http://chromiumcodereview.appspot.com/10836285/diff/6/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:157: bool
should_fetch_multiple_feeds,
On 2012/08/17 11:01:31, satorux1 wrote:
> so the parameter was unsed in the existing code?

Yes, it is unused.
The default value was true, and it was never changed.

Added a note in the CL description.

http://chromiumcodereview.appspot.com/10836285/diff/6/chrome/browser/chromeos...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h (right):

http://chromiumcodereview.appspot.com/10836285/diff/6/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:121: // Starts retrieving
feed for a specific directory from the server.
On 2012/08/17 11:01:31, satorux1 wrote:
> please write more function comments. please describe parameters. we also need:
> 
>  // |callback| must not be null.

Done.

http://chromiumcodereview.appspot.com/10836285/diff/6/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:127: // Starts retrieving
search results from the server.
On 2012/08/17 11:01:31, satorux1 wrote:
> more function comment.

Done.

[satorux1, 2012-08-17 16:00:12]

LGTM

http://codereview.chromium.org/10836285/diff/4005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h (right):

http://codereview.chromium.org/10836285/diff/4005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.h:133: // must not be null.
nit: move the last sentence to a separate line.

// |feed_load_callback| must not be null.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/10836285/15001

[commit-bot: I haz the power, 2012-08-17 20:52:49]

Try job failure for 10836285-15001 (retry) on win_rel for steps
"interactive_ui_tests, browser_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-18 06:28:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/10836285/15001

[commit-bot: I haz the power, 2012-08-18 08:57:45]

Change committed as 152252