Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703003002:
Set null testing Favicon factories on iOS too (Closed)
|
Created:
3 years, 10 months ago by mastiz Modified:
3 years, 10 months ago Reviewers:
noyau (Ping after 24h) CC:
chromium-reviews, marq+watch_chromium.org, noyau+watch_chromium.org, pkl (ping after 24h if needed), browser-components-watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet null testing Favicon factories on iOS too
This makes the iOS tests consistent with the rest.
It also declares a missing dependency which for some reason
caused no issues so far.
BUG=694312
Review-Url: https://codereview.chromium.org/2703003002
Cr-Commit-Position: refs/heads/master@{#451741}
Committed: https://chromium.googlesource.com/chromium/src/+/06b09f98b83b06cdc7879af14022c54db54f4d61
Patch Set 1 #Patch Set 2 : Traces. #Patch Set 3 : Traces. #Patch Set 4 : Same for largeicon factory. #Patch Set 5 : Same for largeicon factory. #Patch Set 6 : Same for largeicon factory. #Patch Set 7 : Revert traces. #
Total comments: 2
Messages
Total messages: 33 (27 generated)
The CQ bit was checked by mastiz@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mastiz@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mastiz@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mastiz@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== TEMPO: Set null testing factory for FaviconService. BUG= ========== to ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. BUG= ==========
Description was changed from ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. BUG= ========== to ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. It also declares a missing dependency which for some reason caused no issues so far. BUG= ==========
mastiz@chromium.org changed reviewers: + noyau@chromium.org
noyau@: this got on my way to submit https://codereview.chromium.org/2698473004/
The CQ bit was checked by mastiz@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 ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. It also declares a missing dependency which for some reason caused no issues so far. BUG= ========== to ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. It also declares a missing dependency which for some reason caused no issues so far. BUG=694312 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2703003002/diff/120001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/favicon_service_factory.h (right): https://codereview.chromium.org/2703003002/diff/120001/ios/chrome/browser/fav... ios/chrome/browser/favicon/favicon_service_factory.h:46: bool ServiceIsNULLWhileTesting() const override; I hate things added to public APIs to use only for tests...
Thanks!! https://codereview.chromium.org/2703003002/diff/120001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/favicon_service_factory.h (right): https://codereview.chromium.org/2703003002/diff/120001/ios/chrome/browser/fav... ios/chrome/browser/favicon/favicon_service_factory.h:46: bool ServiceIsNULLWhileTesting() const override; On 2017/02/21 12:28:16, noyau wrote: > I hate things added to public APIs to use only for tests... Agreed :-/
On 2017/02/21 12:37:10, mastiz wrote: > Thanks!! > > https://codereview.chromium.org/2703003002/diff/120001/ios/chrome/browser/fav... > File ios/chrome/browser/favicon/favicon_service_factory.h (right): > > https://codereview.chromium.org/2703003002/diff/120001/ios/chrome/browser/fav... > ios/chrome/browser/favicon/favicon_service_factory.h:46: bool > ServiceIsNULLWhileTesting() const override; > On 2017/02/21 12:28:16, noyau wrote: > > I hate things added to public APIs to use only for tests... > > Agreed :-/ The issue is that KeyedService factories are more or less singleton. As such they requires a way to be configured differently for testing (though I'd like to get to a point where none of the unittests uses the KeyedService factories and instead create all the fake/test version of the services they depend upon and inject them directly in the tested services, at which point we could remove those methods).
The CQ bit was checked by mastiz@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": 120001, "attempt_start_ts": 1487680935708600,
"parent_rev": "621821c9c2eeb41418191826528871b109ec5908", "commit_rev":
"06b09f98b83b06cdc7879af14022c54db54f4d61"}
Message was sent while issue was closed.
Description was changed from ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. It also declares a missing dependency which for some reason caused no issues so far. BUG=694312 ========== to ========== Set null testing Favicon factories on iOS too This makes the iOS tests consistent with the rest. It also declares a missing dependency which for some reason caused no issues so far. BUG=694312 Review-Url: https://codereview.chromium.org/2703003002 Cr-Commit-Position: refs/heads/master@{#451741} Committed: https://chromium.googlesource.com/chromium/src/+/06b09f98b83b06cdc7879af14022... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/06b09f98b83b06cdc7879af14022... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
