Add early-injection capability to user scripts. I haven't yet

Add early-injection capability to user scripts.

[Erik does not do reviews, 2009-02-02 16:31:44]

http://codereview.chromium.org/19624/diff/211/214
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/19624/diff/211/214#newcode39
Line 39: bool *run_at_document_start) {
I think it will be cleaner to just pass in a UserScriptInfo pointer rather than
pointers to its members.  As you add more metadata, this becomes unwieldy.

http://codereview.chromium.org/19624/diff/211/216
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/19624/diff/211/216#newcode1468
Line 1468: if (g_render_thread)  // Will be NULL when testing.
I'm not sure what "when testing" means.  Debug mode?  Single process? 
test_shell?  Could you be more specific?

http://codereview.chromium.org/19624/diff/211/216#newcode1536
Line 1536: if (g_render_thread)  // Will be NULL when testing.
ditto

http://codereview.chromium.org/19624/diff/211/217
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/19624/diff/211/217#newcode137
Line 137: bool UserScriptSlave::InjectScripts(WebFrame* frame, bool
is_document_start) {
Add a couple of stats counters here: time how long this took, count how many
scripts were injected.  Maybe record them as a histogram.  That way we can track
the performance impact and the usage level of the scripts.

http://codereview.chromium.org/19624/diff/211/219
File chrome/renderer/user_script_slave_unittest.cc (right):

http://codereview.chromium.org/19624/diff/211/219#newcode20
Line 20: UserScript script("foo", "bar", false);
can you add a Match4 test where early load is true?

http://codereview.chromium.org/19624/diff/211/220
File webkit/port/bindings/v8/v8_proxy.cpp (left):

http://codereview.chromium.org/19624/diff/211/220#oldcode2399
Line 2399: m_frame->loader()->dispatchWindowObjectAvailable();
I think you should bounce this off of fqian and mbelshe.  They know the binding
code the best.

[Aaron Boodman, 2009-02-11 05:23:58]

Ready for another look. I ended up modifying WebCore to work better for this use
case.

I already found a case where my modification doesn't work though: if a script
gets injected in DOCUMENT_START and calls document.write(), it seems to force
the tokenizer to finish all the data it has pending.

I haven't decided whether to try and fix this issue now or leave it as a
temporary bug, but I'd appreciate feedback on the code anyway.

http://codereview.chromium.org/19624/diff/211/214
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/19624/diff/211/214#newcode39
Line 39: bool *run_at_document_start) {
On 2009/02/02 16:31:44, Erik Kay wrote:
> I think it will be cleaner to just pass in a UserScriptInfo pointer rather
than
> pointers to its members.  As you add more metadata, this becomes unwieldy.

This comment isn't really relevant any more since the UserScriptInfo struct got
refactored away.

http://codereview.chromium.org/19624/diff/211/216
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/19624/diff/211/216#newcode1468
Line 1468: if (g_render_thread)  // Will be NULL when testing.
On 2009/02/02 16:31:44, Erik Kay wrote:
> I'm not sure what "when testing" means.  Debug mode?  Single process? 
> test_shell?  Could you be more specific?

Done.

http://codereview.chromium.org/19624/diff/211/217
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/19624/diff/211/217#newcode137
Line 137: bool UserScriptSlave::InjectScripts(WebFrame* frame, bool
is_document_start) {
On 2009/02/02 16:31:44, Erik Kay wrote:
> Add a couple of stats counters here: time how long this took, count how many
> scripts were injected.  Maybe record them as a histogram.  That way we can
track
> the performance impact and the usage level of the scripts.

Cool, I have never played with the Histogram stuff before. I wasn't sure whether
to use UMA_HISTOGRAM_* or not. Opinion?

Also, I'm not sure if I'm even doing this right. I end up with a bunch of odd
stats in about:stats that seem to be generated by the histogram code.

http://codereview.chromium.org/19624/diff/211/219
File chrome/renderer/user_script_slave_unittest.cc (right):

http://codereview.chromium.org/19624/diff/211/219#newcode20
Line 20: UserScript script("foo", "bar", false);
On 2009/02/02 16:31:44, Erik Kay wrote:
> can you add a Match4 test where early load is true?

I think I need to extract out a UserScriptFrame interface to be able to test
UserScriptSlave. I will have to do that in a separate CL.

http://codereview.chromium.org/19624/diff/211/220
File webkit/port/bindings/v8/v8_proxy.cpp (left):

http://codereview.chromium.org/19624/diff/211/220#oldcode2399
Line 2399: m_frame->loader()->dispatchWindowObjectAvailable();
On 2009/02/02 16:31:44, Erik Kay wrote:
> I think you should bounce this off of fqian and mbelshe.  They know the
binding
> code the best.

I ended up doing this differently.

[Aaron Boodman, 2009-02-11 19:04:55]

Changing reviewer to mpcomplete as erikkay is out sick.

Matt, I've decided to go ahead and check this in as-is, and fix the bug I
referred to in the previous message separately.

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

LGTM.

http://codereview.chromium.org/19624/diff/1420/407
File chrome/common/extensions/user_script_unittest.cc (right):

http://codereview.chromium.org/19624/diff/1420/407#newcode104
Line 104: TEST(UserScriptTest, Defaults) {
Do these defaults need to be this way for compatibility?  If so, add a comment. 
Otherwise, it seems strange that you have a unit test for variable
initialization :).

[Aaron Boodman, 2009-02-11 20:40:08]

On 2009/02/11 19:28:46, Matt Perry wrote:
> http://codereview.chromium.org/19624/diff/1420/407#newcode104
> Line 104: TEST(UserScriptTest, Defaults) {
> Do these defaults need to be this way for compatibility?  If so, add a
comment. 
> Otherwise, it seems strange that you have a unit test for variable
> initialization :).

Yeah, the only one I really cared about was how run_location_ was defaulted,
since there is other code that relies on it defaulting a certain way. The others
I just put in for completeness I guess.

[Erik does not do reviews, 2009-02-11 21:16:24]

LGTM

http://codereview.chromium.org/19624/diff/211/216
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/19624/diff/211/216#newcode1468
Line 1468: if (g_render_thread)  // Will be NULL when testing.
On 2009/02/11 05:23:58, Aaron Boodman wrote:
> On 2009/02/02 16:31:44, Erik Kay wrote:
> > I'm not sure what "when testing" means.  Debug mode?  Single process? 
> > test_shell?  Could you be more specific?
> 
> Done.

Looking at the most recent diff, I don't see an update to this comment.

http://codereview.chromium.org/19624/diff/211/217
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/19624/diff/211/217#newcode137
Line 137: bool UserScriptSlave::InjectScripts(WebFrame* frame, bool
is_document_start) {
On 2009/02/11 05:23:58, Aaron Boodman wrote:
> On 2009/02/02 16:31:44, Erik Kay wrote:
> > Add a couple of stats counters here: time how long this took, count how many
> > scripts were injected.  Maybe record them as a histogram.  That way we can
> track
> > the performance impact and the usage level of the scripts.
> 
> Cool, I have never played with the Histogram stuff before. I wasn't sure
whether
> to use UMA_HISTOGRAM_* or not. Opinion?
> 
> Also, I'm not sure if I'm even doing this right. I end up with a bunch of odd
> stats in about:stats that seem to be generated by the histogram code.

Yes, I'd use UMA_HISTOGRAM*.  Check out about:histograms to see the data
(about:stats will have some stuff as well).