Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2768083004:
Use OneShotTimer in AddToHomescreenDataFetcher (Closed)
DescriptionUse OneShotTimer in AddToHomescreenDataFetcher
Also stop it when the data is successfully fetched.
BUG=705537
Review-Url: https://codereview.chromium.org/2768083004
Cr-Commit-Position: refs/heads/master@{#460418}
Committed: https://chromium.googlesource.com/chromium/src/+/e939ee93a310041757e12014be35961e368b5588
Patch Set 1 #
Total comments: 2
Patch Set 2 : Stop earlier #
Total comments: 4
Patch Set 3 : Timer addressing comments #
Messages
Total messages: 41 (31 generated)
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zpeng@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, PTAL. Thanks!
If you could add a BUG link that would be great :) https://codereview.chromium.org/2768083004/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2768083004/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:208: data_timeout_timer_.Stop(); Move this to be the first thing in this method (otherwise we may return from this method due to the web_contents being dead, then the timer fires, and we have to do the same thing in OnDataTimedout. May as well stop early).
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Dominick! PTAL https://codereview.chromium.org/2768083004/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2768083004/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:208: data_timeout_timer_.Stop(); On 2017/03/23 22:37:35, dominickn wrote: > Move this to be the first thing in this method (otherwise we may return from > this method due to the web_contents being dead, then the timer fires, and we > have to do the same thing in OnDataTimedout. May as well stop early). Done.
I made this CL after seeing that we use OneShotTImer and stop it in success callback in WebApkInstaller. I think this CL does not change the code's behavior from user's perspective, so I'm not sure whether I should file a bug for this. WDUT?
It's always good to have a bug unless the CL is truly trivial. You don't know if it will need a revert, at which time having a bug is pretty important. :) https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:203: data_timeout_timer_.Stop(); I would do this even before badge_icon_.reset(). Also remove the extra newline at line 202.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:193: if (!is_icon_saved_) { I think that with your change you can assume that |is_icon_saved_| is always false.
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use OneShotTimer in AddToHomescreenDataFetcher Also stop it when the data is successfully fetched. ========== to ========== Use OneShotTimer in AddToHomescreenDataFetcher Also stop it when the data is successfully fetched. BUG=705537 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Dominick & Peter! PTAL https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right): https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:193: if (!is_icon_saved_) { On 2017/03/24 17:38:47, pkotwicz wrote: > I think that with your change you can assume that |is_icon_saved_| is always > false. Done. https://codereview.chromium.org/2768083004/diff/20001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:203: data_timeout_timer_.Stop(); On 2017/03/24 02:42:37, dominickn wrote: > I would do this even before badge_icon_.reset(). Also remove the extra newline > at line 202. Done.
lgtm, thanks
The CQ bit was checked by zpeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490802669467810,
"parent_rev": "481c74e61a605fe6169e5b06372ce75b18384cfb", "commit_rev":
"e939ee93a310041757e12014be35961e368b5588"}
Message was sent while issue was closed.
Description was changed from ========== Use OneShotTimer in AddToHomescreenDataFetcher Also stop it when the data is successfully fetched. BUG=705537 ========== to ========== Use OneShotTimer in AddToHomescreenDataFetcher Also stop it when the data is successfully fetched. BUG=705537 Review-Url: https://codereview.chromium.org/2768083004 Cr-Commit-Position: refs/heads/master@{#460418} Committed: https://chromium.googlesource.com/chromium/src/+/e939ee93a310041757e12014be35... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e939ee93a310041757e12014be35... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
