Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396943002:
Adds methods for Cast support to ios::ChromeBrowserProvider. (Closed)
|
Created:
4 years, 2 months ago by rohitrao (ping after 24h) Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds methods for Cast support to ios::ChromeBrowserProvider.
BUG=653233
TEST=None
Committed: https://crrev.com/427a0c5423cedc0da9d0268996112655e8dc1f57
Cr-Commit-Position: refs/heads/master@{#423535}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased. #
Total comments: 2
Patch Set 3 : Modify args. #Patch Set 4 : Still need tab_id. #Patch Set 5 : Update API. #
Total comments: 2
Patch Set 6 : TODO #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by rohitrao@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...
rohitrao@chromium.org changed reviewers: + olivierrobin@chromium.org, sdefresne@chromium.org
Comments welcome on the structure and naming of the methods themselves. These seemed to be the most straightforward to me -- they hid the boilerplate code on the internal side of the provider. https://codereview.chromium.org/2396943002/diff/1/ios/public/provider/chrome/... File ios/public/provider/chrome/browser/chrome_browser_provider.h (right): https://codereview.chromium.org/2396943002/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/chrome_browser_provider.h:124: // TODO(rohitrao): Change from |id| to |TabModel*| once TabModel is moved into Thoughts on using id here instead of TabModel*? Are there any better options? https://codereview.chromium.org/2396943002/diff/1/ios/public/provider/chrome/... ios/public/provider/chrome/browser/chrome_browser_provider.h:131: NSString* tab_id) const; I thought about having this API take a std::string instead, but that would require conversions from and to NSString. It's only once per tab though, so we could take the hit if the API would be better with std::string.
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 rohitrao@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.
https://codereview.chromium.org/2396943002/diff/20001/ios/public/provider/chr... File ios/public/provider/chrome/browser/chrome_browser_provider.h (right): https://codereview.chromium.org/2396943002/diff/20001/ios/public/provider/chr... ios/public/provider/chrome/browser/chrome_browser_provider.h:130: virtual void InitializeCastService(id tab_model) const; The name suggests that this will be called only once, and not once per tab_model.
https://codereview.chromium.org/2396943002/diff/20001/ios/public/provider/chr... File ios/public/provider/chrome/browser/chrome_browser_provider.h (right): https://codereview.chromium.org/2396943002/diff/20001/ios/public/provider/chr... ios/public/provider/chrome/browser/chrome_browser_provider.h:130: virtual void InitializeCastService(id tab_model) const; On 2016/10/06 09:26:45, Olivier Robin wrote: > The name suggests that this will be called only once, and not once per > tab_model. Looking at the callsites, this is only ever called once. I could rename the argument to |main_tab_model|, to try and make it clearer (because we only ever create one main tab model).
lgtm
The CQ bit was checked by rohitrao@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...
Per IRC discussion, made the call more generic, which will allow us to add other tab helpers as needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2396943002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/chrome_browser_provider.h (right): https://codereview.chromium.org/2396943002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/chrome_browser_provider.h:135: virtual void AttachTabHelpers(web::WebState* web_state, id tab) const; nit: // TODO(rohitrao): Change from |id| to |Tab*| once Tab is moved into Chromium // tree.
https://codereview.chromium.org/2396943002/diff/80001/ios/public/provider/chr... File ios/public/provider/chrome/browser/chrome_browser_provider.h (right): https://codereview.chromium.org/2396943002/diff/80001/ios/public/provider/chr... ios/public/provider/chrome/browser/chrome_browser_provider.h:135: virtual void AttachTabHelpers(web::WebState* web_state, id tab) const; On 2016/10/06 14:56:43, sdefresne wrote: > nit: > > // TODO(rohitrao): Change from |id| to |Tab*| once Tab is moved into Chromium > // tree. Done.
The CQ bit was checked by rohitrao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2396943002/#ps100001 (title: "TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adds methods for Cast support to ios::ChromeBrowserProvider. BUG=653233 TEST=None ========== to ========== Adds methods for Cast support to ios::ChromeBrowserProvider. BUG=653233 TEST=None Committed: https://crrev.com/427a0c5423cedc0da9d0268996112655e8dc1f57 Cr-Commit-Position: refs/heads/master@{#423535} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/427a0c5423cedc0da9d0268996112655e8dc1f57 Cr-Commit-Position: refs/heads/master@{#423535} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
