New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 765250 link

Starred by 4 users

Issue metadata

Status: WontFix
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 765587



Sign in to add a comment

Turn on PlzNavigate for webview in 62

Project Member Reported by jam@chromium.org, Sep 14 2017

Issue description

Per  bug 702785 , Android WebView doesn't use Finch.

We should probably turn it on the m62 branch now, to get bug reports from beta users.
 
Cc: changwan@chromium.org
clamy@, do we have a launch bug around this? m62 is already branched out, is there a TPM following up on this?

Comment 2 by clamy@chromium.org, Sep 14 2017

The launch bug is issue 689549. I think all TPMs know about it by now :) - in particular we have been working with amineer@ and govind@.
Cc: gov...@chromium.org cma...@chromium.org amineer@chromium.org
I see. cc'ing them here, along with cmasso@ who is webview TPM.

Comment 4 by sgu...@chromium.org, Sep 14 2017

Cc: jam@chromium.org sgu...@chromium.org
 Issue 765482  has been merged into this issue.

Comment 5 by sgu...@chromium.org, Sep 14 2017

Owner: ntfschr@chromium.org
Components: Mobile>WebView
Labels: -OS-Mac M-62 OS-Android
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbc2bb9217483a8ceac23cd41bb8dcc7453c8f4b

commit dbc2bb9217483a8ceac23cd41bb8dcc7453c8f4b
Author: Nate Fischer <ntfschr@chromium.org>
Date: Fri Sep 15 01:14:47 2017

AW: enable PlzNavigate via CLI switch

This CL enables PlzNavigate (browser side navigation) by default for
WebView only. This is to try out the new code path before launching at
100% for all of chrome.

Bug:  765250 
Test: Manual
Change-Id: Iff2d0585e11075e71fe196df47503d950e2b26b3
Reviewed-on: https://chromium-review.googlesource.com/667502
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502121}
[modify] https://crrev.com/dbc2bb9217483a8ceac23cd41bb8dcc7453c8f4b/android_webview/lib/aw_main_delegate.cc

Labels: Merge-Request-62
Requesting a merge so that we can try this out on beta before it's 100% on stable.

I've manually tested PlzNavigate on Gmail, Inbox, Google Play Books, GSA, WV shell, and some 3rd party apps. So far I haven't seen any major regressions.
Project Member

Comment 9 by sheriffbot@chromium.org, Sep 15 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -Merge-Review-62 Merge-Approved-62
I am approving this merge so we can get something out of beta channel. 
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ac77fd0987d08feb63ca059d582cae89a30ec88

commit 3ac77fd0987d08feb63ca059d582cae89a30ec88
Author: Tim Volodine <timvolodine@chromium.org>
Date: Fri Sep 15 14:40:24 2017

Revert "AW: enable PlzNavigate via CLI switch"

This reverts commit dbc2bb9217483a8ceac23cd41bb8dcc7453c8f4b.

Reason for revert:
speculative revert, might have broken Android Webview L/M/N bots, see crbug.com/765587

Original change's description:
> AW: enable PlzNavigate via CLI switch
> 
> This CL enables PlzNavigate (browser side navigation) by default for
> WebView only. This is to try out the new code path before launching at
> 100% for all of chrome.
> 
> Bug:  765250 
> Test: Manual
> Change-Id: Iff2d0585e11075e71fe196df47503d950e2b26b3
> Reviewed-on: https://chromium-review.googlesource.com/667502
> Reviewed-by: Selim Gurun <sgurun@chromium.org>
> Commit-Queue: Nate Fischer <ntfschr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#502121}

TBR=sgurun@chromium.org,ntfschr@chromium.org

Change-Id: Ia07d03630a71b0c1beabeee4faf5ce685131bf70
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  765250 
Reviewed-on: https://chromium-review.googlesource.com/667150
Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Commit-Queue: Tim Volodine <timvolodine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502249}
[modify] https://crrev.com/3ac77fd0987d08feb63ca059d582cae89a30ec88/android_webview/lib/aw_main_delegate.cc

A summary of meeting between webview leads and jam@ about this (sorry cmasso@, we missed you there):

1) Webview team is ok with enabling this on M62 this time because a) finch isn't there b) too little beta/dev/canary channel users anyways.
2) We should be able to disable the feature on M63. (or M64? cmasso@ can tell)
3) changwan@ will work with brettw@ and cmasso@ to update the launch process template to note about the peculiarities about webview launch in order to avoid this type of confusion.

cmasso@, I think release merge should always be accompanied by CTS tests at all times. Could we update the merge process?

Thanks.

Hey changwan@, sorry I received the meeting invite late to make it on time. The plan sounds good and I will take a closer look at 3) to understand what could be improved.

I am not sure I understand you comment about release merge and CTS tests :) Please elaborate a little more? we could also just chat if you are available.
cmasso@, 

Since we are planning to release as part of M62 (if everything looks good), I think we should mark as RBS to be in the radar?

Thanks!
Labels: ReleaseBlock-Stable
Status: Assigned (was: Available)
+RBS (this doesn't have to block betas, but it should get in the earliest beta reasonable).

clamy@ already enabled PlzNavigate on trunk (0dbded05ad0ba1c3d8388fd649130509ab97f07a), so there's no sense in relanding this CL on trunk. Instead, I'm just going to merge the CL back to M62 once issue 765587 is resolved & merged back.

Please holler if anyone objects.
Blockedon: 765587
Project Member

Comment 17 by sheriffbot@chromium.org, Sep 18 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 18 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/267e445a8ff514d5169b4727f583ef3a8988b7fd

commit 267e445a8ff514d5169b4727f583ef3a8988b7fd
Author: Nate Fischer <ntfschr@chromium.org>
Date: Mon Sep 18 19:41:16 2017

AW: enable PlzNavigate via CLI switch

This CL enables PlzNavigate (browser side navigation) by default for
WebView only. This is to try out the new code path before launching at
100% for all of chrome.

TBR=ntfschr@chromium.org

(cherry picked from commit dbc2bb9217483a8ceac23cd41bb8dcc7453c8f4b)

Bug:  765250 
Test: Manual
Change-Id: Iff2d0585e11075e71fe196df47503d950e2b26b3
Reviewed-on: https://chromium-review.googlesource.com/667502
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502121}
Reviewed-on: https://chromium-review.googlesource.com/671187
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#300}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/267e445a8ff514d5169b4727f583ef3a8988b7fd/android_webview/lib/aw_main_delegate.cc

Status: Fixed (was: Assigned)
Ok, PlzNavigate should now be enabled for the next WebView beta.

Current plan is to revert this before M62 stable ( issue 766257 ), so expect to see the revert land against this bug.

---

To reiterate on the plan, if anyone sees navigation-related bugs, *please* file them against me so I can triage them (tracking bug: issue 766255). Even though we plan to not launch in M62 stable, it's still critical to know about these bugs for future launch.
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/332b11b2433d815d9bcb387acc5247ac588af755

commit 332b11b2433d815d9bcb387acc5247ac588af755
Author: Nate Fischer <ntfschr@chromium.org>
Date: Tue Sep 19 03:38:24 2017

Revert "AW: enable PlzNavigate via CLI switch"

This reverts commit 267e445a8ff514d5169b4727f583ef3a8988b7fd.

Reason for revert: We've already seen lots of breakages with PlzNavigate

Original change's description:
> AW: enable PlzNavigate via CLI switch
> 
> This CL enables PlzNavigate (browser side navigation) by default for
> WebView only. This is to try out the new code path before launching at
> 100% for all of chrome.
> 
> TBR=ntfschr@chromium.org
> 
> (cherry picked from commit dbc2bb9217483a8ceac23cd41bb8dcc7453c8f4b)
> 
> Bug:  765250 
> Test: Manual
> Change-Id: Iff2d0585e11075e71fe196df47503d950e2b26b3
> Reviewed-on: https://chromium-review.googlesource.com/667502
> Reviewed-by: Selim Gurun <sgurun@chromium.org>
> Commit-Queue: Nate Fischer <ntfschr@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#502121}
> Reviewed-on: https://chromium-review.googlesource.com/671187
> Reviewed-by: Nate Fischer <ntfschr@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3202@{#300}
> Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}

TBR=sgurun@chromium.org,ntfschr@chromium.org

Change-Id: I173db64902172655a72161a244f3923c9c9ef196
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  765250 
Reviewed-on: https://chromium-review.googlesource.com/672156
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#316}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/332b11b2433d815d9bcb387acc5247ac588af755/android_webview/lib/aw_main_delegate.cc

Status: Assigned (was: Fixed)
Reverted due to the bugs we've seen. The plan now is to fix the known bugs and reland in time for a future beta.
Cc: -sgu...@chromium.org
ntfschr@, Just wondering the above change has been reverted from M62 branch (3202) as well? Since we are promoting M62 to Stable soon.

Also please tag the respective milestone for future tracking.

Thank you!
Status: WontFix (was: Assigned)
The merge has indeed been reverted (comment #10), which is what we want.

The feature is already shipped in M63, so there's no more work for this bug.

Sign in to add a comment