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

Issue 685383 link

Starred by 1 user

Issue metadata

Status: Fixed
Merged: issue 560466
Owner:
Last visit > 30 days ago
Closed: Mar 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 560466



Sign in to add a comment

Cleanup upstream/downstream object instantiation mechanism

Project Member Reported by cco3@chromium.org, Jan 25 2017

Issue description

The current upstream/downstream object instantiation mechanism requires a bit much and could be simplified.

E.g., it would be nice if we could instantiate objects with a static method, and if creating differing upstream downstream/upstream implementations of classes didn't require registering a new method both upstream and downstream to instantiate objects.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 26 2017

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

commit 5b52d760b18cc7b955ee0bf35cdfe7239f9ba0c7
Author: cco3 <cco3@chromium.org>
Date: Thu Jan 26 19:35:21 2017

Add new pattern for creating downstream objects

This change introduces a new pattern for instantiating objects of
classes that have have differing upstream/downstream implementations.
If a class's constructor requires no parameters (as is most frequently
the case in this scenario), it can be created with:

    ChromeApplication.createObject(MyType.class);

And registered with no additional change upstream, and only a small
implementation (that does not require creating additional methods)
downstream.  Without this change, one must register a new method both
upstream and downstream as well as cast an application context to a
ChromeApplication before calling that newly registered return
type-specifc method.

BUG= 685383 

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

[modify] https://crrev.com/5b52d760b18cc7b955ee0bf35cdfe7239f9ba0c7/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 26 2017

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

commit 5b52d760b18cc7b955ee0bf35cdfe7239f9ba0c7
Author: cco3 <cco3@chromium.org>
Date: Thu Jan 26 19:35:21 2017

Add new pattern for creating downstream objects

This change introduces a new pattern for instantiating objects of
classes that have have differing upstream/downstream implementations.
If a class's constructor requires no parameters (as is most frequently
the case in this scenario), it can be created with:

    ChromeApplication.createObject(MyType.class);

And registered with no additional change upstream, and only a small
implementation (that does not require creating additional methods)
downstream.  Without this change, one must register a new method both
upstream and downstream as well as cast an application context to a
ChromeApplication before calling that newly registered return
type-specifc method.

BUG= 685383 

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

[modify] https://crrev.com/5b52d760b18cc7b955ee0bf35cdfe7239f9ba0c7/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 27 2017

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

commit bf26a5aa18bcd875deed4af9e4c7995f701de3f9
Author: cco3 <cco3@chromium.org>
Date: Fri Jan 27 22:28:37 2017

Add an mImplementationMap to ChromeApplication

This removes the need for a createObjectImpl method that needs to
be implemented both upstream and downstream.

BUG= 685383 

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

[modify] https://crrev.com/bf26a5aa18bcd875deed4af9e4c7995f701de3f9/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

Project Member

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

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/c38d8f2aee490b9479ac1ff16e633f8834b51ce7

commit c38d8f2aee490b9479ac1ff16e633f8834b51ce7
Author: Conley Owens <cco3@google.com>
Date: Thu Jan 26 20:27:54 2017

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 30 2017

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

commit f77ebec0c7db165ee6039e617432faa12c436641
Author: cco3 <cco3@chromium.org>
Date: Mon Jan 30 21:48:36 2017

Use new app-level object instantiation mechanism

This change uses a new mechanism for instantiating objects from the
Chrome application for several classes.

BUG= 685383 

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

[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/help/HelpAndFeedback.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java
[modify] https://crrev.com/f77ebec0c7db165ee6039e617432faa12c436641/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 31 2017

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

commit a4b96272c745c096c11b55f4f7a33b3d1b766101
Author: aberent <aberent@chromium.org>
Date: Tue Jan 31 14:29:14 2017

Revert of Use new app-level object instantiation mechanism (patchset #3 id:40001 of https://codereview.chromium.org/2652383002/ )

Reason for revert:
This causes Clank to crash on startup (consistently).

BUG=687142

Original issue's description:
> Use new app-level object instantiation mechanism
>
> This change uses a new mechanism for instantiating objects from the
> Chrome application for several classes.
>
> BUG= 685383 
>
> Review-Url: https://codereview.chromium.org/2652383002
> Cr-Commit-Position: refs/heads/master@{#447078}
> Committed: https://chromium.googlesource.com/chromium/src/+/f77ebec0c7db165ee6039e617432faa12c436641

TBR=mariakhomenko@chromium.org,cco3@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 685383 

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

[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtils.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/gsa/GSAServiceClient.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/help/HelpAndFeedback.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/instantapps/InstantAppsHandler.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java
[modify] https://crrev.com/a4b96272c745c096c11b55f4f7a33b3d1b766101/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebEnvironment.java

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 3 2017

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

commit f5b9f2202525189a68dae03f3fc9adf1f299b8fc
Author: cco3 <cco3@chromium.org>
Date: Fri Feb 03 22:07:52 2017

Remove implementation map from ChromeApplication

Some exploration of this approach to generating upstream/downstream
objects has revealed that it doesn't properly handle some use cases.

BUG= 685383 , 687142

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

[modify] https://crrev.com/f5b9f2202525189a68dae03f3fc9adf1f299b8fc/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

Mergedinto: 560466
Status: Duplicate (was: Untriaged)
Blocking: 560466
Cc: estevenson@chromium.org
Labels: OS-Android
Status: Started (was: Duplicate)
Changing to blocking, as the bug I duped it in has a broader scope.
Status: Fixed (was: Started)
Going to close this one anyways since I forgot to change the bug on this CL: https://codereview.chromium.org/2697933002/.

Sign in to add a comment