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

Issue 649782 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Last visit 16 days ago
Closed: Jan 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

arc/mojo: Make arc::InstanceHolder<T>::GetInstanceForMethod() easier to use

Project Member Reported by lhchavez@chromium.org, Sep 23 2016

Issue description

Currently, developers use arc::InstanceHolder<T>::GetInstanceForMethod() defined in components/arc/instance_holder.h for getting a mojo instance like arc::mojom::AppInstance*. The call is usually like this:

 components/arc/net/arc_net_host_impl.cc:  auto* net_instance = arc_bridge_service()->net()->GetInstanceForMethod(
 components/arc/net/arc_net_host_impl.cc-      "ScanCompleted", kScanCompletedMinInstanceVersion);

but there are two small issues here: 1) the first argument (string) is only for logging, and 2) developers have to specify the version manually (which is error-prone.)  It'd be nice if the interface looks like something like:

 auto* net_instance = arc_bridge_service()->net()->GetInstanceForMethod(&mojom::NetInstance::ScanCompleted);
 
Welp, I thought it was trivial at first, but now I realized that the type of "&mojom::NetInstance::ScanCompleted" is "void(mojom::NetInstance::*)()", so anything that would try to get this out of just the function pointer or type information will not work as well as I wanted originally :(

Comment 2 by yzshen@chromium.org, Sep 23 2016

Cc: roc...@chromium.org
We already have constants defined for each method's ordinal number, something like mojom::internal::kNetInstance_ScanCompleted_Name. We could expose those constants, e.g., mojom::NetInstance::kScanCompletedMethodId.

We could also generate mapping between methods and versions:

static const struct {
  uint32_t version;
  uint32_t max_method_oridinal_in_that_version;  // better name needed.
} kVersionInfo;

With such support, we could then do:
auto* net_instance = arc_bridge_service()->net()->GetInstanceForMethod(mojom::NetInstance::kScanCompletedMethodId);

WDYT?
that plus exposing the name in string form somewhere (that would allow us to avoid using macros to stringify them for logging) would be perfect :)

Comment 4 by yzshen@chromium.org, Sep 23 2016

WRT string names, we may need to take a look at how much/whether that impacts binary size. I think it shouldn't be much though.
Owner: lhchavez@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 23 2016

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

commit 5b863cf97b57423bddb6b2c3167dd6411b257b33
Author: lhchavez <lhchavez@chromium.org>
Date: Fri Dec 23 00:36:04 2016

[mojo] Expose interface method min_versions

This change adds constants Interface::kMethodNameMinVersion to be able
to identify the minimum version the interface must support to be able to
call that method.

BUG= 649782 
TEST=git try

Review-Url: https://codereview.chromium.org/2594203002
Cr-Commit-Position: refs/heads/master@{#440560}

[modify] https://crrev.com/5b863cf97b57423bddb6b2c3167dd6411b257b33/mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 4 2017

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

commit 0843b0c2e8dcb65871f1c2be55dcd1198124666a
Author: lhchavez <lhchavez@chromium.org>
Date: Wed Jan 04 21:12:30 2017

arc: Use GET_INTERFACE_FOR_METHOD macro

Now that Mojo can generate the min_version information automatically,
migrate arc::InstanceHolder<T>::GetInterfaceForMethod() to
GET_INTERFACE_FOR_METHOD(), which does not require the explicit version
number to be passed.

BUG= 649782 
TEST=git try

Review-Url: https://codereview.chromium.org/2599673005
Cr-Commit-Position: refs/heads/master@{#441464}

[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/arc_auth_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/downloads_watcher/arc_downloads_watcher_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/enterprise/arc_enterprise_reporting_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/policy/arc_policy_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/print/arc_print_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/process/arc_process_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/tts/arc_tts_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/note_taking_helper.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/chromeos/policy/device_status_collector.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/memory/tab_manager_delegate_chromeos.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/speech/tts_chromeos.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/sync/test/integration/sync_arc_package_helper.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/task_manager/providers/arc/arc_process_task.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/ui/app_list/arc/arc_app_utils.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/audio/arc_audio_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/bluetooth/arc_bluetooth_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/boot_phase_monitor/arc_boot_phase_monitor_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/clipboard/arc_clipboard_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/crash_collector/arc_crash_collector_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/ime/arc_ime_bridge_impl.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/instance_holder.h
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/intent_helper/arc_intent_helper_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/kiosk/arc_kiosk_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/metrics/arc_metrics_service.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/net/arc_net_host_impl.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/obb_mounter/arc_obb_mounter_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/power/arc_power_bridge.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/components/arc/storage_manager/arc_storage_manager.cc
[modify] https://crrev.com/0843b0c2e8dcb65871f1c2be55dcd1198124666a/ui/arc/notification/arc_notification_manager.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 7 2017

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 9 2017

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

commit d0c37cba3d84dc551c4d4b98a3dcf23321ed4b17
Author: yusukes <yusukes@chromium.org>
Date: Mon Jan 09 18:20:52 2017

arc: Use ARC_GET_INSTANCE_FOR_METHOD for getting app instance

This is safer than manually specifying min versions.

BUG= 649782 
TEST=git try, ARC still works

Review-Url: https://codereview.chromium.org/2613383002
Cr-Commit-Position: refs/heads/master@{#442295}

[modify] https://crrev.com/d0c37cba3d84dc551c4d4b98a3dcf23321ed4b17/chrome/browser/ui/app_list/arc/arc_app_utils.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 9 2017

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

commit 1411eb8bc047a30d081180b8c30ccdf5124ddfc5
Author: yusukes <yusukes@chromium.org>
Date: Mon Jan 09 18:22:13 2017

arc: Use ARC_GET_INSTANCE_FOR_METHOD for getting file_system instance

This is safer than manually specifying min versions.

BUG= 649782 
TEST=git try

Review-Url: https://codereview.chromium.org/2616223002
Cr-Commit-Position: refs/heads/master@{#442296}

[modify] https://crrev.com/1411eb8bc047a30d081180b8c30ccdf5124ddfc5/chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jan 9 2017

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

commit 74c6655efd699fabfbf40856e2d6717f273812f5
Author: yusukes <yusukes@chromium.org>
Date: Mon Jan 09 19:46:02 2017

arc: rename GetInstanceForVersion

to GetInstanceForVersionDoNotCallDirectly so developers will
always use ARC_GET_INSTANCE_FOR_METHOD instead.

BUG= 649782 
TEST=try

Review-Url: https://codereview.chromium.org/2619843002
Cr-Commit-Position: refs/heads/master@{#442317}

[modify] https://crrev.com/74c6655efd699fabfbf40856e2d6717f273812f5/components/arc/instance_holder.h

Status: Fixed (was: Started)

Comment 13 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58
Labels: code-cleanup
Status: Verified (was: Fixed)

Sign in to add a comment