Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570203004:
Fix low risk nullable => non nullable change for NavigatorLanguage.language (Closed)
DescriptionFix low risk nullable => non nullable change for NavigatorLanguage.language
NavigatorLanguage::language() returns defaultLanguage() which either
preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or
platformLanguage() which will never return null; therefore, the change is safe.
Added web platform test for InputEvent
BUG=662005
Committed: https://crrev.com/3d59b1cb65c0b2566316b0886e69e49ef7589d6e
Cr-Commit-Position: refs/heads/master@{#439111}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added layout test for inputevent.data and modified layout test expectation for two input tests #
Total comments: 2
Patch Set 3 : Added web platfom test for InputEvent #
Total comments: 4
Patch Set 4 : Codereview update #Patch Set 5 : Revert changes made to InputEvent, leaved web platform test entry for input-events for future platf… #
Total comments: 3
Patch Set 6 : Revert InputEvent #
Total comments: 2
Patch Set 7 : Move inputevent constructor web platform test to the appropriate place #
Total comments: 5
Patch Set 8 : Codereview update #
Total comments: 4
Patch Set 9 : Codereview update #Patch Set 10 : Added InputEvent constructor platform test to MANIFEST #
Messages
Total messages: 68 (41 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...
lunalu@chromium.org changed reviewers: + foolip@chromium.org
https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/InputEvent.idl:14: readonly attribute DOMString data; This one needs a test, as new InputEvent('type', { data: null }).data will change from null to "null". https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/NavigatorLanguage.idl (right): https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/NavigatorLanguage.idl:11: readonly attribute DOMString language; Can you mention in the description why this change is safe? (Perhaps dig a bit more to see if there's any way it can return null, hopefully find nothing.)
Description was changed from ========== Fix low risk nullable => non nullable attributes / arugments BUG=662005 ========== to ========== Fix low risk nullable => non nullable attributes / arguments BUG=662005 ==========
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_...)
Description was changed from ========== Fix low risk nullable => non nullable attributes / arguments BUG=662005 ========== to ========== Fix low risk nullable => non nullable attributes / arguments Changed blink tests expectation for the following tests: fast/events/inputevents/inputevent-data-keyboard.html fast/events/inputevents/inputevent-cut-paste.html Reasoning: https://w3c.github.io/input-events/#dfn-data ""If the editing host is not a contenteditable element and the inputType is "insertFromPaste", "insertFromDrop", "insertTranspose", "insertReplacementText", or "insertFromYank" the value of data is to be the plain text string to be inserted."" Added layout test for inputevent.data should be should be non nullable Reasoning for changing NavigatorLanguage.language to be non nullable is safe: NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null BUG=662005 ==========
Description was changed from ========== Fix low risk nullable => non nullable attributes / arguments Changed blink tests expectation for the following tests: fast/events/inputevents/inputevent-data-keyboard.html fast/events/inputevents/inputevent-cut-paste.html Reasoning: https://w3c.github.io/input-events/#dfn-data ""If the editing host is not a contenteditable element and the inputType is "insertFromPaste", "insertFromDrop", "insertTranspose", "insertReplacementText", or "insertFromYank" the value of data is to be the plain text string to be inserted."" Added layout test for inputevent.data should be should be non nullable Reasoning for changing NavigatorLanguage.language to be non nullable is safe: NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null BUG=662005 ========== to ========== Fix low risk nullable => non nullable attributes / arguments Changed blink tests expectation for the following tests: fast/events/inputevents/inputevent-data-keyboard.html fast/events/inputevents/inputevent-cut-paste.html Reasoning: https://w3c.github.io/input-events/#dfn-data ""If the editing host is not a contenteditable element and the inputType is "insertFromPaste", "insertFromDrop", "insertTranspose", "insertReplacementText", or "insertFromYank" the value of data is to be the plain text string to be inserted."" Added layout test for inputevent.data should be should be non nullable Reasoning for changing NavigatorLanguage.language to be non nullable is safe: NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null BUG=662005 ==========
lunalu@chromium.org changed reviewers: + tdresser@chromium.org
Hi Tim, Could you please take a look at my change in: fast/events/inputevents/inputevent-data-keyboard.html fast/events/inputevents/inputevent-cut-paste.html I changed the idl file for InputEvent and InputEventInit to match the spec. As a result the layout test expectation needed to be updated. Please let me know if my change is wrong. Thanks https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/InputEvent.idl:14: readonly attribute DOMString data; On 2016/12/14 19:07:31, foolip wrote: > This one needs a test, as new InputEvent('type', { data: null }).data will > change from null to "null". Done. https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/NavigatorLanguage.idl (right): https://codereview.chromium.org/2570203004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/NavigatorLanguage.idl:11: readonly attribute DOMString language; On 2016/12/14 19:07:31, foolip wrote: > Can you mention in the description why this change is safe? (Perhaps dig a bit > more to see if there's any way it can return null, hopefully find nothing.) Done.
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...
Patchset #2 (id:20001) has been deleted
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...
navigator.language change lgtm, for input let's tinker with tests and wait for tdresser@ https://codereview.chromium.org/2570203004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-cut-paste.html (right): https://codereview.chromium.org/2570203004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-cut-paste.html:43: assert_equals(event.data, "", 'Cut&Paste should have empty plain text |data| field') s/empty plain text/empty string/ (https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_... would suggest a description like "beforeinput event data attribute", but no need to rewrite existing descriptions all over the place.) https://codereview.chromium.org/2570203004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-data.html (right): https://codereview.chromium.org/2570203004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-data.html:2: <title>InputEvent data should be non nullable</title> This can be upstreamed directly to imported/wpt/input-events/ :) (Will discuss offline)
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tdresser@chromium.org changed reviewers: + dtapuska@chromium.org
LGTM, but over to dtapuska who has context on InputEvent. https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/W3CImportExpectations (right): https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: tdresser@chromium.org I'm not the right owner for this. Probably input-dev@chromium.org would be best. https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/W3CImportExpectations:295: imported/wpt/progress-es [ Skip ] Was this intentional?
dtapuska@chromium.org changed reviewers: + chongz@chromium.org
On 2016/12/15 13:35:56, tdresser wrote: > LGTM, but over to dtapuska who has context on InputEvent. > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/W3CImportExpectations (right): > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: > mailto:tdresser@chromium.org > I'm not the right owner for this. > > Probably mailto:input-dev@chromium.org would be best. > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/W3CImportExpectations:295: > imported/wpt/progress-es [ Skip ] > Was this intentional? you can mark me as the owner for input-events in the import file that is fine. Deferring to chongz so he can see the IDL changes.
On 2016/12/15 14:09:34, dtapuska wrote: > On 2016/12/15 13:35:56, tdresser wrote: > > LGTM, but over to dtapuska who has context on InputEvent. > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/W3CImportExpectations (right): > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: > > mailto:tdresser@chromium.org > > I'm not the right owner for this. > > > > Probably mailto:input-dev@chromium.org would be best. > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/W3CImportExpectations:295: > > imported/wpt/progress-es [ Skip ] > > Was this intentional? > > you can mark me as the owner for input-events in the import file that is fine. > Deferring to chongz so he can see the IDL changes. Specifically the spec text indicates the InputEvent can be null... InputEvent.data: the string containing the data that was added to the element, which may be null if it doesn't apply.
On 2016/12/15 14:11:18, dtapuska wrote: > On 2016/12/15 14:09:34, dtapuska wrote: > > On 2016/12/15 13:35:56, tdresser wrote: > > > LGTM, but over to dtapuska who has context on InputEvent. > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > File third_party/WebKit/LayoutTests/W3CImportExpectations (right): > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: > > > mailto:tdresser@chromium.org > > > I'm not the right owner for this. > > > > > > Probably mailto:input-dev@chromium.org would be best. > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/W3CImportExpectations:295: > > > imported/wpt/progress-es [ Skip ] > > > Was this intentional? > > > > you can mark me as the owner for input-events in the import file that is fine. > > Deferring to chongz so he can see the IDL changes. > > > Specifically the spec text indicates the InputEvent can be null... > > InputEvent.data: the string containing the data that was added to the element, > which may be null if it doesn't apply. Also the WebKit IDL is nullable... https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/InputEvent.idl Perhaps the spec is wrong?
On 2016/12/15 14:11:18, dtapuska wrote: > On 2016/12/15 14:09:34, dtapuska wrote: > > On 2016/12/15 13:35:56, tdresser wrote: > > > LGTM, but over to dtapuska who has context on InputEvent. > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > File third_party/WebKit/LayoutTests/W3CImportExpectations (right): > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: > > > mailto:tdresser@chromium.org > > > I'm not the right owner for this. > > > > > > Probably mailto:input-dev@chromium.org would be best. > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/W3CImportExpectations:295: > > > imported/wpt/progress-es [ Skip ] > > > Was this intentional? > > > > you can mark me as the owner for input-events in the import file that is fine. > > Deferring to chongz so he can see the IDL changes. > > > Specifically the spec text indicates the InputEvent can be null... > > InputEvent.data: the string containing the data that was added to the element, > which may be null if it doesn't apply. Also the WebKit IDL is nullable... https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/InputEvent.idl Perhaps the spec is wrong?
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 checked by lunalu@chromium.org to run a CQ dry run
Thank you for reviewing my CL. I have reverted the change made to the InputEvent*, please take another look. Thanks https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/W3CImportExpectations (right): https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: tdresser@chromium.org On 2016/12/15 13:35:56, tdresser wrote: > I'm not the right owner for this. > > Probably mailto:input-dev@chromium.org would be best. Done. https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/W3CImportExpectations:295: imported/wpt/progress-es [ Skip ] On 2016/12/15 13:35:56, tdresser wrote: > Was this intentional? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/15 14:14:10, dtapuska wrote: > On 2016/12/15 14:11:18, dtapuska wrote: > > On 2016/12/15 14:09:34, dtapuska wrote: > > > On 2016/12/15 13:35:56, tdresser wrote: > > > > LGTM, but over to dtapuska who has context on InputEvent. > > > > > > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > > File third_party/WebKit/LayoutTests/W3CImportExpectations (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > > third_party/WebKit/LayoutTests/W3CImportExpectations:273: ## Owners: > > > > mailto:tdresser@chromium.org > > > > I'm not the right owner for this. > > > > > > > > Probably mailto:input-dev@chromium.org would be best. > > > > > > > > > > > > > > https://codereview.chromium.org/2570203004/diff/60001/third_party/WebKit/Layo... > > > > third_party/WebKit/LayoutTests/W3CImportExpectations:295: > > > > imported/wpt/progress-es [ Skip ] > > > > Was this intentional? > > > > > > you can mark me as the owner for input-events in the import file that is > fine. > > > Deferring to chongz so he can see the IDL changes. > > > > > > Specifically the spec text indicates the InputEvent can be null... > > > > InputEvent.data: the string containing the data that was added to the element, > > which may be null if it doesn't apply. > > Also the WebKit IDL is nullable... > https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/InputEvent.idl > > Perhaps the spec is wrong? I think UIEvent spec is wrong and we should keep |InputEvent.data| as nullable. I've filed UIEvent issue here: https://github.com/w3c/uievents/issues/116
https://codereview.chromium.org/2570203004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-cut-paste.html (right): https://codereview.chromium.org/2570203004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-cut-paste.html:43: assert_equals(event.data, null, 'Cut&Paste should have empty plain text |data| field') this should likely be reverted too. https://codereview.chromium.org/2570203004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/input-events/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/input-events/inputevent-constructor.html:9: assert_equals(e.data, ''); This test is likely to fail as well now.
My bad. Sorry for missing a few places for the revert. https://codereview.chromium.org/2570203004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-cut-paste.html (right): https://codereview.chromium.org/2570203004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-cut-paste.html:43: assert_equals(event.data, null, 'Cut&Paste should have empty plain text |data| field') On 2016/12/15 16:12:49, dtapuska wrote: > this should likely be reverted too. Done.
https://codereview.chromium.org/2570203004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/input-events/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/input-events/inputevent-constructor.html:3: <script src="/resources/testharness.js"></script> Shouldn't imported tests first be added to the github repository and then pulled into this directory?
https://codereview.chromium.org/2570203004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/input-events/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/input-events/inputevent-constructor.html:3: <script src="/resources/testharness.js"></script> On 2016/12/15 17:00:03, dtapuska wrote: > Shouldn't imported tests first be added to the github repository and then pulled > into this directory? Until now, yes, but https://crbug.com/657117 is about making it possible to write tests directly in our copy and have them exported, so we're creating real test CLs for jeffcarp@ to take through the process. lunalu@, since InputEvent and its data attribute is defined in https://w3c.github.io/uievents/#interface-inputevent I think putting this test in wpt/uievents/ would be more appropriate.
done. thanks
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...
I said to split the test into a separate CL, but never mind actually, just say in the description what's going on. https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:10: assert_equals(e.dataTransfer, null); Since https://w3c.github.io/uievents/#events-inputevents only has data and isComposing, I'm thinking test only those two here. Since data is what we cared about leaving it there is fine, but the others things would then have to be under wpt/input-events/. (Splitting the event into two specs is the root problem, but I don't know if there's a good reason for it. https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:12: }, 'Test InputEvent constructor without inputEventInit dictionary'); Test descriptions need not include "Test for ..." or similar, they can just name the thing being tested, i.e. "InputEvent constructor without InputEventInit dictionary" https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:15: var e = new InputEvent('type', { data: null, isComposing: true }); A test with { data: "" } and some non-empty string would also be good.
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 lunalu@chromium.org to run a CQ dry run
done. thanks https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:10: assert_equals(e.dataTransfer, null); On 2016/12/16 10:01:44, foolip wrote: > Since https://w3c.github.io/uievents/#events-inputevents only has data and > isComposing, I'm thinking test only those two here. Since data is what we cared > about leaving it there is fine, but the others things would then have to be > under wpt/input-events/. (Splitting the event into two specs is the root > problem, but I don't know if there's a good reason for it. Done. https://codereview.chromium.org/2570203004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:12: }, 'Test InputEvent constructor without inputEventInit dictionary'); On 2016/12/16 10:01:44, foolip wrote: > Test descriptions need not include "Test for ..." or similar, they can just name > the thing being tested, i.e. "InputEvent constructor without InputEventInit > dictionary" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % nit https://codereview.chromium.org/2570203004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:8: assert_equals(e.inputType, ''); inputType is also from https://w3c.github.io/input-events/#interface-InputEvent so omit here https://codereview.chromium.org/2570203004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:15: assert_equals(e.data, null, 'inputevent.data should be null'); With tiny tests like this, I tend to just omit assert descriptions, since they're not needed to debug a failure. But if you want one, it should ideally just describe the expected value, so "inputevent.data" would do it. (On a failure it will say something like "inputevent.data expected to be null but was foo".)
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2570203004/#ps180001 (title: "Codereview update")
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 foolip@chromium.org
foolip@chromium.org changed reviewers: + jeffcarp@chromium.org
jeffcarp@, here's another test export CL for you.
Description was changed from ========== Fix low risk nullable => non nullable attributes / arguments Changed blink tests expectation for the following tests: fast/events/inputevents/inputevent-data-keyboard.html fast/events/inputevents/inputevent-cut-paste.html Reasoning: https://w3c.github.io/input-events/#dfn-data ""If the editing host is not a contenteditable element and the inputType is "insertFromPaste", "insertFromDrop", "insertTranspose", "insertReplacementText", or "insertFromYank" the value of data is to be the plain text string to be inserted."" Added layout test for inputevent.data should be should be non nullable Reasoning for changing NavigatorLanguage.language to be non nullable is safe: NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null BUG=662005 ========== to ========== Fix low risk nullable => non nullable change for NavigatorLanguage.language NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null; therefore, the change is safe. Added web platform test for InputEvent BUG=662005 ==========
lunalu@chromium.org changed reviewers: - jeffcarp@chromium.org
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2570203004/#ps200001 (title: "Added InputEvent constructor platform test to MANIFEST")
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": 200001, "attempt_start_ts": 1481897490667990,
"parent_rev": "9e1783bf54ea95ac6a2f2c3a835da14bb86bb356", "commit_rev":
"7d2074e5780fc2288e032d35fbf3be921c945d9c"}
Message was sent while issue was closed.
Description was changed from ========== Fix low risk nullable => non nullable change for NavigatorLanguage.language NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null; therefore, the change is safe. Added web platform test for InputEvent BUG=662005 ========== to ========== Fix low risk nullable => non nullable change for NavigatorLanguage.language NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null; therefore, the change is safe. Added web platform test for InputEvent BUG=662005 Review-Url: https://codereview.chromium.org/2570203004 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Fix low risk nullable => non nullable change for NavigatorLanguage.language NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null; therefore, the change is safe. Added web platform test for InputEvent BUG=662005 Review-Url: https://codereview.chromium.org/2570203004 ========== to ========== Fix low risk nullable => non nullable change for NavigatorLanguage.language NavigatorLanguage::language() returns defaultLanguage() which either preferredLanguagesOverride()[0] if preferredLanguagesOverride() is not empty or platformLanguage() which will never return null; therefore, the change is safe. Added web platform test for InputEvent BUG=662005 Committed: https://crrev.com/3d59b1cb65c0b2566316b0886e69e49ef7589d6e Cr-Commit-Position: refs/heads/master@{#439111} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3d59b1cb65c0b2566316b0886e69e49ef7589d6e Cr-Commit-Position: refs/heads/master@{#439111}
Message was sent while issue was closed.
Done. Thanks https://codereview.chromium.org/2570203004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html (right): https://codereview.chromium.org/2570203004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:8: assert_equals(e.inputType, ''); On 2016/12/16 12:28:48, foolip wrote: > inputType is also from https://w3c.github.io/input-events/#interface-InputEvent > so omit here Done. https://codereview.chromium.org/2570203004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/imported/wpt/uievents/constructors/inputevent-constructor.html:15: assert_equals(e.data, null, 'inputevent.data should be null'); On 2016/12/16 12:28:48, foolip wrote: > With tiny tests like this, I tend to just omit assert descriptions, since > they're not needed to debug a failure. But if you want one, it should ideally > just describe the expected value, so "inputevent.data" would do it. (On a > failure it will say something like "inputevent.data expected to be null but was > foo".) Done.
Message was sent while issue was closed.
Upstreamed! https://github.com/w3c/web-platform-tests/pull/4360 On Fri, Dec 16, 2016 at 6:09 AM <foolip@chromium.org> wrote: > jeffcarp@, here's another test export CL for you. > > https://codereview.chromium.org/2570203004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Upstreamed! https://github.com/w3c/web-platform-tests/pull/4360 On Fri, Dec 16, 2016 at 6:09 AM <foolip@chromium.org> wrote: > jeffcarp@, here's another test export CL for you. > > https://codereview.chromium.org/2570203004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
