Fix content script injection not working with PlzNavigate because of different navigation timings.

Fix content script injection not working with PlzNavigate because of different navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL at the start. With PlzNavigate, this will only be set once the load is completed. ProgrammaticScriptInjector was caching the frame's URL too early and was getting an empty URL as a result. Fix this by calculating it later when it's needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347
Committed: https://crrev.com/69e711577ac88bc21ee1823c3dffaa9ee9b2593e
Cr-Commit-Position: refs/heads/master@{#429174}

[jam, 2016-11-01 19:22:10]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2016-11-01 19:22:15]

Patchset #1 (id:1) has been deleted

[jam, 2016-11-01 19:22:24]

Description was changed from

==========
Fix content script injection not working with PlzNavigate because of different
navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL
at the start. With PlzNavigate, this will only be set once the load is
completed. ProgrammaticScriptInjector was caching the frame's URL too early and
was getting an empty URL as a result. Fix this by calculating it later when it's
needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347
==========

to

==========
Fix content script injection not working with PlzNavigate because of different
navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL
at the start. With PlzNavigate, this will only be set once the load is
completed. ProgrammaticScriptInjector was caching the frame's URL too early and
was getting an empty URL as a result. Fix this by calculating it later when it's
needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347
==========

[jam, 2016-11-01 19:22:24]

jam@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-11-01 19:22:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 19:59:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 19:59:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-11-01 20:19:24]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 20:19:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-11-01 21:03:11]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-01 21:04:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 21:04:48]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-01 23:39:32]

lgtm

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.cc:72: url_ =
ScriptContext::GetDataSourceURLForFrame(frame);
Do we document anywhere at which point we *do* have the url for the frame?  I
imagine it's fine here, but it would be good to know the exact before this/after
this moment.

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.h (right):

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.h:74: mutable GURL url_;
It makes me sad that these are mutable.  I think having
ScriptInjector::CanExecuteOnFrame be const is mostly legacy (from when there was
a single ScriptInjector).  I'd slightly prefer just removing the const from
there, rather than introducing mutable members.

Mind doing that here?  Or putting
// TODO(devlin): These shouldn't need to be mutable.
?

[jam, 2016-11-01 23:47:03]

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.cc:72: url_ =
ScriptContext::GetDataSourceURLForFrame(frame);
On 2016/11/01 23:39:32, Devlin wrote:
> Do we document anywhere at which point we *do* have the url for the frame?  I
> imagine it's fine here, but it would be good to know the exact before
this/after
> this moment.

Sure I'll add a comment here.

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.h (right):

https://codereview.chromium.org/2471553003/diff/40001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.h:74: mutable GURL url_;
On 2016/11/01 23:39:32, Devlin wrote:
> It makes me sad that these are mutable.  I think having
> ScriptInjector::CanExecuteOnFrame be const is mostly legacy (from when there
was
> a single ScriptInjector).  I'd slightly prefer just removing the const from
> there, rather than introducing mutable members.
> 
> Mind doing that here?  Or putting
> // TODO(devlin): These shouldn't need to be mutable.
> ?

agreed I prefer mutable too, I thought there was a reason it had to be const.
updated.

[jam, 2016-11-01 23:47:19]

The CQ bit was checked by jam@chromium.org

[jam, 2016-11-01 23:47:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2471553003/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2016-11-01 23:48:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-02 00:02:58]

https://codereview.chromium.org/2471553003/diff/60001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/2471553003/diff/60001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.cc:73: //
UserScript::DOCUMENT_END.
I'm confused - how would we inject at document start, then?

[jam, 2016-11-02 00:14:43]

https://codereview.chromium.org/2471553003/diff/60001/extensions/renderer/pro...
File extensions/renderer/programmatic_script_injector.cc (right):

https://codereview.chromium.org/2471553003/diff/60001/extensions/renderer/pro...
extensions/renderer/programmatic_script_injector.cc:73: //
UserScript::DOCUMENT_END.
On 2016/11/02 00:02:57, Devlin wrote:
> I'm confused - how would we inject at document start, then?

I was trying to mean that on construction of this object, we could be at
DOCUMENT_START with PlzNavigate.

[jam, 2016-11-02 00:21:52]

The CQ bit was unchecked by jam@chromium.org

[jam, 2016-11-02 00:27:59]

The CQ bit was checked by jam@chromium.org

[jam, 2016-11-02 00:27:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2471553003/#ps80001
(title: "review comments")

[commit-bot: I haz the power, 2016-11-02 00:28:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 01:16:07]

Description was changed from

==========
Fix content script injection not working with PlzNavigate because of different
navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL
at the start. With PlzNavigate, this will only be set once the load is
completed. ProgrammaticScriptInjector was caching the frame's URL too early and
was getting an empty URL as a result. Fix this by calculating it later when it's
needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347
==========

to

==========
Fix content script injection not working with PlzNavigate because of different
navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL
at the start. With PlzNavigate, this will only be set once the load is
completed. ProgrammaticScriptInjector was caching the frame's URL too early and
was getting an empty URL as a result. Fix this by calculating it later when it's
needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347
==========

[commit-bot: I haz the power, 2016-11-02 01:16:08]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-11-02 01:19:46]

Description was changed from

==========
Fix content script injection not working with PlzNavigate because of different
navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL
at the start. With PlzNavigate, this will only be set once the load is
completed. ProgrammaticScriptInjector was caching the frame's URL too early and
was getting an empty URL as a result. Fix this by calculating it later when it's
needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347
==========

to

==========
Fix content script injection not working with PlzNavigate because of different
navigation timings.

Without PlzNavigat, when a frame is created it will have a provisional load URL
at the start. With PlzNavigate, this will only be set once the load is
completed. ProgrammaticScriptInjector was caching the frame's URL too early and
was getting an empty URL as a result. Fix this by calculating it later when it's
needed, since at that point it's guaranteed that the frame would have loaded.

This fixes
ExtensionApiTest.Incognito
ExtensionApiTest.JavaScriptURLPermissions
ExtensionApiTest.JavasScriptEncodedURL
with PlzNavigate.

BUG=504347

Committed: https://crrev.com/69e711577ac88bc21ee1823c3dffaa9ee9b2593e
Cr-Commit-Position: refs/heads/master@{#429174}
==========

[commit-bot: I haz the power, 2016-11-02 01:19:47]

Patchset 3 (id:??) landed as
https://crrev.com/69e711577ac88bc21ee1823c3dffaa9ee9b2593e
Cr-Commit-Position: refs/heads/master@{#429174}