Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
328763003:
Added usefull utility methods to Screen and ScreenContext. (Closed)
|
Created:
6 years, 6 months ago by dzhioev (left Google) Modified:
6 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded usefull utility methods to Screen and ScreenContext.
List of changes:
* Added ability to annotate HTML elements (see login.Screen.initialize method comment).
* Prefix is added to messages sent from JS to C++ (login.Screen.send method).
* Added ability to observe changes in specific keys of context (on JS side).
* And more.
BUG=NONE
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278724
Patch Set 1 #
Total comments: 35
Patch Set 2 : Renamed "button clicked" to "user acted". #Patch Set 3 : Comments addressed. #Patch Set 4 : s/is/alias/ #Patch Set 5 : Comments addressed. #
Messages
Total messages: 17 (0 generated)
Hi, PTAL. CL that demonstrates usage of new features http://codereview.chromium.org/326933004
https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/screen_context.js (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:23: function checkValueIsValid(value, t) { nit: from the function definition and usage it's not obvious what's the purpose of the |t| argument. Could you please add a comment to this method? https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:30: this.screen_ = screen; Cound you please point where |screen_| is used? https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:131: nit: drop empty line and trailing comma. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... File chrome/browser/resources/login/screen.js (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:89: makeMagic: function() { Don't see how this method is used in https://codereview.chromium.org/326933004/. In any case, we need to devise a better name than "makeMagic". https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:156: contextChanged_: function(diff) { nit: add @private annotation. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:160: getPropertyNameOf_: function(value) { nit: add comment to this function + @private annotation. https://codereview.chromium.org/328763003/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:69: LOG(ERROR) << "Returning false"; nit: debug code?
https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/chrome... File chrome/browser/chromeos/login/screens/screen_context.h (right): https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screens/screen_context.h:74: // are stored in |keys|. |keys| is optional and can be NULL. "|Keys| are" ? https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/login/screen_context.js (right): https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/login/screen_context.js:23: function checkValueIsValid(value, t) { On 2014/06/11 10:44:00, ygorshenin1 wrote: > nit: from the function definition and usage it's not obvious what's the purpose > of the |t| argument. Could you please add a comment to this method? Also, guide recommends to avoid abbreviations for parameter names. https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/login/screen_context.js:58: checkValueIsValid(value, 2); It is not obvious what "2" means. Could you use some named constant instead? https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/login/screen_context.js:83: checkValueIsValid(changes[key], 1); What is the "1"? https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/login/screen_context.js:107: observe: function(key, observer) { subscribe/unsubscribe? "Observe" is the local action. https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... File chrome/browser/resources/login/screen.js (right): https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/login/screen.js:32: * Dictionary of context observers that are methods of |this| binded to s/binded/bound/ https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/login/screen.js:89: makeMagic: function() { On 2014/06/11 10:44:00, ygorshenin1 wrote: > Don't see how this method is used in https://codereview.chromium.org/326933004/. > In any case, we need to devise a better name than "makeMagic". I agree about naming. I'm also concerned about using one more non-standard element atttribute here, especially with such short and non-noun name. I'd better stick with identifying div's/buttons by id, and having some "wire" function that would take id, and do the stuff. While it will require few more lines in screen initialization, everything will be explicit (bugs caused by implicit behavior are much harder to detect/trace/fix). https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/login/screen.js:116: observeContext: function(key, observer) { subscribeForKeyChange / watchKeyChange?
https://codereview.chromium.org/328763003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/screen_context.h (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/screen_context.h:74: // are stored in |keys|. |keys| is optional and can be NULL. On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > "|Keys| are" ? Rephrased to "|keys| argument is optional". https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/screen_context.js (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:23: function checkValueIsValid(value, t) { On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > nit: from the function definition and usage it's not obvious what's the > purpose > > of the |t| argument. Could you please add a comment to this method? > Also, guide recommends to avoid abbreviations for parameter names. It is a garbage that I forgot to remove. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:30: this.screen_ = screen; On 2014/06/11 10:44:00, ygorshenin1 wrote: > Cound you please point where |screen_| is used? It is not used anymore. I forgot to remove it. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:58: checkValueIsValid(value, 2); On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > It is not obvious what "2" means. > Could you use some named constant instead? Removed. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:83: checkValueIsValid(changes[key], 1); On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > What is the "1"? Removed. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:107: observe: function(key, observer) { On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > subscribe/unsubscribe? > "Observe" is the local action. I used same wording as new JS API use ( http://www.html5rocks.com/en/tutorials/es7/observe ). I can rename to subscribe/unsubscribe if you insist. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:131: On 2014/06/11 10:44:00, ygorshenin1 wrote: > nit: drop empty line and trailing comma. Done. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... File chrome/browser/resources/login/screen.js (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:89: makeMagic: function() { On 2014/06/11 10:44:00, ygorshenin1 wrote: > Don't see how this method is used in https://codereview.chromium.org/326933004/. > In any case, we need to devise a better name than "makeMagic". It is called in |decorate| method of new screen. I'll rename it. Note: in the future this method will be called automatically for every registered screen. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:89: makeMagic: function() { On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > Don't see how this method is used in > https://codereview.chromium.org/326933004/. > > In any case, we need to devise a better name than "makeMagic". > I agree about naming. > I'm also concerned about using one more non-standard element atttribute here, > especially with such short and non-noun name. > I'd better stick with identifying div's/buttons by id, and having some "wire" > function that would take id, and do the stuff. > While it will require few more lines in screen initialization, everything will > be explicit (bugs caused by implicit behavior are much harder to > detect/trace/fix). * The main reason why I don't want use "id"s is that "id"s should be unique. This restriction makes us to use long identifiers, like |id="locally-managed-user-creation-some-label"|. For custom attribute we don't have such restriction so we can use short name, like |is="someLabel_"| and don't afraid of name conflicts with other screens. * Using "id" weakens encapsulation, because it is easy to write something like |$("element-from-another-screen").hidden = true| in any place outside screen. * The main purpose of Screen and ScreenContext is to get rid of boilerplate code, and I think this explicit initialization is a good place to start. Moreover I think new method is less error prone. * IMHO, the "is" name perfectly describes purpose of this attribute, and I'm glad that it is so short. Can you explain what is so bad about short names? https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:116: observeContext: function(key, observer) { On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > subscribeForKeyChange / watchKeyChange? > See comment to ScreenContext.observe https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:156: contextChanged_: function(diff) { On 2014/06/11 10:44:00, ygorshenin1 wrote: > nit: add @private annotation. Done. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:160: getPropertyNameOf_: function(value) { On 2014/06/11 10:44:00, ygorshenin1 wrote: > nit: add comment to this function + @private annotation. Done. https://codereview.chromium.org/328763003/diff/1/chrome/browser/ui/webui/chro... File chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/ui/webui/chro... chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:69: LOG(ERROR) << "Returning false"; On 2014/06/11 10:44:00, ygorshenin1 wrote: > nit: debug code? Yes. Removed.
On 2014/06/18 14:43:39, dzhioev wrote: > https://codereview.chromium.org/328763003/diff/1/chrome/browser/chromeos/logi... > File chrome/browser/chromeos/login/screens/screen_context.h (right): > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/chromeos/logi... > chrome/browser/chromeos/login/screens/screen_context.h:74: // are stored in > |keys|. |keys| is optional and can be NULL. > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > "|Keys| are" ? > > Rephrased to "|keys| argument is optional". > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > File chrome/browser/resources/chromeos/login/screen_context.js (right): > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/login/screen_context.js:23: function > checkValueIsValid(value, t) { > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > > nit: from the function definition and usage it's not obvious what's the > > purpose > > > of the |t| argument. Could you please add a comment to this method? > > Also, guide recommends to avoid abbreviations for parameter names. > It is a garbage that I forgot to remove. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/login/screen_context.js:30: this.screen_ = > screen; > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > Cound you please point where |screen_| is used? > > It is not used anymore. I forgot to remove it. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/login/screen_context.js:58: > checkValueIsValid(value, 2); > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > It is not obvious what "2" means. > > Could you use some named constant instead? > > Removed. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/login/screen_context.js:83: > checkValueIsValid(changes[key], 1); > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > What is the "1"? > Removed. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/login/screen_context.js:107: observe: > function(key, observer) { > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > subscribe/unsubscribe? > > "Observe" is the local action. > I used same wording as new JS API use ( > http://www.html5rocks.com/en/tutorials/es7/observe ). I can rename to > subscribe/unsubscribe if you insist. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... > chrome/browser/resources/chromeos/login/screen_context.js:131: > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > nit: drop empty line and trailing comma. > > Done. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... > File chrome/browser/resources/login/screen.js (right): > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... > chrome/browser/resources/login/screen.js:89: makeMagic: function() { > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > Don't see how this method is used in > https://codereview.chromium.org/326933004/. > > In any case, we need to devise a better name than "makeMagic". > It is called in |decorate| method of new screen. > I'll rename it. Note: in the future this method will be called automatically for > every registered screen. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... > chrome/browser/resources/login/screen.js:89: makeMagic: function() { > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > > Don't see how this method is used in > > https://codereview.chromium.org/326933004/. > > > In any case, we need to devise a better name than "makeMagic". > > I agree about naming. > > I'm also concerned about using one more non-standard element atttribute here, > > especially with such short and non-noun name. > > I'd better stick with identifying div's/buttons by id, and having some "wire" > > function that would take id, and do the stuff. > > While it will require few more lines in screen initialization, everything will > > be explicit (bugs caused by implicit behavior are much harder to > > detect/trace/fix). > * The main reason why I don't want use "id"s is that "id"s should be unique. > This restriction makes us to use long identifiers, like > |id="locally-managed-user-creation-some-label"|. For custom attribute we don't > have such restriction so we can use short name, like |is="someLabel_"| and don't > afraid of name conflicts with other screens. > * Using "id" weakens encapsulation, because it is easy to write something like > |$("element-from-another-screen").hidden = true| in any place outside screen. > * The main purpose of Screen and ScreenContext is to get rid of boilerplate > code, and I think this explicit initialization is a good place to start. > Moreover I think new method is less error prone. > * IMHO, the "is" name perfectly describes purpose of this attribute, and I'm > glad that it is so short. Can you explain what is so bad about short names? > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... > chrome/browser/resources/login/screen.js:116: observeContext: function(key, > observer) { > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > subscribeForKeyChange / watchKeyChange? > > > > See comment to ScreenContext.observe > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... > chrome/browser/resources/login/screen.js:156: contextChanged_: function(diff) { > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > nit: add @private annotation. > > Done. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... > chrome/browser/resources/login/screen.js:160: getPropertyNameOf_: > function(value) { > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > nit: add comment to this function + @private annotation. > > Done. > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/ui/webui/chro... > File chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h (right): > > https://codereview.chromium.org/328763003/diff/1/chrome/browser/ui/webui/chro... > chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:69: > LOG(ERROR) << "Returning false"; > On 2014/06/11 10:44:00, ygorshenin1 wrote: > > nit: debug code? > Yes. Removed. I've renamed "is" attribute to "alias".
lgtm
lgtm, but I strongly ask you to reconsider using "observe" name for method. https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/chrome... File chrome/browser/chromeos/login/screens/screen_context.h (right): https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/chrome... chrome/browser/chromeos/login/screens/screen_context.h:74: // are stored in |keys|. |keys| is optional and can be NULL. > Rephrased to "|keys| argument is optional". Nit : capitalize to "Keys" https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... File chrome/browser/resources/chromeos/login/screen_context.js (right): https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/chromeos/login/screen_context.js:107: observe: function(key, observer) { On 2014/06/18 14:43:39, dzhioev wrote: > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > subscribe/unsubscribe? > > "Observe" is the local action. > I used same wording as new JS API use ( > http://www.html5rocks.com/en/tutorials/es7/observe ). I can rename to > subscribe/unsubscribe if you insist. Then it's even more relevant : we need to avoid name clashing with built-in names, as it can lead to unpredictable errors somewhere in the future. https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... File chrome/browser/resources/login/screen.js (right): https://chromiumcodereview.appspot.com/328763003/diff/1/chrome/browser/resour... chrome/browser/resources/login/screen.js:89: makeMagic: function() { > * IMHO, the "is" name perfectly describes purpose of this attribute, and I'm > glad that it is so short. Can you explain what is so bad about short names? First, attribute names are usually nouns. "Is" is a verb. Second, it can be mistaken for some abbreviation.
https://codereview.chromium.org/328763003/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/screens/screen_context.h (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/screens/screen_context.h:74: // are stored in |keys|. |keys| is optional and can be NULL. On 2014/06/19 19:16:36, Denis Kuznetsov wrote: > > Rephrased to "|keys| argument is optional". > Nit : capitalize to "Keys" It is a name of argument, I don't think we should capitalize it. $ git gs "\. |[a-z]" | wc -l 2298 https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/login/screen_context.js (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/login/screen_context.js:107: observe: function(key, observer) { On 2014/06/19 19:16:36, Denis Kuznetsov wrote: > On 2014/06/18 14:43:39, dzhioev wrote: > > On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > > > subscribe/unsubscribe? > > > "Observe" is the local action. > > I used same wording as new JS API use ( > > http://www.html5rocks.com/en/tutorials/es7/observe ). I can rename to > > subscribe/unsubscribe if you insist. > Then it's even more relevant : we need to avoid name clashing with built-in > names, as it can lead to unpredictable errors somewhere in the future. OK, renamed to familiar (add/remove)Observer. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... File chrome/browser/resources/login/screen.js (right): https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:32: * Dictionary of context observers that are methods of |this| binded to On 2014/06/11 14:29:17, Denis Kuznetsov wrote: > s/binded/bound/ Done. https://codereview.chromium.org/328763003/diff/1/chrome/browser/resources/log... chrome/browser/resources/login/screen.js:89: makeMagic: function() { On 2014/06/19 19:16:36, Denis Kuznetsov wrote: > > * IMHO, the "is" name perfectly describes purpose of this attribute, and I'm > > glad that it is so short. Can you explain what is so bad about short names? > > First, attribute names are usually nouns. "Is" is a verb. > Second, it can be mistaken for some abbreviation. Renamed to "alias"
The CQ bit was checked by dzhioev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/328763003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by dzhioev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dzhioev@chromium.org/328763003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Message was sent while issue was closed.
Change committed as 278724 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
