Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574303002:
Fix low risk non-nullable => nullable attributes / arguments to match the spec (Closed)
|
Created:
4 years ago by lunalu1 Modified:
4 years ago CC:
chromium-reviews, awdf+watch_chromium.org, Peter Beverloo, johnme+watch_chromium.org, sof, eae+blinkwatch, haraken, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, harkness+watch_chromium.org, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix low risk non-nullable => nullable attributes / arguments to match the spec
BUG=662005
Committed: https://crrev.com/f35734e8595bf59e62f8b8b87c0dd638bd01e472
Cr-Commit-Position: refs/heads/master@{#438869}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Revert change on ImageCapture.idl #
Total comments: 2
Patch Set 3 : Codereview update #
Messages
Total messages: 30 (18 generated)
lunalu@chromium.org changed reviewers: + foolip@chromium.org
Thanks for reviewing my CL's
The CQ bit was checked by lunalu@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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_...)
Bots seems pretty angry :) https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl (right): https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl:19: [CallWith=ScriptState, RaisesException] Promise<void> setOptions(PhotoSettings? photoSettings); The spec needs to change here, a nullable dictionary doesn't make sense. As a drive-by, can you also update the link above to https://w3c.github.io/mediacapture-image/#ImageCaptureAPI? https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.idl (right): https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.idl:16: readonly attribute DOMString id; Can PresentationConnection::m_id ever be a null WTF::String? Spec-side in https://w3c.github.io/presentation-api/#dfn-presentation-identifier it seems like it never could be, but better make sure.
https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl (right): https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl:19: [CallWith=ScriptState, RaisesException] Promise<void> setOptions(PhotoSettings? photoSettings); On 2016/12/14 19:17:12, foolip wrote: > The spec needs to change here, a nullable dictionary doesn't make sense. As a > drive-by, can you also update the link above to > https://w3c.github.io/mediacapture-image/#ImageCaptureAPI? Actually, leave the spec link for another CL when you actually touch the file. (Mass-replacement of /TR/ links also welcome.)
The CQ bit was checked by lunalu@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...
lunalu@chromium.org changed reviewers: + mlamouri@chromium.org
Hi mlamouri@, Could you please confirm that PresentationConnection#id could never be null so that I can make the change in PresentationConnection.idl to make id nullable? Thanks https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl (right): https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl:19: [CallWith=ScriptState, RaisesException] Promise<void> setOptions(PhotoSettings? photoSettings); On 2016/12/14 19:18:02, foolip wrote: > On 2016/12/14 19:17:12, foolip wrote: > > The spec needs to change here, a nullable dictionary doesn't make sense. As a > > drive-by, can you also update the link above to > > https://w3c.github.io/mediacapture-image/#ImageCaptureAPI? > > Actually, leave the spec link for another CL when you actually touch the file. > (Mass-replacement of /TR/ links also welcome.) I will reverse the change because the change will not compile. https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.idl (right): https://codereview.chromium.org/2574303002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.idl:16: readonly attribute DOMString id; On 2016/12/14 19:17:12, foolip wrote: > Can PresentationConnection::m_id ever be a null WTF::String? Spec-side in > https://w3c.github.io/presentation-api/#dfn-presentation-identifier it seems > like it never could be, but better make sure. Will ask mlamouri@ to confirm that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
peter@chromium.org changed reviewers: + peter@chromium.org
push_messaging lgtm
On 2016/12/15 at 11:33:59, lunalu wrote: > Hi mlamouri@, > > Could you please confirm that PresentationConnection#id could never be null so that I can make the change in PresentationConnection.idl to make id nullable? Should be fine, yes. lgtm
lgtm % ? https://codereview.chromium.org/2574303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl (right): https://codereview.chromium.org/2574303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl:5: // https://w3c.github.io/mediacapture-image/#ImageCaptureAPI? Trailing question mark here, needs more certainty :)
The CQ bit was checked by lunalu@chromium.org
The CQ bit was unchecked by lunalu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, foolip@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2574303002/#ps40001 (title: "Codereview update")
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": 40001, "attempt_start_ts": 1481815641116820,
"parent_rev": "f10a88b3f6ce4f332e2076c9baa884d41dac5f38", "commit_rev":
"b65febb51e0ede9ec727268f945721fc99f09dc7"}
Message was sent while issue was closed.
Description was changed from ========== Fix low risk non-nullable => nullable attributes / arguments to match the spec BUG=662005 ========== to ========== Fix low risk non-nullable => nullable attributes / arguments to match the spec BUG=662005 Review-Url: https://codereview.chromium.org/2574303002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix low risk non-nullable => nullable attributes / arguments to match the spec BUG=662005 Review-Url: https://codereview.chromium.org/2574303002 ========== to ========== Fix low risk non-nullable => nullable attributes / arguments to match the spec BUG=662005 Committed: https://crrev.com/f35734e8595bf59e62f8b8b87c0dd638bd01e472 Cr-Commit-Position: refs/heads/master@{#438869} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f35734e8595bf59e62f8b8b87c0dd638bd01e472 Cr-Commit-Position: refs/heads/master@{#438869}
Message was sent while issue was closed.
https://codereview.chromium.org/2574303002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl (right): https://codereview.chromium.org/2574303002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/imagecapture/ImageCapture.idl:5: // https://w3c.github.io/mediacapture-image/#ImageCaptureAPI? On 2016/12/15 15:04:58, foolip wrote: > Trailing question mark here, needs more certainty :) Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
