Compile chrome://settings, part 9: yet another final battle

Compile chrome://settings, part 9: yet another 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

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

[Dan Beam, 2014-10-01 01:25:58]

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/alert_overlay.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/alert_overlay.js:78: this.canShow_ = false;
whhhhhhat

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/network_list.js:952:
assertNotReached();
how do we know this isn't being hit?

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
File ui/webui/resources/js/cr/ui/touch_handler.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
ui/webui/resources/js/cr/ui/touch_handler.js:792: assert(this.startTouchY_ !==
undefined);
nit: assert(typeof this.startTouchY_ == 'number');

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
File ui/webui/resources/js/util.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
ui/webui/resources/js/util.js:261: assert(element, 'Missing required element: '
+ id);
why does the rest of this method exist now?

[Vitaly Pavlenko, 2014-10-01 03:07:32]

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/alert_overlay.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/alert_overlay.js:78: this.canShow_ = false;
On 2014/10/01 01:25:57, Dan Beam wrote:
> whhhhhhat

It's hard to understand who actually added this line to source.

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/network_list.js:952:
assertNotReached();
On 2014/10/01 01:25:57, Dan Beam wrote:
> how do we know this isn't being hit?

We'll see on try bots if it is being hit or not. I don't get you, probably.

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
File ui/webui/resources/js/cr/ui/touch_handler.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
ui/webui/resources/js/cr/ui/touch_handler.js:792: assert(this.startTouchY_ !==
undefined);
On 2014/10/01 01:25:57, Dan Beam wrote:
> nit: assert(typeof this.startTouchY_ == 'number');

Done.

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
File ui/webui/resources/js/util.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
ui/webui/resources/js/util.js:261: assert(element, 'Missing required element: '
+ id);
On 2014/10/01 01:25:57, Dan Beam wrote:
> why does the rest of this method exist now?

Done.

[Vitaly Pavlenko, 2014-10-01 21:24:43]

On 2014/10/01 03:07:32, Vitaly Pavlenko wrote:
>
https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
> File chrome/browser/resources/options/alert_overlay.js (right):
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
> chrome/browser/resources/options/alert_overlay.js:78: this.canShow_ = false;
> On 2014/10/01 01:25:57, Dan Beam wrote:
> > whhhhhhat
> 
> It's hard to understand who actually added this line to source.
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
> File chrome/browser/resources/options/chromeos/network_list.js (right):
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
> chrome/browser/resources/options/chromeos/network_list.js:952:
> assertNotReached();
> On 2014/10/01 01:25:57, Dan Beam wrote:
> > how do we know this isn't being hit?
> 
> We'll see on try bots if it is being hit or not. I don't get you, probably.
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
> File ui/webui/resources/js/cr/ui/touch_handler.js (right):
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
> ui/webui/resources/js/cr/ui/touch_handler.js:792: assert(this.startTouchY_ !==
> undefined);
> On 2014/10/01 01:25:57, Dan Beam wrote:
> > nit: assert(typeof this.startTouchY_ == 'number');
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
> File ui/webui/resources/js/util.js (right):
> 
>
https://chromiumcodereview.appspot.com/604373006/diff/1/ui/webui/resources/js...
> ui/webui/resources/js/util.js:261: assert(element, 'Missing required element:
'
> + id);
> On 2014/10/01 01:25:57, Dan Beam wrote:
> > why does the rest of this method exist now?
> 
> Done.

ping dbeam@

[Dan Beam, 2014-10-01 23:25:45]

too many suppressions, try again later

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/alert_overlay.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/alert_overlay.js:78: this.canShow_ = false;
On 2014/10/01 03:07:31, Vitaly Pavlenko wrote:
> On 2014/10/01 01:25:57, Dan Beam wrote:
> > whhhhhhat
> 
> It's hard to understand who actually added this line to source.

https://codereview.chromium.org/6480039/diff/2004/chrome/browser/resources/op...

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/automatic_settings_reset_banner.js:27: *
TODO(vitalyp): remove the suppression.
why?

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/controlled_setting.js:59: * TODO(vitalyp):
remove the suppression.
why?

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/reset_profile_settings_banner.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/reset_profile_settings_banner.js:27: *
TODO(vitalyp): remove the suppression.
nnnnnnoooooooo

[Vitaly Pavlenko, 2014-10-01 23:38:18]

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/automatic_settings_reset_banner.js:27: *
TODO(vitalyp): remove the suppression.
On 2014/10/01 23:25:45, Dan Beam wrote:
> why?

Method AutomaticSettingsResetBanner.dismiss() is declared with pattern "Forward
public APIs to protected implementations":

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

So it requires either to handle it in the compiler pass (slightly different as
"forward API to private implementations" - here method names don't have trailing
underscore) or to write some for-compilation-only in-place fake declarations. Or
not-so-fake declarations.

Do you want me to use one of this ways?

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/controlled_setting.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/controlled_setting.js:59: * TODO(vitalyp):
remove the suppression.
On 2014/10/01 23:25:45, Dan Beam wrote:
> why?

this.controlledBy defined as cr.defineProperty(), so compiler thinks it's a
string. But in fact, it's a nullable string, and it needs some refactoring to
make it be non-nullable.

Do you think we should do that right now?

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/reset_profile_settings_banner.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/1/chrome/browser/resour...
chrome/browser/resources/options/reset_profile_settings_banner.js:27: *
TODO(vitalyp): remove the suppression.
On 2014/10/01 23:25:45, Dan Beam wrote:
> nnnnnnoooooooo

Same reason as in automatic_settings_reset_banner.js

[Dan Beam, 2014-10-02 02:54:39]

lgtm if this greens the bot and you add a comment to each @suppress

https://codereview.chromium.org/604373006/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/bubble.js (right):

https://codereview.chromium.org/604373006/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/bubble.js:492: if (event.button == 0 &&
this.anchorNode_.contains(target)) {
nit: no curlies

[Dan Beam, 2014-10-02 02:56:35]

slgtm

https://codereview.chromium.org/604373006/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/604373006/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:280:
$('choose-data-types-form').querySelector('#passphrase').value;
i don't know why this is like this, but $('passphrase') should probably work,
no?

https://codereview.chromium.org/604373006/diff/20001/ui/webui/resources/js/ut...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/604373006/diff/20001/ui/webui/resources/js/ut...
ui/webui/resources/js/util.js:260: var element = $(id);
^ remove

[Vitaly Pavlenko, 2014-10-02 18:18:23]

https://chromiumcodereview.appspot.com/604373006/diff/20001/chrome/browser/re...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://chromiumcodereview.appspot.com/604373006/diff/20001/chrome/browser/re...
chrome/browser/resources/sync_setup_overlay.js:280:
$('choose-data-types-form').querySelector('#passphrase').value;
On 2014/10/02 02:56:35, Dan Beam wrote:
> i don't know why this is like this, but $('passphrase') should probably work,
> no?

Done.

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

https://chromiumcodereview.appspot.com/604373006/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr/ui/bubble.js:492: if (event.button == 0 &&
this.anchorNode_.contains(target)) {
On 2014/10/02 02:54:38, Dan Beam wrote:
> nit: no curlies

Done.

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

https://chromiumcodereview.appspot.com/604373006/diff/20001/ui/webui/resource...
ui/webui/resources/js/util.js:260: var element = $(id);
On 2014/10/02 02:56:35, Dan Beam wrote:
> ^ remove

Done.

[Vitaly Pavlenko, 2014-10-02 18:18:26]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-10-02 18:19:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/604373006/40001

[commit-bot: I haz the power, 2014-10-02 19:36:41]

Committed patchset #3 (id:40001) as 2bec05fa2f0c7af5be8c33487f0cfc1096d421de

[commit-bot: I haz the power, 2014-10-02 19:38:39]

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