Add public API generation with cr.makePublic() and handle it in compiler pass

Add public API generation with cr.makePublic() and handle it in compiler pass

R=dbeam@chromium.org,tbreisacher@chromium.org
BUG=393873
TEST=third_party/closure_compiler/runner/how_to_test_compiler_pass.md

Committed: https://crrev.com/730652530d108372ef14f62b89665197628f7880
Cr-Commit-Position: refs/heads/master@{#295173}

Committed: https://crrev.com/b38e60a8e1162458f9c4d787c44cf17c32eff414
Cr-Commit-Position: refs/heads/master@{#295410}

[Vitaly Pavlenko, 2014-09-09 18:36:05]

There's one problem I don't know yet how to address well. For every exported
method I search for it's private declaration in the class passed to
cr.makePublic(), copy it's JSDoc and create public declaration. But I search for
private method declarations only in the same class, and not in base classes.
There are 9 cases in code base when method is declared in base class.

[Vitaly Pavlenko, 2014-09-09 19:15:09]

On 2014/09/09 18:36:05, Vitaly Pavlenko wrote:
> There's one problem I don't know yet how to address well. For every exported
> method I search for it's private declaration in the class passed to
> cr.makePublic(), copy it's JSDoc and create public declaration. But I search
for
> private method declarations only in the same class, and not in base classes.
> There are 9 cases in code base when method is declared in base class.

Ok, further investigations showed that there are only three such cases. So I can
handle them manually.

Please, review.

[Tyler Breisacher (Chromium), 2014-09-09 19:34:46]

lgtm

https://chromiumcodereview.appspot.com/557633002/diff/40001/third_party/closu...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/40001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:163:
assert publicAPI != null && publicAPI.isArrayLit();
This should probably report an error to the user, rather than just failing. At
the very least you should probably throw an error with a meaningful message.

https://chromiumcodereview.appspot.com/557633002/diff/40001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:167:
assert methodName.isString();
Same here.

[Vitaly Pavlenko, 2014-09-09 20:31:57]

https://chromiumcodereview.appspot.com/557633002/diff/40001/third_party/closu...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/40001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:163:
assert publicAPI != null && publicAPI.isArrayLit();
On 2014/09/09 19:34:45, Tyler Breisacher (Chromium) wrote:
> This should probably report an error to the user, rather than just failing. At
> the very least you should probably throw an error with a meaningful message.

Done.

https://chromiumcodereview.appspot.com/557633002/diff/40001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:167:
assert methodName.isString();
On 2014/09/09 19:34:45, Tyler Breisacher (Chromium) wrote:
> Same here.

Done.

[Vitaly Pavlenko, 2014-09-09 21:27:49]

I handled another case: when we need to have a dummy declaration on subclass
because exported private methods is defined in base class where it's invisible
to our compiler.

[Tyler Breisacher (Chromium), 2014-09-09 21:31:38]

On 2014/09/09 21:27:49, Vitaly Pavlenko wrote:
> I handled another case: when we need to have a dummy declaration on subclass
> because exported private methods is defined in base class where it's invisible
> to our compiler.

That's kind of an awkward case. Arguably if the original method is on the base
class then it should also be exported publicly as a static method of that base
class, not the subclass. But I assume it would be kind of a pain to refactor in
that way so this is fine. lgtm

[Dan Beam, 2014-09-10 19:26:28]

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:301: * @param {Array.<string>} publicAPI List of
public method names that have
publicAPI -> methods

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:303: * @param {HTMLElement=} opt_target Target node
(used in
*= or ?= as it makes sense to keep this generic

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:304: *     oobe_screen_user_image.js)
remove used in

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:306: function makePublic(ctor, publicAPI,
opt_target) {
var target = opt_target || ctor.getInstance();
methods.forEach(function(method) {
  ctor[method] = function() {
    return target[method + '_'].apply(target, arguments);
  };
}

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/automatic_settings_reset_banner.js:54: * in the
base class.
private methods should not be shared, make this protected

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/browser_options.js:1934:
BrowserOptions.prototype.updateStartupPages_;
please minimize the amount of useless code we write for just the compiler at all
costs

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:166:
Node publicAPI = call.getChildAtIndex(2);
methods

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:173:
Set<String> publicAPIStrings = new HashSet<>();
methodNames

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
File
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:369:
"Class = function() {};\n" +
why are you doing this with global assignment rather than:

  var Class = function() {};

or

  function Class() {}  // more common

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:372:
"  /** @return {number} */\n" +
@private?

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:427:
"Class = function() {}\n" +
ident off

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:488:

what about the case of:

  function Class() {
    /** @private */
    this.doMagic_ = function() {
      // magic happens
    };
  }

https://chromiumcodereview.appspot.com/557633002/diff/80001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/ui/webui/resource...
ui/webui/resources/js/cr.js:330: makePublic: makePublic,
expose might be a better name if it's apparent enough

[Vitaly Pavlenko, 2014-09-10 20:25:22]

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/automatic_settings_reset_banner.js:54: * in the
base class.
On 2014/09/10 19:26:27, Dan Beam wrote:
> private methods should not be shared, make this protected

Should I make them protected in base class and here?

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/browser_options.js:1934:
BrowserOptions.prototype.updateStartupPages_;
On 2014/09/10 19:26:27, Dan Beam wrote:
> please minimize the amount of useless code we write for just the compiler at
all
> costs

For this feature I need exactly these three fake declarations, no more.
Otherwise I can write some kind of suppression options for makePublic, for the
cost of additional awkward code in JS and in Compiler pass.

So I suggest to leave these three fake declarations. What do you think?

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
File
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:166:
Node publicAPI = call.getChildAtIndex(2);
On 2014/09/10 19:26:27, Dan Beam wrote:
> methods

Done.

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:173:
Set<String> publicAPIStrings = new HashSet<>();
On 2014/09/10 19:26:27, Dan Beam wrote:
> methodNames

Done.

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
File
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:369:
"Class = function() {};\n" +
On 2014/09/10 19:26:27, Dan Beam wrote:
> why are you doing this with global assignment rather than:
> 
>   var Class = function() {};
> 
> or
> 
>   function Class() {}  // more common

Done.

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:372:
"  /** @return {number} */\n" +
On 2014/09/10 19:26:27, Dan Beam wrote:
> @private?

You mean, we should remove @private declaration from JSDoc if it was there?

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:427:
"Class = function() {}\n" +
On 2014/09/10 19:26:27, Dan Beam wrote:
> ident off

Done.

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:488:

On 2014/09/10 19:26:27, Dan Beam wrote:
> what about the case of:
> 
>   function Class() {
>     /** @private */
>     this.doMagic_ = function() {
>       // magic happens
>     };
>   }

Looks like there's no such case right now in Chrome code base. When there
appears one, we'll spot it via compiler errors like

ERROR: Method "doMagic_" exported by cr.makePublic() on "Class" has no
declaration.

https://chromiumcodereview.appspot.com/557633002/diff/80001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/ui/webui/resource...
ui/webui/resources/js/cr.js:330: makePublic: makePublic,
On 2014/09/10 19:26:27, Dan Beam wrote:
> expose might be a better name if it's apparent enough

So could you propose the best method name to use?

[Tyler Breisacher (Chromium), 2014-09-10 20:50:33]

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/browser_options.js:1934:
BrowserOptions.prototype.updateStartupPages_;
Hang on, if this is a private method of the base class, it shouldn't be
accessible here. It should probably be protected, not private, no?

[Dan Beam, 2014-09-10 21:08:19]

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/browser_options.js:1934:
BrowserOptions.prototype.updateStartupPages_;
On 2014/09/10 20:50:32, Tyler Breisacher (Chromium) wrote:
> Hang on, if this is a private method of the base class, it shouldn't be
> accessible here. It should probably be protected, not private, no?

see other comments on this review

[Vitaly Pavlenko, 2014-09-12 00:20:05]

On 2014/09/10 21:08:19, Dan Beam wrote:
>
https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
> chrome/browser/resources/options/browser_options.js:1934:
> BrowserOptions.prototype.updateStartupPages_;
> On 2014/09/10 20:50:32, Tyler Breisacher (Chromium) wrote:
> > Hang on, if this is a private method of the base class, it shouldn't be
> > accessible here. It should probably be protected, not private, no?
> 
> see other comments on this review

So what's the status of this issue?

[Dan Beam, 2014-09-12 00:57:51]

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/automatic_settings_reset_banner.js:54: * in the
base class.
On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> On 2014/09/10 19:26:27, Dan Beam wrote:
> > private methods should not be shared, make this protected
> 
> Should I make them protected in base class and here?

yes

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/chrome/browser/re...
chrome/browser/resources/options/browser_options.js:1934:
BrowserOptions.prototype.updateStartupPages_;
On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> On 2014/09/10 19:26:27, Dan Beam wrote:
> > please minimize the amount of useless code we write for just the compiler at
> all
> > costs
> 
> For this feature I need exactly these three fake declarations, no more.
> Otherwise I can write some kind of suppression options for makePublic, for the
> cost of additional awkward code in JS and in Compiler pass.
> 
> So I suggest to leave these three fake declarations. What do you think?

no, if possible, don't add them

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
File
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java
(right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/third_party/closu...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:488:

On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> On 2014/09/10 19:26:27, Dan Beam wrote:
> > what about the case of:
> > 
> >   function Class() {
> >     /** @private */
> >     this.doMagic_ = function() {
> >       // magic happens
> >     };
> >   }
> 
> Looks like there's no such case right now in Chrome code base. When there
> appears one, we'll spot it via compiler errors like
> 
> ERROR: Method "doMagic_" exported by cr.makePublic() on "Class" has no
> declaration.

that's fine

https://chromiumcodereview.appspot.com/557633002/diff/80001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/80001/ui/webui/resource...
ui/webui/resources/js/cr.js:330: makePublic: makePublic,
On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> On 2014/09/10 19:26:27, Dan Beam wrote:
> > expose might be a better name if it's apparent enough
> 
> So could you propose the best method name to use?

Tyler and I agreed that makePublic is fine

[Vitaly Pavlenko, 2014-09-12 18:38:07]

https://codereview.chromium.org/557633002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/557633002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/automatic_settings_reset_banner.js:54: * in the
base class.
On 2014/09/12 00:57:51, Dan Beam wrote:
> On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> > On 2014/09/10 19:26:27, Dan Beam wrote:
> > > private methods should not be shared, make this protected
> > 
> > Should I make them protected in base class and here?
> 
> yes

Done.

https://codereview.chromium.org/557633002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/557633002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1934:
BrowserOptions.prototype.updateStartupPages_;
On 2014/09/12 00:57:51, Dan Beam wrote:
> On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> > On 2014/09/10 19:26:27, Dan Beam wrote:
> > > please minimize the amount of useless code we write for just the compiler
at
> > all
> > > costs
> > 
> > For this feature I need exactly these three fake declarations, no more.
> > Otherwise I can write some kind of suppression options for makePublic, for
the
> > cost of additional awkward code in JS and in Compiler pass.
> > 
> > So I suggest to leave these three fake declarations. What do you think?
> 
> no, if possible, don't add them

Ok, then I'd like to remove that as a dead code :)

https://codereview.chromium.org/557633002/diff/80001/third_party/closure_comp...
File
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java
(right):

https://codereview.chromium.org/557633002/diff/80001/third_party/closure_comp...
third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:488:

On 2014/09/12 00:57:51, Dan Beam wrote:
> On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> > On 2014/09/10 19:26:27, Dan Beam wrote:
> > > what about the case of:
> > > 
> > >   function Class() {
> > >     /** @private */
> > >     this.doMagic_ = function() {
> > >       // magic happens
> > >     };
> > >   }
> > 
> > Looks like there's no such case right now in Chrome code base. When there
> > appears one, we'll spot it via compiler errors like
> > 
> > ERROR: Method "doMagic_" exported by cr.makePublic() on "Class" has no
> > declaration.
> 
> that's fine

Acknowledged.

https://codereview.chromium.org/557633002/diff/80001/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/557633002/diff/80001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr.js:330: makePublic: makePublic,
On 2014/09/12 00:57:51, Dan Beam wrote:
> On 2014/09/10 20:25:21, Vitaly Pavlenko wrote:
> > On 2014/09/10 19:26:27, Dan Beam wrote:
> > > expose might be a better name if it's apparent enough
> > 
> > So could you propose the best method name to use?
> 
> Tyler and I agreed that makePublic is fine

Acknowledged.

https://codereview.chromium.org/557633002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/557633002/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/automatic_settings_reset_banner.js:64:
AutomaticSettingsResetBanner.prototype.dismiss_;
Moved here declarations from base class.

[Dan Beam, 2014-09-12 23:43:54]

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:303: * @param {HTMLElement=} opt_target Target node
(used in
On 2014/09/10 19:26:26, Dan Beam wrote:
> *= or ?= as it makes sense to keep this generic

ping

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:304: *     oobe_screen_user_image.js)
On 2014/09/10 19:26:27, Dan Beam wrote:
> remove used in

ping

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:306: function makePublic(ctor, publicAPI,
opt_target) {
On 2014/09/10 19:26:26, Dan Beam wrote:
> var target = opt_target || ctor.getInstance();
> methods.forEach(function(method) {
>   ctor[method] = function() {
>     return target[method + '_'].apply(target, arguments);
>   };
> }

ping

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
chrome/browser/resources/options/browser_options.js:735: /** @suppress
{uselessCode} */
i was wrong, can you change this to suspiciousCode?

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
chrome/browser/resources/options/manage_profile_overlay.js:369: }).bind(this);
nit: no () around function() {}

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
File chrome/browser/resources/options/settings_banner.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
chrome/browser/resources/options/settings_banner.js:50:
setVisibilibyDomElement_: null,
rebase, your code is out of date

[Vitaly Pavlenko, 2014-09-13 01:22:39]

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
File ui/webui/resources/js/cr.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:301: * @param {Array.<string>} publicAPI List of
public method names that have
On 2014/09/10 19:26:26, Dan Beam wrote:
> publicAPI -> methods

Done.

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:303: * @param {HTMLElement=} opt_target Target node
(used in
On 2014/09/12 23:43:54, Dan Beam wrote:
> On 2014/09/10 19:26:26, Dan Beam wrote:
> > *= or ?= as it makes sense to keep this generic
> 
> ping

Done.

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:304: *     oobe_screen_user_image.js)
On 2014/09/12 23:43:54, Dan Beam wrote:
> On 2014/09/10 19:26:27, Dan Beam wrote:
> > remove used in
> 
> ping

Done.

https://chromiumcodereview.appspot.com/557633002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr.js:306: function makePublic(ctor, publicAPI,
opt_target) {
On 2014/09/12 23:43:54, Dan Beam wrote:
> On 2014/09/10 19:26:26, Dan Beam wrote:
> > var target = opt_target || ctor.getInstance();
> > methods.forEach(function(method) {
> >   ctor[method] = function() {
> >     return target[method + '_'].apply(target, arguments);
> >   };
> > }
> 
> ping

Done.

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
chrome/browser/resources/options/browser_options.js:735: /** @suppress
{uselessCode} */
On 2014/09/12 23:43:54, Dan Beam wrote:
> i was wrong, can you change this to suspiciousCode?

Done.

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
chrome/browser/resources/options/manage_profile_overlay.js:369: }).bind(this);
On 2014/09/12 23:43:54, Dan Beam wrote:
> nit: no () around function() {}

Done.

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
File chrome/browser/resources/options/settings_banner.js (right):

https://chromiumcodereview.appspot.com/557633002/diff/120001/chrome/browser/r...
chrome/browser/resources/options/settings_banner.js:50:
setVisibilibyDomElement_: null,
On 2014/09/12 23:43:54, Dan Beam wrote:
> rebase, your code is out of date

Hopefully, done.

[Dan Beam, 2014-09-13 01:26:51]

lgtm (actually,
https://chromiumcodereview.appspot.com/533183002/diff/100001/chrome/browser/r...
hasn't landed yet)

[Vitaly Pavlenko, 2014-09-16 19:54:58]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-16 19:56:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557633002/160001

[Vitaly Pavlenko, 2014-09-16 21:15:16]

The CQ bit was unchecked by vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-16 22:00:03]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-16 22:01:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557633002/180001

[commit-bot: I haz the power, 2014-09-16 23:14:32]

Committed patchset #10 (id:180001) as 257798a621721c6de5b8a25c229031af35290996

[commit-bot: I haz the power, 2014-09-16 23:15:09]

Patchset 10 (id:??) landed as
https://crrev.com/730652530d108372ef14f62b89665197628f7880
Cr-Commit-Position: refs/heads/master@{#295173}

[Ken Rockot(use gerrit already), 2014-09-17 01:04:14]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/577843003/ by rockot@chromium.org.

The reason for reverting is: Likely culprit for OOBE WebUI test breakage.

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[Vitaly Pavlenko, 2014-09-18 03:00:04]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-18 03:01:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557633002/200001

[commit-bot: I haz the power, 2014-09-18 04:00:02]

Committed patchset #11 (id:200001) as 6d9ec49ea5db98f9c64290ab6ef7e7d7bf5d4a69

[commit-bot: I haz the power, 2014-09-18 04:10:08]

Patchset 11 (id:??) landed as
https://crrev.com/b38e60a8e1162458f9c4d787c44cf17c32eff414
Cr-Commit-Position: refs/heads/master@{#295410}