Add hooks for capturing meaningful text info, gently.

Add hooks for capturing meaningful text info, gently.

The two hooks are:
* WebLocalFrame::isNavigationPending
* WebViewClient::didFirstLayoutAfterFinishedParsing

The WebLocalFrame::isNavigationPending provides a
simple check whether the content of the document intends to
change the location (via Refresh or window.location change).

The WebViewClient::didFirstLayoutAfterFinishedParsing is called
when we finish the first layout after the document had finished
parsing. Another way to explain it is "first layout after
DOMContentLoaded".

BUG=521149
R=esprehn

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201042

[dglazkov, 2015-08-21 22:27:47]

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

[commit-bot: I haz the power, 2015-08-21 22:27:59]

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

[Nate Chapin, 2015-08-21 22:38:31]

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

[Nate Chapin, 2015-08-21 22:38:31]

https://codereview.chromium.org/1309883003/diff/1/Source/web/WebLocalFrameImp...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1309883003/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.cpp:2121: return frame() &&
(frame()->navigationScheduler().isRedirect() ||
frame()->navigationScheduler().locationChangePending());
Is there a reason this has to be picky about what kind of navigation is being
scheduled? I'm not thrilled with adding more locationChangePending() callers.

https://codereview.chromium.org/1309883003/diff/1/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/1309883003/diff/1/public/web/WebViewClient.h#...
public/web/WebViewClient.h:201: virtual void
didFirstLayoutAfterFinishedParsing() { }
This seems like it it is a property of the frame and not the view. Is there a
reason this should be main frame specific? If not, it should probably go on
WebFrameClient.

[dglazkov, 2015-08-21 22:54:53]

https://codereview.chromium.org/1309883003/diff/1/Source/web/WebLocalFrameImp...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1309883003/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.cpp:2121: return frame() &&
(frame()->navigationScheduler().isRedirect() ||
frame()->navigationScheduler().locationChangePending());
Good question. For the specific user of the API, it doesn't seem very
interesting. Should I just change it to isNavigationScheduled?

https://codereview.chromium.org/1309883003/diff/1/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/1309883003/diff/1/public/web/WebViewClient.h#...
public/web/WebViewClient.h:201: virtual void
didFirstLayoutAfterFinishedParsing() { }
It's currently only called on main frame, because it's actually tied to
producing a frame (in the rendering sense -- the WebViewImpl::layout), rather
than just layout (in the document layout sense -- the FrameView::layout). I
don't have a better name for that, but maybe we should invent something.

[Nate Chapin, 2015-08-21 23:00:29]

https://codereview.chromium.org/1309883003/diff/1/Source/web/WebLocalFrameImp...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1309883003/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.cpp:2121: return frame() &&
(frame()->navigationScheduler().isRedirect() ||
frame()->navigationScheduler().locationChangePending());
On 2015/08/21 22:54:53, dglazkov wrote:
> Good question. For the specific user of the API, it doesn't seem very
> interesting. Should I just change it to isNavigationScheduled?

I think that's preferable, yeah.

https://codereview.chromium.org/1309883003/diff/1/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/1309883003/diff/1/public/web/WebViewClient.h#...
public/web/WebViewClient.h:201: virtual void
didFirstLayoutAfterFinishedParsing() { }
On 2015/08/21 22:54:53, dglazkov wrote:
> It's currently only called on main frame, because it's actually tied to
> producing a frame (in the rendering sense -- the WebViewImpl::layout), rather
> than just layout (in the document layout sense -- the FrameView::layout). I
> don't have a better name for that, but maybe we should invent something.
> 

Ok, makes sense.

I'm mostly concerned about this being OOPIF-friendly. I guess this is currently?
Adding things to WebViewClient just sets off alarms for me.

[dglazkov, 2015-08-21 23:14:46]

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

[commit-bot: I haz the power, 2015-08-21 23:15:20]

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

[dglazkov, 2015-08-21 23:16:05]

PTAL.

[commit-bot: I haz the power, 2015-08-21 23:30:04]

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

[commit-bot: I haz the power, 2015-08-21 23:30:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[esprehn, 2015-08-21 23:32:27]

On 2015/08/21 at 23:16:05, dglazkov wrote:
> PTAL.

Do the users of this API actually want to wait until onload? It seems like
translate/distiller would want to wait for dynamic JS apps to start?

WebViewClient seems okay since this notification is only for top level pages
never frames so oopif shouldn't be involved as the main frame of a tab should
always be local.

[dglazkov, 2015-08-22 00:07:39]

On 2015/08/21 at 23:32:27, esprehn wrote:
> On 2015/08/21 at 23:16:05, dglazkov wrote:
> > PTAL.
> 
> Do the users of this API actually want to wait until onload? It seems like
translate/distiller would want to wait for dynamic JS apps to start?

Good question. Both distiller and translate have the logic to do the text scan
twice: once after load (didStopLoading), and once sometime before that. The
translate has an arbitrary 8-second timer. The distiller has something similar.
Both do this because didStopLoading can take too long. My thinking was to first
remove that arbitrary timing thing and replace it with this callback (see
https://codereview.chromium.org/1308113002/), then tackle first layout after
onload.

Speaking of which -- this looks like an "interesting layouts" API. I was
thinking that as a next step, I should combine this new function and
WebViewImpl::layoutUpdated, and return an enum indicating the "interestness".
WDYT?

[dglazkov, 2015-08-22 00:07:52]

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

[commit-bot: I haz the power, 2015-08-22 00:08:11]

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

[commit-bot: I haz the power, 2015-08-22 01:55:05]

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

[commit-bot: I haz the power, 2015-08-22 01:55:05]

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

[dglazkov, 2015-08-22 03:21:21]

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

[commit-bot: I haz the power, 2015-08-22 03:21:30]

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

[commit-bot: I haz the power, 2015-08-22 04:08:09]

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

[commit-bot: I haz the power, 2015-08-22 04:08:10]

Dry run: This issue passed the CQ dry run.

[esprehn, 2015-08-22 15:14:54]

lgtm but your description needs updating.

https://codereview.chromium.org/1309883003/diff/40001/public/web/WebLocalFrame.h
File public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1309883003/diff/40001/public/web/WebLocalFram...
public/web/WebLocalFrame.h:103: virtual bool isNavigationScheduled() const = 0;
Your description needs updating, it mentions the old name and purpose.

[dglazkov, 2015-08-22 15:29:29]

On 2015/08/22 at 15:14:54, esprehn wrote:
> lgtm but your description needs updating.
> 
>
https://codereview.chromium.org/1309883003/diff/40001/public/web/WebLocalFrame.h
> File public/web/WebLocalFrame.h (right):
> 
>
https://codereview.chromium.org/1309883003/diff/40001/public/web/WebLocalFram...
> public/web/WebLocalFrame.h:103: virtual bool isNavigationScheduled() const =
0;
> Your description needs updating, it mentions the old name and purpose.

Durr!!

[dglazkov, 2015-08-22 15:29:42]

The CQ bit was checked by dglazkov@chromium.org

[commit-bot: I haz the power, 2015-08-22 15:29:49]

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

[commit-bot: I haz the power, 2015-08-22 15:33:29]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201042

[nasko, 2015-08-24 22:57:49]

nasko@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org

[nasko, 2015-08-24 22:57:50]

One comment from me, but adding dcheng@, who is the OOPIF master of Blink.

https://codereview.chromium.org/1309883003/diff/40001/public/web/WebViewClient.h
File public/web/WebViewClient.h (right):

https://codereview.chromium.org/1309883003/diff/40001/public/web/WebViewClien...
public/web/WebViewClient.h:201: virtual void
didFirstLayoutAfterFinishedParsing() { }
Is there a reason this method isn't on WebFrameClient? Even the comment on it
talks about the "frame's document".

[dglazkov, 2015-08-25 02:58:14]

On 2015/08/24 at 22:57:50, nasko wrote:
> One comment from me, but adding dcheng@, who is the OOPIF master of Blink.
> 
>
https://codereview.chromium.org/1309883003/diff/40001/public/web/WebViewClient.h
> File public/web/WebViewClient.h (right):
> 
>
https://codereview.chromium.org/1309883003/diff/40001/public/web/WebViewClien...
> public/web/WebViewClient.h:201: virtual void
didFirstLayoutAfterFinishedParsing() { }
> Is there a reason this method isn't on WebFrameClient? Even the comment on it
talks about the "frame's document".

See earlier discussion on comment 6:
https://codereview.chromium.org/1309883003/#msg6