about:debugging should be available to all applications that ship the Devtools client (and not just Firefox)

RESOLVED FIXED in Firefox 46

Status

()

Firefox
Developer Tools: about:debugging
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

unspecified
Firefox 46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [devtools-ux])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Like say SeaMonkey....
(Assignee)

Comment 1

3 years ago
Created attachment 8700334 [details] [diff] [review]
Patch v1.0 Move the about page to /docshell and wrap with an #ifdef MOZ_DEVTOOLS

Try server job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=549ce7592e25
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Attachment #8700334 - Attachment description: Patch v1.0 Move the about page to /docshell and wrap with an #ifdef MOZ_DEVTOOL → Patch v1.0 Move the about page to /docshell and wrap with an #ifdef MOZ_DEVTOOLS
(Assignee)

Updated

3 years ago
Attachment #8700334 - Flags: review?(jryans)
Comment on attachment 8700334 [details] [diff] [review]
Patch v1.0 Move the about page to /docshell and wrap with an #ifdef MOZ_DEVTOOLS

Review of attachment 8700334 [details] [diff] [review]:
-----------------------------------------------------------------

The concept seems fine, just want to see the define be less confusing.

Also, I believe a review from a build peer is needed, perhaps :glandium since he reviewed other recent DevTools build things.

::: docshell/base/moz.build
@@ +80,5 @@
>  
>  if CONFIG['MOZ_TOOLKIT_SEARCH']:
>      DEFINES['MOZ_TOOLKIT_SEARCH'] = True
> +
> +if CONFIG['MOZ_DEVTOOLS'] == 'all':

It seems simpler to follow if the DEFINE is set to the same value as the CONFIG, instead of becoming a boolean.

::: docshell/base/nsAboutRedirector.cpp
@@ +51,5 @@
>    {
>      "credits", "https://www.mozilla.org/credits/",
>      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT
>    },
> +#ifdef MOZ_DEVTOOLS

We should test that this set to the value "all", not just defined in general, once the moz.build is changed.  That is the logic used to include the client files:

https://dxr.mozilla.org/mozilla-central/source/devtools/moz.build#10
Attachment #8700334 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 3

2 years ago
Created attachment 8701783 [details] [diff] [review]
Patch v2 Use

Changes since the last patch:
1. Set DEFINE to the same value as CONFIG
> +if CONFIG['MOZ_DEVTOOLS'] == 'all':
> +    DEFINES['MOZ_DEVTOOLS'] = CONFIG['MOZ_DEVTOOLS']
2. Test that MOZ_DEVTOOLS is set to the value "all"

Also, Needs review by a build peer for the build-config changes.
Attachment #8700334 - Attachment is obsolete: true
Attachment #8701783 - Flags: review?(mh+mozilla)
Attachment #8701783 - Flags: review?(jryans)
Comment on attachment 8701783 [details] [diff] [review]
Patch v2 Use

Review of attachment 8701783 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/moz.build
@@ +80,5 @@
>  
>  if CONFIG['MOZ_TOOLKIT_SEARCH']:
>      DEFINES['MOZ_TOOLKIT_SEARCH'] = True
> +
> +if CONFIG['MOZ_DEVTOOLS'] == 'all':

It would make more sense to me to unconditionally copy the config value to the define, but let's see what :glandium thinks.

::: docshell/build/moz.build
@@ +29,5 @@
>  
>  if CONFIG['GNU_CXX']:
>      CXXFLAGS += ['-Wshadow']
> +
> +if CONFIG['MOZ_DEVTOOLS'] == 'all':

Same note as other moz.build.
Attachment #8701783 - Flags: review?(jryans) → review+
Comment on attachment 8701783 [details] [diff] [review]
Patch v2 Use

Review of attachment 8701783 [details] [diff] [review]:
-----------------------------------------------------------------

Why hardcode this in C++, when you could register the about:debugging url with a simple nsIAboutModule component. For example this is what I have for about:tabs in the tabstats addon:

function AboutTabs() {}

AboutTabs.prototype = {
  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutModule]),
  classDescription: 'about:tabs',
  classID: Components.ID('{21dfe065-bb20-4f02-a969-e4bf3cf73e90}'),
  contractID: '@mozilla.org/network/protocol/about;1?what=tabs',

  newChannel: function(uri)
  {
    var channel = Services.io.newChannel('resource://tabstats/abouttabs.html', null, null);
    var securityManager = Cc['@mozilla.org/scriptsecuritymanager;1'].getService(Ci.nsIScriptSecurityManager);
    var principal = securityManager.getSystemPrincipal(uri);
    channel.originalURI = uri;
    channel.owner = principal;
    return channel;
  },

  getURIFlags: function(uri)
  {
    return Ci.nsIAboutModule.ALLOW_SCRIPT;
  }
};

const AboutTabsFactory = XPCOMUtils.generateNSGetFactory([AboutTabs])(AboutTabs.prototype.classID);

function startup(aData, aReason) {
  Cm.registerFactory(AboutTabs.prototype.classID,
                     AboutTabs.prototype.classDescription,
                     AboutTabs.prototype.contractID,
                     AboutTabsFactory);
  /* (...) */
}
Attachment #8701783 - Flags: review?(mh+mozilla)
(Assignee)

Comment 7

2 years ago
Created attachment 8702015 [details]
Alternative standalone nsAbout component.

(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #6)

> Why hardcode this in C++, when you could register the about:debugging url
> with a simple nsIAboutModule component. For example this is what I have for
> about:tabs in the tabstats addon:

1. I have a version that does that too (see attachment). However this method requires applications to explicitly buy in to this by including the component in the relevant package-manifest.in
2. We are not an addon; we are part of Devtools.
3. nsAboutRedirector.cpp already exists for this very purpose.
4. The nsIAboutModule boilerplate is unnecessary bloat. The C++ patch is smaller and more compact.
Flags: needinfo?(mh+mozilla)
Attachment #8702015 - Flags: feedback?(mh+mozilla)
Comment on attachment 8702015 [details]
Alternative standalone nsAbout component.

>browser/installer/package-manifest.in:
>
>#if MOZ_DEVTOOLS == all
>@RESPATH@/components/nsAboutDebug.js
>@RESPATH@/components/aboutDebug.manifest 
>#endif
>
>--------------------------------------------------
>moz.build:
>
>if CONFIG['MOZ_DEVTOOLS'] == 'all':
>    EXTRA_COMPONENTS += [
>        nsAboutDebug.js,
>        aboutDebug.manifest,
>    ]
>
>--------------------------------------------------
>aboutDebug.manifest:
>
>component {5797adeb-1e00-4187-9c81-ed716aec2313} nsAboutDebug.js
>contract @mozilla.org/network/protocol/about;1?what=debugging {5797adeb-1e00-4187-9c81-ed716aec2313}

You can, in fact, abuse the system: you can actually register a component in a chrome directory. So you can, in fact put the following in a jar.mn in devtools:

devtools.jar:
% component {5797adeb-1e00-4187-9c81-ed716aec2313} devtools/nsAboutDebug.js
% contract @mozilla.org/network/protocol/about;1?what=debugging {5797adeb-1e00-4187-9c81-ed716aec2313}
  nsAboutDebug.js


And that will work (except if you package as jar instead of omni or flat)

That said, I do agree that the boilerplate for nsIAboutModule is rather big, and it seems to me we could have something like the 'resource' registration in chrome manifests, but dedicated to about:, but that would be a followup.
Flags: needinfo?(mh+mozilla)
Attachment #8702015 - Flags: feedback?(mh+mozilla)
(Assignee)

Comment 9

2 years ago
Created attachment 8702568 [details] [diff] [review]
Patch v3 Minimalist patch usind existing nsAboutRedirector.cpp and MOZ_DEVTOOLS_ALL

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7eafd52d6e5&group_state=expanded

> <RattyAway>	So something like:
> 	if CONFIG['MOZ_DEVTOOLS'] == 'all':
> 	DEFINES['FOOBAR'] = True
> <glandium>	s/FOOBAR/MOZ_DEVTOOLS_ALL/
Attachment #8701783 - Attachment is obsolete: true
Attachment #8702015 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8702568 - Flags: review?(jryans)
Comment on attachment 8702568 [details] [diff] [review]
Patch v3 Minimalist patch usind existing nsAboutRedirector.cpp and MOZ_DEVTOOLS_ALL

Review of attachment 8702568 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming you discussed this with :glandium already, then it works for me.
Attachment #8702568 - Flags: review?(jryans) → review+

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b969a8b88621
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 13

2 years ago
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Why are you going through tons of fixed bugs and adding unnecessary information? What system are you going through?
You need to log in before you can comment on or make changes to this bug.