Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(46)

Issue 2706403014: Add Android implementation of navigator.getInstalledRelatedApps. (Closed)

Created:
8 months, 4 weeks ago by Matt Giuca
Modified:
8 months ago
Reviewers:
Nico, boliu, dcheng, nasko
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ Re-land of previous CL that landed on refs/heads/master@{#458632} BUG=587623 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2706403014 Cr-Commit-Position: refs/heads/master@{#458732} Committed: https://chromium.googlesource.com/chromium/src/+/4b6c1fc9bc0f7ef9455a04cbf3f05de02d1a0a99

Patch Set 1 #

Patch Set 2 : Fix compile on Android after huge changes upstream. #

Patch Set 3 : Major cleanup and added TODOs. #

Patch Set 4 : Major work on tests (massive refactoring); plus minor work on code. #

Patch Set 5 : Rebase. #

Patch Set 6 : Heaps heaps heaps more testing. #

Patch Set 7 : Finished writing tests and cleaning up code. #

Patch Set 8 : Factor getting the site out of the main method. #

Patch Set 9 : A bit more cleanup. #

Patch Set 10 : Fix implementation to read string-with-JSON-array not array-of-strings-with-JSON-objects. #

Patch Set 11 : Major rework: Get asset_statements key from manifest, not resources (previous behaviour based on ba… #

Patch Set 12 : Major rework: Get origin from render frame (browser) not from Mojo interface (renderer). #

Patch Set 13 : Rebase from 2671683002 (update Mojo interface). #

Patch Set 14 : Minor (rename argument to match Mojo). #

Patch Set 15 : Rebase. #

Patch Set 16 : Rebase. #

Patch Set 17 : Fixed iframes (now has the correct origin of the frame, not the tab). #

Total comments: 38

Patch Set 18 : Respond to boliu review. #

Patch Set 19 : Rebase. #

Patch Set 20 : Respond to review (importantly, getting the URL on every request, not caching). #

Total comments: 10

Patch Set 21 : Respond to review (nullcheck and s/page/frame). #

Patch Set 22 : Fix compile on Java <= 7 (expectedInstalledRelatedApps is final). #

Patch Set 23 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1086 lines, -3 lines) Patch
M chrome/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +4 lines, -3 lines 0 comments Download
M content/public/android/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +6 lines, -0 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +14 lines, -0 lines 0 comments Download
A content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +49 lines, -0 lines 0 comments Download
A content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +239 lines, -0 lines 0 comments Download
A content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +767 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/installedapp/installed_app_provider.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/installedapp/related_application.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 80 (49 generated)
Matt Giuca
Hi all; OWNERS breakdown: thakis@chromium.org: chrome/browser/DEPS nasko@chromium.org: content/browser/frame_host/render_frame_host_impl.cc boliu@chromium.org: content/public/android/* dcheng: IPC security owners (Mojo ...
8 months, 1 week ago (2017-03-17 05:54:52 UTC) #10
Matt Giuca
On 2017/03/17 05:54:52, Matt Giuca wrote: > Hi all; OWNERS breakdown: > > mailto:thakis@chromium.org: chrome/browser/DEPS ...
8 months, 1 week ago (2017-03-17 05:56:28 UTC) #13
nasko
content/browser/frame_host/render_frame_host_impl.cc looks good. I'll stamp it for real once the full review is complete.
8 months, 1 week ago (2017-03-17 16:27:21 UTC) #16
Nico
chrome/ lgtm
8 months, 1 week ago (2017-03-17 16:32:26 UTC) #17
boliu
https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java#newcode30 content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: String pageUrl = mRenderFrameHost.getLastCommittedURL(); hmm, can the RFH navigate? ...
8 months, 1 week ago (2017-03-17 18:02:56 UTC) #18
Matt Giuca
https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java#newcode30 content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: String pageUrl = mRenderFrameHost.getLastCommittedURL(); On 2017/03/17 18:02:55, boliu wrote: ...
8 months ago (2017-03-20 06:31:57 UTC) #23
boliu
nasko: can RenderFrameHost navigate and thus have URL etc for its lifetime? https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java ...
8 months ago (2017-03-20 16:56:20 UTC) #28
Matt Giuca
https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java#newcode30 content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: String pageUrl = mRenderFrameHost.getLastCommittedURL(); On 2017/03/20 16:56:20, boliu wrote: ...
8 months ago (2017-03-21 04:16:13 UTC) #31
dcheng
(I haven't reviewed the rest of the CL yet, but wanted to comment on the ...
8 months ago (2017-03-21 07:19:43 UTC) #34
dcheng
https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java File content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java#newcode95 content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:95: private static class ContentRenderFrameHostInterfaceRegistrar Nit: though this CL didn't ...
8 months ago (2017-03-21 07:37:51 UTC) #35
Matt Giuca
https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java File content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java#newcode95 content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:95: private static class ContentRenderFrameHostInterfaceRegistrar On 2017/03/21 07:37:50, dcheng wrote: ...
8 months ago (2017-03-21 07:46:01 UTC) #36
boliu
https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java#newcode30 content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: return new URI(mRenderFrameHost.getLastCommittedURL()); On 2017/03/21 07:46:01, Matt Giuca wrote: ...
8 months ago (2017-03-21 15:36:59 UTC) #37
Matt Giuca
https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java#newcode30 content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: return new URI(mRenderFrameHost.getLastCommittedURL()); On 2017/03/21 15:36:59, boliu wrote: > ...
8 months ago (2017-03-22 00:28:48 UTC) #39
boliu
lgtm
8 months ago (2017-03-22 00:33:21 UTC) #42
dcheng
mojo lgtm https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java File content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java#newcode95 content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:95: private static class ContentRenderFrameHostInterfaceRegistrar On 2017/03/21 07:46:01, ...
8 months ago (2017-03-22 01:23:26 UTC) #43
Matt Giuca
On 2017/03/22 01:23:26, dcheng wrote: > mojo lgtm > > https://codereview.chromium.org/2706403014/diff/530001/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java > File > content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java ...
8 months ago (2017-03-22 01:47:32 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706403014/570001
8 months ago (2017-03-22 02:04:13 UTC) #49
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder ...
8 months ago (2017-03-22 02:31:20 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706403014/570001
8 months ago (2017-03-22 04:36:52 UTC) #53
commit-bot: I haz the power
Committed patchset #21 (id:570001) as https://chromium.googlesource.com/chromium/src/+/8773b38c48cf8b27ed4dd1c8ca249944929d3d16
8 months ago (2017-03-22 04:55:26 UTC) #56
horo
A revert of this CL (patchset #21 id:570001) has been created in https://codereview.chromium.org/2762343003/ by horo@chromium.org. ...
8 months ago (2017-03-22 06:07:26 UTC) #57
findit-for-me
Findit identified this CL at revision 458632 as the culprit for failures in the build ...
8 months ago (2017-03-22 06:12:00 UTC) #58
Matt Giuca
On 2017/03/22 06:12:00, findit-for-me wrote: > Findit identified this CL at revision 458632 as the ...
8 months ago (2017-03-22 06:22:57 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706403014/590001
8 months ago (2017-03-22 06:29:37 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706403014/590001
8 months ago (2017-03-22 06:32:23 UTC) #67
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/388552)
8 months ago (2017-03-22 07:13:26 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706403014/610001
8 months ago (2017-03-22 07:50:02 UTC) #72
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/255170)
8 months ago (2017-03-22 08:37:48 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706403014/610001
8 months ago (2017-03-22 12:22:41 UTC) #76
commit-bot: I haz the power
Committed patchset #23 (id:610001) as https://chromium.googlesource.com/chromium/src/+/4b6c1fc9bc0f7ef9455a04cbf3f05de02d1a0a99
8 months ago (2017-03-22 13:28:10 UTC) #79
Matt Giuca
8 months ago (2017-03-23 00:39:26 UTC) #80
Message was sent while issue was closed.
On 2017/03/22 13:28:10, commit-bot: I haz the power wrote:
> Committed patchset #23 (id:610001) as
>
https://chromium.googlesource.com/chromium/src/+/4b6c1fc9bc0f7ef9455a04cbf3f0...

Update on the previous compile failure:

1. I filed a bug https://crbug.com/704013 about a bad builder config.
2. It turns out that all Android builds are now supposed to be using Java 8 (so
the previous commit was valid), but one of the builders is erroneously still
using Java 7. This is being fixed.
3. I've put up a new CL for review https://codereview.chromium.org/2768883003
that reverts Patch Set 22 (requiring Java 8), as a test case to make sure there
are no builders still using Java 7.

Powered by Google App Engine
This is Rietveld efc10ee0f