Closed Bug 1461970 Opened 6 years ago Closed 6 years ago

Remove gcli UI

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement)

Product:
DevTools Graveyard
Component:
Graphic Commandline and Toolbar
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 2 obsolete files)

Bug 1461970 - Drop gcli tests involving helpers.openToolbar.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Removing DeveloperToolbar, its menu, its pref and everything using gDevToolsBrowser.getDeveloperToolbar.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Remove tests using helpers.openToolbar.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Drop all tests using helpers.addTabWithToolbar.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Remove gcli tests using helpers.runTestModule (i.e all of them) and cleanup helpers.js from DeveloperToolbar usages.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Remove html and css files related to gcli.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Remove gcli references from browser/.
59 bytes, text/x-review-board-request
jryans
: review+
Details
Bug 1461970 - Remove gcli references from startup folder.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
In the process of removing gcli, we can start by removing only its UI.

We could remove only:
  devtools/client/commandline.js (tests and html+css of gcli)
  devtools/client/shared/developer-toolbar.js (all but CommandsUtils)
  leftovers of gcli in browser/ (mostly css)
  all tests in devtools/ trying to display gcli
  gcli webdeveloper menu and shortcuts

While we keep:
  devtools/server/actors/gcli.js (its actor)
  devtools/shared/gcli/ (all gcli framework, which allows to keep gcli commands to work)
  devtools/client/shared/developer-toolbar.js (only for CommandsUtils, required for commands)

We probably can iterate on that and strip some more stuff within devtools/shared/gcli/, like unused commands.
These two patches are the main removal:
  Removing DeveloperToolbar, its menu, its pref and everything using gDevToolsBrowser.getDeveloperToolbar.
  Remove html and css files related to gcli.

Then as the UI no longer exists, I removed all the tests involving gcli UI:
  Drop gcli tests involving helper.openToolbar.
  Remove gcli tests using helpers.runTestModule (i.e all of them) and cleanup helpers.js from DeveloperToolbar usages.
  Remove tests using helpers.openToolbar.
  Drop all tests using helpers.addTabWithToolbar.
Here, I may drop some coverage on features still depending on gcli codebase, but I'm not sure it is really important as we want to refactor them to a gcli-less implementation and would need to write brand new test.

I add to do such thing, to keep exposing gcli test helper that is used from other devtools folders:
  Readd a browser.ini file to still reference gcli helper tests. r=?
I can also try to move this helper somewhere else if this hack is too bad, otherwise, this should be removed in bug 1447494.

Finally, remove other external references
  Remove gcli menu.
  Remove gcli references from browser/.
  Remove gcli references from startup folder.
Assignee: nobody → poirot.alex
Comment on attachment 8976138 [details]
Bug 1461970 - Drop gcli tests involving helpers.openToolbar.

https://reviewboard.mozilla.org/r/244330/#review250348

::: commit-message-cf3ee:1
(Diff revision 2)
> +Bug 1461970 - Drop gcli tests involving helper.openToolbar. r=jryans

Nit: helper -> helpers
Attachment #8976138 - Flags: review?(jryans) → review+
Comment on attachment 8976141 [details]
Bug 1461970 - Remove tests using helpers.openToolbar.

https://reviewboard.mozilla.org/r/244336/#review250366
Attachment #8976141 - Flags: review?(jryans) → review+
Comment on attachment 8976142 [details]
Bug 1461970 - Drop all tests using helpers.addTabWithToolbar.

https://reviewboard.mozilla.org/r/244338/#review250370
Attachment #8976142 - Flags: review?(jryans) → review+
Comment on attachment 8976147 [details]
Bug 1461970 - Readd a browser.ini file to still reference gcli helper tests.

https://reviewboard.mozilla.org/r/244348/#review250352

If you want to land this set without squashing, I think you should revert the deletion of this test manifest and then edit it as needed (instead of deleting and readding).

::: devtools/client/commandline/test/browser.ini:9
(Diff revision 3)
> +support-files =
> +  head.js
> +  helpers.js
> +  mockCommands.js
> +
> +[browser.js]

Why do you need an empty, skipped test file?  To share support files with other directories?

::: devtools/client/commandline/test/browser.ini:10
(Diff revision 3)
> +  head.js
> +  helpers.js
> +  mockCommands.js
> +
> +[browser.js]
> +skip-if= true

Nit: We seem to use " = " as the style for these files
Attachment #8976147 - Flags: review?(jryans) → review+
Not sure if I missed it, but these patches don't seem to touch the GCLI localization file. Is that expected? I only see removal from shared files.
Comment on attachment 8976140 [details]
Bug 1461970 - Remove gcli menu.

https://reviewboard.mozilla.org/r/244334/#review250372

Since it uses `getDeveloperToolbar`, I would suggest squashing with the previous commit.
Attachment #8976140 - Flags: review?(jryans) → review+
Comment on attachment 8976143 [details]
Bug 1461970 - Remove gcli tests using helpers.runTestModule (i.e all of them) and cleanup helpers.js from DeveloperToolbar usages.

https://reviewboard.mozilla.org/r/244340/#review250374

::: devtools/client/commandline/test/helpers.js:42
(Diff revision 3)
> -
> -    return automator;
> -  };
>  
>  /**
>   * Warning: For use with Firefox Mochitests only.

Should we remove all such Firefox helpers at this time?
Attachment #8976143 - Flags: review?(jryans) → review+
(In reply to Francesco Lodolo [:flod] from comment #35)
> Not sure if I missed it, but these patches don't seem to touch the GCLI
> localization file. Is that expected? I only see removal from shared files.

Seems like the right time to remove the string as well, what do you think :ochameau?
Flags: needinfo?(poirot.alex)
Comment on attachment 8976144 [details]
Bug 1461970 - Remove html and css files related to gcli.

https://reviewboard.mozilla.org/r/244342/#review250376
Attachment #8976144 - Flags: review?(jryans) → review+
Comment on attachment 8976145 [details]
Bug 1461970 - Remove gcli references from browser/.

https://reviewboard.mozilla.org/r/244344/#review250378
Attachment #8976145 - Flags: review?(jryans) → review+
Comment on attachment 8976146 [details]
Bug 1461970 - Remove gcli references from startup folder.

https://reviewboard.mozilla.org/r/244346/#review250380
Attachment #8976146 - Flags: review?(jryans) → review+
Comment on attachment 8976139 [details]
Bug 1461970 - Removing DeveloperToolbar, its menu, its pref and everything using gDevToolsBrowser.getDeveloperToolbar.

https://reviewboard.mozilla.org/r/244332/#review250384

Great, thanks for working this cleanup!

::: devtools/client/shared/developer-toolbar.js
(Diff revision 3)
>  const promise = require("promise");
> -const Services = require("Services");
> -const { TargetFactory } = require("devtools/client/framework/target");
> -const Telemetry = require("devtools/client/shared/telemetry");
> -const {LocalizationHelper} = require("devtools/shared/l10n");
> -const L10N = new LocalizationHelper("devtools/client/locales/toolbox.properties");

Maybe this commit should also remove at least GCLI UI strings?  (Perhaps we also want to remove command strings as well...?  Not sure.)
Attachment #8976139 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #34)
> Comment on attachment 8976147 [details]
> Bug 1461970 - Readd a browser.ini file to still reference gcli helper tests.
> 
> https://reviewboard.mozilla.org/r/244348/#review250352
> 
> If you want to land this set without squashing, I think you should revert
> the deletion of this test manifest and then edit it as needed (instead of
> deleting and readding).
> 
> ::: devtools/client/commandline/test/browser.ini:9
> (Diff revision 3)
> > +support-files =
> > +  head.js
> > +  helpers.js
> > +  mockCommands.js
> > +
> > +[browser.js]
> 
> Why do you need an empty, skipped test file?  To share support files with
> other directories?

That's because `mach` yells when there is a browser.ini without any test file being registered...

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #37)
> ::: devtools/client/commandline/test/helpers.js:42
> (Diff revision 3)
> > -
> > -    return automator;
> > -  };
> >  
> >  /**
> >   * Warning: For use with Firefox Mochitests only.
> 
> Should we remove all such Firefox helpers at this time?

I'm not sure I get what you meant here?
These helpers here are still used outside of gcli:
  https://searchfox.org/mozilla-central/search?q=helpers.(openTab%7CaddTabWithToolbar%7Cpromisify)&case=false&regexp=true&path=devtools
  This is bug 1447494.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42)
> Comment on attachment 8976139 [details]
> Bug 1461970 - Removing DeveloperToolbar, its menu, its pref and everything
> using gDevToolsBrowser.getDeveloperToolbar.
> 
> https://reviewboard.mozilla.org/r/244332/#review250384
> 
> Great, thanks for working this cleanup!
> 
> ::: devtools/client/shared/developer-toolbar.js
> (Diff revision 3)
> >  const promise = require("promise");
> > -const Services = require("Services");
> > -const { TargetFactory } = require("devtools/client/framework/target");
> > -const Telemetry = require("devtools/client/shared/telemetry");
> > -const {LocalizationHelper} = require("devtools/shared/l10n");
> > -const L10N = new LocalizationHelper("devtools/client/locales/toolbox.properties");
> 
> Maybe this commit should also remove at least GCLI UI strings?  (Perhaps we
> also want to remove command strings as well...?  Not sure.)

Thanks for the reminder about l10n. I removed the few strings used by the code that I removed.
I'll let the commands strings in shared until we remove them (I will followup right after to remove all the ones that aren't used by the toolbox).
Flags: needinfo?(poirot.alex)
Attachment #8976140 - Attachment is obsolete: true
Attachment #8976147 - Attachment is obsolete: true
Blocks: 1462398
Blocks: 1462399
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c33590719e4e
Drop gcli tests involving helpers.openToolbar. r=jryans
https://hg.mozilla.org/integration/autoland/rev/9e89ec8f95a9
Removing DeveloperToolbar, its menu, its pref and everything using gDevToolsBrowser.getDeveloperToolbar. r=jryans
https://hg.mozilla.org/integration/autoland/rev/2f774e9a7e5d
Remove tests using helpers.openToolbar. r=jryans
https://hg.mozilla.org/integration/autoland/rev/4379cbe42ce3
Drop all tests using helpers.addTabWithToolbar. r=jryans
https://hg.mozilla.org/integration/autoland/rev/6a62f53c8098
Remove gcli tests using helpers.runTestModule (i.e all of them) and cleanup helpers.js from DeveloperToolbar usages. r=jryans
https://hg.mozilla.org/integration/autoland/rev/fe9e01a0dce7
Remove html and css files related to gcli. r=jryans
https://hg.mozilla.org/integration/autoland/rev/19e29df404fc
Remove gcli references from browser/. r=jryans
https://hg.mozilla.org/integration/autoland/rev/93405049f3b5
Remove gcli references from startup folder. r=jryans
Product: Firefox → DevTools
Added a comment to the release notes: https://developer.mozilla.org/en-US/Firefox/Releases/62

and also added the notice of the removal to: https://developer.mozilla.org/en-US/docs/Tools/GCLI

as, at least for now, it is possible that someone is using an older version of Firefox.
Flags: needinfo?(poirot.alex)
Thank Irene, it looks good to me.

I'm wondering if we should also drop a word about CmdOrCtrl+Alt+R from the browser console to restart and :screenshot (but only in 63)?

I imagine Sole should have a look at this.
Flags: needinfo?(poirot.alex) → needinfo?(spenades)
(In reply to Alexandre Poirot [:ochameau] from comment #62)
> Thank Irene, it looks good to me.
> 
> I'm wondering if we should also drop a word about CmdOrCtrl+Alt+R from the
> browser console to restart and :screenshot (but only in 63)?
> 
> I imagine Sole should have a look at this.

If something is being added in 63, then I can make a GitHub issue for myself to document this in the next sprint.
(In reply to Irene Smith from comment #63)
> (In reply to Alexandre Poirot [:ochameau] from comment #62)
> > Thank Irene, it looks good to me.
> > 
> > I'm wondering if we should also drop a word about CmdOrCtrl+Alt+R from the
> > browser console to restart and :screenshot (but only in 63)?
> > 
> > I imagine Sole should have a look at this.
> 
> If something is being added in 63, then I can make a GitHub issue for myself
> to document this in the next sprint.

Yes, I think it would be worth documenting this item for 63 changelog:
  bug 1464461, implements ":screenshot" command in the Web console. This commands allows to take screenshots of web pages and accept most arguments GCLI's equivalent command used to support.

The following one appeared in 61:
  bug 1458840, allows to restart firefox while restoring your previous tabs by opening the browser console and doing Command or Control (command on MacOSX, control otherwise) + Alt + R key shortcut.

But both of these items may be worth refering about as they are mitigation related to GCLI removal, and so while announcing it has been removal, it may be great to say we introduced these two things that used to be available in GCLI.
Irene, I have been documenting these changes in a 'readable' format here: https://docs.google.com/document/d/e/2PACX-1vSh02ilZT6BwMnYJ2XC_DrrN7GoIWktNAFlYLvGlTMyM7KFelrB6DBWBQEsgHIIiOCH561TWcTBevBk/pub
Flags: needinfo?(spenades) → needinfo?(irenesmith13)
(In reply to Soledad Penades [:sole] [:spenades] from comment #65)
> Irene, I have been documenting these changes in a 'readable' format here:
> https://docs.google.com/document/d/e/2PACX-
> 1vSh02ilZT6BwMnYJ2XC_DrrN7GoIWktNAFlYLvGlTMyM7KFelrB6DBWBQEsgHIIiOCH561TWcTBe
> vBk/pub

Thank you for sharing this. It will be a useful way to track the changes.
Keywords: dev-doc-needed → dev-doc-complete
1. https://developer.mozilla.org/en-US/docs/Tools/GCLI -- Added this statement to Developer Toolbar page: "The functions provided by the Developer Toolbar are available in the Firefox Developer Tools Console Tab."

Added information to the Firefox 63 Release notes about the :screenshot command.

2. Added the screenshot command to: https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Helpers as follows:

:screenshot <filename.png>
    Create a screenshot of the current page with the supplied filename. If you don't supply a filename, the image file will be named: Screen Shot yyy-mm-dd at hh.mm.ss.png

3. Added the following information under the "Controlling the browser" section of the Browser Console page:

Restart the browser with the command Ctrl + Alt + R (Windows, Linux) or Cmd + Alt + R (Mac) This command restarts the browser with the same tabs open as before the restart.
Flags: needinfo?(irenesmith13) → needinfo?(poirot.alex)
Thanks Irene!

(In reply to Irene Smith (ismith) from comment #68)
> 1. https://developer.mozilla.org/en-US/docs/Tools/GCLI -- Added this
> statement to Developer Toolbar page: 
> "The functions provided by the Developer Toolbar are available in the Firefox Developer Tools Console Tab."

I'm wondering if that statement is misleading?
We only migrated screenshot (in the console via :screenshot) and browser reload (in the browser console only, via the key shortcut). With such sentence, you may expect to see any gcli command to work in the console.

> Added information to the Firefox 63 Release notes about the :screenshot
> command.

Note that this feature has been uplifted to Firefox 62. (bug 1464461)
Flags: needinfo?(poirot.alex)
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: