Fix #235 Rewrite NTT as webextension #245

kyoshino · 2017-08-17T03:50:09Z

This PR merges my WebExtension port into this official repository so it can soon be published on AMO. The current version just demonstrates what can be done with the standard WebExtensions API. As explained in #235 (comment), I'll explore how to implement other features using WebExtensions Experiments.

xabolcs · 2017-08-17T07:20:36Z

From #235

I won't implement screenshot tools in favour of Firefox Screenshots.

I would happy to see a link in the toolbar item menu pointing to that new Firefox Screenshots feature (at least for a few releases).
It would be even more cool if Firefox Screenshot will have an API for triggering screenshots or opening its entry point.

kyoshino · 2017-08-17T11:32:19Z

There's the Firefox Screenshots button on the toolbar by default, so I think an extra menu item may not be necessary.

xabolcs · 2017-08-17T12:56:25Z

OK.

kyoshino · 2017-08-17T20:30:56Z

@whimboo Can you or someone get this reviewed and published on AMO?

whimboo

First, this is awesome work @kyoshino! Thanks a lot, really! Only with your help we are able to keep the extension alive. And the basics look pretty good and are working as expected. Even the l18n support and all the locales is fantastic.

Nevertheless I have added a couple of review comments. Please go through them and let me know if you have questions. If we have to solve something you can also find me in the #automation channel on IRC. Thanks!

whimboo · 2017-08-18T07:57:00Z

History.md

@@ -1,3 +1,7 @@
+4.0 / 2017-08-16
+==================
+  * Rewrite as WebExtension (#235)


I think we should add a bit more context here, so it explains that this is necessary for Firefox 57.0 onwards.

I could say:

Rewrite as a WebExtension for Firefox 57+ since the legacy XUL format is no longer supported

We call webextension only for the API, so a wording like "Rewrite extension to use the WebExtension API since the legacy format is no longer supported" makes more sense.

whimboo · 2017-08-18T07:57:56Z

README.md

 # Nightly Tester Tools
-Nightly Tester Tools is an addon for aiding testers of nightly builds of Mozilla apps like Firefox and Thunderbird. Some features are:
+
+Nightly Tester Tools (NTT) is an add-on for aiding testers of Nightly builds of Mozilla apps including Firefox.


What is the status for Thunderbird and SeaMonkey? Would the current code also work in those applications?

Thunderbird doesn't support WebExtensions yet. SeaMonkey does, but I've got an error like this. Given Firefox is our priority, will figure out how to support SeaMonkey later.

There was an error during installation: Add-on {8620c15f-30dc-4dba-a131-7c5d20cf4a29} is not compatible with application version. add-on minVersion: 56.0.

Ok, please file a new issue so that we don't forget about it. Thanks!

Filed #246.

whimboo · 2017-08-18T07:59:09Z

README.md


 * Copy Build ID to Clipboard
 * Copy List of Extensions to Clipboard
+* Insert Build ID into Textbox


Combine it with the other item for Build ID or just move it up.

whimboo · 2017-08-18T07:59:14Z

README.md

 * Copy Build ID to Clipboard
 * Copy List of Extensions to Clipboard
+* Insert Build ID into Textbox
+* Insert List of Extensions into Textbox


whimboo · 2017-08-18T08:01:46Z

README.md

+
+The following features found in the original XUL-based extension are not yet implemented:
+
 * Copy about:support to Pastebin


It is even busted with the old extension for a long time now. I don't think we should mention it here and re-implement. If there is a need for, we can do it once it comes up.

I strongly disagree.

IMHO it should be mentioned. It's our fault that we didn't changed the faulty backend.
Sure, it shouldn't be reimplemented for now. And the #93 should be reevaluated.

Whatever, this part doesn't block a final review. So lets leave it in.

I'll keep it for reference.

whimboo · 2017-08-18T08:30:08Z

extension/options.html

+      table { border-collapse: collapse; table-layout: fixed; }
+      caption { margin-bottom: 8px; }
+      tbody tr:hover { background-color: #EBEBEB; }
+      th, td { border: 1px solid #C1C1C1; padding: 4px 8px; }


Looks fantastic, even there are some quirks left. But those can be fixed later. But have a look. Some might be quick to do. Here what I noticed:

If a variable is already part of the title, we might want to remove it when clicking it again. Right now it can be added multiple times.

Using Ctrl/Cmd+Z to undo changes is not working

s/XUL/Gecko/

Implemented the toggle (insert/remove) behaviour.

Hmm yes, undo doesn't work for programmatically added values. I think it may be tricky to make it happen, because each textbox will require an event listener.

Replaced XUL with Gecko.

Great! So lets leave undo/redo out for now.

whimboo · 2017-08-18T08:34:40Z

extension/shared.js

+      .filter(ext => ext.type === 'extension')
+      .sort((a, b) => a.name > b.name)
+      .map(ext => ext.name + ' ' + ext.version + (ext.enabled ? '' : ' [DISABLED]'))
+      .join('\n');


Please note #232. Maybe add a TODO item here.

TODO added in the function comment. (see the interdiff)

whimboo · 2017-08-18T08:35:24Z

extension/shared.js

+ * @param {String} command - A distinguishable command name.
+ */
+const handle_command = async command => {
+  if (command === 'copy_build_info') {


Lets make it a switch statement.

Actually I like neither a series of if nor case. There's a better approach like this 😺

This is fine as long as the underlying code to execute is not long enough. Lets keep what we have right now.

whimboo · 2017-08-18T08:40:30Z

README.md

-* Title bar customization
+* Screenshots utility (Use Firefox Screenshots)
 * Crash options
 * Extension compatibility fixing


Please add the toolbar menu. Maybe we can reference the bugs which block us in all the above cases?

I think not all of them have a relevant bug, so I'll keep the list. Once we have an issue for each on this repository, it can be referred here.

whimboo · 2017-08-18T08:44:15Z

extension/_locales/cs/messages.json

@@ -0,0 +1,113 @@
+{


Lets not add locales which weren't part of the old extension. Those would need a signoff first. We can perfectly do this as a follow-up issue and PR.

Agree with @whimboo, let @MikkCZ contribute in PR #242.

The cs locale has been added by @MikkCZ in kyoshino/nightlyttr#1 but agree to remove it for now so he can be credited properly via #242.

whimboo · 2017-08-30T10:40:29Z

@kyoshino would you have the time to address the remaining review comments?

kyoshino · 2017-08-30T10:52:46Z

Sorry! Will work on it today.

kyoshino · 2017-09-01T02:03:55Z

Apologies for the delay, but I think all the concerns are now solved. It's ready for 🚢

Here's the interdiff: kyoshino/nightlyttr@a1f4293

If you're okay, add me as a NTT developer on AMO so I myself can submit the new version and update the description as well as the screenshot.

whimboo · 2017-09-05T13:52:38Z

@kyoshino mind checking my latest replies? If all is fine I will do a second pass review.

kyoshino · 2017-09-05T17:25:58Z

Updated History.md and manifest.json as mentioned. Interdiff: kyoshino/nightlyttr@ea6b297

whimboo · 2017-09-12T16:38:15Z

@kyoshino thank you for the update and the bit of delay. Would you mind to change the version to 4.0pre-build1 and then push a beta build to AMO? I would like to have people testing the extension before we are releasing it. If you send me your AMO account name, I can add you for sure.

kyoshino · 2017-09-12T17:04:34Z

Agree to push this to the beta/dev channel first. Changed the version to 4.0pre-build1 and also changed the release date to TBD. My account is kyoshino (kohei.yoshino@gmail.com)

whimboo · 2017-09-12T17:22:42Z

It's done now. So please feel to upload to AMO. Thanks.

kyoshino · 2017-09-12T17:33:24Z

Uploaded! Looks like the version format has been tightened so I used 4.0pre20170912 instead like previous beta versions.

MikkCZ · 2017-09-12T17:47:34Z

extension/manifest.json

+  "homepage_url": "https://wiki.mozilla.org/QA/Automation/Projects/Addons/NightlyTesterTools",
+  "default_locale": "en_US",
+  "permissions": [
+    "<all_urls>",


I wonder why is this permission needed. I do not see any requests here, and the content script has the permission on its own.

Ah, I guess it's no longer required since I've removed the (function to enable/disable) data pasting menus from the toolbar button. Will double check.

It's originally from https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#activeTab_permission

MikkCZ · 2017-09-12T18:10:32Z

extension/manifest.json

+  "default_locale": "en_US",
+  "permissions": [
+    "<all_urls>",
+    "activeTab",


The activeTab permission can be probably avoided too. If it's because of the title manipulation, you can always change it from the content script, which is being loaded everywhere.

MikkCZ · 2017-09-12T18:13:02Z

extension/content.js

+  const is_textbox = 'placeholder' in $active && !$active.readOnly;
+
+  if (message.command === 'insert_to_textbox' && is_textbox) {
+    $active.value = message.value;


This does not work as expected. You should at least append the text, instead of replacing it (or if possible insert where the cursor is). Imagine a situation when you write a very long bug report and adding the information at the end, because you forgot do it first. This replaces everything you have written, and for some reason Ctrl+Z does not work to undo it for me. 👎

Agree, it should just append the string to the textbox.

MikkCZ

Please check the permissions (I know it's for nightly population), they seem to open to me. Inserting into textarea should really insert and not replace. Btw. is there any reason why you cannot insert into regular text input?

kyoshino · 2017-09-12T18:20:52Z

It should work for any regular testboxes, both <textarea> and <input>.

kyoshino · 2017-09-12T20:59:26Z

Fixed the permissions and text inserting function as @MikkCZ suggested. Interdiff: kyoshino/nightlyttr@4e00238

Still, undo (Ctrl-Z) in textareas doesn't work. The Paste event might be the solution, but it's not supported in the content script yet. I also tried document.execCommand('insertText', false, str) with no luck.

whimboo · 2017-09-13T11:37:49Z

I would suggest that we create a new issue for pasting the content. It makes it easier to review without having to go through this whole pile of code changes again. @MikkCZ mind filing an issue which blocks the 4.0 milestone?

Also in preparation of the merge to master I created the legacy branch so we keep the code of the old XUL based extension, and could do minor releases if really needed.

whimboo · 2017-09-13T11:39:13Z

Still, undo (Ctrl-Z) in textareas doesn't work. The Paste event might be the solution, but it's not supported in the content script yet. I also tried document.execCommand('insertText', false, str) with no luck.

@kyoshino please also file a new issue for that. Thanks.

whimboo · 2017-09-13T11:43:30Z

Ok, I'm going to merge this huge patch into master now. Then we can fix the remaining issues, and finally do the 4.0 release. @kyoshino, again thank you a lot for spending your time on this extension so we can keep it alive!

whimboo requested changes Aug 18, 2017

View reviewed changes

kyoshino added a commit to kyoshino/nightlyttr that referenced this pull request Sep 1, 2017

Resolve feedback in mozilla/nightlytt#245

a1f4293

kyoshino mentioned this pull request Sep 5, 2017

Add SeaMonkey support to WebExtension #246

Open

kyoshino added a commit to kyoshino/nightlyttr that referenced this pull request Sep 5, 2017

Address more feedback in mozilla/nightlytt#245

ea6b297

MikkCZ reviewed Sep 12, 2017

View reviewed changes

MikkCZ suggested changes Sep 12, 2017

View reviewed changes

Fix #235 Rewrite NTT as webextension

83e5065

MikkCZ approved these changes Sep 12, 2017

View reviewed changes

whimboo approved these changes Sep 13, 2017

View reviewed changes

whimboo merged commit d220575 into mozilla:master Sep 13, 2017

whimboo mentioned this pull request Sep 13, 2017

Rewrite NTT as webextension #235

Closed

kyoshino deleted the 235-webextension branch September 13, 2017 11:52

This was referenced Sep 13, 2017

Add Czech (cs) locale #242

Merged

Enable undo in textboxes #250

Open


		The following features found in the original XUL-based extension are not yet implemented:

		* Copy about:support to Pastebin

Fix #235 Rewrite NTT as webextension #245

Fix #235 Rewrite NTT as webextension #245

Uh oh!

Conversation

kyoshino commented Aug 17, 2017

Uh oh!

xabolcs commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyoshino commented Aug 17, 2017

Uh oh!

xabolcs commented Aug 17, 2017

Uh oh!

kyoshino commented Aug 17, 2017

Uh oh!

whimboo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyoshino Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyoshino Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whimboo commented Aug 30, 2017

Uh oh!

kyoshino commented Aug 30, 2017

Uh oh!

kyoshino commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

xabolcs commented Aug 17, 2017 •

edited

Loading

kyoshino Sep 1, 2017 •

edited

Loading

kyoshino Sep 5, 2017 •

edited

Loading

kyoshino commented Sep 1, 2017 •

edited

Loading

kyoshino commented Sep 12, 2017 •

edited

Loading

kyoshino Sep 12, 2017 •

edited

Loading

kyoshino commented Sep 12, 2017 •

edited

Loading