New issue
Advanced search Search tips

Issue 857155 link

Starred by 1 user
Status: Fixed
Owner:
s...@chromium.org
Closed: Jul 2
Cc:
pnoland@chromium.org
agrieve@chromium.org
fgor...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Task



Sign in to add a comment

Replace overloaded RunCallbackAndroid methods with uniquely named ones.

Project Member Reported by s...@chromium.org, Jun 27 2018 
base::Bind has a difficult time understanding which of the overloaded methods you want. It is possible to get around this, by doing things such as

void (*runBoolCallback)(const base::android::JavaRef<jobject>&, bool) =
      &base::android::RunCallbackAndroid;
base::Bind(runBoolCallback,...

However, this is clunky, complex, and often developers do not realize they could do this, and create pass through methods instead such as in https://cs.chromium.org/chromium/src/chrome/browser/android/download/service/download_background_task.cc?l=19

It would be better if each of these methods simply had a unique name. These methods are very likely to be bound into a callback, and we should make it as easy as possible for this common use case.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 2 

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

commit ae1742ae90dd54567ac9c9a9707f177a19e82f88
Author: Sky Malice <skym@google.com>
Date: Mon Jul 02 18:40:59 2018

Disambiguate RunCallbackAndroid.

RunCallbackAndroid was very commonly bound into a callback itself, and
due to being overloaded, it was difficult for base::Bind to figure work
correctly. This change changes each version to have a unique name and
updates all call sites. If there was a useless method previously
disambiguating this change removes it.

Additionally any lint violations were fixed.

TBR=nyquist, carlosk, treib, tedchoc, fgorski, boliu, maxmorin

Bug:  857155 
Change-Id: Ia09c936983b95f466c831bade04821ab29ebb14a
Reviewed-on: https://chromium-review.googlesource.com/1117619
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571952}
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/base/android/callback_android.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/base/android/callback_android.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/contextual_suggestions/contextual_suggestions_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/download/service/download_background_task.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/explore_sites/explore_sites_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/feed/feed_image_loader_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/feed/feed_image_loader_bridge.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/feed/feed_network_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/feed/feed_network_bridge.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/ntp/ntp_snippets_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/password_ui_view_android.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/preferences/website_preference_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/rlz/rlz_ping_handler.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/rlz/rlz_ping_handler.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/signin/signin_manager_android.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/android/webapk/webapk_update_manager.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/offline_pages/android/background_scheduler_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/offline_pages/android/background_scheduler_bridge.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/offline_pages/android/cct_request_observer.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/offline_pages/android/evaluation/offline_page_evaluation_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/offline_pages/android/offline_page_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/supervised_user/child_accounts/child_account_service_android.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/chrome/browser/ui/android/context_menu_helper.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/components/feature_engagement/internal/android/tracker_impl_android.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/components/offline_items_collection/core/android/offline_content_aggregator_bridge.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/components/offline_pages/core/background/scheduler.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/components/offline_pages/core/background/scheduler_stub.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/components/offline_pages/core/background/scheduler_stub.h
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/content/browser/frame_host/render_frame_host_android.cc
[modify] https://crrev.com/ae1742ae90dd54567ac9c9a9707f177a19e82f88/media/base/android/media_drm_storage_bridge.cc

Comment 2 by s...@chromium.org, Jul 2

Status: Fixed (was: Started)
►

Sign in to add a comment