Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1140713005:
[PresentationAPI] Implements send API for Blob data from WebPresentationClient (Closed)
|
Created:
5 years, 7 months ago by USE s.singapati at gmail.com Modified:
5 years, 6 months ago Reviewers:
Avi (use Gerrit), jochen (gone - plz use gerrit), mark a. foltz, jam, haibinlu, imcheng (use chromium acct), whywhat, imcheng CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[PresentationAPI] Implements send API for Blob data from WebPresentationClient
to the PresentationServiceDelegate.
Depends on the Blink CL: https://codereview.chromium.org/1131463006/
BUG=459008
Committed: https://crrev.com/7aaddf97ca0db3bb64a5a61880265ecd0477c7c1
Cr-Commit-Position: refs/heads/master@{#333505}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Updated unit tests #
Total comments: 11
Patch Set 3 : review fixes and cl format. #
Total comments: 6
Patch Set 4 : #
Total comments: 3
Patch Set 5 : reverted "using presentation::PresentationMessageType" #
Messages
Total messages: 39 (14 generated)
s.singapati@samsung.com changed reviewers: + avayvod@chromium.org, haibinlu@chromium.org, imcheng@chromium.org
PTAL. Need to update this when blink CL is finalized. https://codereview.chromium.org/1140713005/diff/1/content/public/browser/pres... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1140713005/diff/1/content/public/browser/pres... content/public/browser/presentation_session_message.h:16: enum CONTENT_EXPORT PresentationMessageType { This is some what duplication. Probably enum BinaryType {ArrayBuffer, Blob} should be enough.
imcheng@google.com changed reviewers: + imcheng@google.com
https://codereview.chromium.org/1140713005/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1140713005/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:191: DCHECK(data); This code looks mostly identical to sendArrayBuffer above -- could it be refactored to a helper method that can be invoked from both places?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
PTAL. https://codereview.chromium.org/1140713005/diff/1/content/renderer/presentati... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1140713005/diff/1/content/renderer/presentati... content/renderer/presentation/presentation_dispatcher.cc:191: DCHECK(data); On 2015/05/18 18:24:48, imcheng wrote: > This code looks mostly identical to sendArrayBuffer above -- could it be > refactored to a helper method that can be invoked from both places? Done. Added helper method that returns new SessionMessage.
First I need to address the review comments in Blink CL then see possible changes in this CL.
s.singapati@samsung.com changed reviewers: + mfoltz@chromium.org
PTAL.
Mostly minor comments https://codereview.chromium.org/1140713005/diff/20002/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/20002/content/browser/present... content/browser/presentation/presentation_service_impl.cc:85: // ArrayBuffer or Blob data. Prefer using switch() statement with NOTREACHED so new enum values don't fall through to this case. https://codereview.chromium.org/1140713005/diff/20002/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1140713005/diff/20002/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:836: SaveQuitClosureAndRunLoop(); Nice test case :) https://codereview.chromium.org/1140713005/diff/20002/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1140713005/diff/20002/content/common/presenta... content/common/presentation/presentation_service.mojom:40: // Used for both ArrayBuffer and Blob data. nit: ARRAY_BUFFER and BLOB https://codereview.chromium.org/1140713005/diff/20002/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1140713005/diff/20002/content/public/browser/... content/public/browser/presentation_session_message.h:36: static scoped_ptr<PresentationSessionMessage> CreateBinaryMessage( Prefer explicit methods to parallel the above: CreateArrayBufferMessage() CreateBlobMessage() https://codereview.chromium.org/1140713005/diff/20002/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1140713005/diff/20002/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:68: // Fill session_message->type in the caller. Why not pass the type as an argument and fill it in here? https://codereview.chromium.org/1140713005/diff/20002/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:233: // ArrayBuffer or Blob types. Prefer seeing a switch { } statement here with a default NOTREACHED() so we don't assume that new types are binary.
PTaL. https://codereview.chromium.org/1140713005/diff/20002/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/20002/content/browser/present... content/browser/presentation/presentation_service_impl.cc:85: // ArrayBuffer or Blob data. On 2015/06/02 23:51:02, mark a. foltz wrote: > Prefer using switch() statement with NOTREACHED so new enum values don't fall > through to this case. Done. https://codereview.chromium.org/1140713005/diff/20002/content/common/presenta... File content/common/presentation/presentation_service.mojom (right): https://codereview.chromium.org/1140713005/diff/20002/content/common/presenta... content/common/presentation/presentation_service.mojom:40: // Used for both ArrayBuffer and Blob data. On 2015/06/02 23:51:02, mark a. foltz wrote: > nit: ARRAY_BUFFER and BLOB Done. https://codereview.chromium.org/1140713005/diff/20002/content/public/browser/... File content/public/browser/presentation_session_message.h (right): https://codereview.chromium.org/1140713005/diff/20002/content/public/browser/... content/public/browser/presentation_session_message.h:36: static scoped_ptr<PresentationSessionMessage> CreateBinaryMessage( On 2015/06/02 23:51:02, mark a. foltz wrote: > Prefer explicit methods to parallel the above: > > CreateArrayBufferMessage() > CreateBlobMessage() Done. https://codereview.chromium.org/1140713005/diff/20002/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1140713005/diff/20002/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:68: // Fill session_message->type in the caller. On 2015/06/02 23:51:02, mark a. foltz wrote: > Why not pass the type as an argument and fill it in here? Done. Well, Yes. https://codereview.chromium.org/1140713005/diff/20002/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:233: // ArrayBuffer or Blob types. On 2015/06/02 23:51:02, mark a. foltz wrote: > Prefer seeing a switch { } statement here with a default NOTREACHED() so we > don't assume that new types are binary. Done.
PTAL.
lgtm with some minor comments. Thanks for the patch! https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:57: case presentation::PresentationMessageType:: Consider a using presentation::PresentationMessageType declaration in this .cc and others to keep the line wrapping to a minimum. https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:815: .WillOnce(DoAll(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit), I preferred the previous formatting that kept the arguments to DoAll() horizontally aligned. https://codereview.chromium.org/1140713005/diff/70001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1140713005/diff/70001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:186: presentation::PresentationMessageType:: Add using presentation::PresentationMessageType in this .cc as well
The CQ bit was checked by s.singapati@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1140713005/#ps90001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140713005/90001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:57: case presentation::PresentationMessageType:: On 2015/06/05 22:40:55, mark a. foltz wrote: > Consider a using presentation::PresentationMessageType declaration in this .cc > and others to keep the line wrapping to a minimum. Done. https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... File content/browser/presentation/presentation_service_impl_unittest.cc (right): https://codereview.chromium.org/1140713005/diff/70001/content/browser/present... content/browser/presentation/presentation_service_impl_unittest.cc:815: .WillOnce(DoAll(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit), On 2015/06/05 22:40:56, mark a. foltz wrote: > I preferred the previous formatting that kept the arguments to DoAll() > horizontally aligned. Done. https://codereview.chromium.org/1140713005/diff/70001/content/renderer/presen... File content/renderer/presentation/presentation_dispatcher.cc (right): https://codereview.chromium.org/1140713005/diff/70001/content/renderer/presen... content/renderer/presentation/presentation_dispatcher.cc:186: presentation::PresentationMessageType:: On 2015/06/05 22:40:56, mark a. foltz wrote: > Add using presentation::PresentationMessageType in this .cc as well Done.
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140713005/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
s.singapati@samsung.com changed reviewers: + avi@chromium.org
Hi Avi, Could you please take a look? Thanks.
s.singapati@samsung.com changed reviewers: + jam@chromium.org, jochen@chromium.org
lgtm with comment addressed https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:25: using presentation::PresentationMessageType; please don't use "using" other for helping for large-scale refactoring
https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:25: using presentation::PresentationMessageType; On 2015/06/09 08:29:06, jochen wrote: > please don't use "using" other for helping for large-scale refactoring Acknowledged. Yes this is a small change from PS#3 to #4 to only avoid long enum values. Can we proceed with this?
On 2015/06/09 08:29:06, jochen wrote: > lgtm with comment addressed > > https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... > File content/browser/presentation/presentation_service_impl.cc (right): > > https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... > content/browser/presentation/presentation_service_impl.cc:25: using > presentation::PresentationMessageType; > please don't use "using" other for helping for large-scale refactoring @jochen: Yes, this was a small change from PS#3 to #4 to only avoid long enum values. Can we proceed with this?
I'd recommend using clang-format to deal with long lines instead
RE: On 2015/06/09 10:04:29, jochen wrote: > I'd recommend using clang-format to deal with long lines instead Done. https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... File content/browser/presentation/presentation_service_impl.cc (right): https://codereview.chromium.org/1140713005/diff/90001/content/browser/present... content/browser/presentation/presentation_service_impl.cc:25: using presentation::PresentationMessageType; On 2015/06/09 08:29:06, jochen wrote: > please don't use "using" other for helping for large-scale refactoring Done.
The CQ bit was checked by s.singapati@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/1140713005/#ps110001 (title: "reverted "using presentation::PresentationMessageType"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140713005/110001
On 2015/06/08 15:26:02, s.singapati wrote: > Hi Avi, Could you please take a look? Thanks. You have all the LGs you need to commit; is there something specific you'd like me to look at?
On 2015/06/09 14:41:51, Avi wrote: > On 2015/06/08 15:26:02, s.singapati wrote: > > Hi Avi, Could you please take a look? Thanks. > > You have all the LGs you need to commit; is there something specific you'd like > me to look at? It needed LG for content/public/browser/*. But, jochen reviewed it already today. Ofc, if you have any other comments, I will address those. Thank You.
Message was sent while issue was closed.
Committed patchset #5 (id:110001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7aaddf97ca0db3bb64a5a61880265ecd0477c7c1 Cr-Commit-Position: refs/heads/master@{#333505} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
