Make UserScriptIdleScheduler not tied to NavigationState, as part of removing the extension depen...

Make UserScriptIdleScheduler not tied to NavigationState, as part of removing the extension dependencies from content.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80029

[Aaron Boodman, 2011-03-30 21:11:48]

This is cool. LGTM

http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_helper.cc (right):

http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_helper.cc:39: // Check first if we creatd a
scheduler for the frame, since this function
sp: created

http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_helper.cc:44: new
UserScriptIdleScheduler(render_view(), frame);
The naked 'new' here is a bit weird. If you're going to have
UserScriptIdleScheduler delete itself, then how about making the constructor
private and using a factory method instead.

http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/user_script_...
File chrome/renderer/user_script_idle_scheduler.cc (right):

http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/user_script_...
chrome/renderer/user_script_idle_scheduler.cc:40: if (message.type() !=
ExtensionMsg_ExecuteCode::ID)
Why is this bit needed?

http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/user_script_...
chrome/renderer/user_script_idle_scheduler.cc:43: WebFrame* main_frame =
GetMainFrame();
Add a comment here:

chrome.tabs.executeScript() only supports execution in either the top frame or
all frames. We handle both cases in the top frame.

[jam, 2011-03-30 21:31:17]

This isn't ready yet (I didn't publish it for that reason).  It seems like
the WebKit API has a bug where WebFrameClient::willClose is called at the
wrong time, hence the failing tests.  I'm investigating.

On Wed, Mar 30, 2011 at 2:11 PM, <[email protected]> wrote:

> This is cool. LGTM
>
>
>
>
http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/extensions/e...
> File chrome/renderer/extensions/extension_helper.cc (right):
>
>
>
http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/extensions/e...
> chrome/renderer/extensions/extension_helper.cc:39: // Check first if we
> creatd a scheduler for the frame, since this function
> sp: created
>

done


>
>
>
http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/extensions/e...
> chrome/renderer/extensions/extension_helper.cc:44: new
> UserScriptIdleScheduler(render_view(), frame);
> The naked 'new' here is a bit weird. If you're going to have
> UserScriptIdleScheduler delete itself, then how about making the
> constructor private and using a factory method instead.
>

This is how all the RenderViewObserver implementations are created.


>
>
>
http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/user_script_...
> File chrome/renderer/user_script_idle_scheduler.cc (right):
>
>
>
http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/user_script_...
> chrome/renderer/user_script_idle_scheduler.cc:40: if (message.type() !=
> ExtensionMsg_ExecuteCode::ID)
> Why is this bit needed?
>

it's because of the code in the function.  we always return true, but only
if the message was for this particular frame, or if there's no main frame at
that point (at which the observer that sees this function will reply with
the error).

>
>
>
http://codereview.chromium.org/6731080/diff/7001/chrome/renderer/user_script_...
> chrome/renderer/user_script_idle_scheduler.cc:43: WebFrame* main_frame =
> GetMainFrame();
> Add a comment here:
>
> chrome.tabs.executeScript() only supports execution in either the top
> frame or all frames. We handle both cases in the top frame.
>
> http://codereview.chromium.org/6731080/
>

[Aaron Boodman, 2011-03-31 00:34:00]

On 2011/03/30 21:31:17, John Abd-El-Malek wrote:
> This isn't ready yet (I didn't publish it for that reason).  It seems like
> the WebKit API has a bug where WebFrameClient::willClose is called at the
> wrong time, hence the failing tests.  I'm investigating.

Sorry, I always forget to check the publish state.

[jam, 2011-03-31 00:47:57]

On Wed, Mar 30, 2011 at 5:34 PM, <[email protected]> wrote:

> On 2011/03/30 21:31:17, John Abd-El-Malek wrote:
>
>> This isn't ready yet (I didn't publish it for that reason).  It seems like
>> the WebKit API has a bug where WebFrameClient::willClose is called at the
>> wrong time, hence the failing tests.  I'm investigating.
>>
>
> Sorry, I always forget to check the publish state.


np.  I have a new version out that passes the failing tests.  I switched
from FrameWillClose to FrameDetached, since the former happens across
navigations and even multiple times while loading a page.  Since the
UserScriptIdleScheuler now lasts throughout the lifetime of a frame, I reset
the state in DidStartProvisionalLoad so that we keep injecting as the frame
goes to different pages.


>
> http://codereview.chromium.org/6731080/
>