Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569663005:
Low risk nullable => non-nullable change (Closed)
|
Created:
4 years ago by lunalu1 Modified:
4 years ago Reviewers:
foolip CC:
chromium-reviews, blink-reviews-style_chromium.org, blink-reviews-css, haraken, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLow risk nullable => non-nullable change
Made the following arguments / attributes non-nullable to match the spec.
History#pushState argument title seemed to be never used by our code.
https://github.com/whatwg/html/issues/2174
BUG=662005
Committed: https://crrev.com/9a86e1807f73676102d1b6cb4d660604390d3e76
Cr-Commit-Position: refs/heads/master@{#438548}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Codereview update #
Total comments: 2
Patch Set 3 : Codereview update #Patch Set 4 : Revert CSSStyleDeclaration.idl to pass layout tests #
Total comments: 1
Messages
Total messages: 26 (18 generated)
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...
foolip@chromium.org changed reviewers: + foolip@chromium.org
https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl:34: [CEReactions, RaisesException] void setProperty(DOMString property, DOMString value, optional DOMString priority = null); Here I think we should be adding [TreatNullAs=EmptyString] at the same time as the argument is made non-nullable. Behavior does change for undefined, so a test in web-platform-tests for this should be possible. https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/History.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/History.idl:40: [RaisesException] void pushState(SerializedScriptValue data, DOMString title, optional DOMString? url = null); Can you check if the string passed in here ever re-emerges where a script can see it? If so this change can be tested, passing in null should now behave as "null" instead of whatever the previous behavior was, "" or null presumably. https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:15: [RuntimeEnabled=ExperimentalCanvasFeatures, EnforceRange] unsigned long resizeWidth; Let's take a look at the change in the generated code tomorrow to figure out if this can be tested or not. https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/gamepad/GamepadEventInit.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/gamepad/GamepadEventInit.idl:8: // TODO(foolip): |gamepad| should be required and not nullable. I think that the whole set of changes around gamepad on GamepadEvent and GamepadEventInit would go well together.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Description was changed from ========== Low risk nullable => non-nullable change Made the following arguments / attributes non-nullable to match the spec. BUG=662005 ========== to ========== Low risk nullable => non-nullable change Made the following arguments / attributes non-nullable to match the spec. History#pushState argument title seemed to be never used by our code. https://github.com/whatwg/html/issues/2174 BUG=662005 ==========
Done. Thanks https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl:34: [CEReactions, RaisesException] void setProperty(DOMString property, DOMString value, optional DOMString priority = null); On 2016/12/13 22:43:24, foolip wrote: > Here I think we should be adding [TreatNullAs=EmptyString] at the same time as > the argument is made non-nullable. Behavior does change for undefined, so a test > in web-platform-tests for this should be possible. Done. https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/History.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/History.idl:40: [RaisesException] void pushState(SerializedScriptValue data, DOMString title, optional DOMString? url = null); On 2016/12/13 22:43:24, foolip wrote: > Can you check if the string passed in here ever re-emerges where a script can > see it? If so this change can be tested, passing in null should now behave as > "null" instead of whatever the previous behavior was, "" or null presumably. The title is never touched by our code. https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:15: [RuntimeEnabled=ExperimentalCanvasFeatures, EnforceRange] unsigned long resizeWidth; On 2016/12/13 22:43:24, foolip wrote: > Let's take a look at the change in the generated code tomorrow to figure out if > this can be tested or not. Done. https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/gamepad/GamepadEventInit.idl (right): https://codereview.chromium.org/2569663005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/gamepad/GamepadEventInit.idl:8: // TODO(foolip): |gamepad| should be required and not nullable. On 2016/12/13 22:43:24, foolip wrote: > I think that the whole set of changes around gamepad on GamepadEvent and > GamepadEventInit would go well together. Done.
https://codereview.chromium.org/2569663005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl (right): https://codereview.chromium.org/2569663005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl:33: // [TreatNullAs=EmptyString] and should not be nullable. Can remove the TODO now. Tests needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Done. Thanks https://codereview.chromium.org/2569663005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl (right): https://codereview.chromium.org/2569663005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/CSSStyleDeclaration.idl:33: // [TreatNullAs=EmptyString] and should not be nullable. On 2016/12/14 12:07:00, foolip wrote: > Can remove the TODO now. Tests needed. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by foolip@chromium.org
lgtm https://codereview.chromium.org/2569663005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right): https://codereview.chromium.org/2569663005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:15: [RuntimeEnabled=ExperimentalCanvasFeatures, EnforceRange] unsigned long resizeWidth; For any drive-by reviewers: a change like this would typically be observable, because { resizeWidth: null } goes from behaving like {} to { resizeWidth: 0 }. However, as used in the C++ code, missing is treated as 0, so it's not observable.
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": 60001, "attempt_start_ts": 1481733395605680,
"parent_rev": "58a2a559679e301152aee1ba04e69970902acb80", "commit_rev":
"b6196514ea20f2e53005c21b8437ad31184b3280"}
Message was sent while issue was closed.
Description was changed from ========== Low risk nullable => non-nullable change Made the following arguments / attributes non-nullable to match the spec. History#pushState argument title seemed to be never used by our code. https://github.com/whatwg/html/issues/2174 BUG=662005 ========== to ========== Low risk nullable => non-nullable change Made the following arguments / attributes non-nullable to match the spec. History#pushState argument title seemed to be never used by our code. https://github.com/whatwg/html/issues/2174 BUG=662005 Review-Url: https://codereview.chromium.org/2569663005 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Low risk nullable => non-nullable change Made the following arguments / attributes non-nullable to match the spec. History#pushState argument title seemed to be never used by our code. https://github.com/whatwg/html/issues/2174 BUG=662005 Review-Url: https://codereview.chromium.org/2569663005 ========== to ========== Low risk nullable => non-nullable change Made the following arguments / attributes non-nullable to match the spec. History#pushState argument title seemed to be never used by our code. https://github.com/whatwg/html/issues/2174 BUG=662005 Committed: https://crrev.com/9a86e1807f73676102d1b6cb4d660604390d3e76 Cr-Commit-Position: refs/heads/master@{#438548} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9a86e1807f73676102d1b6cb4d660604390d3e76 Cr-Commit-Position: refs/heads/master@{#438548} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
