Re-enable JSWindowIDManagerTest.WindowID

Re-enable JSWindowIDManagerTest.WindowID

Creates CRWTestJSWindowIdManager to enable force injection,
bypassing hasBeenInjected check in the inject method.

Moves crw_js_window_id_manager_unittest.mm out of public directory.

BUG=465898
Committed: https://crrev.com/7745012e799ee09abf12246929efb3aa311f388d
Cr-Commit-Position: refs/heads/master@{#329438}

[Jackie Quinn, 2015-05-08 23:16:38]

jyquinn@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2015-05-09 16:40:37]

https://codereview.chromium.org/1129763003/diff/1/ios/web/public/web_state/js...
File ios/web/public/web_state/js/crw_js_window_id_manager_unittest.mm (left):

https://codereview.chromium.org/1129763003/diff/1/ios/web/public/web_state/js...
ios/web/public/web_state/js/crw_js_window_id_manager_unittest.mm:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Is there a gyp file which should be changed as a part of this move?

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
File ios/web/web_state/js/crw_js_window_id_manager_unittest.mm (right):

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:21: -
(void)forceInject {
The fact that second injection does not happen is expected behavior, so I don't
think that we should bypass the check.
There are certain cases when windowID is re-injected in the application, so I
believe the test should emulate one of those cases.
How about removing "window.__gCrWeb" beacon from the page before performing the
second injection?

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:50:
TEST_F(JSWindowIDManagerTest, WindowID) {
NIT: Let's take a chance to add a comment for this test.

[Jackie Quinn, 2015-05-11 18:21:34]

https://codereview.chromium.org/1129763003/diff/1/ios/web/public/web_state/js...
File ios/web/public/web_state/js/crw_js_window_id_manager_unittest.mm (left):

https://codereview.chromium.org/1129763003/diff/1/ios/web/public/web_state/js...
ios/web/public/web_state/js/crw_js_window_id_manager_unittest.mm:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/05/09 16:40:37, eugenebut wrote:
> Is there a gyp file which should be changed as a part of this move?

Yes, Done.

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
File ios/web/web_state/js/crw_js_window_id_manager_unittest.mm (right):

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:21: -
(void)forceInject {
On 2015/05/09 16:40:37, eugenebut wrote:
> The fact that second injection does not happen is expected behavior, so I
don't
> think that we should bypass the check.
> There are certain cases when windowID is re-injected in the application, so I
> believe the test should emulate one of those cases.
> How about removing "window.__gCrWeb" beacon from the page before performing
the
> second injection?

Just changing the beacon won't do anything for WKWebView, at least, since we
don't use the beacon for checking for injection in WKWebView. The best would be
to override hasBeenInjected, but that is called before and after then injection
in the inject method, making things tricky. 

For now, should I make the test in question expect equal windowIDs, if that is
expected behavior? Or should I remove the test? It would test injection
guarding, rather than WindowID.

[Jackie Quinn, 2015-05-12 17:04:38]

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
File ios/web/web_state/js/crw_js_window_id_manager_unittest.mm (right):

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:21: -
(void)forceInject {
On 2015/05/11 18:21:33, Jackie Quinn wrote:
> On 2015/05/09 16:40:37, eugenebut wrote:
> > The fact that second injection does not happen is expected behavior, so I
> don't
> > think that we should bypass the check.
> > There are certain cases when windowID is re-injected in the application, so
I
> > believe the test should emulate one of those cases.
> > How about removing "window.__gCrWeb" beacon from the page before performing
> the
> > second injection?
> 
> Just changing the beacon won't do anything for WKWebView, at least, since we
> don't use the beacon for checking for injection in WKWebView. The best would
be
> to override hasBeenInjected, but that is called before and after then
injection
> in the inject method, making things tricky. 
> 
> For now, should I make the test in question expect equal windowIDs, if that is
> expected behavior? Or should I remove the test? It would test injection
> guarding, rather than WindowID. 

As discussed offline, changing the presence beacon and adding a bug to rewrite
CRWTestJSInjectionReceiver to be web view agnostic.

https://codereview.chromium.org/1129763003/diff/1/ios/web/web_state/js/crw_js...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:50:
TEST_F(JSWindowIDManagerTest, WindowID) {
On 2015/05/09 16:40:37, eugenebut wrote:
> NIT: Let's take a chance to add a comment for this test.

Done.

[Eugene But (OOO till 7-30), 2015-05-12 17:14:32]

lgtm with a change

https://codereview.chromium.org/1129763003/diff/40001/ios/web/web_state/js/cr...
File ios/web/web_state/js/crw_js_window_id_manager_unittest.mm (right):

https://codereview.chromium.org/1129763003/diff/40001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:46: [receiver_
evaluateJavaScript:@"__gCrWeb = undefined;"
Please don't use async API in tests. If evaluation is interrupted for some
reason "EXPECT_FALSE([windowID isEqualToString:windowID2]);" will never be
called.

Use web::EvaluateJavaScriptAsString(CRWJSInjectionReceiver*, NSString*) instead.

[Jackie Quinn, 2015-05-12 17:50:01]

https://codereview.chromium.org/1129763003/diff/40001/ios/web/web_state/js/cr...
File ios/web/web_state/js/crw_js_window_id_manager_unittest.mm (right):

https://codereview.chromium.org/1129763003/diff/40001/ios/web/web_state/js/cr...
ios/web/web_state/js/crw_js_window_id_manager_unittest.mm:46: [receiver_
evaluateJavaScript:@"__gCrWeb = undefined;"
On 2015/05/12 17:14:32, eugenebut wrote:
> Please don't use async API in tests. If evaluation is interrupted for some
> reason "EXPECT_FALSE([windowID isEqualToString:windowID2]);" will never be
> called.
> 
> Use web::EvaluateJavaScriptAsString(CRWJSInjectionReceiver*, NSString*)
instead.

Done.

[Jackie Quinn, 2015-05-12 17:50:13]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2015-05-12 17:50:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/1129763003/#ps60001
(title: "Use web::EvaluateJavaScriptAsString")

[commit-bot: I haz the power, 2015-05-12 17:50:47]

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

[commit-bot: I haz the power, 2015-05-12 17:58:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-12 17:59:51]

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