Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517113005:
[NTP::PhysicalWeb] Wire Physical Web provider to the data source. (Closed)
Description[NTP::PhysicalWeb] Wire Physical Web provider to the data source.
Previously we had a provider, which was not connected to any data
source. This CL connects it to PhysicalWebDataSource,rewrites the
existing test and adds a new one.
BUG=635893
Committed: https://crrev.com/e140976b2f07df1e4e3af2ce6d697b13627937e2
Cr-Commit-Position: refs/heads/master@{#434919}
Patch Set 1 #Patch Set 2 : proper deps. #
Total comments: 34
Patch Set 3 : treib@ comments. #Patch Set 4 : treib@ comment. #
Total comments: 2
Patch Set 5 : rebase and treib@ comment. #
Total comments: 2
Patch Set 6 : rebase. #Patch Set 7 : rebase. #
Depends on Patchset: Messages
Total messages: 61 (48 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource. BUG=635893 ========== to ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource and rewrites the existing test and adds a new one. BUG=635893 ==========
Description was changed from ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource and rewrites the existing test and adds a new one. BUG=635893 ========== to ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource and rewrites the existing test and adds a new one. BUG=635893 ==========
Description was changed from ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource and rewrites the existing test and adds a new one. BUG=635893 ========== to ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource,rewrites the existing test and adds a new one. BUG=635893 ==========
Patchset #1 (id:20001) has been deleted
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, could you have a look?
The CQ bit was checked by vitaliii@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitaliii@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...
Hi Marc, I corrected dependencies, please have a look at the new patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitaliii@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...
https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.cc:11: namespace {} // namespace ? https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.h (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.h:30: std::unique_ptr<base::ListValue> metadata_; nit: #include <memory> https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:21: using base::Value; The "base::" is spelled out for ListValue and DictionaryValue, please be consistent and either do "using" for all or none. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:36: category_status_(CategoryStatus::AVAILABLE_LOADING), The status is immediately set to AVAILABLE in FetchPhysicalWebPages, might as well do that right away and get rid of _LOADING? https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:143: DCHECK(success); Are all the DCHECKs in this method really DCHECKs? Do we want to crash if we get invalid data from physical web? (IMO if there are such strong constraints on the data, then it should be provided in a struct, not in a dictionary.) https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:164: break; nit: braces now :) https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:175: success = page->GetString(kPhysicalWebScannedUrlKey, &scanned_url); Oh wow, they have global constants without a namespace... https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:197: DCHECK(base::IsStringUTF8(resolved_url.host())); Isn't this guaranteed anyway if the GURL parsed without errors? https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:212: void PhysicalWebPageSuggestionsProvider::OnDistanceChanged( nit: empty lines between method implementations https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:67: // |provided_category_|. nit: I wouldn't mention provided_category_; it's not really relevant here. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:69: const base::DictionaryValue* out_value) const; out_value isn't a good name; this is an input. Also presumably it can't be null, so it should be a const& rather than a pointer. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:36: const std::string description, & https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:39: base::MakeUnique<base::DictionaryValue>(); optional nit: auto page = ... (the type is already spelled out in the MakeUnique, no need to do it twice) https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:59: const std::vector<int> ids) { & https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:82: : pref_service_(new TestingPrefServiceSimple()) {} nit: MakeUnique, to make clear what's going on? https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:109: ntp_snippets::KnownCategories::PHYSICAL_WEB_PAGES); nit: "ntp_snippets::" not required
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 vitaliii@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...
Hi Marc, I addressed your comments, could you check? https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.cc:11: namespace {} // namespace On 2016/11/23 09:47:52, Marc Treib wrote: > ? Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.h (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/fake_physical_web_data_source.h:30: std::unique_ptr<base::ListValue> metadata_; On 2016/11/23 09:47:52, Marc Treib wrote: > nit: #include <memory> Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:21: using base::Value; On 2016/11/23 09:47:52, Marc Treib wrote: > The "base::" is spelled out for ListValue and DictionaryValue, please be > consistent and either do "using" for all or none. Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:36: category_status_(CategoryStatus::AVAILABLE_LOADING), On 2016/11/23 09:47:52, Marc Treib wrote: > The status is immediately set to AVAILABLE in FetchPhysicalWebPages, might as > well do that right away and get rid of _LOADING? Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:143: DCHECK(success); On 2016/11/23 09:47:52, Marc Treib wrote: > Are all the DCHECKs in this method really DCHECKs? Do we want to crash if we get > invalid data from physical web? > (IMO if there are such strong constraints on the data, then it should be > provided in a struct, not in a dictionary.) We definitely can live without a title or a description, but I would like to know when this happens. What are the downsides of DCHECKs here? My assumption is that only developer build may crash and before the feature is enabled by default, this means only mine. (IMO a struct is definitely better, but the first chance to express this will be only on Monday and in case I am wrong - crbug.com/668141) https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:164: break; On 2016/11/23 09:47:52, Marc Treib wrote: > nit: braces now :) Done. This might have been written before we decided :) https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:175: success = page->GetString(kPhysicalWebScannedUrlKey, &scanned_url); On 2016/11/23 09:47:52, Marc Treib wrote: > Oh wow, they have global constants without a namespace... Acknowledged. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:197: DCHECK(base::IsStringUTF8(resolved_url.host())); On 2016/11/23 09:47:52, Marc Treib wrote: > Isn't this guaranteed anyway if the GURL parsed without errors? Done. You are right, "... the host, path, etc. will be guaranteed ASCII ...". https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:212: void PhysicalWebPageSuggestionsProvider::OnDistanceChanged( On 2016/11/23 09:47:52, Marc Treib wrote: > nit: empty lines between method implementations Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:67: // |provided_category_|. On 2016/11/23 09:47:52, Marc Treib wrote: > nit: I wouldn't mention provided_category_; it's not really relevant here. Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:69: const base::DictionaryValue* out_value) const; On 2016/11/23 09:47:52, Marc Treib wrote: > out_value isn't a good name; this is an input. > Also presumably it can't be null, so it should be a const& rather than a > pointer. Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:36: const std::string description, On 2016/11/23 09:47:52, Marc Treib wrote: > & Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:39: base::MakeUnique<base::DictionaryValue>(); On 2016/11/23 09:47:52, Marc Treib wrote: > optional nit: auto page = ... > (the type is already spelled out in the MakeUnique, no need to do it twice) Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:59: const std::vector<int> ids) { On 2016/11/23 09:47:52, Marc Treib wrote: > & Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:82: : pref_service_(new TestingPrefServiceSimple()) {} On 2016/11/23 09:47:52, Marc Treib wrote: > nit: MakeUnique, to make clear what's going on? Done. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:109: ntp_snippets::KnownCategories::PHYSICAL_WEB_PAGES); On 2016/11/23 09:47:52, Marc Treib wrote: > nit: "ntp_snippets::" not required Done.
https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:143: DCHECK(success); On 2016/11/23 14:16:52, vitaliii wrote: > On 2016/11/23 09:47:52, Marc Treib wrote: > > Are all the DCHECKs in this method really DCHECKs? Do we want to crash if we > get > > invalid data from physical web? > > (IMO if there are such strong constraints on the data, then it should be > > provided in a struct, not in a dictionary.) > > We definitely can live without a title or a description, but I would like to > know when this happens. > What are the downsides of DCHECKs here? My assumption is that only developer > build may crash and before the feature is enabled by default, this means only > mine. > > (IMO a struct is definitely better, but the first chance to express this will be > only on Monday and in case I am wrong - crbug.com/668141) Per the style guide, DCHECKs are treated as asserts - something that can *never* happen. LOG(DFATAL) might serve your needs? Or even just DLOG(ERROR).
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 vitaliii@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...
Hi Marc, PTAL. https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2517113005/diff/60001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:143: DCHECK(success); On 2016/11/23 14:22:45, Marc Treib wrote: > On 2016/11/23 14:16:52, vitaliii wrote: > > On 2016/11/23 09:47:52, Marc Treib wrote: > > > Are all the DCHECKs in this method really DCHECKs? Do we want to crash if we > > get > > > invalid data from physical web? > > > (IMO if there are such strong constraints on the data, then it should be > > > provided in a struct, not in a dictionary.) > > > > We definitely can live without a title or a description, but I would like to > > know when this happens. > > What are the downsides of DCHECKs here? My assumption is that only developer > > build may crash and before the feature is enabled by default, this means only > > mine. > > > > (IMO a struct is definitely better, but the first chance to express this will > be > > only on Monday and in case I am wrong - crbug.com/668141) > > Per the style guide, DCHECKs are treated as asserts - something that can *never* > happen. > > LOG(DFATAL) might serve your needs? Or even just DLOG(ERROR). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM if you change the remaining DCHECKs. https://codereview.chromium.org/2517113005/diff/100001/components/ntp_snippet... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2517113005/diff/100001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:146: DCHECK(success); There's a bunch more DCHECKs here which shouldn't be DCHECKs.
Addressed your comment. No need to look. https://codereview.chromium.org/2517113005/diff/100001/components/ntp_snippet... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2517113005/diff/100001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:146: DCHECK(success); On 2016/11/23 16:16:43, Marc Treib wrote: > There's a bunch more DCHECKs here which shouldn't be DCHECKs. Done.
The CQ bit was checked by vitaliii@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
olivierrobin@chromium.org changed reviewers: + olivierrobin@chromium.org
Dependency to component/PW lgtm https://codereview.chromium.org/2517113005/diff/120001/components/ntp_snippet... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2517113005/diff/120001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:169: "physical_web_pages/fake_physical_web_data_source.h", Can this be in the physical web component?
The CQ bit was checked by vitaliii@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 #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by vitaliii@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by vitaliii@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 vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2517113005/#ps200001 (title: "rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Replied to a comment, no need to look. https://codereview.chromium.org/2517113005/diff/120001/components/ntp_snippet... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2517113005/diff/120001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:169: "physical_web_pages/fake_physical_web_data_source.h", On 2016/11/24 08:07:36, Olivier Robin wrote: > Can this be in the physical web component? Done in codereview.chromium.org/2529303002.
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480402421469990,
"parent_rev": "367eedfced77eda0014904ed11faef6e6e346165", "commit_rev":
"18e6566c9fef47208d7729f002ec606c6810f901"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource,rewrites the existing test and adds a new one. BUG=635893 ========== to ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource,rewrites the existing test and adds a new one. BUG=635893 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource,rewrites the existing test and adds a new one. BUG=635893 ========== to ========== [NTP::PhysicalWeb] Wire Physical Web provider to the data source. Previously we had a provider, which was not connected to any data source. This CL connects it to PhysicalWebDataSource,rewrites the existing test and adds a new one. BUG=635893 Committed: https://crrev.com/e140976b2f07df1e4e3af2ce6d697b13627937e2 Cr-Commit-Position: refs/heads/master@{#434919} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e140976b2f07df1e4e3af2ce6d697b13627937e2 Cr-Commit-Position: refs/heads/master@{#434919} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
