Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(17)

Issue 164157: Delay releasing plugin process for 10s. (Closed)

Created:
11 years, 4 months ago by chase
Modified:
9 years, 7 months ago
Reviewers:
jam
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Delay releasing plugin process for 10s. Change plugin process release from immediately after we navigate away from our last plugin reference to a task that is run 10s later. Original patch by John Abd-El-Malek (jam@chromium.org). BUG=11341 TEST=Open Chrome, load youtube.com, click Videos, verify all pages load correctly.

Patch Set 1 #

Patch Set 2 : <=80chars #

Total comments: 1

Patch Set 3 : prefer constant over magic number #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -1 line) Patch
M chrome/plugin/plugin_channel.cc View 1 2 2 chunks +12 lines, -1 line 1 comment Download

Messages

Total messages: 4 (0 generated)
chase
11 years, 4 months ago (2009-08-07 21:11:25 UTC) #1
jam
lgtm with the one nit, and also updating the changelist description. what does "TEST=Open Chrome, ...
11 years, 4 months ago (2009-08-07 21:26:38 UTC) #2
chase
On 2009/08/07 21:26:38, John Abd-El-Malek wrote: > lgtm with the one nit, and also updating ...
11 years, 4 months ago (2009-08-07 22:56:11 UTC) #3
jam
11 years, 4 months ago (2009-08-07 23:33:26 UTC) #4
lgtm

being extra nitty, the issue I have with the "Test" line is that it passes
before and after this change.  The Test line should really be something that
didn't work before, and now works.  so it should specify that the same plugin
process is still being used.

http://codereview.chromium.org/164157/diff/8/9
File chrome/plugin/plugin_channel.cc (right):

http://codereview.chromium.org/164157/diff/8/9#newcode28
Line 28: static const int kPluginReleaseTimeMS = 10000;
nit: the convention is to use "Ms"

Powered by Google App Engine
This is Rietveld 408576698