Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
844313002:
Add ContentBrowserClient::OpenURL to allow tab opening wo/ WebContents. (Closed)
|
Created:
5 years, 11 months ago by mlamouri (slow - plz ping) Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, Fady Samuel, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ContentBrowserClient::OpenURL to allow tab opening wo/ WebContents.
This is fairly similar to WebContentsDelegate::OpenURLFromTab without
the WebContents requirement.
This is required in order to open windows from workers. Especially, to
allow ServiceWorker's Clients.openWindow() to work properly given that
there is no associated WebContents.
BUG=437151
Patch Set 1 #Patch Set 2 : #Patch Set 3 : add thread check #Patch Set 4 : rebase #Patch Set 5 : fix android/ios build #
Total comments: 2
Patch Set 6 : fix typo #
Messages
Total messages: 39 (6 generated)
mlamouri@chromium.org changed reviewers: + jochen@chromium.org, mkwst@chromium.org
mlamouri@chromium.org changed required reviewers: + jochen@chromium.org
Jochen and Mike, could you review this CL? The description should explain its purpose. Jochen, your review is required (you own all the files). Mike, your review is optional given that you only own content/shell/ but you might end up reviewing the Blink side of things later.
On 2015/01/12 15:26:25, Mounir Lamouri wrote: > Jochen and Mike, could you review this CL? The description should explain its > purpose. > > Jochen, your review is required (you own all the files). > Mike, your review is optional given that you only own content/shell/ but you > might end up reviewing the Blink side of things later. My initial reaction is that I don't want workers opening windows. :) How does the popup blocker work in this scenario? How we we tie `clients.openWindow` to a user gesture?
jochen@chromium.org changed required reviewers: - jochen@chromium.org
yeah, same reaction here can we have some discussion about this feature on blink-dev first please?
On 2015/01/12 at 15:28:29, mkwst wrote: > On 2015/01/12 15:26:25, Mounir Lamouri wrote: > > Jochen and Mike, could you review this CL? The description should explain its > > purpose. > > > > Jochen, your review is required (you own all the files). > > Mike, your review is optional given that you only own content/shell/ but you > > might end up reviewing the Blink side of things later. > > My initial reaction is that I don't want workers opening windows. :) > > How does the popup blocker work in this scenario? How we we tie `clients.openWindow` to a user gesture? My plan is to extend what I did for WindowFocus (ie. it's now handled by the ExecutionContext). The token is currently given for the duration of waitUntil() in the notificationclick event handler.
Jochen and I discussed the issue and we agreed that we could handle opening window from service workers the same way as focusing window (ie. for notificationclick only) with the addition of a timeout to prevent an attacker to grab multiple tokens and open many windows when needed. Jochen, is there anything else you need from me?
i wonder what kind of events the webNavigation API sees for a tab opened like this? maybe we should send a onCreatedNavigationTarget and indicate that it's coming from a service worker? can you file a bug for this please? Can you make sure that when you use this API in a chromium debug build (i.e. with the webNavigation API compiled in, you don't trigger any DCHECK)?
On 2015/01/13 at 14:36:46, jochen wrote: > i wonder what kind of events the webNavigation API sees for a tab opened like this? onBeforeNavigate onCommitted onDOMContentLoaded onCompleted Do we expect anything else? > maybe we should send a onCreatedNavigationTarget and indicate that it's coming from a service worker? can you file a bug for this please? Sure, I can. > Can you make sure that when you use this API in a chromium debug build (i.e. with the webNavigation API compiled in, you don't trigger any DCHECK)? Nope, everything is fine.
cool, what about the compile errors?
On 2015/01/13 at 15:45:07, Mounir Lamouri wrote: > > maybe we should send a onCreatedNavigationTarget and indicate that it's coming from a service worker? can you file a bug for this please? > > Sure, I can. https://code.google.com/p/chromium/issues/detail?id=448390
Jochen, PTAL.
I have updated the CL to not compile the content of
ChromeContentBrowserClient::OpenURL() on Android/iOS. Unfortunately,
browser_navigator.cc will add non-mobile dependencies to the build.
My plan is to have a browser_navigator_base.cc for the two methods that are
mobile-friendly. Then a browser_navigator_desktop.cc for chrome::Navigate() as
it is then a browser_navigator_{android,ios}.cc.
On Android, we could have a similar code than WebContentsDelegateAndroid. On
iOS, I'm a bit unsure what can be done.
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
thanks for picking this up, Mounir! https://codereview.chromium.org/844313002/diff/80001/content/public/browser/c... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/844313002/diff/80001/content/public/browser/c... content/public/browser/content_browser_client.h:592: // Allows programmatic opening of a new tab/window without going trough typo: "trough" -> "through"
https://codereview.chromium.org/844313002/diff/80001/content/public/browser/c... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/844313002/diff/80001/content/public/browser/c... content/public/browser/content_browser_client.h:592: // Allows programmatic opening of a new tab/window without going trough On 2015/01/13 at 17:08:46, jsbell wrote: > typo: "trough" -> "through" Done.
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
virtual WebContents* OpenURL(BrowserContext* browser_context,
const OpenURLParams& params);
How does this work given site isolation / storage partitioning. A browsercontext
can have be multiple storage partitions, each of which has a distinct
serviceworkercontext. Can a url opened by a sw in one partition result in a
window being opened for a different partition, and associated with a different
sw registered in that different partition?
On 2015/01/13 at 22:23:40, michaeln wrote: > virtual WebContents* OpenURL(BrowserContext* browser_context, > const OpenURLParams& params); > > How does this work given site isolation / storage partitioning. A browsercontext can have be multiple storage partitions, each of which has a distinct serviceworkercontext. Can a url opened by a sw in one partition result in a window being opened for a different partition, and associated with a different sw registered in that different partition? Hopefully, the navigation code tries to find the right SiteInstance associated with the tab's site. Though, in our case, we are limiting the URL to be same-origin and the first iteration will even limit it to be inside the SW scope (given that we don't handle uncontrolled Clients yet) so we should definitely be in the same SiteInstance.
> Hopefully, the navigation code tries to find the right SiteInstance associated > with the tab's site. Though, in our case, we are limiting the URL to be > same-origin and the first iteration will even limit it to be inside the SW scope > (given that we don't handle uncontrolled Clients yet) so we should definitely be > in the same SiteInstance. Ok, maybe another input the sw system can provide to this is the StoragePartition ptr, with the expectation that the resulting tab will utilize the sw's of that partition?
On 2015/01/13 at 23:32:33, mlamouri wrote: > On 2015/01/13 at 22:23:40, michaeln wrote: > > virtual WebContents* OpenURL(BrowserContext* browser_context, > > const OpenURLParams& params); > > > > How does this work given site isolation / storage partitioning. A browsercontext can have be multiple storage partitions, each of which has a distinct serviceworkercontext. Can a url opened by a sw in one partition result in a window being opened for a different partition, and associated with a different sw registered in that different partition? > > Hopefully, the navigation code tries to find the right SiteInstance associated with the tab's site. Though, in our case, we are limiting the URL to be same-origin and the first iteration will even limit it to be inside the SW scope (given that we don't handle uncontrolled Clients yet) so we should definitely be in the same SiteInstance. can we somehow enforce this that you can't pass in arbitrary stuff to this method?
On 2015/01/14 at 13:20:45, jochen wrote: > On 2015/01/13 at 23:32:33, mlamouri wrote: > > On 2015/01/13 at 22:23:40, michaeln wrote: > > > virtual WebContents* OpenURL(BrowserContext* browser_context, > > > const OpenURLParams& params); > > > > > > How does this work given site isolation / storage partitioning. A browsercontext can have be multiple storage partitions, each of which has a distinct serviceworkercontext. Can a url opened by a sw in one partition result in a window being opened for a different partition, and associated with a different sw registered in that different partition? > > > > Hopefully, the navigation code tries to find the right SiteInstance associated with the tab's site. Though, in our case, we are limiting the URL to be same-origin and the first iteration will even limit it to be inside the SW scope (given that we don't handle uncontrolled Clients yet) so we should definitely be in the same SiteInstance. > > can we somehow enforce this that you can't pass in arbitrary stuff to this method? According to page_navigator.h and browser_navigator.h, OpenURLParams and NavigatorParams both have a site_instance that is meant to be used for empty pages (so there is a default site instance). It seems that passing null is fine and the proper site instance will be used. Site isolation is able to swap pages from a site instance to another anyway (when navigating). I don't think having any kind of limitation here would be sensible because we could definitely re-use that API to do other page opening from content/ that wouldn't be triggered by a Web facing API.
> > can we somehow enforce this that you can't pass in arbitrary stuff to this > method? > > It seems that passing null is fine > and the proper site instance will be used. Is that a leap of faith? Given how profiles can be partitioned, w/o a webcontents or siteintance as input, how are the other inputs enough info to identify what's proper? > I don't think having any kind of limitation here would be sensible because we > could definitely re-use that API to do other page opening from content/ that > wouldn't be triggered by a Web facing API. Two separate things. 1. what the new openurl api lets a caller express 2. how that's used in support of sw.clients.open Based on what was said earlier, the expectation is that the window being opened will result in a new client of the sw that invoked the open method. For that to occur, the new renderer must utilize the storage partition containing the sw. How is that expressed in the new api? (Maybe by passing in an optional storagepartition ptr to this method?)
On 2015/01/14 at 19:46:09, michaeln wrote: > > > can we somehow enforce this that you can't pass in arbitrary stuff to this > > method? > > > > It seems that passing null is fine > > and the proper site instance will be used. > > Is that a leap of faith? Given how profiles can be partitioned, w/o a webcontents or siteintance as input, how are the other inputs enough info to identify what's proper? We create a new site instance when chrome::Navigate() is called unless a target_webcontents is specified. That's the behaviour we seem to have on purpose, see the comment at the end of GetSiteInstanceForNewTab. Note that we could easily use a SiteInstance that matches the new tab site url. I guess that is what used to be done. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tab... Note that even if it's a different SiteInstance, we will still use the same storage partition. To find the storage partition, the browser context uses only the site url (see BrowserContext::GetStoragePartition). I don't think we need to pass the StoragePartition when opening that new window. > > I don't think having any kind of limitation here would be sensible because we > > could definitely re-use that API to do other page opening from content/ that > > wouldn't be triggered by a Web facing API. > > Two separate things. > 1. what the new openurl api lets a caller express > 2. how that's used in support of sw.clients.open > > Based on what was said earlier, the expectation is that the window being opened will result in a new client of the sw that invoked the open method. For that to occur, the new renderer must utilize the storage partition containing the sw. How is that expressed in the new api? (Maybe by passing in an optional storagepartition ptr to this method?) I don't want ContentBrowserClient::OpenURL() to be seen as made only for sw.clients.openWindow(). openWindow(), for now, will only allow opening URLs that are controlled by the SW so we will expect the new page to be controlled. The only reason we will require that is because we only handle controlled clients at the moment and allowing opening uncontrolled URL would break things like postMessage() on the client returned by the call (because we wouldn't know where to send that message). Note that the WIP code I wrote that utilizes this CL is checking whether the newly opened page is controlled. It could definitely support uncontrolled pages if our SW implementation was able to handle uncontrolled clients.
On 2015/01/15 at 16:37:52, Mounir Lamouri wrote: > On 2015/01/14 at 19:46:09, michaeln wrote: > > > > can we somehow enforce this that you can't pass in arbitrary stuff to this > > > method? > > > > > > It seems that passing null is fine > > > and the proper site instance will be used. > > > > Is that a leap of faith? Given how profiles can be partitioned, w/o a webcontents or siteintance as input, how are the other inputs enough info to identify what's proper? > > We create a new site instance when chrome::Navigate() is called unless a target_webcontents is specified. That's the behaviour we seem to have on purpose, see the comment at the end of GetSiteInstanceForNewTab. Note that we could easily use a SiteInstance that matches the new tab site url. I guess that is what used to be done. > See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tab... > > Note that even if it's a different SiteInstance, we will still use the same storage partition. To find the storage partition, the browser context uses only the site url (see BrowserContext::GetStoragePartition). I don't think we need to pass the StoragePartition when opening that new window. > > > > I don't think having any kind of limitation here would be sensible because we > > > could definitely re-use that API to do other page opening from content/ that > > > wouldn't be triggered by a Web facing API. > > > > Two separate things. > > 1. what the new openurl api lets a caller express > > 2. how that's used in support of sw.clients.open > > > > Based on what was said earlier, the expectation is that the window being opened will result in a new client of the sw that invoked the open method. For that to occur, the new renderer must utilize the storage partition containing the sw. How is that expressed in the new api? (Maybe by passing in an optional storagepartition ptr to this method?) > > I don't want ContentBrowserClient::OpenURL() to be seen as made only for sw.clients.openWindow(). openWindow(), for now, will only allow opening URLs that are controlled by the SW so we will expect the new page to be controlled. The only reason we will require that is because we only handle controlled clients at the moment and allowing opening uncontrolled URL would break things like postMessage() on the client returned by the call (because we wouldn't know where to send that message). > > Note that the WIP code I wrote that utilizes this CL is checking whether the newly opened page is controlled. It could definitely support uncontrolled pages if our SW implementation was able to handle uncontrolled clients. Actually, after talking a bit more about sw.clients.openWindow() with Jake, we decided to drop the same-origin requirement. We will allow opening any URL and will return a WindowClient if possible.
This new sw function should probably have similar behavior to plain old window.open(). What happens when content in a webview hosted by a chromeapp (the webview is partitioned) calls window.open()? > I don't want ContentBrowserClient::OpenURL() to be seen as made only for > sw.clients.openWindow(). openWindow() I don't think i suggested it be made only for this purpose?
On 2015/01/15 20:37:33, michaeln wrote: > This new sw function should probably have similar behavior to plain old > window.open(). What happens when content in a webview hosted by a chromeapp (the > webview is partitioned) calls window.open()? Not a precise answer but: basically the same thing, but routed through a WebContents. Deferring entirely to the navigation code to find a SiteInstance was suggested by jam@ - maybe we want to loop him in?
Ok, maybe in the chromeapp + webview case, an attempt by a sw buried in there should result in a 'newwindow' event being raised into the embeddeing chromeapp? https://developer.chrome.com/apps/tags/webview#event-newwindow
On 2015/01/15 at 22:13:34, michaeln wrote: > Ok, maybe in the chromeapp + webview case, an attempt by a sw buried in there should result in a 'newwindow' event being raised into the embeddeing chromeapp? > > https://developer.chrome.com/apps/tags/webview#event-newwindow If I have this Chrome Apps opened: https://github.com/GoogleChrome/chrome-app-samples/tree/master/samples/webvie... and a browser window, running openWindow() from the SW will open the window in the regular browser window and the 'newwindow' event will not fire. I will have a look tomorrow.
ok, I think we really crossed the line where this should get some form of discussion on blink-dev first
On 2015/01/16 08:03:42, jochen (slow) wrote: > ok, I think we really crossed the line where this should get some form of > discussion on blink-dev first There's been discussion about this Clients.openWindow(x) api, my questions are just prying into an impl detail when chromeapps (so storagepartitions) are involved. In light of the 'newwindow' event, I think the answer to my question is pretty clear.
On 2015/01/16 at 21:45:35, michaeln wrote: > On 2015/01/16 08:03:42, jochen (slow) wrote: > > ok, I think we really crossed the line where this should get some form of > > discussion on blink-dev first > > There's been discussion about this Clients.openWindow(x) api, my questions are just prying into an impl detail when chromeapps (so storagepartitions) are involved. In light of the 'newwindow' event, I think the answer to my question is pretty clear. I looked at more code and my understanding is that the difference between window.open() and sw.clients.openWindow() lives mostly in WebContentsImpl::CreateNewWindow(). This is the method called by window.open() and the first line will be different whether the call is coming from a <webview> or not. A <webview> is considered as a guest so the window will be created using GetBrowserPluginGuest()->CreateNewGuestWindow() instead of the usual path (it does also involve some site_instance/storage_partition dance). AFAICT, we would need to associate the SW with a BrowserPluginGuest in order to correctly open a new window in that situation. When a SW is running inside a <webview>, it currently isn't aware of that and only think itself as in incognito (I guess because things are not saved on disk). In order to access a BrowserPluginGuest, we need a WebContents that is associated to one. I think we could do the following: - from the SWVersion, check whether the process we are associated too is a associated to a 'guest' plugin; - if we are in such situation, try to find an associated WebContents; - if there is an associated WebContents, open a new window from the WebContents. Also, as a side question, why don't we use a different BrowserContext for isolated guest processes instead of passing the usual BrowserContext with a boolean? It seems that the behaviour is going to be close to incognito except that many guests processes might exist. Is that right?
mlamouri@chromium.org changed reviewers: + creis@chromium.org
+creis@
I don't know. It's not clear to me that we want to allow a service worker to open arbitrary URLs and window types. With the API you're adding, it could also start downloads, open popups etc..
It seems that there are two different issues here: 1. michaeln@ is worried about the site instance being wrong but after researches it seems that it wouldn't really be, this method is meant to open an url in the browser context, not in a <webview> or <appview>. If sw.clients.openWindow() is called for a SW that is owned by such a guest, the window will be opened in the browser and will have the site instance that would match that site url in the browser. This is slightly odd and probably not ideal but I think it's a problem that should be tackled somewhere than here (the caller has to be aware of the context and do the right thing). In addition, I do not think we consider chrome apps <webview> as first tier for SW - they are not even saved to disk when created from there - so maybe that specific issue could be handled in another CL/bug? (Note: in Incognito mode, the BrowserContext is different so we will be opening the window/tab in the right place with the right site instance.) (Note2: wrt to <webview>, the issue isn't limited to the site instance, there is also a problem with how the window is opened, we should use chrome::Navigate() which means that the ContentBrowserClient::OpenURL() should be avoided entirely in that case likely.) 2. jochen@ is worried about SW opening arbitrary URLs and window types. I think we should talk about that and I would be glad to reduce the scope of the feature. Though, I'm not sure this CL is the right medium. The Blink side of the feature will do the origin checks anyway? Maybe we could proceed here knowing that the discussion will happen? (I hope that we will have the chance to talk about that face to face next week.)
as discussed offline, can you please merge the service worker change into this CL?
I'm applying the change we discussed and will upload a merged CL.
I've merged everything to https://codereview.chromium.org/843583005. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
