Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2863623004:
Expand hosted app docs (Closed)
DescriptionExpand hosted app docs
BUG=none
R=rdevlin.cronin@chromium.org
Review-Url: https://codereview.chromium.org/2863623004
Cr-Commit-Position: refs/heads/master@{#472203}
Committed: https://chromium.googlesource.com/chromium/src/+/b323ccf05a36077b39dd990685abc2e4473cb6be
Patch Set 1 #
Total comments: 13
Patch Set 2 : Correcter and conciser #
Total comments: 3
Patch Set 3 : . #Messages
Total messages: 22 (5 generated)
I did not know about this feature. Devlin, can you confirm my understanding?
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:78: communicate with other instances of itself on the client side. Yep. Not sure if you want to include any of this, but the following things are also true: 1) This requires the "background" permission. 2) All instances of the hosted app share the same process if this is specified, unless "allow_js_access" is set to false in the manifest (in which case the hosted app cannot directly script the background page or other instances of itself). Note that we're investigating how much this is used to see whether we can remove it.
Thanks Charlie. https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:78: communicate with other instances of itself on the client side. On 2017/05/04 22:33:21, Charlie Reis wrote: > Yep. Not sure if you want to include any of this, but the following things are > also true: > 1) This requires the "background" permission. > 2) All instances of the hosted app share the same process if this is specified, > unless "allow_js_access" is set to false in the manifest (in which case the > hosted app cannot directly script the background page or other instances of > itself). I'll probably leave it out -- this doc is meant as an overview of the differences between these app types, so I'm aiming for a low amount of detail that is still sufficient to define them. > > Note that we're investigating how much this is used to see whether we can remove > it. yep, that's how I found out about it (https://docs.google.com/document/d/18L_7fb7aeCxQUNmbw1ny21EuHUhCLp6q_ENCWamyX...)
lgtm https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which Nit: s/A page loaded as a hosted app/a hosted app The page doesn't create the background page, Chrome does, right? I'd also s/background window/background page, which is a little more common. https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:77: is shared among all instances of the hosted app, allowing the site to all pages of the hosted app? https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:78: communicate with other instances of itself on the client side. s/instances/pages?
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which On 2017/05/05 16:07:37, Devlin wrote: > Nit: s/A page loaded as a hosted app/a hosted app > > The page doesn't create the background page, Chrome does, right? I'd also > s/background window/background page, which is a little more common. I don't know. Is the background page created at startup, or only when the hosted site calls window.open(..., ..., 'background')? https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:77: is shared among all instances of the hosted app, allowing the site to On 2017/05/05 16:07:37, Devlin wrote: > all pages of the hosted app? Well, I want to emphasize that the "app" can span multiple tabs or windows or whatever, which can communicate with each other, whereas "all pages" sounds like navigating between pages in one tab. Is there a better way to explain that here?
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which On 2017/05/05 19:24:53, michaelpg wrote: > On 2017/05/05 16:07:37, Devlin wrote: > > Nit: s/A page loaded as a hosted app/a hosted app > > > > The page doesn't create the background page, Chrome does, right? I'd also > > s/background window/background page, which is a little more common. > > I don't know. Is the background page created at startup, or only when the hosted > site calls window.open(..., ..., 'background')? I think created here: https://chromium.googlesource.com/chromium/src/+/4a7bb05ec61c43d03b6cba254dec... (in response to the extension loading, which happens at startup or installation) https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:77: is shared among all instances of the hosted app, allowing the site to On 2017/05/05 19:24:53, michaelpg wrote: > On 2017/05/05 16:07:37, Devlin wrote: > > all pages of the hosted app? > > Well, I want to emphasize that the "app" can span multiple tabs or windows or > whatever, which can communicate with each other, whereas "all pages" sounds like > navigating between pages in one tab. Is there a better way to explain that here? Tabs? WebContents?
creis@chromium.org changed reviewers: + creis@chromium.org
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which On 2017/05/05 16:07:37, Devlin wrote: > Nit: s/A page loaded as a hosted app/a hosted app > > The page doesn't create the background page, Chrome does, right? I'd also > s/background window/background page, which is a little more common. Side note: Both under the hood and from the user's perspective, there's a big difference between BackgroundContents (which this feature uses) and BackgroundPage (which is the more common extension case). Among other things, the former is around for the lifetime of the OS, while the latter is around for the lifetime of Chrome. See https://crbug.com/77790. Not sure if we want to conflate the two?
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which On 2017/05/05 20:25:15, Devlin wrote: > On 2017/05/05 19:24:53, michaelpg wrote: > > On 2017/05/05 16:07:37, Devlin wrote: > > > Nit: s/A page loaded as a hosted app/a hosted app > > > > > > The page doesn't create the background page, Chrome does, right? I'd also > > > s/background window/background page, which is a little more common. > > > > I don't know. Is the background page created at startup, or only when the > hosted > > site calls window.open(..., ..., 'background')? > > I think created here: > https://chromium.googlesource.com/chromium/src/+/4a7bb05ec61c43d03b6cba254dec... > (in response to the extension loading, which happens at startup or > installation) How do I actually get at my hosted app's background from a page? window.open(<url>, '', 'background') has some odd behavior when run by multiple tabs.
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which On 2017/05/05 21:29:06, michaelpg wrote: > On 2017/05/05 20:25:15, Devlin wrote: > > On 2017/05/05 19:24:53, michaelpg wrote: > > > On 2017/05/05 16:07:37, Devlin wrote: > > > > Nit: s/A page loaded as a hosted app/a hosted app > > > > > > > > The page doesn't create the background page, Chrome does, right? I'd also > > > > s/background window/background page, which is a little more common. > > > > > > I don't know. Is the background page created at startup, or only when the > > hosted > > > site calls window.open(..., ..., 'background')? > > > > I think created here: > > > https://chromium.googlesource.com/chromium/src/+/4a7bb05ec61c43d03b6cba254dec... > > (in response to the extension loading, which happens at startup or > > installation) > > How do I actually get at my hosted app's background from a page? > window.open(<url>, '', 'background') has some odd behavior when run by multiple > tabs. Yeah, Nick and I hit that too when trying to test it. You have to use a non-empty window name (second parameter).
https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can create and script a background window, which On 2017/05/05 21:31:06, Charlie Reis wrote: > On 2017/05/05 21:29:06, michaelpg wrote: > > On 2017/05/05 20:25:15, Devlin wrote: > > > On 2017/05/05 19:24:53, michaelpg wrote: > > > > On 2017/05/05 16:07:37, Devlin wrote: > > > > > Nit: s/A page loaded as a hosted app/a hosted app > > > > > > > > > > The page doesn't create the background page, Chrome does, right? I'd > also > > > > > s/background window/background page, which is a little more common. > > > > > > > > I don't know. Is the background page created at startup, or only when the > > > hosted > > > > site calls window.open(..., ..., 'background')? > > > > > > I think created here: > > > > > > https://chromium.googlesource.com/chromium/src/+/4a7bb05ec61c43d03b6cba254dec... > > > (in response to the extension loading, which happens at startup or > > > installation) > > > > How do I actually get at my hosted app's background from a page? > > window.open(<url>, '', 'background') has some odd behavior when run by > multiple > > tabs. > > Yeah, Nick and I hit that too when trying to test it. You have to use a > non-empty window name (second parameter). There's still something odd happening. A window.open call from one tab seems to replace or reset any handles to other tabs' previously-opened 'background' window...
On 2017/05/05 21:39:14, michaelpg wrote: > https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... > File extensions/docs/extension_and_app_types.md (right): > > https://codereview.chromium.org/2863623004/diff/1/extensions/docs/extension_a... > extensions/docs/extension_and_app_types.md:75: A page loaded as a hosted app can > create and script a background window, which > On 2017/05/05 21:31:06, Charlie Reis wrote: > > On 2017/05/05 21:29:06, michaelpg wrote: > > > On 2017/05/05 20:25:15, Devlin wrote: > > > > On 2017/05/05 19:24:53, michaelpg wrote: > > > > > On 2017/05/05 16:07:37, Devlin wrote: > > > > > > Nit: s/A page loaded as a hosted app/a hosted app > > > > > > > > > > > > The page doesn't create the background page, Chrome does, right? I'd > > also > > > > > > s/background window/background page, which is a little more common. > > > > > > > > > > I don't know. Is the background page created at startup, or only when > the > > > > hosted > > > > > site calls window.open(..., ..., 'background')? > > > > > > > > I think created here: > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/4a7bb05ec61c43d03b6cba254dec... > > > > (in response to the extension loading, which happens at startup or > > > > installation) > > > > > > How do I actually get at my hosted app's background from a page? > > > window.open(<url>, '', 'background') has some odd behavior when run by > > multiple > > > tabs. > > > > Yeah, Nick and I hit that too when trying to test it. You have to use a > > non-empty window name (second parameter). > > There's still something odd happening. A window.open call from one tab seems to > replace or reset any handles to other tabs' previously-opened 'background' > window... That's working as intended, I think: the implementation limits us to one BackgroundContents per app. It's a super weird API. If you want to connect without navigating, I think you can call window.open('', 'bg', 'background').
I've reduced the paragraph to a single sentence -- PTAL.
Thanks-- one concern below. https://codereview.chromium.org/2863623004/diff/20001/extensions/docs/extensi... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/20001/extensions/docs/extensi... extensions/docs/extension_and_app_types.md:76: can be scripted from all tabs running the hosted app. Can we please write this in a way that doesn't suggest that people start using this behavior? My team is trying to find ways to deprecate the process-per-site part of it, because it's a huge burden on the process model and leads to bad behavior (e.g., all instances of the hosted app slowing each other down). Even something as simple as adding this might help: "Specifying allow_js_access: false is preferred, to allow multiple instances of the hosted app to run in different processes."
https://codereview.chromium.org/2863623004/diff/20001/extensions/docs/extensi... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/20001/extensions/docs/extensi... extensions/docs/extension_and_app_types.md:76: can be scripted from all tabs running the hosted app. On 2017/05/08 20:15:25, Charlie Reis wrote: > Can we please write this in a way that doesn't suggest that people start using > this behavior? My team is trying to find ways to deprecate the process-per-site > part of it, because it's a huge burden on the process model and leads to bad > behavior (e.g., all instances of the hosted app slowing each other down). > > Even something as simple as adding this might help: "Specifying allow_js_access: > false is preferred, to allow multiple instances of the hosted app to run in > different processes." This isn't supposed to be a tutorial for extension authors, but I see your concern with adding this detail. I've added your note and a comment at the top that this doc is for the extensions module rather than actual usage guidelines.
LGTM https://codereview.chromium.org/2863623004/diff/20001/extensions/docs/extensi... File extensions/docs/extension_and_app_types.md (right): https://codereview.chromium.org/2863623004/diff/20001/extensions/docs/extensi... extensions/docs/extension_and_app_types.md:76: can be scripted from all tabs running the hosted app. On 2017/05/16 00:10:11, michaelpg wrote: > On 2017/05/08 20:15:25, Charlie Reis wrote: > > Can we please write this in a way that doesn't suggest that people start using > > this behavior? My team is trying to find ways to deprecate the > process-per-site > > part of it, because it's a huge burden on the process model and leads to bad > > behavior (e.g., all instances of the hosted app slowing each other down). > > > > Even something as simple as adding this might help: "Specifying > allow_js_access: > > false is preferred, to allow multiple instances of the hosted app to run in > > different processes." > > This isn't supposed to be a tutorial for extension authors, but I see your > concern with adding this detail. I've added your note and a comment at the top > that this doc is for the extensions module rather than actual usage guidelines. Thanks!
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2863623004/#ps40001 (title: ".")
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": 40001, "attempt_start_ts": 1494962539487480,
"parent_rev": "37ca59f82e27b113196d8427acade1b5f04b0f8b", "commit_rev":
"b323ccf05a36077b39dd990685abc2e4473cb6be"}
Message was sent while issue was closed.
Description was changed from ========== Expand hosted app docs BUG=none R=rdevlin.cronin@chromium.org ========== to ========== Expand hosted app docs BUG=none R=rdevlin.cronin@chromium.org Review-Url: https://codereview.chromium.org/2863623004 Cr-Commit-Position: refs/heads/master@{#472203} Committed: https://chromium.googlesource.com/chromium/src/+/b323ccf05a36077b39dd990685ab... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b323ccf05a36077b39dd990685ab... |
