Update UI and catch executeScript errors now shown in Canary

Update UI and catch executeScript errors now shown in Canary

Update the settings and popup pages. Disable the whitelist box when it's not relevant (the checkbox is not checked). Catch errors from the initial executeScript calls running on prohibited pages, because they're now displayed on the Extensions page otherwise.

BUG=645639
R=rdevlin.cronin@chromium.org

Committed: https://chromium.googlesource.com/chromium/extensions-by-google/8f934290259bf42872b67536dcf8dfd00078fcbd

[Pam (message me for reviews), 2016-10-07 22:18:47]

Description was changed from

==========
Update UI strings, and catch executeScript errors now shown in Canary

Update strings shown on the various UI pages. Catch errors from the initial
executeScript calls running on prohibited pages, because they're now displayed
on the Extensions page otherwise.

BUG=645639
==========

to

==========
Update UI and catch executeScript errors now shown in Canary

Update strings shown on the various UI pages. Disable the whitelist box when
it's not relevant (the checkbox is not checked). Catch errors from the initial
executeScript calls running on prohibited pages, because they're now displayed
on the Extensions page otherwise.

BUG=645639
==========

[Pam (message me for reviews), 2016-10-07 23:12:43]

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
File go-back-with-backspace/manifest.json (right):

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
go-back-with-backspace/manifest.json:38: "activeTab",
It turns out this is needed after all, to get the URL for the popup.

[Pam (message me for reviews), 2016-10-26 19:10:27]

Description was changed from

==========
Update UI and catch executeScript errors now shown in Canary

Update strings shown on the various UI pages. Disable the whitelist box when
it's not relevant (the checkbox is not checked). Catch errors from the initial
executeScript calls running on prohibited pages, because they're now displayed
on the Extensions page otherwise.

BUG=645639
==========

to

==========
Update UI and catch executeScript errors now shown in Canary

Update the settings and popup pages. Disable the whitelist box when it's not
relevant (the checkbox is not checked). Catch errors from the initial
executeScript calls running on prohibited pages, because they're now displayed
on the Extensions page otherwise.

BUG=645639
==========

[Pam (message me for reviews), 2016-10-26 19:29:51]

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

[Pam (message me for reviews), 2016-10-26 19:30:18]

On 2016/10/07 23:12:43, Pam (message me for reviews) wrote:
>
https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
> File go-back-with-backspace/manifest.json (right):
> 
>
https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
> go-back-with-backspace/manifest.json:38: "activeTab",
> It turns out this is needed after all, to get the URL for the popup.

New UI pieces. PTAL, thanks! (Let me know if you'd like a link to screenshots.)

[Pam (message me for reviews), 2016-10-28 16:18:05]

On 2016/10/26 19:30:18, Pam (message me for reviews) wrote:
> On 2016/10/07 23:12:43, Pam (message me for reviews) wrote:
> >
>
https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
> > File go-back-with-backspace/manifest.json (right):
> > 
> >
>
https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
> > go-back-with-backspace/manifest.json:38: "activeTab",
> > It turns out this is needed after all, to get the URL for the popup.
> 
> New UI pieces. PTAL, thanks! (Let me know if you'd like a link to
screenshots.)

Ping

[Devlin, 2016-10-29 00:30:32]

sorry, still making my way through this one (busy week).  I'll try to get to it
Monday morning.

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
File go-back-with-backspace/manifest.json (right):

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
go-back-with-backspace/manifest.json:38: "activeTab",
On 2016/10/07 23:12:43, Pam (message me for reviews) wrote:
> It turns out this is needed after all, to get the URL for the popup.

Sorry, don't follow - why is this needed?  Which popup?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/_locales/en/messages.json (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/_locales/en/messages.json:27: "message": "Even in an
applet, backspace should go back on these pages:",
This seems contradictory with line 23, to me.

[Pam (message me for reviews), 2016-10-29 05:10:37]

I appreciate your thoroughness, and I know that takes time.

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
File go-back-with-backspace/manifest.json (right):

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
go-back-with-backspace/manifest.json:38: "activeTab",
On 2016/10/29 00:30:31, Devlin wrote:
> On 2016/10/07 23:12:43, Pam (message me for reviews) wrote:
> > It turns out this is needed after all, to get the URL for the popup.
> 
> Sorry, don't follow - why is this needed?  Which popup?

The popup attached to the browser action, pages/popup.*. It needs the URL of the
current tab, for a button that adds/removes that URL to/from the blacklist. Even
with <all_urls>, the URL is blank unless activeTab permission is also present.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/_locales/en/messages.json (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/_locales/en/messages.json:27: "message": "Even in an
applet, backspace should go back on these pages:",
On 2016/10/29 00:30:31, Devlin wrote:
> This seems contradictory with line 23, to me.

Right, it's a checkbox for the general case and a textbox for entering
whitelisted exceptions to that general case. The textbox is below the checkbox
ad slightly in set, and it's disabled when the checkbox is unchecked.

Maybe the whitelist is unnecessary, but there are enough Flash and other applets
doing all kinds of stuff out there, I wanted to cover that case too.

[Devlin, 2016-11-01 15:12:15]

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
File go-back-with-backspace/manifest.json (right):

https://codereview.chromium.org/2400303003/diff/1/go-back-with-backspace/mani...
go-back-with-backspace/manifest.json:38: "activeTab",
On 2016/10/29 05:10:36, Pam (message me for reviews) wrote:
> On 2016/10/29 00:30:31, Devlin wrote:
> > On 2016/10/07 23:12:43, Pam (message me for reviews) wrote:
> > > It turns out this is needed after all, to get the URL for the popup.
> > 
> > Sorry, don't follow - why is this needed?  Which popup?
> 
> The popup attached to the browser action, pages/popup.*. It needs the URL of
the
> current tab, for a button that adds/removes that URL to/from the blacklist.
Even
> with <all_urls>, the URL is blank unless activeTab permission is also present.

That sounds like a bug to me.  If you have all urls, we shouldn't hide the url
(there's no point).  Can you file a new bug?

In the meantime, try using "tabs" here instead of activeTab - that should
definitely work.

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

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/content_script.js:54: if
(!options.whitelist.includes(url) && disabledInApplet(target))
nit: seems like the options.whitelist check should go in disabledInApplet()?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/common.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/common.js:21: if (i18n) {
When is this falsey? (Same for line 14)

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.css (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:15: width: 100%;
chromium-style: alphabetize these

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:17: padding-__MSG_@@bidi_start_edge__:
4px;
Why not use -webkit-padding variants?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:25: border: solid 1px  #ccc;
nit: looks like you have an extra space here?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:31: color: #036;
optional nit: I don't know how much you want to follow chromium style here, but
I think we have a rule that unless it's 3 or 6 repeated letters (e.g. #ccc or
#dddddd), use rgb() syntax.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:36: text-align:
__MSG_@@bidi_end_edge__;
why not 'end'?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:41: margin-bottom: 2px;
Why not margin: 2px 0;?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:59: margin-bottom: 5px;
margin: 5px 0;

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:8:
document.getElementById('whitelist').disabled = !checkbox.checked;
Suggestion: at the top of this file (or in common.js), do
var $ = document.getElementById;

And then everywhere else, just
$('whitelist').disabled...

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:29:
document.getElementById('blacklist').value.split(/\s+/),
Should we validate these are urls?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:48:
document.getElementById('cancel-button').onclick = function() {
$('cancel-button').onclick = window.close;

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:52:
document.getElementById('feedback-link').onclick = function() {
$('feedback-link').onclick = sendFeedback;

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:70: window.addEventListener('load',
init, false);
useCapture defaults to false.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/popup.html (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/popup.html:18: <a href="" id="feedback-link"
class="image-link">
won't href default to ''?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/popup.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/popup.js:107:
document.getElementById('feedback-link').onclick = function() {
Same comments

[Pam (message me for reviews), 2016-11-01 21:29:03]

Changes uploaded; PTAL. I dislike cluttering up reviews with comments that only
say "Done", but everything with no followup comments or questions has been done.

Thanks for the review! I always like learning something in a code review (and
for me and JS/CSS/HTML, there's plenty of opportunity...).

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

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/manifest.json:38: "activeTab",
What's the advantage of "tabs" over "activeTab" here?

It turns out it's only an issue for chrome:// URLs. Filed crbug.com/661311.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/common.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/common.js:21: if (i18n) {
On 2016/11/01 15:12:14, Devlin wrote:
> When is this falsey? (Same for line 14)

Leftover from a past version, when it iterated over all nodes. Removed.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.css (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:17: padding-__MSG_@@bidi_start_edge__:
4px;
On 2016/11/01 15:12:15, Devlin wrote:
> Why not use -webkit-padding variants?

Because I didn't know about them. I'm only pretending to be a front-end
engineer. Thanks for the tip; I've now read the CSS style guide as well.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:36: text-align:
__MSG_@@bidi_end_edge__;
On 2016/11/01 15:12:15, Devlin wrote:
> why not 'end'?

Because w3schools (the first hit for [css text-align]) doesn't know about it. :P

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:29:
document.getElementById('blacklist').value.split(/\s+/),
On 2016/11/01 15:12:15, Devlin wrote:
> Should we validate these are urls?

Is there a handy, robust way to do that?

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:52:
document.getElementById('feedback-link').onclick = function() {
On 2016/11/01 15:12:15, Devlin wrote:
> $('feedback-link').onclick = sendFeedback;

This one doesn't work, because sendFeedback takes an optional argument. If it's
not in an anonymous function, it's getting passed the click event. I could check
the arg type, but it seems cleaner to me to keep the call inside an anonymous
function.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/popup.html (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/popup.html:18: <a href="" id="feedback-link"
class="image-link">
On 2016/11/01 15:12:15, Devlin wrote:
> won't href default to ''?

It's not needed for the image links, and I've taken it out for those. For the
file-url-message above and the feedback link in options.html, with no href they
don't get the correct text styling and hand cursor for a link.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/popup.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/popup.js:107:
document.getElementById('feedback-link').onclick = function() {
On 2016/11/01 15:12:15, Devlin wrote:
> Same comments

Done, except that these two need to stay inside anonymous functions.
sendFeedback because it needs an argument, and openOptionsPage because otherwise
it gets the event passed as the first arg, which confuses the extension
parameter parser.

[Devlin, 2016-11-05 02:18:21]

lgtm % nits

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.css (right):

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:9: direction: __MSG_@@bidi_dir__;
This should be handled by the user's browser preference, I'd think - is it not?

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:41: $('feedback-link').onclick =
function() {
why not just = sendFeedback?

[Pam (message me for reviews), 2016-11-05 04:12:56]

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.css (right):

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
go-back-with-backspace/pages/options.css:9: direction: __MSG_@@bidi_dir__;
On 2016/11/05 02:18:21, Devlin (slow) wrote:
> This should be handled by the user's browser preference, I'd think - is it
not?

I'll check.

Meanwhile, I started out by following the example in
https://developer.chrome.com/extensions/i18n
That should be updated to use the better practices you suggested:
-webkit-padding-{start|end}, text-align: end, no body direction if it's not
needed. Filed as 662632.

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:41: $('feedback-link').onclick =
function() {
On 2016/11/05 02:18:21, Devlin (slow) wrote:
> why not just = sendFeedback?

sendFeedback takes an optional argument: if it's passed a URL, it includes it in
the feedback message. onclick handlers get passed the event, which confuses
sendFeedback.

That's what I was trying to describe when I said that I could check the argument
type within sendFeedback, to look for a string and ignore events. But this
seemed simpler.

[Devlin, 2016-11-05 04:56:09]

I somehow totally missed responding to your last questions, sorry!

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:29:
document.getElementById('blacklist').value.split(/\s+/),
On 2016/11/01 21:29:03, Pam (message me for reviews) wrote:
> On 2016/11/01 15:12:15, Devlin wrote:
> > Should we validate these are urls?
> 
> Is there a handy, robust way to do that?

One way would be to try to construct an URL object and catch errors, e.g.

try {
  var url = new URL(str);
} catch (e) {
  handleInvalidUrl(str);
}

It's a little inefficient, but if it's only on user action, that'd probably be
fine.

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/popup.html (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/popup.html:18: <a href="" id="feedback-link"
class="image-link">
On 2016/11/01 21:29:03, Pam (message me for reviews) wrote:
> On 2016/11/01 15:12:15, Devlin wrote:
> > won't href default to ''?
> 
> It's not needed for the image links, and I've taken it out for those. For the
> file-url-message above and the feedback link in options.html, with no href
they
> don't get the correct text styling and hand cursor for a link.

You can do these via style - e.g.
a {
  cursor: pointer;
  text-decoration: underline;
}

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/80001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:41: $('feedback-link').onclick =
function() {
On 2016/11/05 04:12:56, Pam (message me for reviews) wrote:
> On 2016/11/05 02:18:21, Devlin (slow) wrote:
> > why not just = sendFeedback?
> 
> sendFeedback takes an optional argument: if it's passed a URL, it includes it
in
> the feedback message. onclick handlers get passed the event, which confuses
> sendFeedback.
> 
> That's what I was trying to describe when I said that I could check the
argument
> type within sendFeedback, to look for a string and ignore events. But this
> seemed simpler.

Whoops, missed your reply, sorry for the repeat!

[Pam (message me for reviews), 2016-11-16 19:23:24]

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
File go-back-with-backspace/pages/options.js (right):

https://codereview.chromium.org/2400303003/diff/60001/go-back-with-backspace/...
go-back-with-backspace/pages/options.js:29:
document.getElementById('blacklist').value.split(/\s+/),
On 2016/11/05 04:56:09, Devlin wrote:
> On 2016/11/01 21:29:03, Pam (message me for reviews) wrote:
> > On 2016/11/01 15:12:15, Devlin wrote:
> > > Should we validate these are urls?
> > 
> > Is there a handy, robust way to do that?
> 
> One way would be to try to construct an URL object and catch errors, e.g.
> 
> try {
>   var url = new URL(str);
> } catch (e) {
>   handleInvalidUrl(str);
> }
> 
> It's a little inefficient, but if it's only on user action, that'd probably be
> fine.

Thinking about it, what I'd like to do is check the field as the user enters
items, and show non-URLs in another color. But that's a bigger change, so I'll
land and publish this first and follow up.

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

Description was changed from

==========
Update UI and catch executeScript errors now shown in Canary

Update the settings and popup pages. Disable the whitelist box when it's not
relevant (the checkbox is not checked). Catch errors from the initial
executeScript calls running on prohibited pages, because they're now displayed
on the Extensions page otherwise.

BUG=645639
==========

to

==========
Update UI and catch executeScript errors now shown in Canary

Update the settings and popup pages. Disable the whitelist box when it's not
relevant (the checkbox is not checked). Catch errors from the initial
executeScript calls running on prohibited pages, because they're now displayed
on the Extensions page otherwise.

BUG=645639
R=rdevlin.cronin@chromium.org

Committed:
https://chromium.googlesource.com/chromium/extensions-by-google/8f934290259bf...
==========

[Pam (message me for reviews), 2016-11-16 19:49:54]

Committed patchset #6 (id:100001) manually as
8f934290259bf42872b67536dcf8dfd00078fcbd (presubmit successful).