*** Original post on bio 2002 at 2013-06-13 09:26:00 UTC *** This command would open about:memory, about:crashes, about:config and about:support (are there others?) in tabs.

*** Original post on bio 2002 at 2013-06-13 10:06:58 UTC *** (In reply to comment #0) > (are there others?) about: about:license about:buildconfig

*** Original post on bio 2002 at 2013-06-13 10:13:01 UTC *** about:mozilla, about:credits, about:addons, about:blank Do we care what they are or do we just transform /about <foo> into "about:<foo>" and open it? :) I'll stop listing them and just point to http://en.wikipedia.org/wiki/About:#Mozilla-specific_about:_URIs

*** Original post on bio 2002 as attmnt 2482 at 2013-06-13 18:34:00 UTC *** What this patch does: - Adds command to display about:<page> in a new tab - Ensures about: links in about: pages open in this tab or a new tab (middle click), and other links are opened externally. - Provides a small demonstration of how the patch for bug 953867 (bio 426) works. What it doesn't: - Check that the page exists before trying to load it - Anything else not mentioned above.

Comment on attachment 8354249 [details] [diff] [review] Add /about command *** Original change on bio 2002 attmnt 2482 at 2013-06-13 20:19:30 UTC *** >Add command to display about:<page> in a new tab Make this "Bug 955438 (bio 2002) - Add an /about command to open the about:* pages, r=clokep." for easy check-in! >+ <field name="aboutPage"> >+ null >+ </field> Can this be replaced by doing aboutBrowser.src or something? Not a big deal, just curious. I wonder if we should strip out the about: here in case someone uses the command wrong (as /about about:config). Plus you wouldn't need to do it on the click handler then. >+ this.aboutBrowser.loadURI("about:" + aPage); >+ this.aboutPage = aPage; >+ this.tab.setAttribute("label", "about:" + aPage); >diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm >+ let helpmsg = >+ Services.strings.createBundle("chrome://instantbird/locale/core.properties") >+ .GetStringFromName("aboutCommand.help"); Can we make this once for the file? >+ Services.cmd.registerCommand({ >+ name: "about", >+ get helpString() helpmsg, >+ usageContext: Ci.imICommand.CMD_CONTEXT_ALL, >+ priority: Ci.imICommand.CMD_PRIORITY_HIGH, PRIORITY_DEFAULT, I think. >+ run: function(aMsg, aConv) { >+ let win = Services.wm.getMostRecentWindow("Messenger:convs"); Does this work OK if you have multiple windows?

*** Original post on bio 2002 at 2013-06-13 21:17:50 UTC *** (In reply to comment #4) > >diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm > >+ let helpmsg = > >+ Services.strings.createBundle("chrome://instantbird/locale/core.properties") > >+ .GetStringFromName("aboutCommand.help"); > Can we make this once for the file? Not sure of what you mean here, but we shouldn't fetch this string at startup. The reason why it's "get helpString()" and not just "helpString:" is so that we can fetch the string lazily. Putting the string in an intermediate variable that has no obvious purpose defeats this. > >+ run: function(aMsg, aConv) { > >+ let win = Services.wm.getMostRecentWindow("Messenger:convs"); > Does this work OK if you have multiple windows? Probably not.

*** Original post on bio 2002 at 2013-06-13 22:01:53 UTC *** (In reply to comment #5) > (In reply to comment #4) > > > >diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm > > >+ let helpmsg = > > >+ Services.strings.createBundle("chrome://instantbird/locale/core.properties") > > >+ .GetStringFromName("aboutCommand.help"); > > Can we make this once for the file? > > Not sure of what you mean here, but we shouldn't fetch this string at startup. > The reason why it's "get helpString()" and not just "helpString:" is so that we > can fetch the string lazily. Putting the string in an intermediate variable > that has no obvious purpose defeats this. This was a "Code Duplication!" warning, http://lxr.instantbird.org/instantbird/source/instantbird/modules/ibCore.jsm#228

*** Original post on bio 2002 at 2013-06-17 00:42:17 UTC *** >> >+ run: function(aMsg, aConv) { >> >+ let win = Services.wm.getMostRecentWindow("Messenger:convs"); >> Does this work OK if you have multiple windows? > Probably not. Works for me. > I wonder if we should strip out the about: here in case someone uses the > command wrong (as /about about:config). Plus you wouldn't need to do it on the > click handler then. I did it this way on purpose so that non-about: pages can't be loaded. I can change this behavior if it isn't desired, though.

*** Original post on bio 2002 at 2013-06-17 10:36:48 UTC *** Comment on attachment 8354249 [details] [diff] [review] (bio-attmnt 2482) Add /about command >diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm >+ run: function(aMsg, aConv) { Nit: you aren't using the aConv optional parameter here, you can remove it. >+ let win = Services.wm.getMostRecentWindow("Messenger:convs"); >+ let panel = win.getTabBrowser().addTabPanelFromBindingName("aboutPanel"); getMostRecentWindow can return null if there's currently no conversation window. I don't think we should care too much about it (the command API isn't tied to conversations windows, so technically an add-on could execute a command from another window; but I'm not aware of any add-on doing it currently), but it I think it would be nice to fail with a nicer error message than "win is undefined".

*** Original post on bio 2002 as attmnt 2505 at 2013-06-22 16:22:00 UTC *** - Updated to work with latest patch on bug 953867 (bio 426). - Checks if page exists before opening - Handles tabbed conversations pref. - Addresses review comments.

Comment on attachment 8354249 [details] [diff] [review] Add /about command *** Original change on bio 2002 attmnt 2482 at 2013-06-22 16:22:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 2002 as attmnt 2506 at 2013-06-22 16:28:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354273 [details] [diff] [review] Some polish *** Original change on bio 2002 attmnt 2505 at 2013-06-22 16:28:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 2002 as attmnt 2509 at 2013-06-22 17:19:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354274 [details] [diff] [review] Fix an oops *** Original change on bio 2002 attmnt 2506 at 2013-06-22 17:19:34 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354277 [details] [diff] [review] Cleanup handling of tabbed conversations preference *** Original change on bio 2002 attmnt 2509 at 2013-06-24 12:27:19 UTC *** >diff --git a/instantbird/content/aboutPanel.xml b/instantbird/content/aboutPanel.xml >+ let page = url.replace("about:", ""); // Cut off the "about:" prefix Nit: . at the end of sentences. >+ // Try to open in new tab if middle clicked. Nit: "Try to open the link in a new tab on middle click." >diff --git a/instantbird/modules/ibCore.jsm b/instantbird/modules/ibCore.jsm >+ Services.cmd.registerCommand({ [...] >+ run: function(aMsg, aConv) { >+ try { >+ Services.io.newChannelFromURI(Services.io.newURI("about:" + aMsg, null, null)); >+ } catch(e) { >+ if (e.result == Components.results.NS_ERROR_MALFORMED_URI) { >+ Services.conversations.getUIConversation(aConv).systemMessage( >+ Services.strings.createBundle("chrome://instantbird/locale/core.properties") >+ .GetStringFromName("aboutCommand.invalidPageMessage")); I find it strange that we create this bundle three separate times in this file now, is there a reason for this? This section needs to be much better commented...it's fairly dense. >+ let win = Services.wm.getMostRecentWindow("Messenger:convs"); Please include a comment about what showPage does and checks for. >+ let showPage = function(aWindow, aPage) { >+ let panel = aWindow.document.createElementNS( >+ "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", >+ "aboutPanel"); What this if statement is doing. >+ if (!aWindow.getTabBrowser().addPanel(panel)) >+ return false; >+ panel.showAboutPage(aPage); >+ return true; >+ } What is being checked for here and happens in this case. >+ if (!win || !showPage(win, aMsg)) { >+ win = Services.ww.openWindow(null, "chrome://instantbird/content/instantbird.xul", >+ "_blank", "chrome,toolbar,resizable", null); >+ win.addEventListener("load", showPage.bind(null, win, aMsg)); >+ return true; >+ } >+ return true; Almost there! :)

*** Original post on bio 2002 as attmnt 2512 at 2013-06-24 14:59:00 UTC *** - Add comments - Focus tab after showing. - Use l10nHelper for core.properties string bundle.

Comment on attachment 8354277 [details] [diff] [review] Cleanup handling of tabbed conversations preference *** Original change on bio 2002 attmnt 2509 at 2013-06-24 14:59:34 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354280 [details] [diff] [review] Address comments *** Original change on bio 2002 attmnt 2512 at 2013-06-24 15:21:53 UTC *** Can't wait to give this a try!

Comment on attachment 8354280 [details] [diff] [review] Address comments *** Original change on bio 2002 attmnt 2512 at 2013-06-25 00:03:52 UTC *** Nit: I think there's more whitespace than needed in aboutPanel.xml. I would remove the empty line between: - <binding id="aboutPanel"> and <content> - <implementation> and <field name="aboutBrowser"> - </method> and </implementation> - </handlers> and </binding> <field name="aboutPage">null</field> can be on a single line. Now real comments (the reason for r-): >+<bindings id="aboutPanelBindings" >+ xmlns="http://www.mozilla.org/xbl" >+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >+ xmlns:xbl="http://www.mozilla.org/xbl"> >+ >+ <binding id="aboutPanel"> >+ >+ <content> >+ <xul:vbox flex="1"> What's the point of this vbox? (you can set attributes on the <content> node to have attributes set to the node that gets the binding attached to it). >+ <field name="aboutBrowser"> >+ document.getAnonymousElementByAttribute(this, "anonid", "aboutBrowser"); >+ </field> This is used only once, so it should probably be inlined. But assuming you used it more than once, it should be a readonly attribute, not a field (a field is like a variable; that is initialized when the binding is created, and kept in memory until the binding is destroyed). >+ <field name="aboutPage"> >+ null >+ </field> This isn't really needed (see a later comment). And even if it was, it would be easy to get this address from the browser's src attribute (or equivalent). >+ <method name="showAboutPage"> >+ <parameter name="aPage"/> >+ <body><![CDATA[ >+ this.aboutBrowser.loadURI("about:" + aPage); >+ this.aboutPage = aPage; >+ this.tab.setAttribute("label", "about:" + aPage); Using about:<page> is good enough as a temporary tab title, but as soon as the page is actually loaded, you should display the real title. I'm sure Firefox's tabbrowser.xml can show you example of how to display the browser's title in the tab; and keep it updated if the page's JS changes the title (iirc there's an event for that). about:addons is a good page to test this. >+ <method name="finishImport"> >+ <parameter name="aAboutPanel"/> >+ <body><![CDATA[ >+ this.showAboutPage(aAboutPanel.aboutPage); You have a real browser, so why aren't you swapping the docshells? (Doing so would also have the side effect of no longer using the aboutPage field). >+ let page = aMsg.replace(/^about:/, ""); >+ let url = "about:" + page; >+ // If the page doesn't exist, we avoid opening a tab. >+ try { >+ Services.io.newChannelFromURI(Services.io.newURI(url, null, null)); >+ } catch(e) { >+ if (e.result == Components.results.NS_ERROR_MALFORMED_URI) { >+ Services.conversations.getUIConversation(aConv).systemMessage( >+ self.bundle("aboutCommand.invalidPageMessage", url)); I think using the value of the 'page' variable (closer to what the user typed) would give a better error message than the value of 'url'. You made great progress here. I tried the patch and it's really cool! :-) I hope you won't be too disappointed to not find this in the next nightly yet.

*** Original post on bio 2002 as attmnt 2514 at 2013-06-25 08:42:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354280 [details] [diff] [review] Address comments *** Original change on bio 2002 attmnt 2512 at 2013-06-25 08:42:34 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 2002 as attmnt 2515 at 2013-06-25 11:55:00 UTC *** This should address your comments, but I'm no longer able to open about:credits and don't know why. I thought I'd upload this anyway, though.

Comment on attachment 8354282 [details] [diff] [review] Address flo's comments, fix bug regarding links in the addon manager. *** Original change on bio 2002 attmnt 2514 at 2013-06-25 11:55:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 2002 at 2013-06-25 17:07:21 UTC *** It appears I can only open the about:credits page from my normal profile and not my testing one. I don't know why. This goes for all patches attached to this bug.

Comment on attachment 8354283 [details] [diff] [review] Address comments *** Original change on bio 2002 attmnt 2515 at 2013-06-25 23:34:37 UTC *** >+ <method name="refreshTitle"> >+ <body> >+ <![CDATA[ >+ this.tab.setAttribute("label", >+ this.browser.contentTitle.replace("\0", "", "g") || this.browser.currentURI.spec); >+ let imageUri = this.browser.currentURI.spec == "about:addons" ? It looks like the browser getter is called up to 3 times here. I would suggest: let browser = this.browser; >+ <property name="tab"> >+ <setter> >+ this._tab = val; >+ // Set default title, because otherwise "undefined" will flash before >+ // the browser finishes loading the page. >+ this._tab.setAttribute("label", "About page viewer"); "About page viewer" looks like it would want to be localized (if it's shown in the UI). I don't understand why you need this. Where does that 'undefined' string come from? Wouldn't calling this.refreshTitle at the end of your showAboutPage method do what you want? If not, why? I dislike the hardcoded account manager special cases, but for the sake of not wasting time on details nobody will notice, I'll pretend it's OK :).

*** Original post on bio 2002 as attmnt 2520 at 2013-06-26 11:46:00 UTC *** The undefined thing isn't necessary and is a remnant of a WIP. I've removed that and the now unneeded tab attribute.

Comment on attachment 8354283 [details] [diff] [review] Address comments *** Original change on bio 2002 attmnt 2515 at 2013-06-26 11:46:31 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354288 [details] [diff] [review] Address suggestions. *** Original change on bio 2002 attmnt 2520 at 2013-06-26 22:51:40 UTC *** Looking forward to using this! :)

*** Original post on bio 2002 at 2013-06-26 23:06:59 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/52401d522970.

*** Original post on bio 2002 at 2013-06-26 23:08:08 UTC *** Marking as fixed, forgot to do it in the previous comment.