New issue
Advanced search Search tips

Issue 810937 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Combine two different versions of UpdateBrowserControlsState IPC

Project Member Reported by dcheng@chromium.org, Feb 9 2018

Issue description

There was one in Chrome, was was moved to Mojo: https://chromium-review.googlesource.com/c/chromium/src/+/836573

But it turns out that there's an identical version of the IPC in content, which has not moved to Mojo: https://chromium-review.googlesource.com/c/chromium/src/+/910108

The latter might not handle interstitials correctly either it seems, but some Java code still calls it: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java?rcl=9aaf197afba69b28d1b9fd56d77d0c2acfeb6619&l=404

Can we delete the latter, or should the Chrome Mojo method be moved into //content and combined with the content version?
 
Cc: tedc...@chromium.org twelling...@chromium.org
Owner: mdjones@chromium.org
Looks like the WebContents version is only called from OverlayPanel, which it appears mdjones@ added:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java?type=cs&q=updateBrowserControlsState&sq=package:chromium&l=440

OverlayPanelContent seems to have native methods, so it looks like we could just add a new native method that would call the similar behavior that was changed in Tab.  That should be callable from anywhere in the chrome layer right?
Status: Started (was: Assigned)
In flight patch:
https://chromium-review.googlesource.com/c/chromium/src/+/917218

I'm not going to be handling interstitial pages since the class in question isn't intended to display them. Though if anyone objects to that I can add the logic. I'll handle the unused path cleanup in a follow-up.
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 15 2018

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

commit 67cbdeb0a26d26b4b033f89dbb5603e061bd9964
Author: Matthew Jones <mdjones@chromium.org>
Date: Thu Feb 15 19:36:49 2018

Replace browser controls update in OverlayPanelContent

This change replaces the browser controls update logic in overlay
panels with a native mojo version. This was the only usage of
updateBrowserControls in java web contents which can now be removed.
The number of parameters used by the function in OverlayPanel has been
reduced to handle how it is being used -- either completely shown or
completely hidden.

BUG= 810937 

Change-Id: If17709f43a150dec30b0b9cd0644161b1f5ad265
Reviewed-on: https://chromium-review.googlesource.com/917218
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537100}
[modify] https://crrev.com/67cbdeb0a26d26b4b033f89dbb5603e061bd9964/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java
[modify] https://crrev.com/67cbdeb0a26d26b4b033f89dbb5603e061bd9964/chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
[modify] https://crrev.com/67cbdeb0a26d26b4b033f89dbb5603e061bd9964/chrome/browser/android/bottombar/overlay_panel_content.cc
[modify] https://crrev.com/67cbdeb0a26d26b4b033f89dbb5603e061bd9964/chrome/browser/android/bottombar/overlay_panel_content.h

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 22 2018

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

commit b4456421b708df741adf44a137e76b943222d398
Author: Matthew Jones <mdjones@chromium.org>
Date: Thu Feb 22 20:29:41 2018

Remove unused browser controls IPC path

BUG= 810937 

Change-Id: I0cc6b8c92f1d71cd127d9bc35e0159eeff35b2e1
Reviewed-on: https://chromium-review.googlesource.com/917329
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538547}
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/browser/web_contents/web_contents_android.h
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/common/view_messages.h
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/public/test/android/javatests/src/org/chromium/content/browser/test/mock/MockWebContents.java
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/renderer/render_view_impl.cc
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/renderer/render_view_impl.h
[modify] https://crrev.com/b4456421b708df741adf44a137e76b943222d398/content/renderer/render_view_impl_android.cc

Status: Fixed (was: Started)

Sign in to add a comment