Updated TODO(shreyasv)

Updated TODO(shreyasv)

Removed stale TODO in SnapshotManager.
The sessionID is not the same for the lifetime of the
SnapshotManager and can change e.g. during
session restore.
Changed TODOs to the format TODO(crbug.com/*)

BUG=546145
Committed: https://crrev.com/f8d23b1d5938b2d3514802bde618194d2466437a
Cr-Commit-Position: refs/heads/master@{#355564}

[shreyasv1, 2015-10-21 23:51:56]

shreyasv@chromium.org changed reviewers:
+ droger@chromium.org, eugenebut@chromium.org

[shreyasv1, 2015-10-21 23:51:57]

droger for ios/net
eugenebut for ios/web

[shreyasv1, 2015-10-21 23:53:16]

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

[shreyasv1, 2015-10-21 23:53:17]

sdefresne for ios/

[droger, 2015-10-21 23:54:24]

ios/net lgtm

[sdefresne, 2015-10-21 23:56:21]

ios/chrome lgtm (but droger is also owners of that)

https://codereview.chromium.org/1423483004/diff/1/ios/web/web_state/web_state...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1423483004/diff/1/ios/web/web_state/web_state...
ios/web/web_state/web_state_impl.h:176: // TODO(shreyasv): Rename this to
ExecuteJavaScript for consistency with
This should have a bug and use the TODO(crbug.com/######) format.

[Eugene But (OOO till 7-30), 2015-10-22 00:12:02]

I have a question and a NIT. Otherwise lgtm

https://codereview.chromium.org/1423483004/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/snapshot_manager.h (left):

https://codereview.chromium.org/1423483004/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_manager.h:31: // TODO(shreyasv): Consider
passing the sessionID into SnapshotManager from Tab
Why this TODO is not relevant anymore? Every method still have a |sessionID|
argument, which does not seems to be the best approach. Please update CL
description with explanation why this TODO is stale.

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
ios/web/public/test/crw_test_js_injection_receiver.mm:31:
base::WeakNSObject<CRWTestUIWebViewEvaluator> weakEvaluator(self);
NIT: s/weakEvaluator/weakSelf since we usually call this variable weakSelf

[shreyasv1, 2015-10-22 00:33:52]

Description was changed from

==========
Updated TODO(shreyasv)

Removed stale TODOs
Changed TODOs to the format TODO(crbug.com/*)

BUG=546145
==========

to

==========
Updated TODO(shreyasv)

Removed stale TODO in SnapshotManager.
The sessionID is not the same for the lifetime of the 
SnapshotManager and can change e.g. during 
session restore.
Changed TODOs to the format TODO(crbug.com/*)

BUG=546145
==========

[shreyasv1, 2015-10-22 00:33:59]

https://codereview.chromium.org/1423483004/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/snapshot_manager.h (left):

https://codereview.chromium.org/1423483004/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_manager.h:31: // TODO(shreyasv): Consider
passing the sessionID into SnapshotManager from Tab
On 2015/10/22 00:12:02, eugenebut wrote:
> Why this TODO is not relevant anymore? Every method still have a |sessionID|
> argument, which does not seems to be the best approach. Please update CL
> description with explanation why this TODO is stale.

Done.

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
ios/web/public/test/crw_test_js_injection_receiver.mm:31:
base::WeakNSObject<CRWTestUIWebViewEvaluator> weakEvaluator(self);
On 2015/10/22 00:12:02, eugenebut wrote:
> NIT: s/weakEvaluator/weakSelf since we usually call this variable weakSelf

Acknowledged.

https://codereview.chromium.org/1423483004/diff/1/ios/web/web_state/web_state...
File ios/web/web_state/web_state_impl.h (right):

https://codereview.chromium.org/1423483004/diff/1/ios/web/web_state/web_state...
ios/web/web_state/web_state_impl.h:176: // TODO(shreyasv): Rename this to
ExecuteJavaScript for consistency with
On 2015/10/21 23:56:21, sdefresne wrote:
> This should have a bug and use the TODO(crbug.com/######) format.

Acknowledged.

[shreyasv1, 2015-10-22 00:34:04]

The CQ bit was checked by shreyasv@chromium.org

[commit-bot: I haz the power, 2015-10-22 00:35:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423483004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423483004/1

[commit-bot: I haz the power, 2015-10-22 01:15:13]

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

[commit-bot: I haz the power, 2015-10-22 01:15:14]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[sdefresne, 2015-10-22 03:17:34]

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
ios/web/public/test/crw_test_js_injection_receiver.mm:33: UIWebView* webView =
weakEvaluator ? weakEvaluator->_webView : nil;
Looks like you need the following to fix the compilation error:

UIWebView* webView = weakEvaluator ? weakEvaluator.get()->_webView : nil;

[shreyasv1, 2015-10-22 17:08:26]

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
File ios/web/public/test/crw_test_js_injection_receiver.mm (right):

https://codereview.chromium.org/1423483004/diff/1/ios/web/public/test/crw_tes...
ios/web/public/test/crw_test_js_injection_receiver.mm:33: UIWebView* webView =
weakEvaluator ? weakEvaluator->_webView : nil;
On 2015/10/22 03:17:33, sdefresne wrote:
> Looks like you need the following to fix the compilation error:
> 
> UIWebView* webView = weakEvaluator ? weakEvaluator.get()->_webView : nil;

Done.

[shreyasv1, 2015-10-22 17:08:43]

The CQ bit was checked by shreyasv@chromium.org

[shreyasv1, 2015-10-22 17:08:43]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
eugenebut@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1423483004/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-10-22 17:09:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423483004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423483004/20001

[commit-bot: I haz the power, 2015-10-22 17:24:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-22 17:25:24]

Patchset 2 (id:??) landed as
https://crrev.com/f8d23b1d5938b2d3514802bde618194d2466437a
Cr-Commit-Position: refs/heads/master@{#355564}