Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2771793003:
Move timeout timer into WebApkIconHasher (Closed)
DescriptionMove timeout timer into WebApkIconHasher
This CL adds a timer to WebApkIconHasher. Consequently, classes that
invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer need
to maintain a timer dedicated to WebApkIconHasher.
This CL also simplifies the error checking and handling logic.
WebApkIconHasher now checks for URL validity itself, and would return
an empty hash string for any errors encountered.
BUG=649771
Review-Url: https://codereview.chromium.org/2771793003
Cr-Commit-Position: refs/heads/master@{#460508}
Committed: https://chromium.googlesource.com/chromium/src/+/477d57e0cb3d510ac379ed3c525741379cd39193
Patch Set 1 : Move timeout timer into WebApkIconHasher #
Total comments: 16
Patch Set 2 : Hasher addressing comments and adding tests #
Total comments: 6
Patch Set 3 : Remove weakptr and use unretained #
Messages
Total messages: 36 (28 generated)
Description was changed from ========== Move timeout timer into WebApkIconHasher BUG= ========== to ========== Move timeout timer into WebApkIconHasher BUG=649771 ==========
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 #1 (id:1) 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...
zpeng@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, PTAL. Thanks! If you think that this idea is okay, I'm going to add more tests.
https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.cc (right): https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:67: FROM_HERE, base::TimeDelta::FromMilliseconds(60000), Can you put this constant in the anonymous namespace please? https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:82: download_timeout_timer_.Stop(); Stop the timer first. Remove the extra newline between 80 and 82 as well. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:104: Remove the extra newline https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.h (right): https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:22: class GURL; If you are #including GURL, you can remove this. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:36: // Downloads icon_url_. Calls callback_ with the Murmur2 hash of the |icon_url_|, |callback_| https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:38: // encoding/decoding beforehand). callback_ is called with an empty string if |callback_| https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:58: // Fetches the image. This comment isn't needed https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:68: base::WeakPtrFactory<WebApkIconHasher> weak_ptr_factory_; Instead of a weak_ptr_factory_ and is_download_complete_, can't you just do url_fetcher_.reset() in OnDownloadTimedOut() in OnDownloadTimedOut()? That should guarantee that if there is a timeout, the task is cancelled (never calls OnURLFetchComplete), and when the fetch completes, the timeout timer is stopped immediately (OnDownloadTimedOut is never called).
Also, a slightly longer description never goes astray - the commit log is an important source of information and you should always try and write at least a brief paragraph describing the rationale / details of your change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Move timeout timer into WebApkIconHasher BUG=649771 ========== to ========== Move timeout timer into WebApkIconHasher This CL adds a timer to WebApkIconHasher. Consequently, classes that invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer needs to maintain a timer dedicated to WebApkIconHasher. This CL also simplifies the error checking and handling logic. WebApkIconHasher now checks for URL validity itself, and would return an empty hash string for any errors encountered. BUG=649771 ==========
Description was changed from ========== Move timeout timer into WebApkIconHasher This CL adds a timer to WebApkIconHasher. Consequently, classes that invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer needs to maintain a timer dedicated to WebApkIconHasher. This CL also simplifies the error checking and handling logic. WebApkIconHasher now checks for URL validity itself, and would return an empty hash string for any errors encountered. BUG=649771 ========== to ========== Move timeout timer into WebApkIconHasher This CL adds a timer to WebApkIconHasher. Consequently, classes that invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer need to maintain a timer dedicated to WebApkIconHasher. This CL also simplifies the error checking and handling logic. WebApkIconHasher now checks for URL validity itself, and would return an empty hash string for any errors encountered. BUG=649771 ==========
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.
Thanks Dominick! PTAL I've added the CL description and additional tests. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.cc (right): https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:67: FROM_HERE, base::TimeDelta::FromMilliseconds(60000), On 2017/03/24 02:41:00, dominickn wrote: > Can you put this constant in the anonymous namespace please? Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:82: download_timeout_timer_.Stop(); On 2017/03/24 02:41:00, dominickn wrote: > Stop the timer first. Remove the extra newline between 80 and 82 as well. Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:104: On 2017/03/24 02:41:00, dominickn wrote: > Remove the extra newline Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.h (right): https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:22: class GURL; On 2017/03/24 02:41:01, dominickn wrote: > If you are #including GURL, you can remove this. Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:36: // Downloads icon_url_. Calls callback_ with the Murmur2 hash of the On 2017/03/24 02:41:01, dominickn wrote: > |icon_url_|, |callback_| Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:38: // encoding/decoding beforehand). callback_ is called with an empty string if On 2017/03/24 02:41:00, dominickn wrote: > |callback_| Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:58: // Fetches the image. On 2017/03/24 02:41:01, dominickn wrote: > This comment isn't needed Done. https://codereview.chromium.org/2771793003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:68: base::WeakPtrFactory<WebApkIconHasher> weak_ptr_factory_; On 2017/03/24 02:41:01, dominickn wrote: > Instead of a weak_ptr_factory_ and is_download_complete_, can't you just do > url_fetcher_.reset() in OnDownloadTimedOut() in OnDownloadTimedOut()? That > should guarantee that if there is a timeout, the task is cancelled (never calls > OnURLFetchComplete), and when the fetch completes, the timeout timer is stopped > immediately (OnDownloadTimedOut is never called). Done.
lgtm % nits https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.cc (right): https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:38: net::URLRequestContextGetter* url_request_context_getter, It looks like everywhere this object is used, it's immediately created and then DownloadAndComputeMurmur2Hash() is called. A suggestion for future refactoring (either here or in a follow up): * a static method WebApkIconHasher::DownloadAndComputeMurmur2Hash(context_getter, url, callback) * which new-allocates a WebApkIconHasher (which now has a private constructor) * which creates and starts the url_fetcher_ in its constructor * and calls delete this in either OnURLFetchComplete or OnDownloadTimedOut once callback is posted This is the same pattern used in WebApkInstaller, so it means that the object entirely manages its own lifetime. Not blocking this CL on this suggestion though. https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.h (right): https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:47: // For retrieving URLRequestContext. Nit: add "Owned by the caller of this class". https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:62: base::WeakPtrFactory<WebApkIconHasher> weak_ptr_factory_; Remove the unneeded WeakPtrFactory
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: 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: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
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2771793003/#ps80001 (title: "Remove weakptr and use unretained")
Thanks Dominick! I'm going to submit it soon. https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.cc (right): https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.cc:38: net::URLRequestContextGetter* url_request_context_getter, On 2017/03/27 23:27:33, dominickn wrote: > It looks like everywhere this object is used, it's immediately created and then > DownloadAndComputeMurmur2Hash() is called. > > A suggestion for future refactoring (either here or in a follow up): > > * a static method > WebApkIconHasher::DownloadAndComputeMurmur2Hash(context_getter, url, callback) > * which new-allocates a WebApkIconHasher (which now has a private constructor) > * which creates and starts the url_fetcher_ in its constructor > * and calls delete this in either OnURLFetchComplete or OnDownloadTimedOut once > callback is posted > > This is the same pattern used in WebApkInstaller, so it means that the object > entirely manages its own lifetime. > > Not blocking this CL on this suggestion though. Thanks for the suggestion! I'll save it for future refactoring. https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_icon_hasher.h (right): https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:47: // For retrieving URLRequestContext. On 2017/03/27 23:27:34, dominickn wrote: > Nit: add "Owned by the caller of this class". Done. https://codereview.chromium.org/2771793003/diff/40001/chrome/browser/android/... chrome/browser/android/webapk/webapk_icon_hasher.h:62: base::WeakPtrFactory<WebApkIconHasher> weak_ptr_factory_; On 2017/03/27 23:27:34, dominickn wrote: > Remove the unneeded WeakPtrFactory Done.
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": 80001, "attempt_start_ts": 1490817614231300,
"parent_rev": "306eec3b648d47328aba78b0c80bf1d59f72f42e", "commit_rev":
"477d57e0cb3d510ac379ed3c525741379cd39193"}
Message was sent while issue was closed.
Description was changed from ========== Move timeout timer into WebApkIconHasher This CL adds a timer to WebApkIconHasher. Consequently, classes that invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer need to maintain a timer dedicated to WebApkIconHasher. This CL also simplifies the error checking and handling logic. WebApkIconHasher now checks for URL validity itself, and would return an empty hash string for any errors encountered. BUG=649771 ========== to ========== Move timeout timer into WebApkIconHasher This CL adds a timer to WebApkIconHasher. Consequently, classes that invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer need to maintain a timer dedicated to WebApkIconHasher. This CL also simplifies the error checking and handling logic. WebApkIconHasher now checks for URL validity itself, and would return an empty hash string for any errors encountered. BUG=649771 Review-Url: https://codereview.chromium.org/2771793003 Cr-Commit-Position: refs/heads/master@{#460508} Committed: https://chromium.googlesource.com/chromium/src/+/477d57e0cb3d510ac379ed3c5257... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/477d57e0cb3d510ac379ed3c5257... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
