Expose PageImportanceSignals via WebContents

Expose PageImportanceSignals via WebContents

Depends on Blink side change: https://codereview.chromium.org/1345023002/

PageImportanceSignals contains Blink-tracked signals which is useful for
judging an relative importance of the web content hosted on a tab.
For example, we can know if a tab has a pending HTML form entry, etc.

This CL introduces a method WebContents::GetPageImportanceSignals(),
which allows browser process to query PageImportanceSignals for a tab.

In near future, we plan to use this information from OOMPriorityManager to
avoid unwanted tab discards.

BUG=520838
Committed: https://crrev.com/40f03cb9cd139d4dc5d1c22d5ee390a67bb15349
Cr-Commit-Position: refs/heads/master@{#350494}

[kouhei (in TOK), 2015-09-15 23:08:50]

kouhei@chromium.org changed reviewers:
+ mkwst@chromium.org, tzik@chromium.org

[kouhei (in TOK), 2015-09-15 23:08:50]

kouhei@chromium.org changed required reviewers:
+ mkwst@chromium.org

[kouhei (in TOK), 2015-09-15 23:08:50]

Would you take a look?
Requres Blink side: https://codereview.chromium.org/1345023002/

[Mike West, 2015-09-16 12:41:59]

IPC LGTM.

[Mike West, 2015-09-16 12:42:47]

On 2015/09/16 at 12:41:59, Mike West (slow) wrote:
> IPC LGTM.

(Please add a link to the blink-side CL to the CL description, and label this
something like "(2/2)" for clarity).

[kouhei (in TOK), 2015-09-16 13:26:42]

kouhei@chromium.org changed reviewers:
+ georgesak@chromium.org

[kouhei (in TOK), 2015-09-16 13:27:10]

georgesak: Does the WebContents interface look good to you?

On 2015/09/16 12:42:47, Mike West (slow) wrote:
> On 2015/09/16 at 12:41:59, Mike West (slow) wrote:
> > IPC LGTM.
> 
> (Please add a link to the blink-side CL to the CL description, and label this
> something like "(2/2)" for clarity).

Thanks!

[Georges Khalil, 2015-09-18 12:50:30]

lgtm for the API, thanks!

[kouhei (google), 2015-09-24 00:12:27]

kouhei@google.com changed reviewers:
+ kinuko@chromium.org, kouhei@google.com

[kouhei (google), 2015-09-24 00:12:28]

kinuko: Would you take a look?

[kinuko, 2015-09-24 00:26:23]

looks good, a few nits

https://codereview.chromium.org/1345873003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1345873003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:916: const
PageImportanceSignals* WebContentsImpl::GetPageImportanceSignals() const {
nit: maybe could be a const ref if this can't be non-null

https://codereview.chromium.org/1345873003/diff/1/content/public/common/page_...
File content/public/common/page_importance_signals.h (right):

https://codereview.chromium.org/1345873003/diff/1/content/public/common/page_...
content/public/common/page_importance_signals.h:15: bool had_form_interaction;
nit: are you sure this'd be a bool rather than enum? (just checking)

https://codereview.chromium.org/1345873003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1345873003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3525: if (!webview())
nit: Could this happen when this is signaled?

https://codereview.chromium.org/1345873003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3530: if (!web_signals)
ditto

[kouhei (in TOK), 2015-09-24 01:13:46]

Thanks for review!

https://codereview.chromium.org/1345873003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1345873003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:916: const
PageImportanceSignals* WebContentsImpl::GetPageImportanceSignals() const {
On 2015/09/24 00:26:22, kinuko wrote:
> nit: maybe could be a const ref if this can't be non-null

Done.

https://codereview.chromium.org/1345873003/diff/1/content/public/common/page_...
File content/public/common/page_importance_signals.h (right):

https://codereview.chromium.org/1345873003/diff/1/content/public/common/page_...
content/public/common/page_importance_signals.h:15: bool had_form_interaction;
On 2015/09/24 00:26:22, kinuko wrote:
> nit: are you sure this'd be a bool rather than enum? (just checking)

Yes.

https://codereview.chromium.org/1345873003/diff/1/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1345873003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3525: if (!webview())
On 2015/09/24 00:26:22, kinuko wrote:
> nit: Could this happen when this is signaled?

Yes. This can happen when the view is closing.

https://codereview.chromium.org/1345873003/diff/1/content/renderer/render_vie...
content/renderer/render_view_impl.cc:3530: if (!web_signals)
On 2015/09/24 00:26:23, kinuko wrote:
> ditto

web_signals is guaranteed to be non-null now. Fixed.

[kinuko, 2015-09-24 01:19:16]

kinuko@chromium.org changed required reviewers:
- mkwst@chromium.org

[kinuko, 2015-09-24 01:19:17]

lgtm.

[kouhei (in TOK), 2015-09-24 01:19:37]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-09-24 01:19:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
georgesak@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1345873003/#ps20001
(title: "kinuko-san review")

[kouhei (in TOK), 2015-09-24 01:19:42]

On 2015/09/24 01:19:17, kinuko wrote:
> lgtm.

Thanks!

[commit-bot: I haz the power, 2015-09-24 01:35:38]

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

[commit-bot: I haz the power, 2015-09-24 02:32:56]

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

[commit-bot: I haz the power, 2015-09-24 02:32:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kouhei (in TOK), 2015-09-24 03:36:16]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-09-24 03:36:17]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
georgesak@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1345873003/#ps40001
(title: "add missing null check")

[commit-bot: I haz the power, 2015-09-24 03:36:46]

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

[commit-bot: I haz the power, 2015-09-24 04:58:15]

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

[commit-bot: I haz the power, 2015-09-24 04:58:16]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[kouhei (in TOK), 2015-09-24 05:16:42]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-09-24 05:16:55]

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

[commit-bot: I haz the power, 2015-09-24 07:47:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-24 07:48:14]

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

[noyau (Ping after 24h), 2015-09-28 15:36:10]

noyau@chromium.org changed reviewers:
+ noyau@chromium.org

[noyau (Ping after 24h), 2015-09-28 15:36:10]

https://codereview.chromium.org/1345873003/diff/40001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1345873003/diff/40001/content/public/browser/...
content/public/browser/web_contents.h:282: virtual const PageImportanceSignals&
GetPageImportanceSignals() const = 0;
Please add comments to methods in interfaces: We have an iOS subclass for this
and on first glance I have no idea what this method is supposed to do.