Upstream JavaScript injection for iOS

Upstream JavaScript injection for iOS

Committed: https://crrev.com/628e793a2d4f986192db1ada5621ed50fe4eb16f
Cr-Commit-Position: refs/heads/master@{#308072}

[droger, 2014-12-09 15:31:34]

Patchset #4 (id:60001) has been deleted

[droger, 2014-12-09 15:38:50]

Patchset #1 (id:1) has been deleted

[droger, 2014-12-09 15:38:58]

Patchset #1 (id:20001) has been deleted

[droger, 2014-12-09 15:40:46]

Patchset #1 (id:40001) has been deleted

[droger, 2014-12-09 15:40:53]

Patchset #1 (id:80001) has been deleted

[droger, 2014-12-09 15:55:54]

droger@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[droger, 2014-12-09 15:55:54]

I have two small questions below.
Also note that this CL does not include the "js_resources" target with the
actual  scripts. The resources will have to be added as a separate CL.

https://codereview.chromium.org/790803002/diff/120001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/ios_web.gyp#new...
ios/web/ios_web.gyp:59: 'web_view_util.mm',
this file was in common/, but I think we should not have a common/ directory
because it is confusing with the usual meaning of "common" in Chrome which means
"shared by the browser and the renderer".

https://codereview.chromium.org/790803002/diff/120001/ios/web/ios_web.gyp#new...
ios/web/ios_web.gyp:73: 'public/test/crw_test_js_injection_receiver.mm',
I'm following the pattern of test_browser_state here and put the implementation
in public/.
Let me know if you want me to move it out of public.

[droger, 2014-12-10 09:39:54]

droger@chromium.org changed reviewers:
+ sdefresne@chromium.org

[droger, 2014-12-10 09:39:55]

+sdefresne

[sdefresne, 2014-12-10 15:30:24]

The objective C files refer to some javascript files (base.js, common.js, ...)
but I do not see them in the CL and they have not yet been upstreamed. They
should probably be part of this CL.

https://codereview.chromium.org/790803002/diff/120001/ios/web/public/test/crw...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/public/test/crw...
ios/web/public/test/crw_test_js_injection_receiver.mm:13: @interface
CRWWebViewEvaluator : NSObject<CRWJSInjectionEvaluator> {
nit: this class is only used in this file, why not use the occasion to rename
it?

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_state/js/cr...
File ios/web/web_state/js/crw_js_injection_manager.mm (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_injection_manager.mm:16: // Templete with presence
beacon check to prevent re-injection of JavaScript.
nit: Templete -> Template.

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_injection_manager.mm:17: NSString* const
kPresenceCheckTemplate = @"if (typeof %@ != 'object') { %@}";
nit: @"if (typeof %@ !== 'object') { %@ }";

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_state/js/cr...
File ios/web/web_state/js/crw_js_injection_receiver.mm (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_injection_receiver.mm:9: #include
"ios/web/public/web_state/js/crw_js_injection_evaluator.h"
nit: those headers are Objective-C headers too, why not #import them? (same
everywhere that ios/web/public/web_state/js/crw_* is included).

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_injection_receiver.mm:14: __weak
id<CRWJSInjectionEvaluator> _evaluator;
__weak has no meaning if not using ARC. Its presence here contradict the fact
that the file is not compiled using ARC. Please remove.

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_view_util.mm
File ios/web/web_view_util.mm (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_view_util.m...
ios/web/web_view_util.mm:11: bool g_force_ui_web_view = false;
This is a C++ file, so this should use mixedCamelCase.

This global should be documented too.

[droger, 2014-12-10 17:48:55]

Thanks for the review.
I did not add the script as part of this CL because I think it is a large
change.
I can do it as a precursor CL though if you prefer.

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_view_util.mm
File ios/web/web_view_util.mm (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/web_view_util.m...
ios/web/web_view_util.mm:11: bool g_force_ui_web_view = false;
On 2014/12/10 15:30:24, sdefresne wrote:
> This is a C++ file, so this should use mixedCamelCase.
> 
> This global should be documented too.

C++ variables don't use camel case.
A search for
^g_
on cs.chromium.org will show that this is the usual style.

I added a comment though.

[droger, 2014-12-10 18:06:45]

I created https://codereview.chromium.org/788253002/ to upstream the missing
scripts.

[sdefresne, 2014-12-11 15:25:12]

lgtm with nits & comment

https://codereview.chromium.org/790803002/diff/140001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/ios_web.gyp#new...
ios/web/ios_web.gyp:49: 'web_state/js/crw_js_common_manager.h',
Why is this file not in public/web_state/js? I can see others manager
dependending on common.js.

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_state/js/cr...
File ios/web/web_state/js/crw_js_injection_manager.mm (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_injection_manager.mm:25: __weak
CRWJSInjectionReceiver* _receiver;
nity: __weak is useless here too.

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_view_util.mm
File ios/web/web_view_util.mm (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_view_util.m...
ios/web/web_view_util.mm:11: // If true, UIWebView is always used.
nit: // If true, UIWebView is always used, even if WKWebView is available.

[stuartmorgan, 2014-12-11 23:59:38]

LGTM. It's a bit odd to upstream these without upstreaming the JS and the
related build plumbing, but since that doesn't block compilation I'm fine
cleaning that up later.

https://codereview.chromium.org/790803002/diff/120001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/790803002/diff/120001/ios/web/ios_web.gyp#new...
ios/web/ios_web.gyp:73: 'public/test/crw_test_js_injection_receiver.mm',
On 2014/12/09 15:55:54, droger wrote:
> I'm following the pattern of test_browser_state here and put the
implementation
> in public/.
> Let me know if you want me to move it out of public.

Honestly I'm not really clear on when content/ puts impls in public/, so I don't
have a good answer either way. I'm fine doing whatever seems consistent for now,
and we can revisit later if it becomes an issue.

https://codereview.chromium.org/790803002/diff/140001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/ios_web.gyp#new...
ios/web/ios_web.gyp:49: 'web_state/js/crw_js_common_manager.h',
On 2014/12/11 15:25:12, sdefresne wrote:
> Why is this file not in public/web_state/js? I can see others manager
> dependending on common.js.

+jyquinn who has been looking at what here needs to be public.

https://codereview.chromium.org/790803002/diff/140001/ios/web/public/test/crw...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/public/test/crw...
ios/web/public/test/crw_test_js_injection_receiver.mm:31: // TODO(shreyasv):
Change to weaknsobject once weaknsobject is moved to
This TODO can just become "Change to WeakNSObject now that it's reachable."
since the file move happened.

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_state/js/cr...
File ios/web/web_state/js/crw_js_injection_manager.mm (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_injection_manager.mm:21: // JS to inject into the
page for our own nefarious purposes. This may be
Let's drop the "for our own nefarious purposes"

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_view_util.h
File ios/web/web_view_util.h (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/web_view_util.h...
ios/web/web_view_util.h:10: // Returns true if WKWebView is being used instead
of UIWebView.
Could you add a:
TODO(stuartmorgan): Eliminate this global flag in favor of a per-web-view flag.
here?

[droger, 2014-12-12 12:33:15]

Thanks for the review.

https://codereview.chromium.org/790803002/diff/140001/ios/web/public/test/crw...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/790803002/diff/140001/ios/web/public/test/crw...
ios/web/public/test/crw_test_js_injection_receiver.mm:31: // TODO(shreyasv):
Change to weaknsobject once weaknsobject is moved to
On 2014/12/11 23:59:38, stuartmorgan wrote:
> This TODO can just become "Change to WeakNSObject now that it's reachable."
> since the file move happened.

I'll do that as a follow up, because WeakNSObject is not upstreamed yet, see
https://codereview.chromium.org/792443004/.

[droger, 2014-12-12 12:34:18]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-12-12 12:35:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/790803002/160001

[commit-bot: I haz the power, 2014-12-12 14:37:51]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2014-12-12 14:39:11]

Patchset 4 (id:??) landed as
https://crrev.com/628e793a2d4f986192db1ada5621ed50fe4eb16f
Cr-Commit-Position: refs/heads/master@{#308072}