Move *GetWebApplicationInfo IPCs from "view" to "frame".

Move *GetWebApplicationInfo IPCs from "view" to "frame".

This CL addresses a TODO from
chrome/renderer/chrome_render_view_observer.cc to associate
ChromeViewMsg_GetWebApplicationInfo and
ChromeViewHostMsg_DidGetWebApplicationInfo IPCs with frame, rather than
view.

This CL helps ensure that
ChromeRenderFrameObserver::OnGetWebApplicationInfo (formerly a method of
ChromeRender*View*Observer) always operates on a local frame (and
therefore this CL helps with moving GetDocument method from WebFrame to
WebLocalFrame in a follow-up CL - https://crrev.com/2928033002).

BUG=416660
Review-Url: https://codereview.chromium.org/2942583004
Cr-Commit-Position: refs/heads/master@{#481026}
Committed: https://chromium.googlesource.com/chromium/src/+/53fe8fd80924fbc0e06f92ec85d5a7207d65ce77

[Łukasz Anforowicz, 2017-06-14 23:09:27]

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

[commit-bot: I haz the power, 2017-06-14 23:09:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 23:39:39]

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

[commit-bot: I haz the power, 2017-06-14 23:39:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Łukasz Anforowicz, 2017-06-14 23:54:37]

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

[commit-bot: I haz the power, 2017-06-14 23:54:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 01:11:06]

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

[commit-bot: I haz the power, 2017-06-15 01:11:07]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-15 04:05:38]

lukasza@chromium.org changed reviewers:
+ mlamouri@chromium.org

[Łukasz Anforowicz, 2017-06-15 04:05:40]

mlamouri@, I wonder if you could PTAL?  (I understand that you might not own
most of the files in this CL, but the TODO being fixed is in your name and you
seem to know quite a bit about web apps;  please shout if you think I should get
somebody from //content/OWNERS to take a first look).

https://codereview.chromium.org/2942583004/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_frame_observer.cc (right):

https://codereview.chromium.org/2942583004/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_frame_observer.cc:244: WebLocalFrame* frame =
render_frame()->GetWebFrame();
The body of this method is mostly copied from the removed
ChromeRenderViewObserver::OnGetWebApplicationInfo (except using |frame| as
appropriate).

https://codereview.chromium.org/2942583004/diff/20001/chrome/renderer/chrome_...
File chrome/renderer/chrome_render_view_observer.cc (left):

https://codereview.chromium.org/2942583004/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:99: // need to check whether or
not the main frame is local.
This is the TODO being addressed by this CL.

https://codereview.chromium.org/2942583004/diff/20001/chrome/renderer/chrome_...
chrome/renderer/chrome_render_view_observer.cc:108:
main_frame->ToWebLocalFrame()->AddMessageToConsole(message);
After this CL, we can avoid ToWebLocalFrame "cast".  This helps with a follow-up
attempt (https://codereview.chromium.org/2928033002) to move GetDocument
accessor from WebFrame to WebLocalFrame (since in case of WebRemoteFrameImpl
GetDocument returns a likely-to-crash null document).

[mlamouri (slow - plz ping), 2017-06-15 13:44:27]

lgtm

Thanks for doing this! :)

[Łukasz Anforowicz, 2017-06-15 15:07:39]

lukasza@chromium.org changed reviewers:
+ jochen@chromium.org

[Łukasz Anforowicz, 2017-06-15 15:07:39]

jochen@, could you PTAL?  (you are an OWNER for 12/13 files in this CL :-)

[Łukasz Anforowicz, 2017-06-15 17:40:50]

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

[commit-bot: I haz the power, 2017-06-15 17:41:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 19:02:56]

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

[commit-bot: I haz the power, 2017-06-15 19:02:57]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-06-19 13:04:32]

can you help me understand why you had to change the OnMessageReceived method to
take the sender arg?

https://codereview.chromium.org/2942583004/diff/40001/content/public/test/tes...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2942583004/diff/40001/content/public/test/tes...
content/public/test/test_renderer_host.h:64: const IPC::Message& msg);
why is the order of args reversed (compared to OnMessageReceived)?

[Łukasz Anforowicz, 2017-06-19 14:18:47]

On 2017/06/19 13:04:32, jochen wrote:
> can you help me understand why you had to change the OnMessageReceived method
to
> take the sender arg?

WebContentsObserver has 2 overloads of OnMessageReceived:

1. bool OnMessageReceived(const IPC::Message& message) override;

This is overrides IPC::Listener's method.  This overload is called for messages
sent to RenderViewHost's routing id.

2. virtual bool OnMessageReceived(const IPC::Message& message,
                                 RenderFrameHost* render_frame_host);

The overload taking RenderFrameHost* sender argument is called (via
WebContentsImpl::OnMessageReceived) from RenderFrameHost::OnMessageReceived to
handle messages sent to a specific RenderFrameHost's routing id.


Because I am changing ChromeViewHostMsg_DidGetWebApplicationInfo into
Chrome*Frame*HostMsg_DidGetWebApplicationInfo (and moving how it is sent from
ChromeRender*View*Observer::OnGetWebApplicationInfo to
ChromeRenderFrameObserver::OnGetWebApplicationInfo), I also have to make sure
the message is handled by the right overload of OnMessageReceived.

https://codereview.chromium.org/2942583004/diff/40001/content/public/test/tes...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2942583004/diff/40001/content/public/test/tes...
content/public/test/test_renderer_host.h:64: const IPC::Message& msg);
On 2017/06/19 13:04:32, jochen wrote:
> why is the order of args reversed (compared to OnMessageReceived)?

For consistency with the already existing
RenderViewHostTester::TestOnMessageReceived.  I think having rvh/rfh as the
first argument makes sense, because this argument sort of acts like |this| would
(i.e. TestOnMessageReceived could alternatively be implemented as a
OnMessageReceivedForTesting *instance* method of RenderViewHost/RenderFrameHost;
 OTOH, I like the original author's idea of having this method here to avoid
polluting the public API of RVH/RFH with test-only methods).

[jochen (gone - plz use gerrit), 2017-06-20 06:46:19]

lgtm

[Łukasz Anforowicz, 2017-06-20 15:07:30]

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

[commit-bot: I haz the power, 2017-06-20 15:07:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-06-20 15:08:07]

dcheng@, could you PTAL do an IPC security review of @
chrome/common/render_messages.h?

[commit-bot: I haz the power, 2017-06-20 16:28:29]

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

[commit-bot: I haz the power, 2017-06-20 16:28:30]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-20 19:39:48]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-06-20 19:39:49]

Actually adding dcheng@ this time... (PTAL @ chrome/common/render_messages.h).

[Łukasz Anforowicz, 2017-06-20 19:47:55]

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

[commit-bot: I haz the power, 2017-06-20 19:48:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 21:07:41]

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

[commit-bot: I haz the power, 2017-06-20 21:07:42]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-20 22:07:09]

Description was changed from

==========
Move *GetWebApplicationInfo IPCs from "view" to "frame".

This CL addresses a TODO from
chrome/renderer/chrome_render_view_observer.cc to associate
ChromeViewMsg_GetWebApplicationInfo and
ChromeViewHostMsg_DidGetWebApplicationInfo IPCs with frame, rather than
view.

This CL helps ensure that
ChromeRenderFrameObserver::OnGetWebApplicationInfo (formerly a method of
ChromeRender*View*Observer) always operates on a local frame (and
therefore this CL helps with moving [at a later time] GetDocument method
from WebFrame to WebLocalFrame).

BUG=416660
==========

to

==========
Move *GetWebApplicationInfo IPCs from "view" to "frame".

This CL addresses a TODO from
chrome/renderer/chrome_render_view_observer.cc to associate
ChromeViewMsg_GetWebApplicationInfo and
ChromeViewHostMsg_DidGetWebApplicationInfo IPCs with frame, rather than
view.

This CL helps ensure that
ChromeRenderFrameObserver::OnGetWebApplicationInfo (formerly a method of
ChromeRender*View*Observer) always operates on a local frame (and
therefore this CL helps with moving GetDocument method from WebFrame to
WebLocalFrame in a follow-up CL - https://crrev.com/2928033002).

BUG=416660
==========

[dcheng, 2017-06-20 23:29:43]

ipc lgtm

(Btw, is this another example of a main-frame only IPC? seems like it?)

[Łukasz Anforowicz, 2017-06-20 23:33:13]

On 2017/06/20 23:29:43, dcheng wrote:
> ipc lgtm

Thanks.

> (Btw, is this another example of a main-frame only IPC? seems like it?)

Yes.  I think that this IPC is frame-oriented, because it extracts data from the
"document" and therefore any html frame can handle this IPC.  It just so
happens, that this IPC is only ever sent to the main frame... :-/

[Łukasz Anforowicz, 2017-06-20 23:33:21]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-06-20 23:33:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2942583004/#ps80001
(title: "Rebasing...")

[commit-bot: I haz the power, 2017-06-20 23:33:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 23:38:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498001601105950,
"parent_rev": "f9af74e761a9ff05a6d2b42197a961b439cc10b7", "commit_rev":
"53fe8fd80924fbc0e06f92ec85d5a7207d65ce77"}

[commit-bot: I haz the power, 2017-06-20 23:38:53]

Description was changed from

==========
Move *GetWebApplicationInfo IPCs from "view" to "frame".

This CL addresses a TODO from
chrome/renderer/chrome_render_view_observer.cc to associate
ChromeViewMsg_GetWebApplicationInfo and
ChromeViewHostMsg_DidGetWebApplicationInfo IPCs with frame, rather than
view.

This CL helps ensure that
ChromeRenderFrameObserver::OnGetWebApplicationInfo (formerly a method of
ChromeRender*View*Observer) always operates on a local frame (and
therefore this CL helps with moving GetDocument method from WebFrame to
WebLocalFrame in a follow-up CL - https://crrev.com/2928033002).

BUG=416660
==========

to

==========
Move *GetWebApplicationInfo IPCs from "view" to "frame".

This CL addresses a TODO from
chrome/renderer/chrome_render_view_observer.cc to associate
ChromeViewMsg_GetWebApplicationInfo and
ChromeViewHostMsg_DidGetWebApplicationInfo IPCs with frame, rather than
view.

This CL helps ensure that
ChromeRenderFrameObserver::OnGetWebApplicationInfo (formerly a method of
ChromeRender*View*Observer) always operates on a local frame (and
therefore this CL helps with moving GetDocument method from WebFrame to
WebLocalFrame in a follow-up CL - https://crrev.com/2928033002).

BUG=416660

Review-Url: https://codereview.chromium.org/2942583004
Cr-Commit-Position: refs/heads/master@{#481026}
Committed:
https://chromium.googlesource.com/chromium/src/+/53fe8fd80924fbc0e06f92ec85d5...
==========

[commit-bot: I haz the power, 2017-06-20 23:38:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/53fe8fd80924fbc0e06f92ec85d5...