Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1617243004:
Stop checking for null selectionModel field. (Closed)
|
Created:
4 years, 11 months ago by Kevin Bailey Modified:
4 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, arv+watch_chromium.org, satorux1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop checking for null selectionModel field.
It's apparent that the field is never null in this method, and if it were, we'd have to change much more. So make the code a little more clear and remove the test.
BUG=
Committed: https://crrev.com/711c45a18e07b784e873225f469efe1019c1a41c
Cr-Commit-Position: refs/heads/master@{#374658}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Corrected comment #Patch Set 3 : Bail if selectionModel not set #
Total comments: 5
Patch Set 4 : Added comment #Patch Set 5 : Remove unneeded code #
Total comments: 2
Patch Set 6 : Restored correct code #Messages
Total messages: 23 (6 generated)
Description was changed from ========== Stop using null selectionModel field. BUG= ========== to ========== Stop using null selectionModel field. BUG= ==========
krb@chromium.org changed reviewers: + dbeam@chromium.org, rouslan@chromium.org
Just an observation. https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_list.js (left): https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_list.js:407: this.selectionModel.selectedIndex = 0; While looking around, trying to figure out how storage of language order preference worked, I spotted this. It uses a null .selectionModel field on line 407. I suspect that it's not a problem because .selectionModel is always set (if ::decorate() is always called) but I assume we still want to fix it. It's also unclear to me how .selectionModel.selectedIndex would even change during the new ArrayDataModel() call, but maybe the side effect of setting .leadIndex is the real purpose (although this is strange too, since the .selectedIndex setter takes care of .leadIndex.) Maybe the whole method can go away? If someone can confirm this understanding, I could clean it up more.
https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_list.js (left): https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_list.js:407: this.selectionModel.selectedIndex = 0; On 2016/01/22 15:12:50, Kevin Bailey wrote: > While looking around, trying to figure out how storage of language order > preference worked, I spotted this. It uses a null .selectionModel field on line > 407. > > I suspect that it's not a problem because .selectionModel is always set (if > ::decorate() is always called) but I assume we still want to fix it. > > It's also unclear to me how .selectionModel.selectedIndex would even change > during the new ArrayDataModel() call, but maybe the side effect of setting > .leadIndex is the real purpose (although this is strange too, since the > .selectedIndex setter takes care of .leadIndex.) Maybe the whole method can go > away? > > If someone can confirm this understanding, I could clean it up more. I defer to Dan as the resident JS expert.
https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_list.js (left): https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_list.js:406: // automatically, hence we manually select the first item here. this is why selectedIndex starts as -1: https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... (which makes sense because the selection model is likely started without a 0th item) https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_list.js:407: this.selectionModel.selectedIndex = 0; On 2016/01/22 15:12:50, Kevin Bailey wrote: > While looking around, trying to figure out how storage of language order > preference worked, I spotted this. It uses a null .selectionModel field on line > 407. this is where selectionModel is created (on page load an existing DOM node is "decorated"): https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... you would only hit a null pointer exception here if load_ is called before the page is ready (which seems unlikely but possible). > > I suspect that it's not a problem because .selectionModel is always set (if > ::decorate() is always called) but I assume we still want to fix it. generally we should defer or drop actions if the item is not yet decorated. most of the code that would require the element to be decorated... runs at the same time, though, which is why we don't typically hit issues. > > It's also unclear to me how .selectionModel.selectedIndex would even change > during the new ArrayDataModel() call, but maybe the side effect of setting > .leadIndex is the real purpose (although this is strange too, since the > .selectedIndex setter takes care of .leadIndex.) this.dataModel = ... actually modifies this.selectionModel as a side effect (in some cases): https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > Maybe the whole method can go away? probably not > > If someone can confirm this understanding, I could clean it up more. i can only confirm we don't have full understanding ;) https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_list.js:398: // valid after the data model is loaded. This is neeeded to keep needed
https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/1617243004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_list.js:398: // valid after the data model is loaded. This is neeeded to keep On 2016/01/25 23:27:16, Dan Beam wrote: > needed Done. https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:391: if (!this.selectionModel) Sorry, it was unclear to me what you preferred. Something like this? (If this.selectionModel is getting set when this.dataModel is set, then I'm simply not seeing it.)
please file a bug with repro steps as to how to trigger load_ for an undecorated LanguageList https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/language_list.js (left): https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:394: this.dataModel = new ArrayDataModel(languageCodes); doing this.dataModel = ...; triggers an ES5 setter in an ancestor class that calls: this.selectionModel.clear(); how does that succeed? does it also need to be updated? https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:407: this.selectionModel.selectedIndex = 0; revert this now https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:391: if (!this.selectionModel) On 2016/01/26 15:00:40, Kevin Bailey wrote: > Sorry, it was unclear to me what you preferred. Something like this? > > (If this.selectionModel is getting set when this.dataModel is set, then I'm > simply not seeing it.) I would prefer load_ only get called after being decorated, but if you stick with this: if (!this.selectionModel) { // Wait until decorate() is called. return; }
I don't know of a way to reproduce ::load_() getting called first. I simply noticed that the code checked for null, and then proceeded to use the reference anyways, so I thought it should get a little defensive programming. https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/1617243004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:391: if (!this.selectionModel) On 2016/01/26 19:38:53, Dan Beam wrote: > On 2016/01/26 15:00:40, Kevin Bailey wrote: > > Sorry, it was unclear to me what you preferred. Something like this? > > > > (If this.selectionModel is getting set when this.dataModel is set, then I'm > > simply not seeing it.) > > I would prefer load_ only get called after being decorated, but if you stick > with this: > > if (!this.selectionModel) { > // Wait until decorate() is called. > return; > } Done.
On 2016/01/26 22:21:46, Kevin Bailey wrote: > I don't know of a way to reproduce ::load_() getting called first. I simply > noticed that the code checked for null, and then proceeded to use the reference > anyways, so I thought it should get a little defensive programming. If you don't understand why it's null, and can't actually reproduce it happening, I would leave the code alone. It's just as likely that it's never null and your check is dead code.
On 2016/01/26 23:05:19, Dan Beam wrote: > ... It's just as likely that it's never > null and your check is dead code. I didn't add the check. If you'll look on line 392 of the original code, you'll see that it's already checking. All I'm trying to do is prevent it from _using_ the pointer in the case that it's null. I fairly sure you're right that it never happens, so maybe the best thing to do is just add an assertion. Anyways, I'm happy to chat about it if I'm still not being clear.
this code was added here: https://codereview.chromium.org/2878079/diff/1/chrome/browser/resources/optio... it doesn't seem like it was necessary to null-check this.selectionModel for any reason that I can tell, so I'm pretty sure you can just delete the ternary
Cool, done.
Hi Dan, If you're still ok with this CL, can you approve? thanks! krb
https://codereview.chromium.org/1617243004/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/language_list.js (left): https://codereview.chromium.org/1617243004/diff/80001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:407: this.selectionModel.selectedIndex = 0; why are you removing this?
https://codereview.chromium.org/1617243004/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/language_list.js (left): https://codereview.chromium.org/1617243004/diff/80001/chrome/browser/resource... chrome/browser/resources/options/language_list.js:407: this.selectionModel.selectedIndex = 0; On 2016/02/09 19:14:47, Dan Beam wrote: > why are you removing this? Sorry, left-over from a misunderstanding. Restored.
lgtm, but can you update CL description?
Description was changed from ========== Stop using null selectionModel field. BUG= ========== to ========== Stop checking for null selectionModel field. It's apparent that the field is never null in this method, and if it were, we'd have to change much more. So make the code a little more clear and remove the test. BUG= ==========
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617243004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617243004/100001
Message was sent while issue was closed.
Description was changed from ========== Stop checking for null selectionModel field. It's apparent that the field is never null in this method, and if it were, we'd have to change much more. So make the code a little more clear and remove the test. BUG= ========== to ========== Stop checking for null selectionModel field. It's apparent that the field is never null in this method, and if it were, we'd have to change much more. So make the code a little more clear and remove the test. BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Stop checking for null selectionModel field. It's apparent that the field is never null in this method, and if it were, we'd have to change much more. So make the code a little more clear and remove the test. BUG= ========== to ========== Stop checking for null selectionModel field. It's apparent that the field is never null in this method, and if it were, we'd have to change much more. So make the code a little more clear and remove the test. BUG= Committed: https://crrev.com/711c45a18e07b784e873225f469efe1019c1a41c Cr-Commit-Position: refs/heads/master@{#374658} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/711c45a18e07b784e873225f469efe1019c1a41c Cr-Commit-Position: refs/heads/master@{#374658} |
