MD Settings: Update some Settings handlers to use new JS lifecycle management.

MD Settings: Update some Settings handlers to use new JS lifecycle management.

This doesn't fix all of them, but fixes ~35% of them.

BUG=602523
Committed: https://crrev.com/f7d960dba900ba8474ac50fa4225c79ba6ca1d52
Cr-Commit-Position: refs/heads/master@{#392999}

[tommycli, 2016-05-09 21:52:37]

Description was changed from

==========
MD Settings: Update handlers to use new JS lifecycle management.

BUG=602523
==========

to

==========
MD Settings: Update some Settings handlers to use new JS lifecycle management.

This doesn't fix all of them, but fixes ~35% of them.

BUG=602523
==========

[tommycli, 2016-05-10 23:41:46]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-10 23:42:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961183002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961183002/40001

[commit-bot: I haz the power, 2016-05-11 00:35:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-11 00:35:15]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-05-11 16:23:07]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-11 16:23:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961183002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961183002/60001

[commit-bot: I haz the power, 2016-05-11 17:10:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-11 17:10:35]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-05-11 17:12:16]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org

[tommycli, 2016-05-11 17:12:17]

dbeam: PTAL. Gets us closer, but not quite there for all Settings handlers being
JS-safe.

[Dan Beam, 2016-05-11 17:19:57]

https://codereview.chromium.org/1961183002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/1961183002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:176:
return;
^ did you write this before you found Destroy()?

[tommycli, 2016-05-11 17:22:05]

https://codereview.chromium.org/1961183002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/1961183002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:176:
return;
On 2016/05/11 17:19:57, Dan Beam wrote:
> ^ did you write this before you found Destroy()?

Done. Oops yes exactly. Removed.

[Dan Beam, 2016-05-11 17:23:22]

lgtm

[tommycli, 2016-05-11 17:25:24]

dbeam: thanks for the lightning fast review!

[tommycli, 2016-05-11 17:25:29]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-05-11 17:26:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961183002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961183002/80001

[Dan Beam, 2016-05-11 17:26:08]

On 2016/05/11 17:25:24, tommycli wrote:
> dbeam: thanks for the lightning fast review!

good reviews are more important than fast

[michaelpg, 2016-05-11 17:31:21]

timely and informative changes, thanks!

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
(right):

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:41: void
OnJavascriptDisallowed() override;
if Allowed has been called, will Disallowed always be called before the
destructor?

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:59:
ScopedObserver<ui::DeviceDataManager, KeyboardHandler> observer_;
the OnJavascriptAllowed/Disallowed stuff is great.

for my lernings: does the ScopedObserver do anything we couldn't do before? ie,
is it just "sugar" so we don't have to call AddObserver and RemoveObserver
manually from the destructor and the OnJavascript*llowed functions? (sugar is
cool, just curious if there's behavior i'm not seeing)

[tommycli, 2016-05-11 17:32:44]

michaelpg: see below, thanks

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
(right):

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:41: void
OnJavascriptDisallowed() override;
On 2016/05/11 17:31:21, michaelpg wrote:
> if Allowed has been called, will Disallowed always be called before the
> destructor?

No.

I wish I could make it always be called, but I couldn't figure out how to.
Destructors can't call virtual functions.

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:59:
ScopedObserver<ui::DeviceDataManager, KeyboardHandler> observer_;
On 2016/05/11 17:31:21, michaelpg wrote:
> the OnJavascriptAllowed/Disallowed stuff is great.
> 
> for my lernings: does the ScopedObserver do anything we couldn't do before?
ie,
> is it just "sugar" so we don't have to call AddObserver and RemoveObserver
> manually from the destructor and the OnJavascript*llowed functions? (sugar is
> cool, just curious if there's behavior i'm not seeing)

The only reason I had to change it: OnJavascriptDisallowed is not guaranteed to
be called before destruction.

[Dan Beam, 2016-05-11 17:33:48]

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
(right):

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:41: void
OnJavascriptDisallowed() override;
On 2016/05/11 17:31:21, michaelpg wrote:
> if Allowed has been called, will Disallowed always be called before the
> destructor?

not currently.  thought it *might* if we start to make RenderProcessGone() call
OnJavascriptDisallowed() and this happens predictably before the webcontents
going away (I'm not sure about that).

[Dan Beam, 2016-05-11 17:34:07]

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
(right):

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:41: void
OnJavascriptDisallowed() override;
On 2016/05/11 17:33:48, Dan Beam wrote:
> On 2016/05/11 17:31:21, michaelpg wrote:
> > if Allowed has been called, will Disallowed always be called before the
> > destructor?
> 
> not currently.  thought it *might* if we start to make RenderProcessGone()
call
> OnJavascriptDisallowed() and this happens predictably before the webcontents
> going away (I'm not sure about that).

though it might*

[tommycli, 2016-05-11 17:35:20]

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
(right):

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:59:
ScopedObserver<ui::DeviceDataManager, KeyboardHandler> observer_;
On 2016/05/11 17:32:44, tommycli wrote:
> On 2016/05/11 17:31:21, michaelpg wrote:
> > the OnJavascriptAllowed/Disallowed stuff is great.
> > 
> > for my lernings: does the ScopedObserver do anything we couldn't do before?
> ie,
> > is it just "sugar" so we don't have to call AddObserver and RemoveObserver
> > manually from the destructor and the OnJavascript*llowed functions? (sugar
is
> > cool, just curious if there's behavior i'm not seeing)
> 
> The only reason I had to change it: OnJavascriptDisallowed is not guaranteed
to
> be called before destruction.

(If I don't have a ScopedObserver, since OnJavascriptDisallowed is not called
before destruction, there can be a dangling reference to this object)

[Dan Beam, 2016-05-11 17:35:22]

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
(right):

https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:59:
ScopedObserver<ui::DeviceDataManager, KeyboardHandler> observer_;
On 2016/05/11 17:31:21, michaelpg wrote:
> the OnJavascriptAllowed/Disallowed stuff is great.
> 
> for my lernings: does the ScopedObserver do anything we couldn't do before?
ie,
> is it just "sugar" so we don't have to call AddObserver and RemoveObserver
> manually from the destructor and the OnJavascript*llowed functions? (sugar is
> cool, just curious if there's behavior i'm not seeing)

this sugar is less error-prone as well

[tommycli, 2016-05-11 17:39:03]

On 2016/05/11 17:35:22, Dan Beam wrote:
>
https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h
> (right):
> 
>
https://codereview.chromium.org/1961183002/diff/80001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h:59:
> ScopedObserver<ui::DeviceDataManager, KeyboardHandler> observer_;
> On 2016/05/11 17:31:21, michaelpg wrote:
> > the OnJavascriptAllowed/Disallowed stuff is great.
> > 
> > for my lernings: does the ScopedObserver do anything we couldn't do before?
> ie,
> > is it just "sugar" so we don't have to call AddObserver and RemoveObserver
> > manually from the destructor and the OnJavascript*llowed functions? (sugar
is
> > cool, just curious if there's behavior i'm not seeing)
> 
> this sugar is less error-prone as well

Yes, since encountering a few of these errors, I now wish the AddObserver(Foo*)
and RemoveObserver(Foo*) pattern using raw pointers did not exist.

[commit-bot: I haz the power, 2016-05-11 18:22:02]

Description was changed from

==========
MD Settings: Update some Settings handlers to use new JS lifecycle management.

This doesn't fix all of them, but fixes ~35% of them.

BUG=602523
==========

to

==========
MD Settings: Update some Settings handlers to use new JS lifecycle management.

This doesn't fix all of them, but fixes ~35% of them.

BUG=602523
==========

[commit-bot: I haz the power, 2016-05-11 18:22:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-11 18:24:13]

Description was changed from

==========
MD Settings: Update some Settings handlers to use new JS lifecycle management.

This doesn't fix all of them, but fixes ~35% of them.

BUG=602523
==========

to

==========
MD Settings: Update some Settings handlers to use new JS lifecycle management.

This doesn't fix all of them, but fixes ~35% of them.

BUG=602523

Committed: https://crrev.com/f7d960dba900ba8474ac50fa4225c79ba6ca1d52
Cr-Commit-Position: refs/heads/master@{#392999}
==========

[commit-bot: I haz the power, 2016-05-11 18:24:14]

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