Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2034543003:
Retry icon fetching after intent_helper is ready (Closed)
|
Created:
4 years, 6 months ago by Yusuke Sato Modified:
4 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRetry icon fetching after intent_helper is ready.
* Modify ActivityIconLoader::GetActivityIcon() so it returns
an error code.
* Modify ArcProcessTask so it calls GetActivityIcon() again
when the function returns a temporary error.
BUG=616159
TEST=All ARC tasks in Task Manager have their icons
Committed: https://crrev.com/b229a826d51cf8d5663184bcade36f3df9ee6014
Cr-Commit-Position: refs/heads/master@{#397581}
Patch Set 1 : review #
Total comments: 12
Patch Set 2 : address comments #Patch Set 3 : address comment #Patch Set 4 : add a missing public #
Total comments: 8
Patch Set 5 : address comments #
Messages
Total messages: 25 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== wip: retry icon fetching if intent_helper not ready BUG=616159 ========== to ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons ==========
Description was changed from ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons ========== to ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons ==========
Patchset #1 (id:40001) has been deleted
yusukes@chromium.org changed reviewers: + lhchavez@chromium.org
PTAL
https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task.cc (left): https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:11: #include "components/arc/arc_bridge_service.h" Keep it for IWYU? https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:72: std::vector<arc::ActivityIconLoader::ActivityName> activity; nit: maybe only do this in the if block? https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:131: base::Bind(base::IgnoreResult(&ArcProcessTask::StartIconLoading), It might be the case that by the time you call StartIconLoading, the IconLoader was destroyed and might be re-loaded at a later point in time. So how about you make StartIconLoading not return anything and have it add the observer to be notified in case it cannot get the IconLoader? https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... File components/arc/intent_helper/activity_icon_loader_unittest.cc (right): https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... components/arc/intent_helper/activity_icon_loader_unittest.cc:92: EXPECT_EQ(ActivityIconLoader::GetResult::SUCCEEDED_SYNC, Oh EXPECTED_EQ now works with case class? Neat! https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... File components/arc/intent_helper/link_handler_model_impl.cc (right): https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... components/arc/intent_helper/link_handler_model_impl.cc:122: (result != ActivityIconLoader::GetResult::SUCCEEDED_ASYNC); Isn't the intention "result == ActivityIconLoader::GetResult::SUCCEEDED_SYNC"?
thanks, ptal https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task.cc (left): https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:11: #include "components/arc/arc_bridge_service.h" On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > Keep it for IWYU? This has been moved to the header side, and as far as I know, it's allowed to remove it from .cc: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes "However, any includes present in the related header do not need to be included again in the related cc (i.e., foo.cc can rely on foo.h's includes)." https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:72: std::vector<arc::ActivityIconLoader::ActivityName> activity; On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > nit: maybe only do this in the if block? Good catch, done. https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:131: base::Bind(base::IgnoreResult(&ArcProcessTask::StartIconLoading), On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > It might be the case that by the time you call StartIconLoading, the IconLoader > was destroyed and might be re-loaded at a later point in time. > > So how about you make StartIconLoading not return anything and have it add the > observer to be notified in case it cannot get the IconLoader? Done. I'd keep this PostTask though (because directly calling into StartIconLoading() which may call AddObserver() doesn't seem safe.) https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... File components/arc/intent_helper/link_handler_model_impl.cc (right): https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... components/arc/intent_helper/link_handler_model_impl.cc:122: (result != ActivityIconLoader::GetResult::SUCCEEDED_ASYNC); On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > Isn't the intention "result == ActivityIconLoader::GetResult::SUCCEEDED_SYNC"? GetResult::FAILED_* also run the callback actually. Updated the ActivityIconLoader function comment.
https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task.cc:131: base::Bind(base::IgnoreResult(&ArcProcessTask::StartIconLoading), On 2016/06/02 18:03:19, Yusuke Sato wrote: > On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > > It might be the case that by the time you call StartIconLoading, the > IconLoader > > was destroyed and might be re-loaded at a later point in time. > > > > So how about you make StartIconLoading not return anything and have it add the > > observer to be notified in case it cannot get the IconLoader? > > Done. I'd keep this PostTask though (because directly calling into > StartIconLoading() which may call AddObserver() doesn't seem safe.) Keeping the PostTask is the right way to do it. https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... File components/arc/intent_helper/link_handler_model_impl.cc (right): https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... components/arc/intent_helper/link_handler_model_impl.cc:122: (result != ActivityIconLoader::GetResult::SUCCEEDED_ASYNC); On 2016/06/02 18:03:19, Yusuke Sato wrote: > On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > > Isn't the intention "result == ActivityIconLoader::GetResult::SUCCEEDED_SYNC"? > > GetResult::FAILED_* also run the callback actually. Updated the > ActivityIconLoader function comment. Makes more sense now :) nit: Can you add a small function in ActivityIconLoader that encapsulates this logic and give it a useful name that indicates that the callback has been triggered to avoid future confusion?
PTAL https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... File components/arc/intent_helper/link_handler_model_impl.cc (right): https://codereview.chromium.org/2034543003/diff/60001/components/arc/intent_h... components/arc/intent_helper/link_handler_model_impl.cc:122: (result != ActivityIconLoader::GetResult::SUCCEEDED_ASYNC); On 2016/06/02 20:03:51, Luis Héctor Chávez wrote: > On 2016/06/02 18:03:19, Yusuke Sato wrote: > > On 2016/06/02 16:59:47, Luis Héctor Chávez wrote: > > > Isn't the intention "result == > ActivityIconLoader::GetResult::SUCCEEDED_SYNC"? > > > > GetResult::FAILED_* also run the callback actually. Updated the > > ActivityIconLoader function comment. > > Makes more sense now :) > > nit: Can you add a small function in ActivityIconLoader that encapsulates this > logic and give it a useful name that indicates that the callback has been > triggered to avoid future confusion? Great idea, done.
Much better :) lgtm
yusukes@chromium.org changed reviewers: + afakhry@chromium.org
Ahmed, could you do an OWNER review?
https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:88: arc::ArcBridgeService::Get()->RemoveObserver(this); What happens if you're not observing?
https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:88: arc::ArcBridgeService::Get()->RemoveObserver(this); On 2016/06/02 21:47:12, afakhry wrote: > What happens if you're not observing? Nothing happens. It's implemented with base::ObserverList and its RemoveObserver() simply ignores unknown pointers. I'll update ArcBridgeService's function comment and its unittest in a separate CL.
c/b/task_management lgtm with a couple of nits. https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:52: process_state_(process_state), Nit: I would add here instead: package_name_(!packages.empty() ? packages.at(0) : ""), and then inside the ctor body, call StartIconLoading() anyway. Inside StartIconLoading(), first line should be: if (package_name_.empty()) return; WDYT? https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:74: std::vector<arc::ActivityIconLoader::ActivityName> activity = { Nit: I would call this |activities| (plural) since it's a vector, even though it contains a single element. https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:88: arc::ArcBridgeService::Get()->RemoveObserver(this); On 2016/06/02 21:56:06, Yusuke Sato wrote: > On 2016/06/02 21:47:12, afakhry wrote: > > What happens if you're not observing? > > Nothing happens. It's implemented with base::ObserverList and its > RemoveObserver() simply ignores unknown pointers. > > I'll update ArcBridgeService's function comment and its unittest in a separate > CL. Acknowledged.
Thanks for the review! Fixed both. https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task.cc (right): https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:52: process_state_(process_state), On 2016/06/02 22:23:20, afakhry wrote: > Nit: I would add here instead: > > package_name_(!packages.empty() ? packages.at(0) : ""), > > and then inside the ctor body, call StartIconLoading() anyway. > > Inside StartIconLoading(), first line should be: > if (package_name_.empty()) > return; > > WDYT? Done. https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:74: std::vector<arc::ActivityIconLoader::ActivityName> activity = { On 2016/06/02 22:23:20, afakhry wrote: > Nit: I would call this |activities| (plural) since it's a vector, even though it > contains a single element. Done. https://codereview.chromium.org/2034543003/diff/110001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task.cc:88: arc::ArcBridgeService::Get()->RemoveObserver(this); On 2016/06/02 22:23:20, afakhry wrote: > On 2016/06/02 21:56:06, Yusuke Sato wrote: > > On 2016/06/02 21:47:12, afakhry wrote: > > > What happens if you're not observing? > > > > Nothing happens. It's implemented with base::ObserverList and its > > RemoveObserver() simply ignores unknown pointers. > > > > I'll update ArcBridgeService's function comment and its unittest in a separate > > CL. > > Acknowledged. Done. CL in CQ: https://codereview.chromium.org/2031143002/
The CQ bit was checked by yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2034543003/#ps130001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034543003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2034543003/130001
Message was sent while issue was closed.
Description was changed from ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons ========== to ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons ==========
Message was sent while issue was closed.
Committed patchset #5 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons ========== to ========== Retry icon fetching after intent_helper is ready. * Modify ActivityIconLoader::GetActivityIcon() so it returns an error code. * Modify ArcProcessTask so it calls GetActivityIcon() again when the function returns a temporary error. BUG=616159 TEST=All ARC tasks in Task Manager have their icons Committed: https://crrev.com/b229a826d51cf8d5663184bcade36f3df9ee6014 Cr-Commit-Position: refs/heads/master@{#397581} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b229a826d51cf8d5663184bcade36f3df9ee6014 Cr-Commit-Position: refs/heads/master@{#397581} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
