HelpApp cut at M38: manifest.json

HelpApp cut at M38: manifest.json

BUG=chromium:404093
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290245

[cnwan, 2014-08-15 11:53:45]

lgtm

[cylee1, 2014-08-15 13:18:08]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2014-08-15 13:19:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cylee@chromium.org/474223002/1

[commit-bot: I haz the power, 2014-08-15 13:19:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 13:19:29]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[cylee1, 2014-08-15 16:36:12]

+ Nikita
committer approval needed. Thanks

[Dmitry Polukhin, 2014-08-18 10:12:20]

https://codereview.chromium.org/474223002/diff/1/chrome/browser/resources/hel...
File chrome/browser/resources/help_app/manifest.json (right):

https://codereview.chromium.org/474223002/diff/1/chrome/browser/resources/hel...
chrome/browser/resources/help_app/manifest.json:18:
"*://support.google.com/chromebook/*"
Why not https?

[cylee1, 2014-08-18 10:35:21]

Also modified OWNERS

https://codereview.chromium.org/474223002/diff/1/chrome/browser/resources/hel...
File chrome/browser/resources/help_app/manifest.json (right):

https://codereview.chromium.org/474223002/diff/1/chrome/browser/resources/hel...
chrome/browser/resources/help_app/manifest.json:18:
"*://support.google.com/chromebook/*"
On 2014/08/18 10:12:20, Dmitry Polukhin wrote:
> Why not https?

Done.

[Dmitry Polukhin, 2014-08-18 10:49:54]

lgtm

https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/help_app/manifest.json (right):

https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
chrome/browser/resources/help_app/manifest.json:11: "js/background-bundle.js"
Please check that background page doesn't remain running in background all the
time i.e. that it starts when needed and shutdown after few sec when it is not
used anymore.

[cylee1, 2014-08-18 11:30:15]

On 2014/08/18 10:49:54, Dmitry Polukhin wrote:
> lgtm
> 
>
https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
> File chrome/browser/resources/help_app/manifest.json (right):
> 
>
https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
> chrome/browser/resources/help_app/manifest.json:11: "js/background-bundle.js"
> Please check that background page doesn't remain running in background all the
> time i.e. that it starts when needed and shutdown after few sec when it is not
> used anymore.

thanks for the reminder.
However since webRequest API cannot be used with event pages
(http://stackoverflow.com/questions/13326105/using-webrequest-api-with-event-page),
and declarativeWebrequest has not gone stable yet
(https://developer.chrome.com/extensions/declarativeWebRequest).
We can't make "persistent: false" now.

[cylee1, 2014-08-18 11:30:27]

The CQ bit was checked by cylee@chromium.org

[commit-bot: I haz the power, 2014-08-18 11:31:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cylee@chromium.org/474223002/40001

[commit-bot: I haz the power, 2014-08-18 12:58:30]

Committed patchset #3 (40001) as 290245

[Dmitry Polukhin, 2014-08-19 07:45:36]

On 2014/08/18 11:30:15, cylee1 wrote:
> On 2014/08/18 10:49:54, Dmitry Polukhin wrote:
>
https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
> > chrome/browser/resources/help_app/manifest.json:11:
"js/background-bundle.js"
> > Please check that background page doesn't remain running in background all
the
> > time i.e. that it starts when needed and shutdown after few sec when it is
not
> > used anymore.
> 
> thanks for the reminder.
> However since webRequest API cannot be used with event pages
>
(http://stackoverflow.com/questions/13326105/using-webrequest-api-with-event-page),
> and declarativeWebrequest has not gone stable yet
> (https://developer.chrome.com/extensions/declarativeWebRequest).
> We can't make "persistent: false" now.

It is not acceptable to keep background page always working for component
extensions. We worked very hard to have none with persistent background page.
Permanent background page waste resources for feature that is used very rare. I
thought that HelpApp is app V2 so its background page transient by default. What
the benefit of having background page? With whom you discussed it? You need to
revert your changes or fix it ASAP.

[cnwan, 2014-08-19 07:53:57]

On 2014/08/19 07:45:36, Dmitry Polukhin wrote:
> On 2014/08/18 11:30:15, cylee1 wrote:
> > On 2014/08/18 10:49:54, Dmitry Polukhin wrote:
> >
>
https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
> > > chrome/browser/resources/help_app/manifest.json:11:
> "js/background-bundle.js"
> > > Please check that background page doesn't remain running in background all
> the
> > > time i.e. that it starts when needed and shutdown after few sec when it is
> not
> > > used anymore.
> > 
> > thanks for the reminder.
> > However since webRequest API cannot be used with event pages
> >
>
(http://stackoverflow.com/questions/13326105/using-webrequest-api-with-event-page),
> > and declarativeWebrequest has not gone stable yet
> > (https://developer.chrome.com/extensions/declarativeWebRequest).
> > We can't make "persistent: false" now.
> 
> It is not acceptable to keep background page always working for component
> extensions. We worked very hard to have none with persistent background page.
> Permanent background page waste resources for feature that is used very rare.
I
> thought that HelpApp is app V2 so its background page transient by default.
What
> the benefit of having background page? With whom you discussed it? You need to
> revert your changes or fix it ASAP.

We are aware of the drawbacks of background pages too. However the feature we
are implementing requires the WebRequest API, which in term forces the extension
uses background pages (the API does not support event pages).

[Dmitry Polukhin, 2014-08-19 07:56:54]

On 2014/08/19 07:53:57, cnwan wrote:
> On 2014/08/19 07:45:36, Dmitry Polukhin wrote:
> > On 2014/08/18 11:30:15, cylee1 wrote:
> > > On 2014/08/18 10:49:54, Dmitry Polukhin wrote:
> > >
> >
>
https://codereview.chromium.org/474223002/diff/40001/chrome/browser/resources...
> > > > chrome/browser/resources/help_app/manifest.json:11:
> > "js/background-bundle.js"
> > > > Please check that background page doesn't remain running in background
all
> > the
> > > > time i.e. that it starts when needed and shutdown after few sec when it
is
> > not
> > > > used anymore.
> > > 
> > > thanks for the reminder.
> > > However since webRequest API cannot be used with event pages
> > >
> >
>
(http://stackoverflow.com/questions/13326105/using-webrequest-api-with-event-page),
> > > and declarativeWebrequest has not gone stable yet
> > > (https://developer.chrome.com/extensions/declarativeWebRequest).
> > > We can't make "persistent: false" now.
> > 
> > It is not acceptable to keep background page always working for component
> > extensions. We worked very hard to have none with persistent background
page.
> > Permanent background page waste resources for feature that is used very
rare.
> I
> > thought that HelpApp is app V2 so its background page transient by default.
> What
> > the benefit of having background page? With whom you discussed it? You need
to
> > revert your changes or fix it ASAP.
> 
> We are aware of the drawbacks of background pages too. However the feature we
> are implementing requires the WebRequest API, which in term forces the
extension
> uses background pages (the API does not support event pages).

It is always costs/benefits analysis. Please send me mail on corp address with
PM involved in CC to discuss all details.

[cylee1, 2014-08-19 09:22:12]

A revert of this CL (patchset #3) has been created in
https://codereview.chromium.org/470313003/ by cylee@chromium.org.

The reason for reverting is: According to Dmitry, we should not use persistent
background page..

[cylee1, 2014-08-19 09:23:25]

On 2014/08/19 09:22:12, cylee1 wrote:
> A revert of this CL (patchset #3) has been created in
> https://codereview.chromium.org/470313003/ by mailto:cylee@chromium.org.
> 
> The reason for reverting is: According to Dmitry, we should not use persistent
> background page..

reverting https://codereview.chromium.org/470313003/