Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(36)

Issue 19624: Add early-injection capability to user scripts. I haven't yet (Closed)

Created:
10 years, 6 months ago by Aaron Boodman
Modified:
8 years, 2 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Add early-injection capability to user scripts.

Patch Set 1 #

Patch Set 2 : merge upstream changes, add @run-at for standalone scripts #

Patch Set 3 : Add commentary #

Total comments: 13

Patch Set 4 : A different approach. This one changes WebCore to add a notification at a better place. #

Patch Set 5 : Use new documentElementAvailable() callback #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+99 lines, -15 lines) Patch
M chrome/browser/extensions/extension.h View 1 2 3 4 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension.cc View 1 2 3 4 3 chunks +23 lines, -0 lines 0 comments Download
M chrome/browser/extensions/user_script_master.cc View 1 2 3 4 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/common/extensions/user_script.h View 3 chunks +20 lines, -1 line 0 comments Download
M chrome/common/extensions/user_script.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/common/extensions/user_script_unittest.cc View 2 chunks +10 lines, -0 lines 1 comment Download
M chrome/renderer/render_view.h View 4 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/renderer/render_view.cc View 1 2 3 4 2 chunks +9 lines, -9 lines 0 comments Download
M chrome/renderer/user_script_slave.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/user_script_slave.cc View 1 2 3 4 1 chunk +4 lines, -2 lines 0 comments Download
M webkit/glue/webframeloaderclient_impl.cc View 4 1 chunk +4 lines, -2 lines 0 comments Download
M webkit/glue/webview_delegate.h View 4 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Aaron Boodman
10 years, 6 months ago (2009-02-02 07:00:50 UTC) #1
Aaron Boodman
10 years, 6 months ago (2009-02-02 07:03:18 UTC) #2
Erik does not do reviews
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 ...
10 years, 6 months ago (2009-02-02 16:31:44 UTC) #3
Aaron Boodman
Ready for another look. I ended up modifying WebCore to work better for this use ...
10 years, 6 months ago (2009-02-11 05:23:58 UTC) #4
Aaron Boodman
Changing reviewer to mpcomplete as erikkay is out sick. Matt, I've decided to go ahead ...
10 years, 6 months ago (2009-02-11 19:04:55 UTC) #5
Matt Perry
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 ...
10 years, 6 months ago (2009-02-11 19:28:46 UTC) #6
Aaron Boodman
On 2009/02/11 19:28:46, Matt Perry wrote: > http://codereview.chromium.org/19624/diff/1420/407#newcode104 > Line 104: TEST(UserScriptTest, Defaults) { > ...
10 years, 6 months ago (2009-02-11 20:40:08 UTC) #7
Erik does not do reviews
10 years, 6 months ago (2009-02-11 21:16:24 UTC) #8
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).

Powered by Google App Engine
This is Rietveld 408576698