Add new user script injection point: document_idle.

Add new user script injection point "document_idle" and make it the default.

Semantically, document-idle means "when the DOM is ready and layout has been idle for awhile", or more loosely, "as soon as we get around to it".

Right now this uses a simple heuristic. It injects scripts 200ms after DOMContentLoaded, or immediately after onload, whichever happens first.

BUG=26126
TEST=Manual. Extensions with content scripts should work.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30784

[Aaron Boodman, 2009-10-29 08:26:10]

*** NOT FOR SUBMISSION -- feedback only ***

[Erik does not do reviews, 2009-10-29 15:06:07]

Yes, this looks roughly like what I was thinking.

Two issues popped out at me:
- could we still be inside of layout in didFinishDocumentLoad?
- I think you need a matching willLayout() as well.  Since we want to
only inject when there's been a 10ms gap between layouts, if you don't
stop when a layout begins, you'll wind up counting layout time in your
idle timer.

Erik


On Thu, Oct 29, 2009 at 1:26 AM,  <aa@chromium.org> wrote:
> Reviewers: Erik Kay, Matt Perry,
>
> Message:
> *** NOT FOR SUBMISSION -- feedback only ***
>
> Description:
> Idle injection idea.
>
> See also the upstream changes:
> http://codereview.chromium.org/340032
>
> Per our offline discussion, here is a first hack at making
> idle-time content script injection work. Basically, the
> strategy is to inject the first time there is no layout for
> 10ms after the DOM is ready.
>
> The code is messy, and I need to implement a new injection
> point instead of reusing =A0DOCUMENT_END. But other than that,
> is this along the lines of what you guys were thinking?
>
> If so, I think the next step is to add some instrumentation
> to find out whether, for pages like google.com this causes
> us to run after onload (which I think would be desirable). I
> could see tuning it down as low as like 100ms if necessary.
>
> 100ms is not going to make any difference at all in the
> perceived performance of scripts that don't need to run
> at DOMContentLoaded.
>
> Please review this at http://codereview.chromium.org/339064
>
> Affected files:
> =A0M chrome/renderer/render_view.h
> =A0M chrome/renderer/render_view.cc
> =A0M webkit/api/public/WebFrameClient.h
> =A0M webkit/glue/empty_webframeclient.h
> =A0M webkit/glue/webframeloaderclient_impl.h
> =A0M webkit/glue/webframeloaderclient_impl.cc
>
>
>

[Matt Perry, 2009-10-29 18:47:35]

This makes sense to me. I only notice some unrelated issues (like the fact that
frame could be deleted before your timer fires), but the general approach seems
good.

[Aaron Boodman, 2009-10-30 00:06:45]

This is ready for a real review now.

[Matt Perry, 2009-10-30 00:15:13]

http://codereview.chromium.org/339064/diff/2002/2007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/339064/diff/2002/2007#newcode2297
Line 2297: void RenderView::InjectIdleUserScripts(WebKit::WebFrame* frame) {
I don't think there's any guarantee that frame will still be valid at this
point. It could have been deleted after the PostTask, or after setting the
timer. Maybe you can just traverse through the frame hierarchy to verify that
'frame' is still in there. Otherwise I think you have to watch for didCloseFrame
(or whatever the name is) callbacks.

[Erik does not do reviews, 2009-10-30 04:15:24]

LGTM

looks like you handle the closing frame case already

http://codereview.chromium.org/339064/diff/2002/2007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/339064/diff/2002/2007#newcode208
Line 208: const int kIdleUserScriptInjectionTimeout = 200;
200ms seems a little high to me.  I'd be tempted to take it down to 100ms.

http://codereview.chromium.org/339064/diff/2002/2007#newcode216
Line 216: void RenderView::IdleUserScriptInjectionTimer::OnTimer() {
nit: newline above

[Aaron Boodman, 2009-10-31 06:55:03]

Ok, new take based on ideas from darin and abarth (cc'd).

Matt, can you take a fresh look at this? Darin, abarth, just an fyi for you.

[abarth-chromium, 2009-10-31 07:07:39]

http://codereview.chromium.org/339064/diff/6010/6022
File chrome/renderer/user_script_idle_scheduler.cc (right):

http://codereview.chromium.org/339064/diff/6010/6022#newcode19
Line 19: frame_(frame), has_run_(false) {
These should each be on a different line according to the style guide.

[Matt Perry, 2009-11-02 19:46:30]

LGTM with small changes.

http://codereview.chromium.org/339064/diff/6010/6019
File chrome/renderer/navigation_state.h (right):

http://codereview.chromium.org/339064/diff/6010/6019#newcode37
Line 37: UserScriptIdleScheduler* user_script_idle_scheduler() {
Very clean. I like it.

http://codereview.chromium.org/339064/diff/6010/6020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/339064/diff/6010/6020#newcode1819
Line 1819: WebDataSource* ds = frame->dataSource();
Can this ever be NULL?

http://codereview.chromium.org/339064/diff/6010/6020#newcode3685
Line 3685: ExecuteCodeImpl(main_frame, request_id, extension_id, is_js_code,
code_string);
line length

http://codereview.chromium.org/339064/diff/6010/6021
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/339064/diff/6010/6021#newcode439
Line 439: void OnUserScriptIdleLocation(WebKit::WebFrame* frame);
"Location" is a confusing descriptor here. How about something like "Triggered"?

http://codereview.chromium.org/339064/diff/6010/6024
File
chrome/test/data/extensions/good/Extensions/bjafgdebaacbbbecmhlhpofkepfkgcpa/1.0/page.js
(right):

http://codereview.chromium.org/339064/diff/6010/6024#newcode6
Line 6: // Do this in an onload handler because I'm not sure if chrome.extension
remove this comment

[Aaron Boodman, 2009-11-03 05:03:11]

fyi...

http://codereview.chromium.org/339064/diff/6010/6020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/339064/diff/6010/6020#newcode1819
Line 1819: WebDataSource* ds = frame->dataSource();
On 2009/11/02 19:46:30, Matt Perry wrote:
> Can this ever be NULL?

No, it is created in didCreateDataSource(), and many places in RenderView assume
it exists after that.

http://codereview.chromium.org/339064/diff/6010/6021
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/339064/diff/6010/6021#newcode439
Line 439: void OnUserScriptIdleLocation(WebKit::WebFrame* frame);
On 2009/11/02 19:46:30, Matt Perry wrote:
> "Location" is a confusing descriptor here. How about something like
"Triggered"?

Done.

[darin (slow to review), 2009-11-03 05:10:59]

http://codereview.chromium.org/339064/diff/6010/6019
File chrome/renderer/navigation_state.h (right):

http://codereview.chromium.org/339064/diff/6010/6019#newcode185
Line 185: user_script_idle_scheduler_(NULL) {
nit: no need to NULL out a scoped_ptr

http://codereview.chromium.org/339064/diff/6010/6020
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/339064/diff/6010/6020#newcode1819
Line 1819: WebDataSource* ds = frame->dataSource();
On 2009/11/03 05:03:11, Aaron Boodman wrote:
> On 2009/11/02 19:46:30, Matt Perry wrote:
> > Can this ever be NULL?
> 
> No, it is created in didCreateDataSource(), and many places in RenderView
assume
> it exists after that.

Right, it cannot be null!