Inject automatically on install etc., add mime types, and fix nits.

Inject automatically on install etc., add mime types, and fix nits.

Remove the installation notes page, and instead inject the content script
automatically when the extension is installed, updated, or re-enabled.
Likewise, disable it (remove the keyboard listener) when the extension is
uninstalled or disabled.

Also fix a punctuation nit, and add PDF and chat mime types to the list of
recognized embedded applets.

BUG=639645, 645517
R=rdevlin.cronin@chromium.org

Committed: https://chromium.googlesource.com/chromium/extensions-by-google/919b672396b9d49a288de74a92bbf3841ad7416c

[Pam (message me for reviews), 2016-09-19 20:28:15]

Description was changed from

==========
Inject automatically on install etc., add mime types, and fix nits.

Remove the installation notes page, and instead inject the content script
automatically when the extension is installed, updated, or re-enabled.
Likewise, disable it (remove the keyboard listener) when the extension is
uninstalled or disabled.

Also fix a punctuation nit, and add PDF and chat mime types to the list of
recognized embedded applets.

BUG=639645, 645517
==========

to

==========
Inject automatically on install etc., add mime types, and fix nits.

Remove the installation notes page, and instead inject the content script
automatically when the extension is installed, updated, or re-enabled.
Likewise, disable it (remove the keyboard listener) when the extension is
uninstalled or disabled.

Also fix a punctuation nit, and add PDF and chat mime types to the list of
recognized embedded applets.

BUG=639645, 645517
==========

[Pam (message me for reviews), 2016-09-19 20:28:15]

pam@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Pam (message me for reviews), 2016-09-19 20:28:59]

Hi Devlin -- since this was your good idea, please take a look.
Ojan -- FYI; comments also welcome, of course.

[Devlin, 2016-09-19 21:22:31]

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
File go-back-with-backspace/background.js (right):

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:13:
chrome.management.onEnabled.addListener(function(info) {
(Just thinking) Hmm... I wonder if we should make a runtime event for this.

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:20: // uninstalled.
This is kind of an abuse of the API.  It means we'll not only need to keep this
port open forever, but also the background page in general - constantly
consuming resources.

In general, chrome makes no effort to clean up content scripts when an extension
has been removed.  I don't think this one should be any different.

If you really want to keep this functionality, the other option would be to do a
ping from the content script when backspace is triggered (which should be
relatively rare).

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:24: var scripts =
chrome.runtime.getManifest().content_scripts[0].js;
given the fact that you're already relying on manifest structure here, I might
just define the file names and save an API call...

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:26: chrome.windows.getAll({populate: true},
function(windows) {
this would be easier with chrome.tabs.query({}, tabs => {
})

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:36: runAt: 'document_start'
Why document start?

[Pam (message me for reviews), 2016-09-20 21:42:59]

Thanks for the review! Changes uploaded.

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
File go-back-with-backspace/background.js (right):

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:13:
chrome.management.onEnabled.addListener(function(info) {
On 2016/09/19 21:22:31, Devlin wrote:
> (Just thinking) Hmm... I wonder if we should make a runtime event for this.

I'd star that bug. Perhaps also onDisabled, since onSuspend doesn't seem to
cover that.

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:20: // uninstalled.
On 2016/09/19 21:22:31, Devlin wrote:
> This is kind of an abuse of the API.  It means we'll not only need to keep
this
> port open forever, but also the background page in general - constantly
> consuming resources.
> 
> In general, chrome makes no effort to clean up content scripts when an
extension
> has been removed.  I don't think this one should be any different.
> 
> If you really want to keep this functionality, the other option would be to do
a
> ping from the content script when backspace is triggered (which should be
> relatively rare).

Chrome can't really clean up content scripts, but judging from the various forum
discussions, some do go to the effort. I'd like to keep the nicer behavior.

I was trying to avoid the extension having to wait for a response or error on
every backspace, and thus being slow. It seems OK, though, so I'll wait and see
if anyone complains.

Note that there is an oddity revealed with the extension system. When the
extension is disconnected (e.g. disabled), runtime.sendMessage throws an error
about the arguments not matching the schema. I spent a day digging into it, then
gave up and just catch it.

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:24: var scripts =
chrome.runtime.getManifest().content_scripts[0].js;
On 2016/09/19 21:22:31, Devlin wrote:
> given the fact that you're already relying on manifest structure here, I might
> just define the file names and save an API call...

True, although the manifest structure is likely to change a lot less often than
the manifest contents. I'm inclined to keep it more robust against the developer
forgetting to update the list. Especially since I'm that potentially forgetful
developer -- but even more if anybody else ever works on the thing.

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:26: chrome.windows.getAll({populate: true},
function(windows) {
On 2016/09/19 21:22:31, Devlin wrote:
> this would be easier with chrome.tabs.query({}, tabs => {
> })

Done, thanks (except that I used function() syntax to be consistent with the
rest of the file).

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:36: runAt: 'document_start'
On 2016/09/19 21:22:31, Devlin wrote:
> Why document start?

A number of users want to be able to hit back-back-back and go back three pages
in quick succession, as they could when it was in the browser. The faster we can
get up and listening to the keyboard, the better.

[Devlin, 2016-09-27 16:08:57]

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
File go-back-with-backspace/background.js (right):

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:13:
chrome.management.onEnabled.addListener(function(info) {
On 2016/09/20 21:42:59, Pam (also PM for reviews) wrote:
> On 2016/09/19 21:22:31, Devlin wrote:
> > (Just thinking) Hmm... I wonder if we should make a runtime event for this.
> 
> I'd star that bug. Perhaps also onDisabled, since onSuspend doesn't seem to
> cover that.

(Orthogonal, but FYI) onDisabled wouldn't make sense, because when the extension
is unloaded, we require that it stop running (synchronously).  Having an
onDisabled event would break that contract.

https://codereview.chromium.org/2351743003/diff/1/go-back-with-backspace/back...
go-back-with-backspace/background.js:20: // uninstalled.
On 2016/09/20 21:42:59, Pam (also PM for reviews) wrote:
> Note that there is an oddity revealed with the extension system. When the
> extension is disconnected (e.g. disabled), runtime.sendMessage throws an error
> about the arguments not matching the schema. I spent a day digging into it,
then
> gave up and just catch it.

Can you file a bug for that?

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/background.js (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/background.js:13:
chrome.management.onEnabled.addListener(function(info) {
actually, the more I think about it, does this work?  If the extension was just
enabled, I don't know that we start the background page fast enough to get the
event.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/background.js:30: for (var tab_index = 0; tab_index <
tabs.length; ++tab_index) {
suggestion:
for (let tab of tabs)
or
tabs.forEach(function(tab) {

});

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/content_script.js (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/content_script.js:60: // extension errors.
please add a crbug.com/nnn here.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/content_script.js:62: chrome.runtime.sendMessage('',
function(response) {
shouldn't need ''

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/manifest.json (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/manifest.json:38: "activeTab",
if you need all_urls, there's not really a reason to also use activeTab.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/manifest.json:41: "tabs",
Do you actually need this?

[Pam (message me for reviews), 2016-09-27 23:37:32]

Thanks -- new version uploaded.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/background.js (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/background.js:13:
chrome.management.onEnabled.addListener(function(info) {
On 2016/09/27 16:08:57, Devlin (catching up) wrote:
> actually, the more I think about it, does this work?  If the extension was
just
> enabled, I don't know that we start the background page fast enough to get the
> event.

It works when I've tested it. Certainly there might be some race condition I
haven't encountered.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/background.js:30: for (var tab_index = 0; tab_index <
tabs.length; ++tab_index) {
On 2016/09/27 16:08:57, Devlin (OOO until Thursday) wrote:
> suggestion:
> for (let tab of tabs)
> or
> tabs.forEach(function(tab) {
> 
> });

Done.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/content_script.js (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/content_script.js:60: // extension errors.
On 2016/09/27 16:08:57, Devlin (OOO until Thursday) wrote:
> please add a crbug.com/nnn here.

I'll take that as confirmation that it's not intended behavior. Done.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/content_script.js:62: chrome.runtime.sendMessage('',
function(response) {
On 2016/09/27 16:08:57, Devlin (OOO until Thursday) wrote:
> shouldn't need ''

That's the message, which is not optional. If it should be, let me know and I'll
file a bug for it.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/manifest.json (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/manifest.json:38: "activeTab",
On 2016/09/27 16:08:57, Devlin (OOO until Thursday) wrote:
> if you need all_urls, there's not really a reason to also use activeTab.

Done.

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/manifest.json:41: "tabs",
On 2016/09/27 16:08:57, Devlin (OOO until Thursday) wrote:
> Do you actually need this?

Apparently not. Thanks.

[Devlin, 2016-09-29 20:55:34]

lgtm

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
File go-back-with-backspace/content_script.js (right):

https://codereview.chromium.org/2351743003/diff/40001/go-back-with-backspace/...
go-back-with-backspace/content_script.js:62: chrome.runtime.sendMessage('',
function(response) {
On 2016/09/27 23:37:32, Pam (also PM for reviews) wrote:
> On 2016/09/27 16:08:57, Devlin (OOO until Thursday) wrote:
> > shouldn't need ''
> 
> That's the message, which is not optional. If it should be, let me know and
I'll
> file a bug for it.

Whoops, you're right.  This is fine.

[Pam (message me for reviews), 2016-10-04 16:19:52]

Description was changed from

==========
Inject automatically on install etc., add mime types, and fix nits.

Remove the installation notes page, and instead inject the content script
automatically when the extension is installed, updated, or re-enabled.
Likewise, disable it (remove the keyboard listener) when the extension is
uninstalled or disabled.

Also fix a punctuation nit, and add PDF and chat mime types to the list of
recognized embedded applets.

BUG=639645, 645517
==========

to

==========
Inject automatically on install etc., add mime types, and fix nits.

Remove the installation notes page, and instead inject the content script
automatically when the extension is installed, updated, or re-enabled.
Likewise, disable it (remove the keyboard listener) when the extension is
uninstalled or disabled.

Also fix a punctuation nit, and add PDF and chat mime types to the list of
recognized embedded applets.

BUG=639645, 645517
R=rdevlin.cronin@chromium.org

Committed:
https://chromium.googlesource.com/chromium/extensions-by-google/919b672396b9d...
==========

[Pam (message me for reviews), 2016-10-04 16:19:53]

Committed patchset #4 (id:60001) manually as
919b672396b9d49a288de74a92bbf3841ad7416c (presubmit successful).