Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583853004:
[chrome.dial] Adds chrome.dial.fetchDeviceDecription API. (Closed)
|
Created:
4 years ago by mark a. foltz Modified:
3 years, 11 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds chrome.dial.fetchDeviceDecription API.
This API fetches the device description for a DIAL device, given a label for a device already discovered.
The request is configured to not send/receive cookies, ignore proxies, and ignore redirects.
Requests are retried with backoff on 5XX responses.
The response is validated in multiple ways:
- Valid Application-URL: header value
- Response body is a valid UTF-8 string
- Response body is <= 256kb
Additional validations are possible by following the DIAL spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details.
Note: The API function implementation is complicated by thread hopping. See comments in dial_api.h for how this may be improved in the future.
BUG=671932
TESTED=Manually, unit tests, API test
Review-Url: https://codereview.chromium.org/2583853004
Cr-Commit-Position: refs/heads/master@{#442713}
Committed: https://chromium.googlesource.com/chromium/src/+/90824416d3eeae5ec6013b250123df65e9d48032
Patch Set 1 #Patch Set 2 : More hacking. #Patch Set 3 : Finish implementation #Patch Set 4 : Add tests. #Patch Set 5 : Fix initialization #
Total comments: 8
Patch Set 6 : Respond to imcheng@ comments #
Total comments: 4
Patch Set 7 : Respond to lazyboy@ comments. Rebase #Patch Set 8 : Respond to lazyboy@ comments. Rebase w/namespace changes #
Total comments: 77
Patch Set 9 : Respond to wez@ comments #Patch Set 10 : Clean up some unneeded code in unittest. #
Total comments: 2
Patch Set 11 : Reply to wez@ #Patch Set 12 : Rebase and fix typo. #Messages
Total messages: 39 (21 generated)
Description was changed from ========== Adds chrome.dial.fetchDeviceDecription API. BUG=671932 ========== to ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device. TODO: Write tests BUG=671932 TESTED=Manually ==========
mfoltz@chromium.org changed reviewers: + imcheng@chromium.org
PTAL at the overall design/implementation. I tested it manually with a Cast device and a fake DIAL server. If there are no major issues, I'll complete tests.
Description was changed from ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device. TODO: Write tests BUG=671932 TESTED=Manually ========== to ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured not to send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the uPnP spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.cc for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ==========
Description was changed from ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured not to send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the uPnP spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.cc for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ========== to ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the uPnP spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.cc for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ==========
Tests are done. PTAL.
Description was changed from ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the uPnP spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.cc for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ========== to ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the DIAL spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.cc for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ==========
The CQ bit was checked by mfoltz@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 ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the DIAL spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.cc for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ========== to ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the DIAL spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.h for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Nice patch and thanks for the tests! https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/device_description_fetcher.h (right): https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/dial/device_description_fetcher.h:28: // with an error reason. This class is not thread safe. Also document on which thread the class is run on? (IO thread?) https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/dial/device_description_fetcher.h:59: Profile* profile_; Profile* const https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:211: BrowserThread::UI, FROM_HERE, Hmm.. I think the fetch should happen in IO thread. What's the reason for it to be on UI thread? https://codereview.chromium.org/2583853004/diff/80001/chrome/common/extension... File chrome/common/extensions/api/dial.idl (right): https://codereview.chromium.org/2583853004/diff/80001/chrome/common/extension... chrome/common/extensions/api/dial.idl:69: // the message will begin "HTTP XXX:" where XXX is the HTTP result code. ... begin with
mfoltz@chromium.org changed reviewers: + isherman@chromium.org, lazyboy@chromium.org
+isherman for extension_function_histogram_value.h +lazyboy for dial.idl PTAL https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/device_description_fetcher.h (right): https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/dial/device_description_fetcher.h:28: // with an error reason. This class is not thread safe. On 2016/12/28 at 05:36:45, imcheng_ooo_until_1_13 wrote: > Also document on which thread the class is run on? (IO thread?) Done. Also use DCHECK_CURRENTLY_ON to enforce that it's accessed only from UI thread. https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/dial/device_description_fetcher.h:59: Profile* profile_; On 2016/12/28 at 05:36:45, imcheng_ooo_until_1_13 wrote: > Profile* const Done. https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2583853004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:211: BrowserThread::UI, FROM_HERE, On 2016/12/28 at 05:36:45, imcheng_ooo_until_1_13 wrote: > Hmm.. I think the fetch should happen in IO thread. What's the reason for it to be on UI thread? The URLFetcher takes a RequestContext generated by the user's Profile (which must be done on the UI thread), and posts a task to the IO thread on the caller's behalf. https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=... https://codereview.chromium.org/2583853004/diff/80001/chrome/common/extension... File chrome/common/extensions/api/dial.idl (right): https://codereview.chromium.org/2583853004/diff/80001/chrome/common/extension... chrome/common/extensions/api/dial.idl:69: // the message will begin "HTTP XXX:" where XXX is the HTTP result code. On 2016/12/28 at 05:36:45, imcheng_ooo_until_1_13 wrote: > ... begin with Done.
Couple of comments/nits in test related parts, but dial.idl lgtm. https://codereview.chromium.org/2583853004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2583853004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.cc:59: if (!test_device_data_.device_id().empty()) { Can we make |test_device_data_| and |test_device_description_| unique_ptrs so we do not default construct them for non-tests? https://codereview.chromium.org/2583853004/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js (right): https://codereview.chromium.org/2583853004/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js:10: chrome.test.assertTrue(typeof(result) == 'object'); optional nit: use assertEq for these.
mfoltz@chromium.org changed reviewers: + wez@chromium.org
+wez Can you please review as Derek is out for a while? https://codereview.chromium.org/2583853004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2583853004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.cc:59: if (!test_device_data_.device_id().empty()) { On 2017/01/04 at 22:13:13, lazyboy wrote: > Can we make |test_device_data_| and |test_device_description_| unique_ptrs so we do not default construct them for non-tests? Done. https://codereview.chromium.org/2583853004/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js (right): https://codereview.chromium.org/2583853004/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js:10: chrome.test.assertTrue(typeof(result) == 'object'); On 2017/01/04 at 22:13:13, lazyboy wrote: > optional nit: use assertEq for these. Done.
TBD: Review dial description fetcher unit-tests. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:21: constexpr int kMaxDescriptionSizeBytes = 262144; nit: For max-size constants it is helpful if you can summarize a rationale based on what the field might contain, e.g. in this case it can presumably be large if the device has a lot of functions? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:47: DCHECK(!fetcher_); nit: I usually recommend a blank line between DCHECK'd pre-conditions and actual code. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:49: net::URLFetcher::GET, this); nit: Consider introducing this with e.g. "// uPnP returns device descriptions via GET request." https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:52: // data. Proxies almost certainly hurt more cases than they help. nit: It might be more readable to create a local int and |= each set of flags into it here, e.g: int flags = 0; // Never use cookies in these requests. flags |= ... which would make it easier to provide helpful comments for each flag. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:64: // actually need redirects. Presumably we would only do that if we had the ability to intercept the redirect and verify the constraint that it is to the same device? Note that Section 5.4 of the current spec says that devices SHALL NOT redirect this request, though. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:76: const net::URLFetcher* source) { nit: DCHECK_EQ(fetcher_.get(), source); https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:88: // other devices) do not comply, so not checking this header specifcially. typo: specifically Also, reword: so specifically do not check that header. Is there much chance that we'd ever want to enable that check, since it seems unlikely that device behaviours will change much? Is it the case that devices actually set the field, but with an incorrect value, or do the misbehaving devices simply omit it? Have we observed devices that we definitely want to support (e.g. smart TVs?) that set bogus Content-Type? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:99: // have path, query or fragment...unsure if that can be enforced. There seem to be a number of issues of this sort - could the DeviceDescriptionFetcher perhaps check for these conditions but report them to the caller (e.g. via getters) rather than enforcing? That would allow calling code to report the correllation of usable-DIAL-device with non-compliance so we can evaluate whether to enforce. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:102: !app_url.HostIsIPAddress() || Strictly the spec says that the |host| must either resolve to an IPv4 address, or be one; what you're enforcing here is that it is either an IPv4 or IPv6 address, instead. I'd suggest simply omitting this check and adding a note, since you already check that the App URL host is the same as the device description URL host - so any restrictions on the latter implicitly apply to the former. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:127: .Run(DialDeviceDescriptionData(device_description, app_url)); nit: |device_description| can be up to 256KB in size - std::move() it here? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:142: std::move(error_cb_).Run(message); I'm not familiar with the new OnceCallback semantics. Is std::move() now preferred over base::ReturnAndClear(&cb)? Will std::move() clear the Callback such that a second Run() would DCHECK? (I think it will; just confirming :) https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:27: // An instance of this class is used to make a single HTTP GET request with nit: No need for "An instance of this class" - can start with "Used to..." https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:28: // |device_description_url| to fetch a uPnP device description. If successful, nit: Is this description retrieval specific to DIAL, or actually uPnP-generic? If the latter then perhaps re-locate/re-name this class accordingly? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:31: // UI thread. nit: Re thread-safety, must this class be constructed, Start()ed AND destroyed from the UI thread? Is it the case that the caller can tear down the class to cancel the request if it hasn't yet completed? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:35: static constexpr int kURLFetcherID = 1; nit: This is only public for testing, and the purpose of the |id| is really to allow tests to disambiguate multiple requests. If we replace the ctor+Start() layout with a single Create() function that creates & returns the fetcher then we could instead have a CreateForTest(id, ...), which would also allow tests to create multiple instances, if necessary (though I see we don't have use for that now). Otherwise, could we at least make this a kURLFetcherIdForTest, or a GetUrlFetcherIdForTest() method (in the latter case the Id can still be hard-wired internally; we'd just be decoupling the test from caring about the constant). https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:39: Profile* profile, Is the caller responsible for ensuring that the instance is torn down before |profile|? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:45: void Start(); nit: Do we need a separate Start() API, or could we start the request directly in the constructor? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:48: // net::URLFetcherDelegate nit: "|class| implementation." or "|class| interface." is more common. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:58: // Runs |error_cb_| with |message|. nit: Not obvious from the comment, but it Run()s and clears |error_cb_|, right? Do we really want to use a string error message here? Could we instead use net::Error codes and HTTP response values to indicate errors? Since HTTP responses are >0 and net::Error <0 you could even fold the two into a single integer error value. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.h:58: // Sets test device data. For tests only. nit: Suggest removing the "For tests only", since that is implicit from context & naming :) https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.h:93: test_device_description_; Rather than have these fields and having to friend the FetchDescriptionFunction, have you considered providing a SetDialRegistryForTest() on DialAPI? Though perhaps that would mean a load more hoop-jumping to feed in the device description for the test, since you'd need to intercept URLRequests to the test device to return the test description data? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_apitest.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_apitest.cc:124: ASSERT_TRUE(api.get()); nit: Omit .get() when testing a ptr as bool. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_apitest.cc:136: "fetch_device_description.html")); nit: Do we have any test cases for the API failing? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_device_data.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_device_data.h:96: public: This should be a struct, not a class. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_device_data.h:101: ~DialDeviceDescriptionData() = default; nit: Do you need to explicitly specify this? https://codereview.chromium.org/2583853004/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js:17: fetchDeviceDescriptionCallback); Shall we add a test case for API failure?
https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:30: void TearDown() override { EXPECT_TRUE(callback_was_run_); } Rather than maintain a separate boolean, would it be adequate to test that both the cb_ members are invalid/null, since they are OnceCallbacks, so get cleared when Run()? https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:66: const DialDeviceDescriptionData& description) { I'd suggest renaming these to OnSuccess/SuccessCallback/HandleSuccess or similar - the name AssertFoo implies that the method verifies that condition Foo is the case, whereas these are actually called when Foo is the case, to verify that that was what was expected (i.e. the other way around). https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:67: EXPECT_TRUE(expect_success_); Will this expectation ever fire? If we ExpectFailure then we only set an error_cb_, so presumably the success callback is null/invalid in that case, and will crash the test if invoked? You could instead always set both, and provided ExpectedSuccess and UnexpectedSuccess, etc, handlers here. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:79: }; nit: DISALLOW_COPY_AND_ASSIGN, or ~Foo() etc = delete https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:94: TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnHttp404) { nit: The name matches the expectation failure; can we rename or add a comment to explain what situation this tests? e.g. is this the expectation for the case of being unable to connect to the device?
histograms.xml lgtm
https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:21: constexpr int kMaxDescriptionSizeBytes = 262144; On 2017/01/05 at 22:29:24, Wez wrote: > nit: For max-size constants it is helpful if you can summarize a rationale based on what the field might contain, e.g. in this case it can presumably be large if the device has a lot of functions? Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:47: DCHECK(!fetcher_); On 2017/01/05 at 22:29:24, Wez wrote: > nit: I usually recommend a blank line between DCHECK'd pre-conditions and actual code. Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:49: net::URLFetcher::GET, this); On 2017/01/05 at 22:29:24, Wez wrote: > nit: Consider introducing this with e.g. "// uPnP returns device descriptions via GET request." Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:52: // data. Proxies almost certainly hurt more cases than they help. On 2017/01/05 at 22:29:24, Wez wrote: > nit: It might be more readable to create a local int and |= each set of flags into it here, e.g: > > int flags = 0; > > // Never use cookies in these requests. > flags |= ... > > which would make it easier to provide helpful comments for each flag. Reformatted comment to be more explicit about flags. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:64: // actually need redirects. On 2017/01/05 at 22:29:24, Wez wrote: > Presumably we would only do that if we had the ability to intercept the redirect and verify the constraint that it is to the same device? > > Note that Section 5.4 of the current spec says that devices SHALL NOT redirect this request, though. Good catch, I was looking at the uPnP spec which does allow certain redirects. Updated comment to state we'll never allow them. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:76: const net::URLFetcher* source) { On 2017/01/05 at 22:29:24, Wez wrote: > nit: DCHECK_EQ(fetcher_.get(), source); This code doesn't refer to fetcher_. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:88: // other devices) do not comply, so not checking this header specifcially. On 2017/01/05 at 22:29:24, Wez wrote: > typo: specifically > > Also, reword: so specifically do not check that header. > > Is there much chance that we'd ever want to enable that check, since it seems unlikely that device behaviours will change much? I know Cast devices don't comply; presumably they could get an update to fix this. If all other devices do comply, then we could turn it on. > Is it the case that devices actually set the field, but with an incorrect value, or do the misbehaving devices simply omit it? So far I've observed Cast devices setting the wrong value. > Have we observed devices that we definitely want to support (e.g. smart TVs?) that set bogus Content-Type? I can check on our lab network to see what they do. We'd probably have to write an UMA to see how many non-compliant devices there are before adding the check. The check doesn't add a ton of value anyway, since XML is the only possible content that can be served for this request. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:99: // have path, query or fragment...unsure if that can be enforced. On 2017/01/05 at 22:29:24, Wez wrote: > There seem to be a number of issues of this sort - could the DeviceDescriptionFetcher perhaps check for these conditions but report them to the caller (e.g. via getters) rather than enforcing? That would allow calling code to report the correllation of usable-DIAL-device with non-compliance so we can evaluate whether to enforce. I would probably start with UMA to determine prevalence before investing in a complete reporting API. In the meantime we might be able to determine from user feedback if there are devices that are non-compliant and non-working (and thus should be flagged as such). https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:102: !app_url.HostIsIPAddress() || On 2017/01/05 at 22:29:24, Wez wrote: > Strictly the spec says that the |host| must either resolve to an IPv4 address, or be one; what you're enforcing here is that it is either an IPv4 or IPv6 address, instead. Yes. We don't want domain name resolution here. > I'd suggest simply omitting this check and adding a note, since you already check that the App URL host is the same as the device description URL host - so any restrictions on the latter implicitly apply to the former. I don't think having independent checks is a bad thing. Also, we don't have restrictions on the device description host in this API, yet. Filed crbug.com/679432 to track this work. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:127: .Run(DialDeviceDescriptionData(device_description, app_url)); On 2017/01/05 at 22:29:24, Wez wrote: > nit: |device_description| can be up to 256KB in size - std::move() it here? Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:142: std::move(error_cb_).Run(message); On 2017/01/05 at 22:29:24, Wez wrote: > I'm not familiar with the new OnceCallback semantics. > > Is std::move() now preferred over base::ReturnAndClear(&cb)? > > Will std::move() clear the Callback such that a second Run() would DCHECK? (I think it will; just confirming :) That's what the following says: https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:27: // An instance of this class is used to make a single HTTP GET request with On 2017/01/05 at 22:29:24, Wez wrote: > nit: No need for "An instance of this class" - can start with "Used to..." Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:28: // |device_description_url| to fetch a uPnP device description. If successful, On 2017/01/05 at 22:29:24, Wez wrote: > nit: Is this description retrieval specific to DIAL, or actually uPnP-generic? If the latter then perhaps re-locate/re-name this class accordingly? It has DIAL-specific logic like retrieving the app URL. Clarified comment. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:31: // UI thread. On 2017/01/05 at 22:29:24, Wez wrote: > nit: Re thread-safety, must this class be constructed, Start()ed AND destroyed from the UI thread? s/run/used/ > Is it the case that the caller can tear down the class to cancel the request if it hasn't yet completed? Yes, destroying |fetcher_| cancels any pending request. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:35: static constexpr int kURLFetcherID = 1; On 2017/01/05 at 22:29:24, Wez wrote: > nit: This is only public for testing, and the purpose of the |id| is really to allow tests to disambiguate multiple requests. > > If we replace the ctor+Start() layout with a single Create() function that creates & returns the fetcher then we could instead have a CreateForTest(id, ...), which would also allow tests to create multiple instances, if necessary (though I see we don't have use for that now). > > Otherwise, could we at least make this a kURLFetcherIdForTest, or a GetUrlFetcherIdForTest() method (in the latter case the Id can still be hard-wired internally; we'd just be decoupling the test from caring about the constant). I don't need multiple request IDs to test this class. Renamed to kURLFetcherIdForTest to clarify its use. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:39: Profile* profile, On 2017/01/05 at 22:29:24, Wez wrote: > Is the caller responsible for ensuring that the instance is torn down before |profile|? The DIAL API is a BrowserContext-keyed service, its extension function objects (and thus this object) will be destroyed before the Profile. Strictly speaking, the fetcher doesn't need to hold a Profile*, it's only used to start the request. If you feel strongly I can refactor to remove the Profile* class member. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:45: void Start(); On 2017/01/05 at 22:29:24, Wez wrote: > nit: Do we need a separate Start() API, or could we start the request directly in the constructor? Generally it's not a good idea to write constructors with heavyweight side effects, unless it's an explicitly designed to be a ScopedFoo / RAII type object. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:48: // net::URLFetcherDelegate On 2017/01/05 at 22:29:24, Wez wrote: > nit: "|class| implementation." or "|class| interface." is more common. Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:58: // Runs |error_cb_| with |message|. On 2017/01/05 at 22:29:24, Wez wrote: > nit: Not obvious from the comment, but it Run()s and clears |error_cb_|, right? Updated comment, but as error_cb is a OnceCallback there's no need to clear. > Do we really want to use a string error message here? Could we instead use net::Error codes and HTTP response values to indicate errors? Since HTTP responses are >0 and net::Error <0 you could even fold the two into a single integer error value. The URLFetcher doesn't expose net::Error codes. There are a variety of error conditions and the chrome.runtime.lastError API only allows a string. If there's some future code that would consume error codes, they would be straightforward to add. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:66: const DialDeviceDescriptionData& description) { On 2017/01/06 at 01:24:03, Wez wrote: > I'd suggest renaming these to OnSuccess/SuccessCallback/HandleSuccess or similar - the name AssertFoo implies that the method verifies that condition Foo is the case, whereas these are actually called when Foo is the case, to verify that that was what was expected (i.e. the other way around). Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:67: EXPECT_TRUE(expect_success_); On 2017/01/06 at 01:24:03, Wez wrote: > Will this expectation ever fire? If we ExpectFailure then we only set an error_cb_, so presumably the success callback is null/invalid in that case, and will crash the test if invoked? > > You could instead always set both, and provided ExpectedSuccess and UnexpectedSuccess, etc, handlers here. Removed expectations. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:79: }; On 2017/01/06 at 01:24:03, Wez wrote: > nit: DISALLOW_COPY_AND_ASSIGN, or ~Foo() etc = delete Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher_unittest.cc:94: TEST_F(DeviceDescriptionFetcherTest, TestFetchFailsOnHttp404) { On 2017/01/06 at 01:24:03, Wez wrote: > nit: The name matches the expectation failure; can we rename or add a comment to explain what situation this tests? e.g. is this the expectation for the case of being unable to connect to the device? Renamed. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.h:58: // Sets test device data. For tests only. On 2017/01/05 at 22:29:24, Wez wrote: > nit: Suggest removing the "For tests only", since that is implicit from context & naming :) Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.h:93: test_device_description_; On 2017/01/05 at 22:29:24, Wez wrote: > Rather than have these fields and having to friend the FetchDescriptionFunction, have you considered providing a SetDialRegistryForTest() on DialAPI? Though perhaps that would mean a load more hoop-jumping to feed in the device description for the test, since you'd need to intercept URLRequests to the test device to return the test description data? I like that idea. I'd rather take it on as part of fixing the threading of DialRegistry and cleaning up that class in general. Updated TODO in dial_api.cc. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_apitest.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_apitest.cc:124: ASSERT_TRUE(api.get()); On 2017/01/05 at 22:29:24, Wez wrote: > nit: Omit .get() when testing a ptr as bool. Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_apitest.cc:136: "fetch_device_description.html")); On 2017/01/05 at 22:29:24, Wez wrote: > nit: Do we have any test cases for the API failing? Added one. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_device_data.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_device_data.h:96: public: On 2017/01/05 at 22:29:24, Wez wrote: > This should be a struct, not a class. Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_device_data.h:101: ~DialDeviceDescriptionData() = default; On 2017/01/05 at 22:29:25, Wez wrote: > nit: Do you need to explicitly specify this? Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/dial/experimental/fetch_device_description.js:17: fetchDeviceDescriptionCallback); On 2017/01/05 at 22:29:25, Wez wrote: > Shall we add a test case for API failure? Done.
LGTM although note that I wasn't able to review histograms.xml because codereview barfed due to its size. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:64: // actually need redirects. On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > Presumably we would only do that if we had the ability to intercept the > redirect and verify the constraint that it is to the same device? > > > > Note that Section 5.4 of the current spec says that devices SHALL NOT redirect > this request, though. > > Good catch, I was looking at the uPnP spec which does allow certain redirects. > Updated comment to state we'll never allow them. Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:76: const net::URLFetcher* source) { On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > nit: DCHECK_EQ(fetcher_.get(), source); > > This code doesn't refer to fetcher_. Doesn't this method expect that |source| is the same URLFetcher that was created in Start(), though? I suppose you can argue that it doesn't matter to this code whether that is the case, or not, so seems legit not to bother checking if you prefer not to. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:88: // other devices) do not comply, so not checking this header specifcially. On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > typo: specifically > > > > Also, reword: so specifically do not check that header. > > > > Is there much chance that we'd ever want to enable that check, since it seems > unlikely that device behaviours will change much? > > I know Cast devices don't comply; presumably they could get an update to fix > this. If all other devices do comply, then we could turn it on. > > > Is it the case that devices actually set the field, but with an incorrect > value, or do the misbehaving devices simply omit it? > > So far I've observed Cast devices setting the wrong value. > > > Have we observed devices that we definitely want to support (e.g. smart TVs?) > that set bogus Content-Type? > > I can check on our lab network to see what they do. We'd probably have to > write an UMA to see how many non-compliant devices there are before adding the > check. The check doesn't add a ton of value anyway, since XML is the only > possible content that can be served for this request. Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:99: // have path, query or fragment...unsure if that can be enforced. On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > There seem to be a number of issues of this sort - could the > DeviceDescriptionFetcher perhaps check for these conditions but report them to > the caller (e.g. via getters) rather than enforcing? That would allow calling > code to report the correllation of usable-DIAL-device with non-compliance so we > can evaluate whether to enforce. > > I would probably start with UMA to determine prevalence before investing in a > complete reporting API. In the meantime we might be able to determine from user > feedback if there are devices that are non-compliant and non-working (and thus > should be flagged as such). Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:102: !app_url.HostIsIPAddress() || On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > Strictly the spec says that the |host| must either resolve to an IPv4 address, > or be one; what you're enforcing here is that it is either an IPv4 or IPv6 > address, instead. > > Yes. We don't want domain name resolution here. The DIAL spec appears to allow for DNS names in app URLs, though, though admittedly it's not clear how you'd actually use those (maybe they have in mind mDNS..?) > > I'd suggest simply omitting this check and adding a note, since you already > check that the App URL host is the same as the device description URL host - so > any restrictions on the latter implicitly apply to the former. > > I don't think having independent checks is a bad thing. > > Also, we don't have restrictions on the device description host in this API, > yet. Filed crbug.com/679432 to track this work. Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:142: std::move(error_cb_).Run(message); On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > I'm not familiar with the new OnceCallback semantics. > > > > Is std::move() now preferred over base::ReturnAndClear(&cb)? > > > > Will std::move() clear the Callback such that a second Run() would DCHECK? (I > think it will; just confirming :) > > That's what the following says: > > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:28: // |device_description_url| to fetch a uPnP device description. If successful, On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > nit: Is this description retrieval specific to DIAL, or actually uPnP-generic? > If the latter then perhaps re-locate/re-name this class accordingly? > > It has DIAL-specific logic like retrieving the app URL. Clarified comment. Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:31: // UI thread. On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > nit: Re thread-safety, must this class be constructed, Start()ed AND destroyed > from the UI thread? > > s/run/used/ > > > Is it the case that the caller can tear down the class to cancel the request > if it hasn't yet completed? > > Yes, destroying |fetcher_| cancels any pending request. > Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:35: static constexpr int kURLFetcherID = 1; On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > nit: This is only public for testing, and the purpose of the |id| is really to > allow tests to disambiguate multiple requests. > > > > If we replace the ctor+Start() layout with a single Create() function that > creates & returns the fetcher then we could instead have a CreateForTest(id, > ...), which would also allow tests to create multiple instances, if necessary > (though I see we don't have use for that now). > > > > Otherwise, could we at least make this a kURLFetcherIdForTest, or a > GetUrlFetcherIdForTest() method (in the latter case the Id can still be > hard-wired internally; we'd just be decoupling the test from caring about the > constant). > > I don't need multiple request IDs to test this class. Renamed to > kURLFetcherIdForTest to clarify its use. Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:39: Profile* profile, On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > Is the caller responsible for ensuring that the instance is torn down before > |profile|? > > The DIAL API is a BrowserContext-keyed service, its extension function objects > (and thus this object) will be destroyed before the Profile. > > Strictly speaking, the fetcher doesn't need to hold a Profile*, it's only used > to start the request. If you feel strongly I can refactor to remove the > Profile* class member. No, I think this is fine, so long as the lifetime of |profile| is well-defined; I'd suggest simply adding a one-line comment to clarify that the caller must ensure that DDF doesn't out-live |profile|. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:45: void Start(); On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > nit: Do we need a separate Start() API, or could we start the request directly > in the constructor? > > Generally it's not a good idea to write constructors with heavyweight side > effects, unless it's an explicitly designed to be a ScopedFoo / RAII type > object. > Granted, but it would be reasonable to have a static Create() method that constructed + started. Up to you, though. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:58: // Runs |error_cb_| with |message|. On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > nit: Not obvious from the comment, but it Run()s and clears |error_cb_|, > right? > > Updated comment, but as error_cb is a OnceCallback there's no need to clear. > > > Do we really want to use a string error message here? Could we instead use > net::Error codes and HTTP response values to indicate errors? Since HTTP > responses are >0 and net::Error <0 you could even fold the two into a single > integer error value. > > The URLFetcher doesn't expose net::Error codes. > > There are a variety of error conditions and the chrome.runtime.lastError API > only allows a string. If there's some future code that would consume error > codes, they would be straightforward to add. Acknowledged. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/dial_api.h:93: test_device_description_; On 2017/01/09 21:38:07, mark a. foltz wrote: > On 2017/01/05 at 22:29:24, Wez wrote: > > Rather than have these fields and having to friend the > FetchDescriptionFunction, have you considered providing a > SetDialRegistryForTest() on DialAPI? Though perhaps that would mean a load more > hoop-jumping to feed in the device description for the test, since you'd need to > intercept URLRequests to the test device to return the test description data? > > I like that idea. I'd rather take it on as part of fixing the threading of > DialRegistry and cleaning up that class in general. Updated TODO in > dial_api.cc. Acknowledged. https://codereview.chromium.org/2583853004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2583853004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:60: // net::LOAD_DO_NOT_SEND_AUTH_DATA: The request shoud not send auth data. typo: should
https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:76: const net::URLFetcher* source) { On 2017/01/09 at 22:12:58, Wez wrote: > On 2017/01/09 21:38:07, mark a. foltz wrote: > > On 2017/01/05 at 22:29:24, Wez wrote: > > > nit: DCHECK_EQ(fetcher_.get(), source); > > > > This code doesn't refer to fetcher_. > > Doesn't this method expect that |source| is the same URLFetcher that was created in Start(), though? I suppose you can argue that it doesn't matter to this code whether that is the case, or not, so seems legit not to bother checking if you prefer not to. Ah, I misread your earlier comment. Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.h (right): https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:39: Profile* profile, On 2017/01/09 at 22:12:58, Wez wrote: > On 2017/01/09 21:38:07, mark a. foltz wrote: > > On 2017/01/05 at 22:29:24, Wez wrote: > > > Is the caller responsible for ensuring that the instance is torn down before > > |profile|? > > > > The DIAL API is a BrowserContext-keyed service, its extension function objects > > (and thus this object) will be destroyed before the Profile. > > > > Strictly speaking, the fetcher doesn't need to hold a Profile*, it's only used > > to start the request. If you feel strongly I can refactor to remove the > > Profile* class member. > > No, I think this is fine, so long as the lifetime of |profile| is well-defined; I'd suggest simply adding a one-line comment to clarify that the caller must ensure that DDF doesn't out-live |profile|. Done. https://codereview.chromium.org/2583853004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.h:45: void Start(); On 2017/01/09 at 22:12:58, Wez wrote: > On 2017/01/09 21:38:07, mark a. foltz wrote: > > On 2017/01/05 at 22:29:24, Wez wrote: > > > nit: Do we need a separate Start() API, or could we start the request directly > > in the constructor? > > > > Generally it's not a good idea to write constructors with heavyweight side > > effects, unless it's an explicitly designed to be a ScopedFoo / RAII type > > object. > > > > Granted, but it would be reasonable to have a static Create() method that constructed + started. Up to you, though. Acknowledged. https://codereview.chromium.org/2583853004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right): https://codereview.chromium.org/2583853004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/dial/device_description_fetcher.cc:60: // net::LOAD_DO_NOT_SEND_AUTH_DATA: The request shoud not send auth data. On 2017/01/09 at 22:12:58, Wez wrote: > typo: should Done.
The CQ bit was checked by mfoltz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, isherman@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2583853004/#ps200001 (title: "Reply to wez@")
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
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...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mfoltz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, lazyboy@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2583853004/#ps220001 (title: "Rebase and fix typo.")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mfoltz@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": 220001, "attempt_start_ts": 1484082459945610,
"parent_rev": "82e5578d15c0661c3306eebdd12bd7287f35cdf6", "commit_rev":
"90824416d3eeae5ec6013b250123df65e9d48032"}
Message was sent while issue was closed.
Description was changed from ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the DIAL spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.h for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test ========== to ========== Adds chrome.dial.fetchDeviceDecription API. This API fetches the device description for a DIAL device, given a label for a device already discovered. The request is configured to not send/receive cookies, ignore proxies, and ignore redirects. Requests are retried with backoff on 5XX responses. The response is validated in multiple ways: - Valid Application-URL: header value - Response body is a valid UTF-8 string - Response body is <= 256kb Additional validations are possible by following the DIAL spec closely at the risk of ignoring some devices. See comments in device_description_fetcher.cc for details. Note: The API function implementation is complicated by thread hopping. See comments in dial_api.h for how this may be improved in the future. BUG=671932 TESTED=Manually, unit tests, API test Review-Url: https://codereview.chromium.org/2583853004 Cr-Commit-Position: refs/heads/master@{#442713} Committed: https://chromium.googlesource.com/chromium/src/+/90824416d3eeae5ec6013b250123... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/90824416d3eeae5ec6013b250123... |
