Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10808027:
gdrive: Get JSON feeds parsing off the UI thread. (Closed)
|
Created:
8 years, 5 months ago by yoshiki Modified:
8 years, 4 months ago Reviewers:
hashimoto CC:
chromium-reviews, achuith+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiongdrive: Get JSON feeds parsing off the UI thread.
BUG=128378
TEST=unit_tests:GData* and browser_tests:GData* passes.
using TBR for gyp changes
TBR=thakis@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148990
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149140
Patch Set 1 #
Total comments: 16
Patch Set 2 : review fix #Patch Set 3 : Call NotifyFinish/NotifySuccess after parse. #
Total comments: 9
Patch Set 4 : fix reviews and rebase #
Total comments: 4
Patch Set 5 : review fix #Patch Set 6 : making OnDataParsed() a private method of GetDataOperation #
Total comments: 12
Patch Set 7 : review fix #Patch Set 8 : Rebase and Fix conflict and build error. #
Messages
Total messages: 30 (0 generated)
re-uploading https://chromiumcodereview.appspot.com/10799018/
https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/gdata/gdata_operations_unittest.cc (right): https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:70: void GetDataOperationParseJsonCallback(GDataErrorCode* error_out, nit: Move this function into the anonymous namespace. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:109: message_loop_.RunAllPending(); Is this line needed? https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:125: EXPECT_TRUE(dict->Remove("foo", &dict_literals[0])); Could you use Get() instead of Remove()? This way, you don't have to delete it and, additionally, you can check the values of entries. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:170: message_loop_.RunAllPending(); Is this line needed? https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:176: getData->NotifySuccess(); Why notifying success after parse failure? https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/gdata/operations_base.cc (right): https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/operations_base.cc:7: #include "base/debug/stack_trace.h" nit: Remove this. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/operations_base.cc:401: ParseJsonOnBlockingPool(data, parsed_value); The comment-outed block below should be used instead of this line? https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/gdata/operations_base.h (right): https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/operations_base.h:31: const AuthStatusCallback& callback, nit: Fix indent
https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/gdata/gdata_operations_unittest.cc (right): https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:70: void GetDataOperationParseJsonCallback(GDataErrorCode* error_out, On 2012/07/19 03:57:57, hashimoto wrote: > nit: Move this function into the anonymous namespace. Done. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:109: message_loop_.RunAllPending(); Done. RunAllPending() is called already in test_util::RunBlockingPoolTask(). On 2012/07/19 03:57:57, hashimoto wrote: > Is this line needed? https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:125: EXPECT_TRUE(dict->Remove("foo", &dict_literals[0])); On 2012/07/19 03:57:57, hashimoto wrote: > Could you use Get() instead of Remove()? > This way, you don't have to delete it and, additionally, you can check the > values of entries. Done. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:170: message_loop_.RunAllPending(); On 2012/07/19 03:57:57, hashimoto wrote: > Is this line needed? Done. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/gdata_operations_unittest.cc:176: getData->NotifySuccess(); On 2012/07/19 03:57:57, hashimoto wrote: > Why notifying success after parse failure? Done. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/gdata/operations_base.cc (right): https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/operations_base.cc:7: #include "base/debug/stack_trace.h" On 2012/07/19 03:57:57, hashimoto wrote: > nit: Remove this. Done. https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/operations_base.cc:401: ParseJsonOnBlockingPool(data, parsed_value); Oops, I forgot to remove debugging code. sorry. On 2012/07/19 03:57:57, hashimoto wrote: > The comment-outed block below should be used instead of this line? https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/gdata/operations_base.h (right): https://chromiumcodereview.appspot.com/10808027/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/gdata/operations_base.h:31: const AuthStatusCallback& callback, On 2012/07/19 03:57:57, hashimoto wrote: > nit: Fix indent Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10808027/6001
Change committed as 147413
hashimoto: Could you take a look again? On Patch set #2, NotifyFinish/NotifySuccess() are called before parse is finished and it breaks the browser_tests:GDataTest.GetDocuments. These methods delete their objects (GetDataOperation object) so GetDataOperation::callback_ has never called. Patch set #3 has 2 changes, - Puts calling of NotifyFinish/NotifySuccess() into after-parse. - Calls GetDataOperation::callback_ even when GetDataOperation object is deleted.
http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_operations.cc (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_operations.cc:540: result_callback.Run(true); nit: Code simply passing true/false is not easy to read. (http://www.chromium.org/developers/coding-style#Code_Formatting) This should be replaced with const bool success = true; result_callback.Run(success); http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.cc:308: weak_ptr_factory_.GetWeakPtr())); nit: Fix indent http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.h (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:24: typedef base::Callback<void(bool result)> ProcessURLFetchResultsCallBack; nit: s/CallBack/Callback/ We use 'callback' as one word. BTW, do we need this typedef at all? Why can't we call OnProcessURLFetchResultsComplete() directly? http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:157: base::WeakPtrFactory<UrlFetchOperationBase> weak_ptr_factory_; Can't this be private instead of protected?
http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_operations.cc (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_operations.cc:540: result_callback.Run(true); On 2012/07/20 04:07:37, hashimoto wrote: > nit: Code simply passing true/false is not easy to read. > (http://www.chromium.org/developers/coding-style#Code_Formatting) > This should be replaced with > const bool success = true; > result_callback.Run(success); Done. http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.cc:308: weak_ptr_factory_.GetWeakPtr())); On 2012/07/20 04:07:37, hashimoto wrote: > nit: Fix indent Done. http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.h (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:24: typedef base::Callback<void(bool result)> ProcessURLFetchResultsCallBack; > nit: s/CallBack/Callback/ We use 'callback' as one word. Done > BTW, do we need this typedef at all? > Why can't we call OnProcessURLFetchResultsComplete() directly? If we call OnProcessURLFetchResultsComplete() directly, we have to change it public because it is called from OnDataParsed() in operations_base.cc. I think passing callback is better than it. http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:157: base::WeakPtrFactory<UrlFetchOperationBase> weak_ptr_factory_; On 2012/07/20 04:07:37, hashimoto wrote: > Can't this be private instead of protected? Done.
http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_operations.cc (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_operations.cc:540: result_callback.Run(true); On 2012/07/20 04:07:37, hashimoto wrote: > nit: Code simply passing true/false is not easy to read. > (http://www.chromium.org/developers/coding-style#Code_Formatting) > This should be replaced with > const bool success = true; > result_callback.Run(success); Done. http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.cc:308: weak_ptr_factory_.GetWeakPtr())); On 2012/07/20 04:07:37, hashimoto wrote: > nit: Fix indent Done. http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.h (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:24: typedef base::Callback<void(bool result)> ProcessURLFetchResultsCallBack; > nit: s/CallBack/Callback/ We use 'callback' as one word. Done > BTW, do we need this typedef at all? > Why can't we call OnProcessURLFetchResultsComplete() directly? If we call OnProcessURLFetchResultsComplete() directly, we have to change it public because it is called from OnDataParsed() in operations_base.cc. I think passing callback is better than it. http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:157: base::WeakPtrFactory<UrlFetchOperationBase> weak_ptr_factory_; On 2012/07/20 04:07:37, hashimoto wrote: > Can't this be private instead of protected? Done.
http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/operations_base.h (right): http://codereview.chromium.org/10808027/diff/6003/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/operations_base.h:24: typedef base::Callback<void(bool result)> ProcessURLFetchResultsCallBack; On 2012/07/24 16:58:32, yoshiki wrote: > > nit: s/CallBack/Callback/ We use 'callback' as one word. > Done > > > BTW, do we need this typedef at all? > > Why can't we call OnProcessURLFetchResultsComplete() directly? > > If we call OnProcessURLFetchResultsComplete() directly, we have to change it > public because it is called from OnDataParsed() in operations_base.cc. I think > passing callback is better than it. How about making OnDataParsed() a private method of GetDataOperation? Having two callbacks (|callback| and |result_callback|) as arguments of OnDataParsed() is confusing. It's good to avoid it if possible. http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_operations.h (right): http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_operations.h:375: nit: Remove blank line. http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:70: void OnDataParsed( Function comment is needed.
> How about making OnDataParsed() a private method of GetDataOperation? It's not better. I want to call GetDataOperation() and |callback| even when the instance of GetDataOperation class has been destructed. > Having two callbacks (|callback| and |result_callback|) as arguments of > OnDataParsed() is confusing. It's good to avoid it if possible. Done. After that, I've found |result_callback| is unnecessary in the unittest so we can call OnProcessURLFetchResultsComplete() directly.
http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/gdata_operations.h (right): http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/gdata_operations.h:375: On 2012/07/25 06:14:21, hashimoto wrote: > nit: Remove blank line. Done. http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/11002/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:70: void OnDataParsed( On 2012/07/25 06:14:21, hashimoto wrote: > Function comment is needed. Done.
Sorry, my explanation below and Patch Set #5 are wrong. I'll upload new CL. On 2012/07/25 17:24:15, yoshiki wrote: > > How about making OnDataParsed() a private method of GetDataOperation? > > It's not better. I want to call GetDataOperation() and |callback| even when the > instance of GetDataOperation class has been destructed. > > > Having two callbacks (|callback| and |result_callback|) as arguments of > > OnDataParsed() is confusing. It's good to avoid it if possible. > > Done. After that, I've found |result_callback| is unnecessary in the unittest so > we can call OnProcessURLFetchResultsComplete() directly.
On 2012/07/25 18:55:14, yoshiki wrote: > On 2012/07/25 17:24:15, yoshiki wrote: > > > How about making OnDataParsed() a private method of GetDataOperation? > > > > It's not better. I want to call GetDataOperation() and |callback| even when the > > instance of GetDataOperation class has been destructed. Sorry again. What I said above is wrong and you are correct. An instance of GetDataOperation class is never destructed before OnDataParsed(), because such a destruction before NotifyFinish() causes a violence of DCHECK in gdata_operation_runner.cc. So we can make OnDataParsed() a private method of GetDataOperation and I did it, as you said. Could you take a look at Patch Set #6?
http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:343: OnProcessURLFetchResultsComplete(true); Do something (replace true with "const bool success = true" or add comment, etc) to make it clear that this 'true' means success. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:375: OnProcessURLFetchResultsComplete(false); ditto. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:403: base::Unretained(this), Use WeakPtr http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:412: const gdata::GetDataCallback& callback) { Since |callback_| is always passed, this argument is no longer needed. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:415: bool ret = true; |ret| is ambiguous, rename this variable to |success| or something more informative. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:423: if (!callback.is_null()) Call RunCallback() instead.
http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... File chrome/browser/chromeos/gdata/operations_base.cc (right): http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:343: OnProcessURLFetchResultsComplete(true); On 2012/07/26 02:08:44, hashimoto wrote: > Do something (replace true with "const bool success = true" or add comment, etc) > to make it clear that this 'true' means success. Done. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:375: OnProcessURLFetchResultsComplete(false); On 2012/07/26 02:08:44, hashimoto wrote: > ditto. Done. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:403: base::Unretained(this), On 2012/07/26 02:08:44, hashimoto wrote: > Use WeakPtr Done. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:412: const gdata::GetDataCallback& callback) { On 2012/07/26 02:08:44, hashimoto wrote: > Since |callback_| is always passed, this argument is no longer needed. Done. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:415: bool ret = true; On 2012/07/26 02:08:44, hashimoto wrote: > |ret| is ambiguous, rename this variable to |success| or something more > informative. Done. http://codereview.chromium.org/10808027/diff/29012/chrome/browser/chromeos/gd... chrome/browser/chromeos/gdata/operations_base.cc:423: if (!callback.is_null()) On 2012/07/26 02:08:44, hashimoto wrote: > Call RunCallback() instead. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10808027/23004
Presubmit check for 10808027-23004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.3s to calculate.
On 2012/07/27 11:19:22, I haz the power (commit-bot) wrote: > Presubmit check for 10808027-23004 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for files in these directories: > chrome > > Presubmit checks took 1.3s to calculate. FYI: You can use TBR for gyp changes (https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/a7oLXoqawtQ...)
Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10808027/23004
Try job failure for 10808027-23004 (retry) on linux_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10808027/23004
Change committed as 148990
Patch set #7 was conflicted with latest tot and causes the build error. Patch #8 fixed this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10808027/27013
Change committed as 149140 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
