WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc: https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo8yy68TN_FUhUzRz7Q/edit?usp=sharing

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

[tommycli, 2016-04-15 23:25:05]

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

[tommycli, 2016-04-15 23:25:06]

dbeam/jochen: PTAL

[Dan Beam, 2016-04-18 23:13:21]

looking pretty good to me

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc (right):

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc:143:
CheckForRemovedFiles();
technically we want to check for removed files on *refresh*.

what are the chances that refresh and disallowing javascript diverge eventually?

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/font_handler.cc (right):

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/font_handler.cc:64:
extensions::ExtensionRegistry::Get(profile_);
indent off

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/font_handler.cc:65: if
(!extension_registry_observer_.IsObserving(observer))
why do you need this if?

https://codereview.chromium.org/1896463003/diff/100001/content/browser/webui/...
File content/browser/webui/web_ui_message_handler.cc (right):

https://codereview.chromium.org/1896463003/diff/100001/content/browser/webui/...
content/browser/webui/web_ui_message_handler.cc:17: 
we should be checking this was called validly here as well, i.e.

CHECK(web_ui());
CHECK(web_ui()->SomeNewMethodThatChecksIfWeCanExecuteJavascript());

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
File content/public/browser/web_ui_message_handler.h (right):

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
content/public/browser/web_ui_message_handler.h:47: bool IsJavascriptAllowed()
const { return javascript_allowed_; }
if this isn't virtual, javascript_allowed() instead maybe

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
content/public/browser/web_ui_message_handler.h:47: bool IsJavascriptAllowed()
const { return javascript_allowed_; }
this should probably also check some other things, too, right?

i.e. whether there's a web_ui(), whether that web_ui() has an associated, ready
renderview, etc.

basically, I think this should potentially be joined with

  return javascript_allowed_ && <everything else required>;

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
content/public/browser/web_ui_message_handler.h:83: template <typename...
Values>
til: "Variadic Templates" are a thing

[tommycli, 2016-04-19 17:45:38]

dbeam: Thanks! I made some revisions. Tommy

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc (right):

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc:143:
CheckForRemovedFiles();
On 2016/04/18 23:13:20, Dan Beam wrote:
> technically we want to check for removed files on *refresh*.
> 
> what are the chances that refresh and disallowing javascript diverge
eventually?

Hmm... I would imagine OnJavascriptDisallowed to being a superset of
OnRenderViewReused.

A Refresh should ALWAYS trigger a OnJavascriptDisallowed. Additionally, a
'Polymer element detached' should probably also trigger a DisallowJavascript
(currently non-existent) call.

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/font_handler.cc (right):

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/font_handler.cc:64:
extensions::ExtensionRegistry::Get(profile_);
On 2016/04/18 23:13:20, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1896463003/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/font_handler.cc:65: if
(!extension_registry_observer_.IsObserving(observer))
On 2016/04/18 23:13:20, Dan Beam wrote:
> why do you need this if?

Done.

https://codereview.chromium.org/1896463003/diff/100001/content/browser/webui/...
File content/browser/webui/web_ui_message_handler.cc (right):

https://codereview.chromium.org/1896463003/diff/100001/content/browser/webui/...
content/browser/webui/web_ui_message_handler.cc:17: 
On 2016/04/18 23:13:20, Dan Beam wrote:
> we should be checking this was called validly here as well, i.e.
> 
> CHECK(web_ui());
> CHECK(web_ui()->SomeNewMethodThatChecksIfWeCanExecuteJavascript());

Done.

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
File content/public/browser/web_ui_message_handler.h (right):

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
content/public/browser/web_ui_message_handler.h:47: bool IsJavascriptAllowed()
const { return javascript_allowed_; }
On 2016/04/18 23:13:20, Dan Beam wrote:
> if this isn't virtual, javascript_allowed() instead maybe

I opted not to use lowercase because the new version checks stuff beyond just
the variable.

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
content/public/browser/web_ui_message_handler.h:47: bool IsJavascriptAllowed()
const { return javascript_allowed_; }
On 2016/04/18 23:13:20, Dan Beam wrote:
> this should probably also check some other things, too, right?
> 
> i.e. whether there's a web_ui(), whether that web_ui() has an associated,
ready
> renderview, etc.
> 
> basically, I think this should potentially be joined with
> 
>   return javascript_allowed_ && <everything else required>;

Done.

https://codereview.chromium.org/1896463003/diff/100001/content/public/browser...
content/public/browser/web_ui_message_handler.h:83: template <typename...
Values>
On 2016/04/18 23:13:21, Dan Beam wrote:
> til: "Variadic Templates" are a thing

lol, I wrote this with the C++11 reference manual in the other tab. Hopefully we
can eliminate this variadic template thing soon by eliminating redundancy with
WebUI method signatures.

[Dan Beam, 2016-04-19 21:27:05]

lgtm

[tommycli, 2016-04-19 21:54:20]

dbeam: awesome thanks!

jochen: PTAL (design doc in description)

[jochen (gone - plz use gerrit), 2016-04-20 14:23:38]

lgtm

[tommycli, 2016-04-20 16:34:47]

Description was changed from

==========
WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo...

BUG=602423
==========

to

==========
WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo...

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

[tommycli, 2016-04-20 16:35:43]

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

[tommycli, 2016-04-20 16:36:18]

carlosk: Adding you since you commented on the doc.

I wanted to get any feedback you might have on this patch before I sent it to
CQ.

[tommycli, 2016-04-20 22:57:18]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-04-20 22:58:01]

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

[Dan Beam, 2016-04-21 00:38:31]

The CQ bit was unchecked by dbeam@chromium.org

[carlosk, 2016-04-21 12:04:04]

On 2016/04/20 16:36:18, tommycli wrote:
> carlosk: Adding you since you commented on the doc.
> 
> I wanted to get any feedback you might have on this patch before I sent it to
> CQ.

I had already reviewed before the files that I have some knowledge of and I had
no specific comments. So you have my LGTM for content/browser/webui/* and
content/public/*.

A comment I have is that I noticed the handlers are notified on RenderViewReused
but not on RenderViewCreated. You really don't need to listen to that other one?
It is called during a navigation when a WebUI is bound to a RenderFrameHost for
the first time (the RFH can have "seen" other instances of WebUIs before).

And I confirmed RenderViewReused is only called when all of these are true [1]:
- There is a live RenderFrameHost with a WebUI bound to it in a previous
navigation commit.
- A new navigation is happening (a refresh for instance) and both the
RenderFrameHost and WebUI will/might be* reused.

(*): In PlzNavigate, this call might currently happen either at navigation
request or commit time (but never both!). At request time there is only the
possibility that both will be reused as the situation might change before commit
(redirects for instance) and that might not be true later. Note that these
situations might be rare; potentially more common with different
multi-processing models. 

Let me know if you need more details on either of these calls.


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Dan Beam, 2016-04-21 16:47:37]

On 2016/04/21 12:04:04, carlosk wrote:
> On 2016/04/20 16:36:18, tommycli wrote:
> > carlosk: Adding you since you commented on the doc.
> > 
> > I wanted to get any feedback you might have on this patch before I sent it
to
> > CQ.
> 
> I had already reviewed before the files that I have some knowledge of and I
had
> no specific comments. So you have my LGTM for content/browser/webui/* and
> content/public/*.
> 
> A comment I have is that I noticed the handlers are notified on
RenderViewReused
> but not on RenderViewCreated. You really don't need to listen to that other
one?
> It is called during a navigation when a WebUI is bound to a RenderFrameHost
for
> the first time (the RFH can have "seen" other instances of WebUIs before).

Just having a render frame really isn't enough to ensure the page's JS is ready
AND it's application-specific logic is set up correctly in many cases.  Many
WebUI pages do something like:

  document.addEventListener('load', function() {
    ... bunch of init() calls ...
    chrome.send('readyForJavascript');  // <-- this is really when we want to
start calling JavaScript
  });

tommycli@ has made it possible for any number of things to allow a
WebUIMessageHandler to start calling Javascript by adding an AllowJavascript()
method that can be called whenever.  Most of the time, it'll be something like:

  void MyHandler::HandleReadyForJavascript(const base::ListValue* args) {
    AllowJavascript();  // add observers, maybe sends initial data to the page
  }

However, AllowJavascript() could be called from RenderViewCreated for handlers
that only do request/response (rather than observe services that may notify at
any time).

> 
> And I confirmed RenderViewReused is only called when all of these are true
[1]:
> - There is a live RenderFrameHost with a WebUI bound to it in a previous
> navigation commit.
> - A new navigation is happening (a refresh for instance) and both the
> RenderFrameHost and WebUI will/might be* reused.
> 
> (*): In PlzNavigate, this call might currently happen either at navigation
> request or commit time (but never both!). At request time there is only the
> possibility that both will be reused as the situation might change before
commit
> (redirects for instance) and that might not be true later. Note that these
> situations might be rare; potentially more common with different
> multi-processing models. 
> 
> Let me know if you need more details on either of these calls.
> 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Dan Beam, 2016-04-21 16:49:35]

/cc creis@ and nasko@ as an FYI

[nasko, 2016-04-21 17:13:04]

On 2016/04/21 16:49:35, Dan Beam wrote:
> /cc creis@ and nasko@ as an FYI

Thanks for cleaning up the RenderViewReused callbacks! Let's chat about follow
up to completely remove it from web_ui_message_handler.cc as a follow up.

[Dan Beam, 2016-04-22 18:38:21]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-04-22 18:39:13]

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

[commit-bot: I haz the power, 2016-04-22 19:59:09]

Description was changed from

==========
WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo...

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

to

==========
WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo...

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

[commit-bot: I haz the power, 2016-04-22 19:59:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-22 20:01:11]

Description was changed from

==========
WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo...

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

to

==========
WebUI: Add JavaScript lifecycle-control to WebUIMessageHandler.

This patch:
 1) Adds AllowJavascript call to WebUIMessageHandler.
 2) Adds JavaScript lifecycle callbacks OnJavascriptAllowed and
    OnJavascriptDisallowed.
 3) Adds a CHECKed CallJavascriptFunction method.
 4) Updates classes previously using the RenderViewReused method
    to the new OnJavascriptDisallowed callback.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1z1diKvwgMmn4YFzlW1kss0yHmo...

BUG=602523

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

[commit-bot: I haz the power, 2016-04-22 20:01:12]

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

[Dan Beam, 2017-05-18 01:13:10]

https://codereview.chromium.org/1896463003/diff/120001/content/public/browser...
File content/public/browser/web_ui_message_handler.h (right):

https://codereview.chromium.org/1896463003/diff/120001/content/public/browser...
content/public/browser/web_ui_message_handler.h:84: void
CallJavascriptFunction(const std::string& function_name,
hey, does this HAVE to be in the .h?

[tommycli, 2017-05-19 02:04:17]

On 2017/05/18 01:13:10, Dan Beam wrote:
>
https://codereview.chromium.org/1896463003/diff/120001/content/public/browser...
> File content/public/browser/web_ui_message_handler.h (right):
> 
>
https://codereview.chromium.org/1896463003/diff/120001/content/public/browser...
> content/public/browser/web_ui_message_handler.h:84: void
> CallJavascriptFunction(const std::string& function_name,
> hey, does this HAVE to be in the .h?

In general, template functions must be defined in the .h file since the
including files need to make new specializations...

However, I think there are tricky ways of avoiding that if you pre-define the
list of specializations. I didn't think it was worth it at the time.