Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706403014:
Add Android implementation of navigator.getInstalledRelatedApps. (Closed)
|
Created:
3 years, 10 months ago by Matt Giuca Modified:
3 years, 9 months ago 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. |
DescriptionAdd 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. #Messages
Total messages: 80 (49 generated)
Description was changed from ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ BUG=587623 ========== to ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ BUG=587623 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #5 (id:80001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #12 (id:250001) has been deleted
Patchset #13 (id:290001) has been deleted
Patchset #14 (id:330001) has been deleted
Patchset #13 (id:310001) has been deleted
Patchset #16 (id:410001) has been deleted
mgiuca@chromium.org changed reviewers: + boliu@chromium.org, dcheng@chromium.org, nasko@chromium.org, thakis@chromium.org
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 service implementation) Thanks!
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/17 05:54:52, Matt Giuca wrote: > Hi all; OWNERS breakdown: > > mailto:thakis@chromium.org: chrome/browser/DEPS > mailto:nasko@chromium.org: content/browser/frame_host/render_frame_host_impl.cc > mailto:boliu@chromium.org: content/public/android/* > dcheng: IPC security owners (Mojo service implementation) > > Thanks! boliu: Sorry this huge CL (mostly tests!) came out of the blue. If this is too big for you, please suggest another Android reviewer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_androi...)
content/browser/frame_host/render_frame_host_impl.cc looks good. I'll stamp it for real once the full review is complete.
chrome/ lgtm
https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: String pageUrl = mRenderFrameHost.getLastCommittedURL(); hmm, can the RFH navigate? in which case, caching the url is probably no a good idea? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:31: return new InstalledAppProviderImpl(pageUrl, contentViewCore.getContext()); this is the application context in chrome iirc, is that the expected one here? if yes, can just get it from ContextUtils? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:28: * content/common/installedapp/installed_app_provider.mojom. cs says this is in third_party/WebKit? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:65: if (app.platform.equals(RELATED_APP_PLATFORM_ANDROID) && app.id != null A nit that reversing the equals call is safer since it also works when app.platform is null as well. I think mojo guarantees platform is never null though? Same nit on the ASSET_STATEMENT_FIELD_NAMESPACE comparison below https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:196: } catch (JSONException e) { nit: you can do "JSONException | URISyntaxException e" I think https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:36: private static final String ASSET_STATEMENTS_KEY = "asset_statements"; some of these can just reuse the constants in InstalledAppProviderImpl? you can add @VisibleForTesting on the fields if you like https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:65: private class FakePackageManager extends DefaultPackageManager { static (I hope) https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:119: private class FakeResources extends Resources { ditto static https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:176: return "{\"relation\": [\"" + relation + "\"]," nit: there is String.format, might be easier to read, but I guess doesn't help with all the escaping https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:217: if (expectedInstalledRelatedApps.length != installedRelatedApps.length) { assertEquals throws an exception on failure, so this should be redundant? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:437: String statements = "[{\"target\" {}}]"; nit: statementsWithSyntaxError? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:490: String statements = "[{\"target\": {\"namespace\": \"" + NAMESPACE_WEB + "\", \"site\": \"" ditto String.format https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:529: String statements = "[{\"relation\": [\"" + RELATION_HANDLE_ALL_URLS + "\"]}]"; ditto String.format https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:545: String statements = "[{\"relation\": [\"" + RELATION_HANDLE_ALL_URLS + "\"]," ditto String.format https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:577: String statements = "[{\"relation\": [\"" + RELATION_HANDLE_ALL_URLS + "\"]," ditto String.format https://codereview.chromium.org/2706403014/diff/450001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/installedapp/related_application.mojom (right): https://codereview.chromium.org/2706403014/diff/450001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:14: string? url; I think this field is never used?
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #18 (id:470001) has been deleted
The CQ bit was unchecked by mgiuca@chromium.org
https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... 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: > hmm, can the RFH navigate? in which case, caching the url is probably no a good > idea? I don't think so; I logged this code path and it gets called every time I navigate (even when simply clicking a link on the same domain), and the filterInstalledApps call seems to always have the correct URL even after a navigation. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:31: return new InstalledAppProviderImpl(pageUrl, contentViewCore.getContext()); On 2017/03/17 18:02:55, boliu wrote: > this is the application context in chrome iirc, is that the expected one here? > if yes, can just get it from ContextUtils? Done. Cool idea, that simplifies the code (I am pretty sure it's all the same context, because it's just used to get the global Android packagemanager). Note that I can't just delete this parameter and call ContextUtils.getApplicationContext() from inside InstalledAppProviderImpl, because for testing we need to be able to pass a fake application context in there. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:28: * content/common/installedapp/installed_app_provider.mojom. On 2017/03/17 18:02:55, boliu wrote: > cs says this is in third_party/WebKit? Done. (Removed the full path, because otherwise there is no guarantee it won't go out of date as WebKit paths are moved around.) https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:65: if (app.platform.equals(RELATED_APP_PLATFORM_ANDROID) && app.id != null On 2017/03/17 18:02:55, boliu wrote: > A nit that reversing the equals call is safer since it also works when > app.platform is null as well. I think mojo guarantees platform is never null > though? > > Same nit on the ASSET_STATEMENT_FIELD_NAMESPACE comparison below Yes, Mojo guarantees platform is not null as it is not optional in the declaration. That's also why there is no test for platform being null (while there are tests for the other two fields). I think I would rather leave it like this because it should not be null and Chrome is supposed to crash when things that shouldn't happen happen. Same goes for the namespace comparison below. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:196: } catch (JSONException e) { On 2017/03/17 18:02:56, boliu wrote: > nit: you can do "JSONException | URISyntaxException e" I think Done. Nice, I didn't know that. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:36: private static final String ASSET_STATEMENTS_KEY = "asset_statements"; On 2017/03/17 18:02:56, boliu wrote: > some of these can just reuse the constants in InstalledAppProviderImpl? you can > add @VisibleForTesting on the fields if you like Done. (I kept these constants but defined them in terms of the others. Deleting them and referencing InstalledAppProviderImpl throughout these tests makes them way more verbose and harder to read.) https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:65: private class FakePackageManager extends DefaultPackageManager { On 2017/03/17 18:02:56, boliu wrote: > static (I hope) Done. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:119: private class FakeResources extends Resources { On 2017/03/17 18:02:56, boliu wrote: > ditto static Done. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:124: super(new AssetManager(), null, null); Hmm, I'm now seeing this deprecation warning: InstalledAppProviderTest.java:125: warning: [deprecation] Resources(AssetManager,DisplayMetrics,Configuration) in Resources has been deprecated super(new AssetManager(), null, null); ^ The docs say "This constructor was deprecated in API level 25. Resources should not be constructed by apps. See createConfigurationContext(Configuration)." (I don't think I was getting this before I synced last week; I think this was just deprecated recently and a new version of the Android API was rolled...?) Is it important that we avoid calling deprecated APIs? If so, how do we make a fake Resources object? If not, should I suppress this warning? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:176: return "{\"relation\": [\"" + relation + "\"]," On 2017/03/17 18:02:56, boliu wrote: > nit: there is String.format, might be easier to read, but I guess doesn't help > with all the escaping Done. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:217: if (expectedInstalledRelatedApps.length != installedRelatedApps.length) { On 2017/03/17 18:02:56, boliu wrote: > assertEquals throws an exception on failure, so this should be redundant? Done. (Oh yeah, I'm used to EXPECT_EQ in C++.) https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:437: String statements = "[{\"target\" {}}]"; On 2017/03/17 18:02:56, boliu wrote: > nit: statementsWithSyntaxError? Not really sure why this needs a special name (there are lots of functions with their own "statements" variable and I don't want to think of a special name for each one when "statements" + the function name is unambiguous). https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:490: String statements = "[{\"target\": {\"namespace\": \"" + NAMESPACE_WEB + "\", \"site\": \"" On 2017/03/17 18:02:56, boliu wrote: > ditto String.format Done. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:529: String statements = "[{\"relation\": [\"" + RELATION_HANDLE_ALL_URLS + "\"]}]"; On 2017/03/17 18:02:56, boliu wrote: > ditto String.format Done. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:545: String statements = "[{\"relation\": [\"" + RELATION_HANDLE_ALL_URLS + "\"]," On 2017/03/17 18:02:56, boliu wrote: > ditto String.format Done. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:577: String statements = "[{\"relation\": [\"" + RELATION_HANDLE_ALL_URLS + "\"]," On 2017/03/17 18:02:56, boliu wrote: > ditto String.format Done. https://codereview.chromium.org/2706403014/diff/450001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/installedapp/related_application.mojom (right): https://codereview.chromium.org/2706403014/diff/450001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/installedapp/related_application.mojom:14: string? url; On 2017/03/17 18:02:56, boliu wrote: > I think this field is never used? Hmm, it isn't used for checking, but it is passed into and back out of the Mojo interface. We need to return the same URL that was passed in, so it has to be in the Mojo interface. This reminds me: Added a test with a non-null URL to make sure the URL is preserved. (Hmm... perhaps a better Mojo interface for InstalledAppProvider would be to return a list of indices of the related applications, rather than the RelatedApplication objects themselves; that way the data does not need to roundtrip through Mojo and we can drop fields like |url| that are not used in the decision process, but need to be preserved. That refactor doesn't need to be done now, though.)
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
nasko: can RenderFrameHost navigate and thus have URL etc for its lifetime? https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: String pageUrl = mRenderFrameHost.getLastCommittedURL(); On 2017/03/20 06:31:56, Matt Giuca wrote: > On 2017/03/17 18:02:55, boliu wrote: > > hmm, can the RFH navigate? in which case, caching the url is probably no a > good > > idea? > > I don't think so; I logged this code path and it gets called every time I > navigate (even when simply clicking a link on the same domain), and the > filterInstalledApps call seems to always have the correct URL even after a > navigation. I don't think that testing is good enough here. nasko@ can probably answer this question definitively. But I think RenderFrames only gets re-used if we run out of renderer processes; but even then they can just be different RenderFrames in the same process, so not sure. otoh, methods like GetLastCommittedURL kinda suggest RFH can navigate. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:124: super(new AssetManager(), null, null); On 2017/03/20 06:31:56, Matt Giuca wrote: > Hmm, I'm now seeing this deprecation warning: > > InstalledAppProviderTest.java:125: warning: [deprecation] > Resources(AssetManager,DisplayMetrics,Configuration) in Resources has been > deprecated > super(new AssetManager(), null, null); > ^ > > The docs say "This constructor was deprecated in API level 25. Resources should > not be constructed by apps. See createConfigurationContext(Configuration)." > > (I don't think I was getting this before I synced last week; I think this was > just deprecated recently and a new version of the Android API was rolled...?) Yeah robolectric got rolled to N recently. Might have something to do with that. > > Is it important that we avoid calling deprecated APIs? If so, how do we make a > fake Resources object? If not, should I suppress this warning? Looking around, robolectric doesn't seem to have good way to add test-only resources, let along in another package. But this is test only, so I'm ok with suppressing this for now. Or if you are really motivated, I guess the part of retrieving things from the resource has to be factored out for the test and can't be tested anymore..
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... 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: > On 2017/03/20 06:31:56, Matt Giuca wrote: > > On 2017/03/17 18:02:55, boliu wrote: > > > hmm, can the RFH navigate? in which case, caching the url is probably no a > > good > > > idea? > > > > I don't think so; I logged this code path and it gets called every time I > > navigate (even when simply clicking a link on the same domain), and the > > filterInstalledApps call seems to always have the correct URL even after a > > navigation. > > I don't think that testing is good enough here. > > nasko@ can probably answer this question definitively. But I think RenderFrames > only gets re-used if we run out of renderer processes; but even then they can > just be different RenderFrames in the same process, so not sure. otoh, methods > like GetLastCommittedURL kinda suggest RFH can navigate. Ah, you're right. https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... suggests that navigation of both the URL and origin is possible (though it's not clear whether changing the origin ever happens in practice; sammc@ suggested that it will never navigate the origin but I don't think we should rely on that). I've rewritten the interface so it takes a RenderFrameHost, and calls getLastCommittedURL on each request. However, I wrapped it in an interface so that it can be faked for testing, and added a test of dynamically changing the URL while the service is open. Note: I could've just implemented a fake RenderFrameHost subclass (instead of making my own wrapper interface) but I figure that people will want to add new methods to RenderFrameHost and I don't want to have to keep updating this fake with null stubs, so this works best. Thanks for spotting this. https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:124: super(new AssetManager(), null, null); On 2017/03/20 16:56:20, boliu wrote: > On 2017/03/20 06:31:56, Matt Giuca wrote: > > Hmm, I'm now seeing this deprecation warning: > > > > InstalledAppProviderTest.java:125: warning: [deprecation] > > Resources(AssetManager,DisplayMetrics,Configuration) in Resources has been > > deprecated > > super(new AssetManager(), null, null); > > ^ > > > > The docs say "This constructor was deprecated in API level 25. Resources > should > > not be constructed by apps. See createConfigurationContext(Configuration)." > > > > (I don't think I was getting this before I synced last week; I think this was > > just deprecated recently and a new version of the Android API was rolled...?) > > Yeah robolectric got rolled to N recently. Might have something to do with that. > > > > > Is it important that we avoid calling deprecated APIs? If so, how do we make a > > fake Resources object? If not, should I suppress this warning? > > Looking around, robolectric doesn't seem to have good way to add test-only > resources, let along in another package. But this is test only, so I'm ok with > suppressing this for now. Or if you are really motivated, I guess the part of > retrieving things from the resource has to be factored out for the test and > can't be tested anymore.. Ah... suppressed ;)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
(I haven't reviewed the rest of the CL yet, but wanted to comment on the RFH navigation issue) https://codereview.chromium.org/2706403014/diff/450001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/450001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: String pageUrl = mRenderFrameHost.getLastCommittedURL(); On 2017/03/21 04:16:13, Matt Giuca wrote: > On 2017/03/20 16:56:20, boliu wrote: > > On 2017/03/20 06:31:56, Matt Giuca wrote: > > > On 2017/03/17 18:02:55, boliu wrote: > > > > hmm, can the RFH navigate? in which case, caching the url is probably no a > > > good > > > > idea? > > > > > > I don't think so; I logged this code path and it gets called every time I > > > navigate (even when simply clicking a link on the same domain), and the > > > filterInstalledApps call seems to always have the correct URL even after a > > > navigation. > > > > I don't think that testing is good enough here. > > > > nasko@ can probably answer this question definitively. But I think > RenderFrames > > only gets re-used if we run out of renderer processes; but even then they can > > just be different RenderFrames in the same process, so not sure. otoh, methods > > like GetLastCommittedURL kinda suggest RFH can navigate. > > Ah, you're right. > > https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_imp... > suggests that navigation of both the URL and origin is possible (though it's not > clear whether changing the origin ever happens in practice; sammc@ suggested > that it will never navigate the origin but I don't think we should rely on > that). > > I've rewritten the interface so it takes a RenderFrameHost, and calls > getLastCommittedURL on each request. > > However, I wrapped it in an interface so that it can be faked for testing, and > added a test of dynamically changing the URL while the service is open. Note: I > could've just implemented a fake RenderFrameHost subclass (instead of making my > own wrapper interface) but I figure that people will want to add new methods to > RenderFrameHost and I don't want to have to keep updating this fake with null > stubs, so this works best. > > Thanks for spotting this. A RenderFrameHost is tied to a specific process ID + frame routing ID. If either of those changes, you'll get a new RFH. Otherwise, you get the original RFH. It is quite common for a RFH's last committed origin and URL to change: renderer-initiated navigations generally do not result in a process swap today, which means both the last committed origin and the URL will change. (You can test this by navigating to your favorite aggregator site and then clicking an outbound cross-origin link. Generally, the tab will still remain in the same process) +nasko, do we have any documentation for this somewhere? It's pretty subtle, hard to understand, and will likely change as we continue with the site isolation work =)
https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:95: private static class ContentRenderFrameHostInterfaceRegistrar Nit: though this CL didn't introduce it, it would be useful to have some comments describing the difference between ContentContextInterfaceRegistrar, ContentWebContentsInterfaceRegistrar, and ContentRenderFrameHostInterfaceRegistrar. I don't actually know what the difference is at all, and it's not clear why there are 3 different types of registrations. https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: return new URI(mRenderFrameHost.getLastCommittedURL()); Nit: seems like the RenderFrameHost interface should just be returning a URI? https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:48: * Small interface for dynamically getting the URL of the current page. s/page/frame (and similarly the interface should probably be named FrameUrlDelegate)
https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... 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: > Nit: though this CL didn't introduce it, it would be useful to have some > comments describing the difference between ContentContextInterfaceRegistrar, > ContentWebContentsInterfaceRegistrar, and > ContentRenderFrameHostInterfaceRegistrar. I don't actually know what the > difference is at all, and it's not clear why there are 3 different types of > registrations. The 3 different types are just for clients that require different arguments. For example, I originally only needed an ApplicationContext so I was registering in ContentContextInterfaceRegistrar, but then I realised I needed the RFH so I moved it here. I can add comments, but this really should be done in a separate CL because there are a number of similar files to this (one in chrome/ for example) and it doesn't make sense to be commenting those other files in this same (already gigantic) CL. https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:30: return new URI(mRenderFrameHost.getLastCommittedURL()); On 2017/03/21 07:37:50, dcheng wrote: > Nit: seems like the RenderFrameHost interface should just be returning a URI? Yes, I agree. I looked at doing this, but the only other client (this was literally added last week) passes it to a function expecting a String. Changing that interface to return a URI would mean parsing then stringifying it in that other client. So... I just left it as-is. https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:48: * Small interface for dynamically getting the URL of the current page. On 2017/03/21 07:37:50, dcheng wrote: > s/page/frame (and similarly the interface should probably be named > FrameUrlDelegate) Ack. (Will do tomorrow.)
https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... 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: > On 2017/03/21 07:37:50, dcheng wrote: > > Nit: seems like the RenderFrameHost interface should just be returning a URI? > > Yes, I agree. I looked at doing this, but the only other client (this was > literally added last week) passes it to a function expecting a String. Changing > that interface to return a URI would mean parsing then stringifying it in that > other client. So... I just left it as-is. getLastCommittedURL can legitimately return null, if the native RFH is gone. I guess renderer side should be gone too by that point, but since these calls are async, probably should be safe than sorry?
Patchset #21 (id:550001) has been deleted
https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... 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: > On 2017/03/21 07:46:01, Matt Giuca wrote: > > On 2017/03/21 07:37:50, dcheng wrote: > > > Nit: seems like the RenderFrameHost interface should just be returning a > URI? > > > > Yes, I agree. I looked at doing this, but the only other client (this was > > literally added last week) passes it to a function expecting a String. > Changing > > that interface to return a URI would mean parsing then stringifying it in that > > other client. So... I just left it as-is. > > getLastCommittedURL can legitimately return null, if the native RFH is gone. I > guess renderer side should be gone too by that point, but since these calls are > async, probably should be safe than sorry? Done. (If this returns null, it's handled in isAppInstalledAndAssociatedWithOrigin which returns false, meaning there are 0 installed apps.) https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:48: * Small interface for dynamically getting the URL of the current page. On 2017/03/21 07:46:01, Matt Giuca wrote: > On 2017/03/21 07:37:50, dcheng wrote: > > s/page/frame (and similarly the interface should probably be named > > FrameUrlDelegate) > > Ack. (Will do tomorrow.) Done.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
mojo lgtm https://codereview.chromium.org/2706403014/diff/530001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java (right): https://codereview.chromium.org/2706403014/diff/530001/content/public/android... content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:95: private static class ContentRenderFrameHostInterfaceRegistrar On 2017/03/21 07:46:01, Matt Giuca wrote: > On 2017/03/21 07:37:50, dcheng wrote: > > Nit: though this CL didn't introduce it, it would be useful to have some > > comments describing the difference between ContentContextInterfaceRegistrar, > > ContentWebContentsInterfaceRegistrar, and > > ContentRenderFrameHostInterfaceRegistrar. I don't actually know what the > > difference is at all, and it's not clear why there are 3 different types of > > registrations. > > The 3 different types are just for clients that require different arguments. For > example, I originally only needed an ApplicationContext so I was registering in > ContentContextInterfaceRegistrar, but then I realised I needed the RFH so I > moved it here. > > I can add comments, but this really should be done in a separate CL because > there are a number of similar files to this (one in chrome/ for example) and it > doesn't make sense to be commenting those other files in this same (already > gigantic) CL. Sure, followup CL is fine. It's just hard for a non-Java person to understand what the difference is between these (especially ContextRegistrar) and if there are lifetime differences.
On 2017/03/22 01:23:26, dcheng wrote: > mojo lgtm > > https://codereview.chromium.org/2706403014/diff/530001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java > (right): > > https://codereview.chromium.org/2706403014/diff/530001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:95: > private static class ContentRenderFrameHostInterfaceRegistrar > On 2017/03/21 07:46:01, Matt Giuca wrote: > > On 2017/03/21 07:37:50, dcheng wrote: > > > Nit: though this CL didn't introduce it, it would be useful to have some > > > comments describing the difference between ContentContextInterfaceRegistrar, > > > ContentWebContentsInterfaceRegistrar, and > > > ContentRenderFrameHostInterfaceRegistrar. I don't actually know what the > > > difference is at all, and it's not clear why there are 3 different types of > > > registrations. > > > > The 3 different types are just for clients that require different arguments. > For > > example, I originally only needed an ApplicationContext so I was registering > in > > ContentContextInterfaceRegistrar, but then I realised I needed the RFH so I > > moved it here. > > > > I can add comments, but this really should be done in a separate CL because > > there are a number of similar files to this (one in chrome/ for example) and > it > > doesn't make sense to be commenting those other files in this same (already > > gigantic) CL. > > Sure, followup CL is fine. It's just hard for a non-Java person to understand > what the difference is between these (especially ContextRegistrar) and if there > are lifetime differences. OK, looks like all the LGs are in place except nasko@ who said it "looks good" but would stamp it later... now he's "out". But boliu@'s LG covers that file. And it's a trivial change. So with that, landing...
The CQ bit was unchecked by mgiuca@chromium.org
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2706403014/#ps570001 (title: "Respond to review (nullcheck and s/page/frame).")
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 570001, "attempt_start_ts": 1490157381474510,
"parent_rev": "74a7a294c2802cf5b65bf0c0d4f44ba093aae216", "commit_rev":
"8773b38c48cf8b27ed4dd1c8ca249944929d3d16"}
Message was sent while issue was closed.
Description was changed from ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ BUG=587623 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ 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@{#458632} Committed: https://chromium.googlesource.com/chromium/src/+/8773b38c48cf8b27ed4dd1c8ca24... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:570001) as https://chromium.googlesource.com/chromium/src/+/8773b38c48cf8b27ed4dd1c8ca24...
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:570001) has been created in https://codereview.chromium.org/2762343003/ by horo@chromium.org. The reason for reverting is: Introduced compile failure. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium%2FAndroid%2F700... ../../content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:255: error: local variable expectedInstalledRelatedApps is accessed from within inner class; needs to be declared final expectedInstalledRelatedApps.length, installedRelatedApps.length); ^ ../../content/public/android/junit/src/org/chromium/content/browser/installedapp/InstalledAppProviderTest.java:259: error: local variable expectedInstalledRelatedApps is accessed from within inner class; needs to be declared final expectedInstalledRelatedApps[i], installedRelatedApps[i]);.
Message was sent while issue was closed.
Findit identified this CL at revision 458632 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/03/22 06:12:00, findit-for-me wrote: > Findit identified this CL at revision 458632 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... Thanks for dealing with this horo@. > error: local variable expectedInstalledRelatedApps is accessed from within inner > class; needs to be declared final grrrRRRRRRR WHAT??? This apparently was an error in Java 7, but was made into a valid program in Java 8: http://docs.oracle.com/javase/tutorial/java/javaOO/localclasses.html#accessin... Do you know what this means?? It means we compile EXCLUSIVELY using Java 8 on the try bots, and then at least one builder uses Java 7. Filing an infra bug...
Message was sent while issue was closed.
Description was changed from ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ 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@{#458632} Committed: https://chromium.googlesource.com/chromium/src/+/8773b38c48cf8b27ed4dd1c8ca24... ========== to ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ 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@{#458632} Committed: https://chromium.googlesource.com/chromium/src/+/8773b38c48cf8b27ed4dd1c8ca24... ==========
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, boliu@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2706403014/#ps590001 (title: "Fix compile on Java <= 7 (expectedInstalledRelatedApps is final).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by mgiuca@chromium.org
Description was changed from ========== Add Android implementation of navigator.getInstalledRelatedApps. Based on a CL by dhnishi: https://codereview.chromium.org/1756793004/ 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@{#458632} Committed: https://chromium.googlesource.com/chromium/src/+/8773b38c48cf8b27ed4dd1c8ca24... ========== to ========== 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 ==========
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, boliu@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2706403014/#ps610001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 610001, "attempt_start_ts": 1490185351725200,
"parent_rev": "c649a390784c87a79d85cefbc6f9bd7097adc207", "commit_rev":
"4b6c1fc9bc0f7ef9455a04cbf3f05de02d1a0a99"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/4b6c1fc9bc0f7ef9455a04cbf3f0... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:610001) as https://chromium.googlesource.com/chromium/src/+/4b6c1fc9bc0f7ef9455a04cbf3f0...
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. |
