Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769993002:
Restrict getInstalledRelatedApps() to non-incognito contexts. (Closed)
|
Created:
3 years, 9 months ago by Timothy Loh Modified:
3 years, 8 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. |
DescriptionRestrict getInstalledRelatedApps() to non-incognito contexts.
The explainer doc states:
"The User Agent should return no installed applications when running in
a privacy preserving mode, for example Incognito in Chrome".
https://github.com/WICG/get-installed-related-apps/blob/master/EXPLAINER.md
BUG=703477
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2769993002
Cr-Commit-Position: refs/heads/master@{#459691}
Committed: https://chromium.googlesource.com/chromium/src/+/16dbfc61f5335aef68b05282eba042632b2ea9a8
Patch Set 1 #
Total comments: 9
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address further comments #
Total comments: 15
Patch Set 4 : fix variable name + todo #
Messages
Total messages: 34 (19 generated)
Description was changed from ========== Restrict getInstalledRelatedApps() to non-incognito contexts. BUG=703477 ========== to ========== Restrict getInstalledRelatedApps() to non-incognito contexts. BUG=703477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by timloh@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...
Description was changed from ========== Restrict getInstalledRelatedApps() to non-incognito contexts. BUG=703477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Restrict getInstalledRelatedApps() to non-incognito contexts. The explainer doc states: "The User Agent should return no installed applications when running in a privacy preserving mode, for example Incognito in Chrome". https://github.com/WICG/get-installed-related-apps/blob/master/EXPLAINER.md BUG=703477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
timloh@chromium.org changed reviewers: + boliu@chromium.org, mgiuca@chromium.org
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...)
On 2017/03/23 07:24:40, commit-bot: I haz the power wrote: > 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...) oops, the test I added doesn't pass yet, hang on..
https://codereview.chromium.org/2769993002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_android.cc (right): https://codereview.chromium.org/2769993002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_android.cc:60: jboolean RenderFrameHostAndroid::IsIncognito( I feel RFH has no business exposing whether it's InCognito or not. this is a slippery slop of these things becoming "god objects" that's just a bunch of unrelated code. How hard is it to have a java counterpart to BrowserContext instead? https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:55: if (mNativeRenderFrameHostAndroid == 0) return false; that default is a bit dangerous, no? better implementation is to just cache the value in java on construction https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:73: callback.call(new RelatedApplication[0]); return? https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java (right): https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:24: boolean isIncognito(); this isn't used outside of content, so doesn't need to be in the public interface content code can just cast things to RFHImpl
The CQ bit was checked by timloh@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/2769993002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_android.cc (right): https://codereview.chromium.org/2769993002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_android.cc:60: jboolean RenderFrameHostAndroid::IsIncognito( On 2017/03/23 18:12:51, boliu wrote: > I feel RFH has no business exposing whether it's InCognito or not. this is a > slippery slop of these things becoming "god objects" that's just a bunch of > unrelated code. > > How hard is it to have a java counterpart to BrowserContext instead? For now I added a comment in the header file -- I'm not at all familiar with RFH or JNI to make an accurate decision on whether we should add a BrowserContext class now, or at some point in the future when we need other functions (do we also need a SiteInstance to get to the BrowserContext?) https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:55: if (mNativeRenderFrameHostAndroid == 0) return false; On 2017/03/23 18:12:52, boliu wrote: > that default is a bit dangerous, no? better implementation is to just cache the > value in java on construction Done. https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java (right): https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderImpl.java:73: callback.call(new RelatedApplication[0]); On 2017/03/23 18:12:52, boliu wrote: > return? Fixed, thanks. This of course makes the test pass :) https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java (right): https://codereview.chromium.org/2769993002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content_public/browser/RenderFrameHost.java:24: boolean isIncognito(); On 2017/03/23 18:12:52, boliu wrote: > this isn't used outside of content, so doesn't need to be in the public > interface > > content code can just cast things to RFHImpl Done, but please double-check I understood correctly, thanks :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2769993002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_android.cc (right): https://codereview.chromium.org/2769993002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_android.cc:60: jboolean RenderFrameHostAndroid::IsIncognito( On 2017/03/24 03:11:31, Timothy Loh wrote: > On 2017/03/23 18:12:51, boliu wrote: > > I feel RFH has no business exposing whether it's InCognito or not. this is a > > slippery slop of these things becoming "god objects" that's just a bunch of > > unrelated code. > > > > How hard is it to have a java counterpart to BrowserContext instead? > > For now I added a comment in the header file -- I'm not at all familiar with RFH > or JNI to make an accurate decision on whether we should add a BrowserContext > class now, or at some point in the future when we need other functions (do we > also need a SiteInstance to get to the BrowserContext?) Can skip SiteInstance if there is no need. This is ok for now since this is not exposed out of content. But someone will draw the short straw at some point.. https://codereview.chromium.org/2769993002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:26: mIncognito = nativeIsIncognito(mNativeRenderFrameHostAndroid); bool can just be passed up through create on, like nativeRenderFrameHostAndroid
The CQ bit was checked by timloh@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/2769993002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:26: mIncognito = nativeIsIncognito(mNativeRenderFrameHostAndroid); On 2017/03/24 03:54:49, boliu wrote: > bool can just be passed up through create on, like nativeRenderFrameHostAndroid Done - removed isIncognito from RenderFrameHostAndroid and moved the comment to isIncognito here.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2769993002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_android.cc (right): https://codereview.chromium.org/2769993002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_android.cc:44: bool isIncognito = render_frame_host_->GetSiteInstance() Naming: is_incognito https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:23: boolean isIncognito) { Formatting looks weird; is this clang-formatted? https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:58: * This function shouldn't really be on here. If we end up needing more Maybe make this a TODO? https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:62: public boolean isIncognito() { Add this method to the interface and @Override it. https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:63: return mIncognito; Should this be calling through to the native implementation rather than caching the value here? (I assume it isn't possible for a RFH to change between incognito/not incognito dynamically? If so then this is fine.) https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:25: mRenderFrameHost = (RenderFrameHostImpl) renderFrameHost; Ew, please don't do this. As I said above, add the method to the interface then you don't need to know anything about Impl.
Cancelling some comments... lgtm with the remaining comments sorted. After the discussion we had offline, I think this actually doesn't belong in content/public (since it doesn't implement any public interface). Will consider this later. https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:62: public boolean isIncognito() { On 2017/03/24 05:06:55, Matt Giuca wrote: > Add this method to the interface and @Override it. Never mind... see below. https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:63: return mIncognito; On 2017/03/24 05:06:55, Matt Giuca wrote: > Should this be calling through to the native implementation rather than caching > the value here? > > (I assume it isn't possible for a RFH to change between incognito/not incognito > dynamically? If so then this is fine.) Never mind, I see you had this earlier and was asked to change it. https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:25: mRenderFrameHost = (RenderFrameHostImpl) renderFrameHost; On 2017/03/24 05:06:55, Matt Giuca wrote: > Ew, please don't do this. As I said above, add the method to the interface then > you don't need to know anything about Impl. Discussed offline. Apparently this ... is a thing? OK go ahead and land this. I think moving this file out of content makes sense actually, and then we should not be accessing the RFHI, but for now this is OK.
https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:25: mRenderFrameHost = (RenderFrameHostImpl) renderFrameHost; On 2017/03/24 05:31:44, Matt Giuca wrote: > On 2017/03/24 05:06:55, Matt Giuca wrote: > > Ew, please don't do this. As I said above, add the method to the interface > then > > you don't need to know anything about Impl. > > Discussed offline. Apparently this ... is a thing? Yeah this is one of the things called out in content API: https://www.chromium.org/developers/content-module/content-api """ content implementation code should use other implementations directly and not go through the interface (i.e. code in content/renderer should use RenderViewImpl instead of content::RenderView) """ > > OK go ahead and land this. I think moving this file out of content makes sense > actually, and then we should not be accessing the RFHI, but for now this is OK.
https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:25: mRenderFrameHost = (RenderFrameHostImpl) renderFrameHost; On 2017/03/24 16:34:46, boliu wrote: > On 2017/03/24 05:31:44, Matt Giuca wrote: > > On 2017/03/24 05:06:55, Matt Giuca wrote: > > > Ew, please don't do this. As I said above, add the method to the interface > > then > > > you don't need to know anything about Impl. > > > > Discussed offline. Apparently this ... is a thing? > > Yeah this is one of the things called out in content API: > https://www.chromium.org/developers/content-module/content-api > """ > content implementation code should use other implementations directly and not go > through the interface (i.e. code in content/renderer should use RenderViewImpl > instead of content::RenderView) > """ > > > > > OK go ahead and land this. I think moving this file out of content makes sense > > actually, and then we should not be accessing the RFHI, but for now this is > OK. > Right, but that advice is kind of telling you "if you have a RFHI, don't upcast it to a RFH and use the interface; keep it as a RFHI". Which is fine. The gross part is that here we have a RFH abstract reference (going way up the chain to InterfaceRegistrar.addRenderFrameHostRegistrar) that isn't guaranteed to be a RFHI, and now we're assuming it's an RFHI and downcasting. Which violates basically the main law of object oriented programming (if you have a base class, don't assume it's a subclass). Apparently I haven't been programming enough in content/ to realise this is a thing that happens a lot, so I guess we just keep doing it here. But as an outside observer, this seems like a disaster.
https://codereview.chromium.org/2769993002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_android.cc (right): https://codereview.chromium.org/2769993002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_android.cc:44: bool isIncognito = render_frame_host_->GetSiteInstance() On 2017/03/24 05:06:55, Matt Giuca wrote: > Naming: is_incognito Done. https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:23: boolean isIncognito) { On 2017/03/24 05:06:55, Matt Giuca wrote: > Formatting looks weird; is this clang-formatted? It is. https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/framehost/RenderFrameHostImpl.java:58: * This function shouldn't really be on here. If we end up needing more On 2017/03/24 05:06:55, Matt Giuca wrote: > Maybe make this a TODO? Done.
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2769993002/#ps60001 (title: "fix variable name + todo")
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": 60001, "attempt_start_ts": 1490580389955530,
"parent_rev": "2e43cb93b5ab8a098a297d27b60d705076c1213c", "commit_rev":
"16dbfc61f5335aef68b05282eba042632b2ea9a8"}
Message was sent while issue was closed.
Description was changed from ========== Restrict getInstalledRelatedApps() to non-incognito contexts. The explainer doc states: "The User Agent should return no installed applications when running in a privacy preserving mode, for example Incognito in Chrome". https://github.com/WICG/get-installed-related-apps/blob/master/EXPLAINER.md BUG=703477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Restrict getInstalledRelatedApps() to non-incognito contexts. The explainer doc states: "The User Agent should return no installed applications when running in a privacy preserving mode, for example Incognito in Chrome". https://github.com/WICG/get-installed-related-apps/blob/master/EXPLAINER.md BUG=703477 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2769993002 Cr-Commit-Position: refs/heads/master@{#459691} Committed: https://chromium.googlesource.com/chromium/src/+/16dbfc61f5335aef68b05282eba0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/16dbfc61f5335aef68b05282eba0...
Message was sent while issue was closed.
https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java (right): https://codereview.chromium.org/2769993002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/installedapp/InstalledAppProviderFactory.java:25: mRenderFrameHost = (RenderFrameHostImpl) renderFrameHost; On 2017/03/27 00:16:29, Matt Giuca wrote: > On 2017/03/24 16:34:46, boliu wrote: > > On 2017/03/24 05:31:44, Matt Giuca wrote: > > > On 2017/03/24 05:06:55, Matt Giuca wrote: > > > > Ew, please don't do this. As I said above, add the method to the interface > > > then > > > > you don't need to know anything about Impl. > > > > > > Discussed offline. Apparently this ... is a thing? > > > > Yeah this is one of the things called out in content API: > > https://www.chromium.org/developers/content-module/content-api > > """ > > content implementation code should use other implementations directly and not > go > > through the interface (i.e. code in content/renderer should use RenderViewImpl > > instead of content::RenderView) > > """ > > > > > > > > OK go ahead and land this. I think moving this file out of content makes > sense > > > actually, and then we should not be accessing the RFHI, but for now this is > > OK. > > > > Right, but that advice is kind of telling you "if you have a RFHI, don't upcast > it to a RFH and use the interface; keep it as a RFHI". Which is fine. > > The gross part is that here we have a RFH abstract reference (going way up the > chain to InterfaceRegistrar.addRenderFrameHostRegistrar) that isn't guaranteed > to be a RFHI, and now we're assuming it's an RFHI and downcasting. Which > violates basically the main law of object oriented programming (if you have a > base class, don't assume it's a subclass). > > Apparently I haven't been programming enough in content/ to realise this is a > thing that happens a lot, so I guess we just keep doing it here. But as an > outside observer, this seems like a disaster. The indirection is to hide implementation details and avoid exposing methods to content embedder. There are is only single implementations the interface (ignoring tests), so casting should always be safe. I mean if you want to avoid casting, then can change ContentRenderFrameHostInterfaceRegistrar to work on RFHI rather than RFH. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
