Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
566063002:
Compile chrome://settings, part 8: the final battle (Closed)
|
Created:
6 years, 3 months ago by Vitaly Pavlenko Modified:
6 years, 2 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, nkostylev+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, stevenjb+watch_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@H_options_errors_6 Project:
chromium Visibility:
Public. |
DescriptionCompile chrome://settings, part 8: the final battle
R=dbeam@chromium.org
BUG=393873
TEST=GYP_GENERATORS=ninja gyp --depth . chrome/browser/resources/options/compiled_resources.gyp && ninja -C out/Default | grep ERROR | wc -l
Committed: https://crrev.com/7abe92adb4c607460d284e95c3df21d3a7f86520
Cr-Commit-Position: refs/heads/master@{#297461}
Patch Set 1 #
Total comments: 23
Patch Set 2 : fixed comments #
Total comments: 2
Patch Set 3 : fixed one nit #
Total comments: 7
Patch Set 4 : instanceof #Patch Set 5 : nit #Patch Set 6 : suppress #
Total comments: 1
Patch Set 7 : rebase #Messages
Total messages: 21 (2 generated)
https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/browser_options.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/browser_options.js:1758: * proxy: {id: string, name: string}}} details A dictionary of @typedef for {id: string, name: string}? https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/chromeos/display_overscan.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/chromeos/display_overscan.js:28: * @private nit: @private {?string} https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/handler_options_list.js (left): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/handler_options_list.js:182: delegate.removeHandler(value, data.handlers[i]); did this not work before? https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/manage_profile_overlay.js:780: * @param {boolean} hasError ^ remove https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/manage_profile_overlay.js:784: this.updateSignedInStatus_(email, hasError); uh, wat? https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) why? so they keys aren't changed during compilation?
https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/browser_options.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/browser_options.js:1758: * proxy: {id: string, name: string}}} details A dictionary of On 2014/09/12 23:36:12, Dan Beam wrote: > @typedef for {id: string, name: string}? Done. https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/chromeos/display_overscan.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/chromeos/display_overscan.js:28: * @private On 2014/09/12 23:36:12, Dan Beam wrote: > nit: @private {?string} Done. https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/handler_options_list.js (left): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/handler_options_list.js:182: delegate.removeHandler(value, data.handlers[i]); On 2014/09/12 23:36:12, Dan Beam wrote: > did this not work before? See recent discussion: https://codereview.chromium.org/518673003/ https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/manage_profile_overlay.js:780: * @param {boolean} hasError On 2014/09/12 23:36:12, Dan Beam wrote: > ^ remove Done. https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/manage_profile_overlay.js:784: this.updateSignedInStatus_(email, hasError); On 2014/09/12 23:36:12, Dan Beam wrote: > uh, wat? So, updateSignedInStatus is defined only in subclass, not in base class. Yet it is called in base class: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Yet it is exported to public API from subclass, so it should be defined as private in subclass. But then if it's private subclass method, there's no other chance for me to call it from base class methods other than create a method wrapper with different visibility. https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/12 23:36:12, Dan Beam wrote: > why? so they keys aren't changed during compilation? .menu key is not defined on EventTarget, says the compiler. And these checks are actually trying to understand the narrow type of e.target. Maybe replace is with instanceof check?
https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/manage_profile_overlay.js:784: this.updateSignedInStatus_(email, hasError); On 2014/09/13 00:02:36, Vitaly Pavlenko wrote: > On 2014/09/12 23:36:12, Dan Beam wrote: > > uh, wat? > > So, updateSignedInStatus is defined only in subclass, not in base class. Yet it > is called in base class: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > Yet it is exported to public API from subclass, so it should be defined as > private in subclass. But then if it's private subclass method, there's no other > chance for me to call it from base class methods other than create a method > wrapper with different visibility. why can't we just update cr.makePublic() to look for obj[name] || obj[name + '_'] and verify it's a function? bad idea? https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:216: doc.addEventListener('focus', this.handleFocus_.bind(this), true); i think this would only get triggered if the document itself gets focused, and i think that never happens. it's probably either supposed to be focusin or we should just remove... https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > On 2014/09/12 23:36:12, Dan Beam wrote: > > why? so they keys aren't changed during compilation? > > .menu key is not defined on EventTarget, says the compiler. And these checks are > actually trying to understand the narrow type of e.target. Maybe replace is with > instanceof check? are you sure we need to bother with this at all? I don't think focus bubbles so this is probably dead code. https://chromiumcodereview.appspot.com/566063002/diff/20001/chrome/browser/re... File chrome/browser/resources/options/browser_options.js (right): https://chromiumcodereview.appspot.com/566063002/diff/20001/chrome/browser/re... chrome/browser/resources/options/browser_options.js:25: * name: string}} nit: /** @typedef {{id: string, name: string}} */
https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... File chrome/browser/resources/options/manage_profile_overlay.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... chrome/browser/resources/options/manage_profile_overlay.js:784: this.updateSignedInStatus_(email, hasError); On 2014/09/16 01:19:10, Dan Beam wrote: > On 2014/09/13 00:02:36, Vitaly Pavlenko wrote: > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > uh, wat? > > > > So, updateSignedInStatus is defined only in subclass, not in base class. Yet > it > > is called in base class: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > > Yet it is exported to public API from subclass, so it should be defined as > > private in subclass. But then if it's private subclass method, there's no > other > > chance for me to call it from base class methods other than create a method > > wrapper with different visibility. > > why can't we just update cr.makePublic() to look for > > obj[name] || obj[name + '_'] > > and verify it's a function? bad idea? We could, but it will be used only in one place, here. We still need change this JS file, so why not do all changes only here? Moreover, it will make compiler pass doing more complex and magic things which we gonna explain to people on final presentation. Do we want to? With that in mind, make a decision, please. https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:216: doc.addEventListener('focus', this.handleFocus_.bind(this), true); On 2014/09/16 01:19:10, Dan Beam wrote: > i think this would only get triggered if the document itself gets focused, and i > think that never happens. it's probably either supposed to be focusin or we > should just remove... After searching a while for document.addEventListener('focus') I think this handler it for capturing focus for inner page elements during capturing phase of event dispatching: http://jsfiddle.net/f9rq3dow/6/ Do we need to try to remove it? https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/16 01:19:10, Dan Beam wrote: > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > why? so they keys aren't changed during compilation? > > > > .menu key is not defined on EventTarget, says the compiler. And these checks > are > > actually trying to understand the narrow type of e.target. Maybe replace is > with > > instanceof check? > > are you sure we need to bother with this at all? I don't think focus bubbles so > this is probably dead code. See my answer above. https://chromiumcodereview.appspot.com/566063002/diff/20001/chrome/browser/re... File chrome/browser/resources/options/browser_options.js (right): https://chromiumcodereview.appspot.com/566063002/diff/20001/chrome/browser/re... chrome/browser/resources/options/browser_options.js:25: * name: string}} On 2014/09/16 01:19:10, Dan Beam wrote: > nit: /** @typedef {{id: string, name: string}} */ Done.
On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... > File chrome/browser/resources/options/manage_profile_overlay.js (right): > > https://chromiumcodereview.appspot.com/566063002/diff/1/chrome/browser/resour... > chrome/browser/resources/options/manage_profile_overlay.js:784: > this.updateSignedInStatus_(email, hasError); > On 2014/09/16 01:19:10, Dan Beam wrote: > > On 2014/09/13 00:02:36, Vitaly Pavlenko wrote: > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > uh, wat? > > > > > > So, updateSignedInStatus is defined only in subclass, not in base class. Yet > > it > > > is called in base class: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > > > > Yet it is exported to public API from subclass, so it should be defined as > > > private in subclass. But then if it's private subclass method, there's no > > other > > > chance for me to call it from base class methods other than create a method > > > wrapper with different visibility. > > > > why can't we just update cr.makePublic() to look for > > > > obj[name] || obj[name + '_'] > > > > and verify it's a function? bad idea? > > We could, but it will be used only in one place, here. We still need change this > JS file, so why not do all changes only here? > > Moreover, it will make compiler pass doing more complex and magic things which > we gonna explain to people on final presentation. Do we want to? > > With that in mind, make a decision, please. > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > File ui/webui/resources/js/cr/ui/command.js (right): > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > ui/webui/resources/js/cr/ui/command.js:216: doc.addEventListener('focus', > this.handleFocus_.bind(this), true); > On 2014/09/16 01:19:10, Dan Beam wrote: > > i think this would only get triggered if the document itself gets focused, and > i > > think that never happens. it's probably either supposed to be focusin or we > > should just remove... > > After searching a while for document.addEventListener('focus') I think this > handler it for capturing focus for inner page elements during capturing phase of > event dispatching: > > http://jsfiddle.net/f9rq3dow/6/ > > Do we need to try to remove it? > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in > target) > On 2014/09/16 01:19:10, Dan Beam wrote: > > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > why? so they keys aren't changed during compilation? > > > > > > .menu key is not defined on EventTarget, says the compiler. And these checks > > are > > > actually trying to understand the narrow type of e.target. Maybe replace is > > with > > > instanceof check? > > > > are you sure we need to bother with this at all? I don't think focus bubbles > so > > this is probably dead code. > > See my answer above. > > https://chromiumcodereview.appspot.com/566063002/diff/20001/chrome/browser/re... > File chrome/browser/resources/options/browser_options.js (right): > > https://chromiumcodereview.appspot.com/566063002/diff/20001/chrome/browser/re... > chrome/browser/resources/options/browser_options.js:25: * name: > string}} > On 2014/09/16 01:19:10, Dan Beam wrote: > > nit: /** @typedef {{id: string, name: string}} */ > > Done. ping dbeam@
https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:216: doc.addEventListener('focus', this.handleFocus_.bind(this), true); On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > On 2014/09/16 01:19:10, Dan Beam wrote: > > i think this would only get triggered if the document itself gets focused, and > i > > think that never happens. it's probably either supposed to be focusin or we > > should just remove... > > After searching a while for document.addEventListener('focus') I think this > handler it for capturing focus for inner page elements during capturing phase of > event dispatching: > > http://jsfiddle.net/f9rq3dow/6/ > > Do we need to try to remove it? no, leave https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > On 2014/09/16 01:19:10, Dan Beam wrote: > > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > why? so they keys aren't changed during compilation? > > > > > > .menu key is not defined on EventTarget, says the compiler. And these checks > > are > > > actually trying to understand the narrow type of e.target. Maybe replace is > > with > > > instanceof check? the problem with instanceof is that it doesn't work across frames. that might be OK, but checking for random properties isn't a great way in a compiled world. > > > > are you sure we need to bother with this at all? I don't think focus bubbles > so > > this is probably dead code. > > See my answer above.
https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:216: doc.addEventListener('focus', this.handleFocus_.bind(this), true); On 2014/09/19 01:05:55, Dan Beam wrote: > On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > > On 2014/09/16 01:19:10, Dan Beam wrote: > > > i think this would only get triggered if the document itself gets focused, > and > > i > > > think that never happens. it's probably either supposed to be focusin or we > > > should just remove... > > > > After searching a while for document.addEventListener('focus') I think this > > handler it for capturing focus for inner page elements during capturing phase > of > > event dispatching: > > > > http://jsfiddle.net/f9rq3dow/6/ > > > > Do we need to try to remove it? > > no, leave Acknowledged. https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/19 01:05:55, Dan Beam wrote: > On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > > On 2014/09/16 01:19:10, Dan Beam wrote: > > > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > > why? so they keys aren't changed during compilation? > > > > > > > > .menu key is not defined on EventTarget, says the compiler. And these > checks > > > are > > > > actually trying to understand the narrow type of e.target. Maybe replace > is > > > with > > > > instanceof check? > > the problem with instanceof is that it doesn't work across frames. that might > be OK, but checking for random properties isn't a great way in a compiled world. > > > > > > > are you sure we need to bother with this at all? I don't think focus > bubbles > > so > > > this is probably dead code. > > > > See my answer above. So what do you think we should do with that?
On 2014/09/19 01:36:26, Vitaly Pavlenko wrote: > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > File ui/webui/resources/js/cr/ui/command.js (right): > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > ui/webui/resources/js/cr/ui/command.js:216: doc.addEventListener('focus', > this.handleFocus_.bind(this), true); > On 2014/09/19 01:05:55, Dan Beam wrote: > > On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > > > On 2014/09/16 01:19:10, Dan Beam wrote: > > > > i think this would only get triggered if the document itself gets focused, > > and > > > i > > > > think that never happens. it's probably either supposed to be focusin or > we > > > > should just remove... > > > > > > After searching a while for document.addEventListener('focus') I think this > > > handler it for capturing focus for inner page elements during capturing > phase > > of > > > event dispatching: > > > > > > http://jsfiddle.net/f9rq3dow/6/ > > > > > > Do we need to try to remove it? > > > > no, leave > > Acknowledged. > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in > target) > On 2014/09/19 01:05:55, Dan Beam wrote: > > On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > > > On 2014/09/16 01:19:10, Dan Beam wrote: > > > > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > > > why? so they keys aren't changed during compilation? > > > > > > > > > > .menu key is not defined on EventTarget, says the compiler. And these > > checks > > > > are > > > > > actually trying to understand the narrow type of e.target. Maybe replace > > is > > > > with > > > > > instanceof check? > > > > the problem with instanceof is that it doesn't work across frames. that might > > be OK, but checking for random properties isn't a great way in a compiled > world. > > > > > > > > > > are you sure we need to bother with this at all? I don't think focus > > bubbles > > > so > > > > this is probably dead code. > > > > > > See my answer above. > > So what do you think we should do with that? ping dbeam@
dbeam@chromium.org changed reviewers: + arv@chromium.org
+arv@ for thoughts https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/19 01:36:25, Vitaly Pavlenko wrote: > On 2014/09/19 01:05:55, Dan Beam wrote: > > On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > > > On 2014/09/16 01:19:10, Dan Beam wrote: > > > > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > > > why? so they keys aren't changed during compilation? > > > > > > > > > > .menu key is not defined on EventTarget, says the compiler. And these > > checks > > > > are > > > > > actually trying to understand the narrow type of e.target. Maybe replace > > is > > > > with > > > > > instanceof check? > > > > the problem with instanceof is that it doesn't work across frames. that might > > be OK, but checking for random properties isn't a great way in a compiled > world. > > > > > > > > > > are you sure we need to bother with this at all? I don't think focus > > bubbles > > > so > > > > this is probably dead code. > > > > > > See my answer above. > > So what do you think we should do with that? this is code is funky to start with. i don't know how a command would get focused (are these ever UI elements?). as for menus: we can probably handle it in the capturing phase and either stop the propagation or prevent the default (and ignore e.preventDefaulted == true). +arv@: any thoughts?
On 2014/09/23 02:19:53, Dan Beam wrote: > +arv@ for thoughts > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > File ui/webui/resources/js/cr/ui/command.js (right): > > https://chromiumcodereview.appspot.com/566063002/diff/1/ui/webui/resources/js... > ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in > target) > On 2014/09/19 01:36:25, Vitaly Pavlenko wrote: > > On 2014/09/19 01:05:55, Dan Beam wrote: > > > On 2014/09/16 03:36:55, Vitaly Pavlenko wrote: > > > > On 2014/09/16 01:19:10, Dan Beam wrote: > > > > > On 2014/09/13 00:02:37, Vitaly Pavlenko wrote: > > > > > > On 2014/09/12 23:36:12, Dan Beam wrote: > > > > > > > why? so they keys aren't changed during compilation? > > > > > > > > > > > > .menu key is not defined on EventTarget, says the compiler. And these > > > checks > > > > > are > > > > > > actually trying to understand the narrow type of e.target. Maybe > replace > > > is > > > > > with > > > > > > instanceof check? > > > > > > the problem with instanceof is that it doesn't work across frames. that > might > > > be OK, but checking for random properties isn't a great way in a compiled > > world. > > > > > > > > > > > > > are you sure we need to bother with this at all? I don't think focus > > > bubbles > > > > so > > > > > this is probably dead code. > > > > > > > > See my answer above. > > > > So what do you think we should do with that? > > this is code is funky to start with. i don't know how a command would get > focused (are these ever UI elements?). > > as for menus: we can probably handle it in the capturing phase and either stop > the propagation or prevent the default (and ignore e.preventDefaulted == true). > > +arv@: any thoughts? ping arv@, ping dbeam@
https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/onc_data.js (right): https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/onc_data.js:157: assert(typeof source == 'string'); I'm not sure we want to add runtime asserts for these things? Can we make getActiveValue return {undefined|string} instead? https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/handler_options_list.js (right): https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/handler_options_list.js:182: delegate.removeHandler(i, data.handlers[i]); Scary. Is this dead code? https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/command.js (right): https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) Why is this needed? This is bad for performance and goes against good coding practices.
https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/handler_options_list.js (right): https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/handler_options_list.js:182: delegate.removeHandler(i, data.handlers[i]); On 2014/09/29 22:46:27, arv wrote: > Scary. Is this dead code? https://codereview.chromium.org/518673003/ https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/command.js (right): https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/29 22:46:27, arv wrote: > Why is this needed? This is bad for performance and goes against good coding > practices. i wouldn't say the original code is optimal either :(. it was added here: https://chromiumcodereview.appspot.com/12189017/diff/11001/ui/webui/resources... there must be some better way to do this via stopping the propagation or asking the target whether focus should be handled...
https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... File chrome/browser/resources/options/chromeos/onc_data.js (right): https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... chrome/browser/resources/options/chromeos/onc_data.js:157: assert(typeof source == 'string'); On 2014/09/29 22:46:27, arv wrote: > I'm not sure we want to add runtime asserts for these things? Can we make > getActiveValue return {undefined|string} instead? getActiveValue sometimes returns arrays: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... and sometimes - booleans: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... File ui/webui/resources/js/cr/ui/command.js (right): https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in target) On 2014/09/29 23:00:15, Dan Beam wrote: > On 2014/09/29 22:46:27, arv wrote: > > Why is this needed? This is bad for performance and goes against good coding > > practices. > > i wouldn't say the original code is optimal either :(. it was added here: > https://chromiumcodereview.appspot.com/12189017/diff/11001/ui/webui/resources... > > there must be some better way to do this via stopping the propagation or asking > the target whether focus should be handled... I'm going to try instanceof approach on try bots.
On 2014/09/30 03:52:02, Vitaly Pavlenko (last week) wrote: > https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... > File chrome/browser/resources/options/chromeos/onc_data.js (right): > > https://codereview.chromium.org/566063002/diff/40001/chrome/browser/resources... > chrome/browser/resources/options/chromeos/onc_data.js:157: assert(typeof source > == 'string'); > On 2014/09/29 22:46:27, arv wrote: > > I'm not sure we want to add runtime asserts for these things? Can we make > > getActiveValue return {undefined|string} instead? > > getActiveValue sometimes returns arrays: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > and sometimes - booleans: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... > File ui/webui/resources/js/cr/ui/command.js (right): > > https://codereview.chromium.org/566063002/diff/40001/ui/webui/resources/js/cr... > ui/webui/resources/js/cr/ui/command.js:244: if ('menu' in target || 'command' in > target) > On 2014/09/29 23:00:15, Dan Beam wrote: > > On 2014/09/29 22:46:27, arv wrote: > > > Why is this needed? This is bad for performance and goes against good coding > > > practices. > > > > i wouldn't say the original code is optimal either :(. it was added here: > > > https://chromiumcodereview.appspot.com/12189017/diff/11001/ui/webui/resources... > > > > there must be some better way to do this via stopping the propagation or > asking > > the target whether focus should be handled... > > I'm going to try instanceof approach on try bots. Well, not going: try bots don't work for me. I'm going to suppress typechecking on that method and revert it to its original state. Is it ok?
lgtm https://chromiumcodereview.appspot.com/566063002/diff/100001/ui/webui/resourc... File ui/webui/resources/js/cr/ui/command.js (right): https://chromiumcodereview.appspot.com/566063002/diff/100001/ui/webui/resourc... ui/webui/resources/js/cr/ui/command.js:240: * TODO(vitalyp): remove the suppression. this is fine by me
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/566063002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as f99b00f208532ff816f85d8864474f018ac887be
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7abe92adb4c607460d284e95c3df21d3a7f86520 Cr-Commit-Position: refs/heads/master@{#297461} |
