swapContentViewCore is used by the Reader mode panel

swapContentViewCore is used by the Reader mode panel

BUG=473205
Committed: https://crrev.com/8c002aa881be89f010166c786e50cd778f587701
Cr-Commit-Position: refs/heads/master@{#330575}

[aruslan, 2015-05-19 01:00:51]

aruslan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[aruslan, 2015-05-19 01:00:52]

PTAL thanks!

[aruslan, 2015-05-19 01:01:28]

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

[commit-bot: I haz the power, 2015-05-19 01:01:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147783002/20001

[commit-bot: I haz the power, 2015-05-19 01:40:00]

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

[commit-bot: I haz the power, 2015-05-19 01:40:01]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2015-05-19 17:37:22]

dtrainor@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-05-19 17:37:23]

Drive by small question about the API, but nbd either way.

https://codereview.chromium.org/1147783002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1147783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2202: public void
swapContentViewCore(ContentViewCore newContentViewCore,
For API cleanliness sake, should this return the old ContentViewCore? 
Especially if we're not destroying the old native web contents...?

Just curious what people think.

[Ted C, 2015-05-19 17:52:06]

lgtm

https://codereview.chromium.org/1147783002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1147783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2202: public void
swapContentViewCore(ContentViewCore newContentViewCore,
On 2015/05/19 17:37:23, David Trainor wrote:
> For API cleanliness sake, should this return the old ContentViewCore? 
> Especially if we're not destroying the old native web contents...?
> 
> Just curious what people think.

swapWebContents does not return the old webcontents, so I think it's ok to not
have it here until needed.

We also have the ability to getWebContents or getContentViewCore from the Tab
prior to calling this method, so I don't think it's a problem with this data
being unavailable.

[aruslan, 2015-05-19 18:36:51]

Thanks!

https://codereview.chromium.org/1147783002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1147783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2202: public void
swapContentViewCore(ContentViewCore newContentViewCore,
On 2015/05/19 17:52:06, Ted C wrote:
> On 2015/05/19 17:37:23, David Trainor wrote:
> > For API cleanliness sake, should this return the old ContentViewCore? 
> > Especially if we're not destroying the old native web contents...?
> > 
> > Just curious what people think.
> 
> swapWebContents does not return the old webcontents, so I think it's ok to not
> have it here until needed.
> 
> We also have the ability to getWebContents or getContentViewCore from the Tab
> prior to calling this method, so I don't think it's a problem with this data
> being unavailable.

Agreed, getWebContents() prior to calling this method is sufficient if the
caller wants to control the lifetime of the old native web contents.

As to returning the old ContentViewCore, David's concern is valid, but on
different grounds: the method name is a confusing misnomer -- it couldn't
possibly return the old ContentViewCore because it destroys the one on the Tab.

I think it should have been named "replaceContentViewCore", but I leave this to
those in the know for a subsequent cross-repo method rename.

[aruslan, 2015-05-19 18:37:01]

The CQ bit was checked by aruslan@chromium.org

[commit-bot: I haz the power, 2015-05-19 18:37:44]

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

[commit-bot: I haz the power, 2015-05-19 18:43:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-19 18:44:05]

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