Add 'NavigationHandle::QueueConsoleMessage'.

Add 'NavigationHandle::QueueConsoleMessage'.

NavigationThrottles like AncestorThrottle[1] and ClearSiteDataThrottle[2]
need to dump messages to the console in order to inform developers of the
success or failure of the web-facing features they implement. Rather than
forcing each to come up with a message queuing system, we can abstract
that into a simple mechanism hanging off of 'NavigationHandle' itself.

This patch adds that mechanism, but only uses it in tests; real usage
won't land until the aforementioned throttles land.

[1]: https://codereview.chromium.org/1617043002/
[2]: https://codereview.chromium.org/2025683003

BUG=555418, 607897
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[Mike West, 2016-06-28 10:00:57]

Description was changed from

==========
Add 'NavigationHandle::QueueConsoleMessage'.

NavigationThrottles like AncestorThrottle[1] and ClearSiteDataThrottle[2]
need to dump messages to the console in order to inform developers of the
success or failure of the web-facing features they implement. Rather than
forcing each to come up with a message queuing system, we can abstract
that into a simple mechanism hanging off of 'NavigationHandle' itself.

This patch adds that mechanism, but only uses it in tests; real usage
won't land until the aforementioned throttles land.

[1]: https://codereview.chromium.org/1617043002/
[2]: https://codereview.chromium.org/2025683003

BUG=555418,607897
==========

to

==========
Add 'NavigationHandle::QueueConsoleMessage'.

NavigationThrottles like AncestorThrottle[1] and ClearSiteDataThrottle[2]
need to dump messages to the console in order to inform developers of the
success or failure of the web-facing features they implement. Rather than
forcing each to come up with a message queuing system, we can abstract
that into a simple mechanism hanging off of 'NavigationHandle' itself.

This patch adds that mechanism, but only uses it in tests; real usage
won't land until the aforementioned throttles land.

[1]: https://codereview.chromium.org/1617043002/
[2]: https://codereview.chromium.org/2025683003

BUG=555418,607897
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-28 10:03:11]

mkwst@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[Mike West, 2016-06-28 10:03:11]

nasko@, clamy@: Based on the discussion in
https://codereview.chromium.org/2097083002#msg10, this seems like it might be a
reasonable abstraction for console messages that 'AncestorThrottle' and
'ClearSiteDataThrottle' wish to send. WDYT?

msramek@: FYI. This might simplify https://codereview.chromium.org/2025683003 a
bit by allowing you to remove the console message queuing bits and pieces.

Thanks!

[clamy, 2016-06-28 11:30:38]

Thanks!

https://codereview.chromium.org/2101323002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2101323002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:406:
DumpMessagesToConsole();
This is problematic right now for transfer navigations. I'm not thrilled about
landing this until we get ReadyToCommitNavigation to execute on all sorts of
navigations.

https://codereview.chromium.org/2101323002/diff/20001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2101323002/diff/20001/content/public/browser/...
content/public/browser/navigation_handle.h:167: virtual void
QueueConsoleMessage(ConsoleMessageLevel level,
The policy of content is not to add a method to the public API if it is not used
outside of content. Since this is not happening in this patch, this should be
added to NavigationHandleImpl instead, and then made available to the public API
when there is an external use case.

[Mike West, 2016-06-28 12:22:33]

https://codereview.chromium.org/2101323002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2101323002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:406:
DumpMessagesToConsole();
On 2016/06/28 at 11:30:38, clamy wrote:
> This is problematic right now for transfer navigations. I'm not thrilled about
landing this until we get ReadyToCommitNavigation to execute on all sorts of
navigations.

It's no more problematic than getting the RenderFrameHost in the first place, is
it?

We could move it to 'ReadyToCommitNavigation' now, and just recognize that we'll
lose some console messages in the meantime. Though, now that I think about it,
that seems incorrect for cases in which the throttle will actually cancel the
navigation. In those cases, we'll never actually hit ReadyToCommitNavigation
(see line 412 below)... Hrm.

https://codereview.chromium.org/2101323002/diff/20001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2101323002/diff/20001/content/public/browser/...
content/public/browser/navigation_handle.h:167: virtual void
QueueConsoleMessage(ConsoleMessageLevel level,
On 2016/06/28 at 11:30:38, clamy wrote:
> The policy of content is not to add a method to the public API if it is not
used outside of content. Since this is not happening in this patch, this should
be added to NavigationHandleImpl instead, and then made available to the public
API when there is an external use case.

You're right, sorry. I'd convinced myself that
https://codereview.chromium.org/2025683003 was calling this from //chrome
because that's where the BrowsingDataRemover lives. Fortunately, the throttle is
implemented in //content. I'll drop this addition. :)

[nasko, 2016-06-28 22:25:46]

https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:241: void
QueueConsoleMessage(ConsoleMessageLevel level,
bikeshed: AppendConsoleMessage? Queue to me implies that it always adds to a
queue, which seems incorrect based on the description. Appending is always what
we want to do, regardless of whether we need to keep them in a queue for some
period of time.

[Mike West, 2016-06-29 05:37:02]

On 2016/06/28 at 22:25:46, nasko wrote:
>
https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/navigation_handle_impl.h (right):
> 
>
https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/navigation_handle_impl.h:241: void
QueueConsoleMessage(ConsoleMessageLevel level,
> bikeshed: AppendConsoleMessage? Queue to me implies that it always adds to a
queue, which seems incorrect based on the description. Appending is always what
we want to do, regardless of whether we need to keep them in a queue for some
period of time.

*shrug* Technically, we always append it to the queue in this implementation.
Sometimes we just dump the queue right away. ;) But yes, point taken.
"AddMessageToConsole" would reflect the RFH method, so it's probably best to use
that name rather than inventing a new one.

[clamy, 2016-06-29 11:07:29]

Thanks! A few more questions.

https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:240: // committed will be
delivered immediately.
The navigation commit corresponds to the destruction of this NavigationHandle,
so this comment seems a bit misleading to me. How about "at commit time" instead
of "after the navigation has committed".

There's also the question of what happens if the navigation doesn't commit.
Should we just drop them? Deliver them to the current RenderFrameHost of the
FrameTreeNode (maybe only if it was renderer initiated)? Once we come to an
agreement, this should be precised in the comment.

https://codereview.chromium.org/2101323002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:282: virtual void
DumpMessagesToConsole();
Why virtual?