|
|
Created:
5 years, 8 months ago by Yuki Modified:
5 years, 7 months ago Reviewers:
arv (Not doing code reviews) CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, arv (Not doing code reviews), Evan Stade Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes wrong inheritance of PrefSelect.
PrefSelect must inherit from HTMLSelectElement, otherwise it will be
broken when the DOM attributes are moved to prototype chains.
Note that HTMLSelectElement does NOT inherit from HTMLInputElement, so
it's simply wrong that PrefSelect inherits from HTMLInputElement.
BUG=43394
Committed: https://crrev.com/ac0bacffa7b4f25a09abaa082a32abb599d6e318
Cr-Commit-Position: refs/heads/master@{#327023}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review comments. #Messages
Total messages: 18 (6 generated)
yukishiino@chromium.org changed reviewers: + dbeam@chromium.org
Could you review this CL?
I am not sure I'm the best reviewer for this. +jhawkins, estade, arv for their opinions https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/pref_ui.js (right): https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/pref_ui.js:393: __proto__: HTMLSelectElement.prototype, keep in mind that settings is being rewritten actively i think it's better to do one of two things: 1) make an abstract class that is concretely derived into Pref{Input,Select}Element 2) make a concrete base class that has-an element (rather than is-an element) both of these sound mildly messy and slightly error prone (but at least we have typechecking now)
On 2015/04/20 15:06:27, Dan Beam wrote: > I am not sure I'm the best reviewer for this. > > +jhawkins, estade, arv for their opinions > I defer to arv on this one, as he's been most involved in the discussions leading up to the need for this CL (on 43394).
On 2015/04/20 15:20:09, James Hawkins wrote: > On 2015/04/20 15:06:27, Dan Beam wrote: > > I am not sure I'm the best reviewer for this. > > > > +jhawkins, estade, arv for their opinions > > > > I defer to arv on this one, as he's been most involved in the discussions > leading up to the need for this CL (on 43394). Let me clarify my goal. My purpose on this CL is fix Blink's DOM attribute implementation (i.e. move DOM attributes to prototype chains in Blink). I'd like to defer it to you guys to bring the best design to cr.ui. My only purpose here is simply fix the broken inheritance. Please refactor cr.ui later for yourselves if needed.
dbeam@chromium.org changed reviewers: + arv@chromium.org - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
-dbeam, +arv
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
Friendly ping? arv@, could you review this CL?
LGTM Since Dan said they are rewriting prefs I don't think you need to refactor prefs to make this better at this point. https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/pref_ui.js (right): https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/pref_ui.js:393: __proto__: HTMLSelectElement.prototype, On 2015/04/20 15:06:26, Dan Beam wrote: > keep in mind that settings is being rewritten actively > > i think it's better to do one of two things: > 1) make an abstract class that is concretely derived into > Pref{Input,Select}Element > 2) make a concrete base class that has-an element (rather than is-an element) > > both of these sound mildly messy and slightly error prone (but at least we have > typechecking now) With ES6 you can do traits. function PrefTrait(superClass) { return class extends superClass { handleChange(e) { ... } }; } class PrefSelect extends PrefTrait(HTMLSelectElement) { ... } class PrefInputElement extends PrefTrait(HTMLInputElement) { ... } Of course you can desugar that to ES5 if you want. https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/pref_ui.js:473: * Custom change handler that is invoked first when the user makes changes Are these comments just copied from PrefInputElement? If so remove them and add something like "see PrefInputElement".
https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/pref_ui.js (right): https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/pref_ui.js:473: * Custom change handler that is invoked first when the user makes changes On 2015/04/24 14:22:45, arv wrote: > Are these comments just copied from PrefInputElement? If so remove them and add > something like "see PrefInputElement". or [just] @override
https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/pref_ui.js (right): https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/pref_ui.js:393: __proto__: HTMLSelectElement.prototype, On 2015/04/24 14:22:45, arv wrote: > On 2015/04/20 15:06:26, Dan Beam wrote: > > keep in mind that settings is being rewritten actively > > > > i think it's better to do one of two things: > > 1) make an abstract class that is concretely derived into > > Pref{Input,Select}Element > > 2) make a concrete base class that has-an element (rather than is-an element) > > > > both of these sound mildly messy and slightly error prone (but at least we > have > > typechecking now) > > With ES6 you can do traits. > > function PrefTrait(superClass) { > return class extends superClass { > handleChange(e) { ... } > }; > } > > class PrefSelect extends PrefTrait(HTMLSelectElement) { > ... > } > > class PrefInputElement extends PrefTrait(HTMLInputElement) { > ... > } > > Of course you can desugar that to ES5 if you want. Looks cool to me. I defer this re-design task to the maintainers (code owners). https://codereview.chromium.org/1061263004/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/pref_ui.js:473: * Custom change handler that is invoked first when the user makes changes On 2015/04/24 21:48:29, Dan Beam wrote: > On 2015/04/24 14:22:45, arv wrote: > > Are these comments just copied from PrefInputElement? If so remove them and > add > > something like "see PrefInputElement". > > or [just] @override Done.
The CQ bit was checked by yukishiino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1061263004/#ps20001 (title: "Addressed review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1061263004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ac0bacffa7b4f25a09abaa082a32abb599d6e318 Cr-Commit-Position: refs/heads/master@{#327023} |