Add basic Seamonkey support to titlebar customization. Fixes Issue #31. #104

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
3 participants
@xabolcs
Collaborator

xabolcs commented Oct 26, 2012

Fix for Issue #31.

The pull request consist of:

  • 'Customize Titlebar' menuitem
  • some titlebar customizing logic
    • mostly based on browser.js
    • and a little bit messenger.js (nightlyApp.getWindowTitleForNavigator)
  • workaround for SeaMonkey Debug and QA UI

Please test it on Mac OS X too! It would be nice to avoid bugs like Bug 717192!

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Oct 26, 2012

Collaborator

@tonymec could You give it a try, and provide feedback?
Of course, review is also welcome!

Collaborator

xabolcs commented Oct 26, 2012

@tonymec could You give it a try, and provide feedback?
Of course, review is also welcome!

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Oct 26, 2012

Owner

nit: titlemodifier){ should have a space between ) and {

nit: titlemodifier){ should have a space between ) and {

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Oct 27, 2012

Collaborator

I created an album on imgur.com with amazing screenshots! :)
Give it a look!

http://imgur.com/a/a395B/embed

Collaborator

xabolcs commented Oct 27, 2012

I created an album on imgur.com with amazing screenshots! :)
Give it a look!

http://imgur.com/a/a395B/embed

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Oct 27, 2012

Contributor

@xabolcs Do you have an XPI that I could download, or can I afford to wait until @whimboo merges this into the mozilla master (which my daily "git pull" would then automatically get into the clone on my HD)?

Contributor

tonymec commented Oct 27, 2012

@xabolcs Do you have an XPI that I could download, or can I afford to wait until @whimboo merges this into the mozilla master (which my daily "git pull" would then automatically get into the clone on my HD)?

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Oct 27, 2012

Collaborator

@tonymec Here You are!
https://github.com/downloads/xabolcs/nightlytt/nightlytt-3.4pre-issue-31-5e0db21c951c.xpi

Btw You could also checkout xabolcs:branch-issue-31-titlebar-basic-seamonkey and play.

Collaborator

xabolcs commented Oct 27, 2012

@tonymec Here You are!
https://github.com/downloads/xabolcs/nightlytt/nightlytt-3.4pre-issue-31-5e0db21c951c.xpi

Btw You could also checkout xabolcs:branch-issue-31-titlebar-basic-seamonkey and play.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Oct 27, 2012

Contributor

The menuitem is present; I tried the following custom title:

${TabTitle} (${AppBuildID}) ${Version}/${GeckoVersion} P=${Profile}

and got what I expected.

I updated https://wiki.mozilla.org/Auto-tools/Automation_Development/Projects/Addons/NightlyTesterTools#Variables according to what I saw. You may want to complete the "Explanations" column for Firefox and especially Thunderbird. I think this wiki section is still useful for current and past versions of NTT.

Contributor

tonymec commented Oct 27, 2012

The menuitem is present; I tried the following custom title:

${TabTitle} (${AppBuildID}) ${Version}/${GeckoVersion} P=${Profile}

and got what I expected.

I updated https://wiki.mozilla.org/Auto-tools/Automation_Development/Projects/Addons/NightlyTesterTools#Variables according to what I saw. You may want to complete the "Explanations" column for Firefox and especially Thunderbird. I think this wiki section is still useful for current and past versions of NTT.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Oct 27, 2012

Contributor

After some time, though, the ${DefaultTitle} is added… and added again… and again… after the custom title. My guess is that the [ Cancel ] button actually is a second [ OK ] button.

Contributor

tonymec commented Oct 27, 2012

After some time, though, the ${DefaultTitle} is added… and added again… and again… after the custom title. My guess is that the [ Cancel ] button actually is a second [ OK ] button.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 6, 2012

Contributor

@tonymec, do you have more precise steps how to reproduce that behavior?

Contributor

whimboo commented Nov 6, 2012

@tonymec, do you have more precise steps how to reproduce that behavior?

@tonymec

This comment has been minimized.

Show comment Hide comment
@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 6, 2012

Contributor

@tonymec that means when you do not use the tree to customize the titlebar it works as expected for you in SeaMonkey?

Contributor

whimboo commented Nov 6, 2012

@tonymec that means when you do not use the tree to customize the titlebar it works as expected for you in SeaMonkey?

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Nov 6, 2012

Contributor

@whimboo yes, the xpi above (5e0db21, 10 days ago) is what is installed ATM and if I double-check the customization string before closing the dialog it works as expected.

Contributor

tonymec commented Nov 6, 2012

@whimboo yes, the xpi above (5e0db21, 10 days ago) is what is installed ATM and if I double-check the customization string before closing the dialog it works as expected.

extension/chrome/content/suite.js
+},
+
+get tabTitle() {
+ var tabbrowser = gBrowser;

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 7, 2012

Contributor

tabbrowser is not necessary to declare in both cases. For SeaMonkey does it mean gBrowser is available in each of the windows?

@whimboo

whimboo Nov 7, 2012

Contributor

tabbrowser is not necessary to declare in both cases. For SeaMonkey does it mean gBrowser is available in each of the windows?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo wrote:

[...]
For SeaMonkey does it mean gBrowser is available in each of the windows?

Yeah, currently overlayed chrome://navigator/content/navigator.xul does have gBrowser.

@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo wrote:

[...]
For SeaMonkey does it mean gBrowser is available in each of the windows?

Yeah, currently overlayed chrome://navigator/content/navigator.xul does have gBrowser.

@@ -14,7 +27,26 @@ init: function()
nightly.variables.name=brandbundle.getString("brandShortName");
}
nightly.variables.brandname=brandbundle.getString("brandFullName");
- nightly.variables.defaulttitle=nightlyApp.storedTitle;

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

May I ask why you remove that line? In case of the browser.js it's present. Btw. where does storedTitle come from? I can't find a reference.

@whimboo

whimboo Nov 8, 2012

Contributor

May I ask why you remove that line? In case of the browser.js it's present. Btw. where does storedTitle come from? I can't find a reference.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

The history of storedTitle

Use the force, Luke! ;)

You can follow the blame of storedTitle to the inital commit c0ed1e99288f which is equal to aedec19990b5 at hg.oxymoronical.com.
You could see, nightlyApp.storedTitle was never defined in suite.js. But it was in browser.js. It was accidentally left out.
After that You would check Philip Chee's commit (was already linked in the issue #31), specifically the diff of suite.js.
You could see, storedTitle introduces. :)

Removing the assignment todefaulttitle

In commit 3bb023c1c4da harthur moved away from this initial, storedTitle related implementation of "title computing".
After that nightly.variables.defaulttitle became a getter. Therefore assigning a value to it is a no-op.
Or just a warning in the Error Console if javascript.options.strict is enabled:

TypeError: setting a property that has only a getter

Examining the commit, You could get the resolution that nightlyApp.storedTitle isn't used in the new version.
So it can be removed. (And should be removed in browser.js!)

By the way, sorry for the long explanation. :)

@xabolcs

xabolcs Nov 8, 2012

Collaborator

The history of storedTitle

Use the force, Luke! ;)

You can follow the blame of storedTitle to the inital commit c0ed1e99288f which is equal to aedec19990b5 at hg.oxymoronical.com.
You could see, nightlyApp.storedTitle was never defined in suite.js. But it was in browser.js. It was accidentally left out.
After that You would check Philip Chee's commit (was already linked in the issue #31), specifically the diff of suite.js.
You could see, storedTitle introduces. :)

Removing the assignment todefaulttitle

In commit 3bb023c1c4da harthur moved away from this initial, storedTitle related implementation of "title computing".
After that nightly.variables.defaulttitle became a getter. Therefore assigning a value to it is a no-op.
Or just a warning in the Error Console if javascript.options.strict is enabled:

TypeError: setting a property that has only a getter

Examining the commit, You could get the resolution that nightlyApp.storedTitle isn't used in the new version.
So it can be removed. (And should be removed in browser.js!)

By the way, sorry for the long explanation. :)

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

Thanks. So lets get this addressed for browser.js in a new issue.

@whimboo

whimboo Nov 14, 2012

Contributor

Thanks. So lets get this addressed for browser.js in a new issue.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 27, 2013

Collaborator

Issue #111 is for that.

@xabolcs

xabolcs Apr 27, 2013

Collaborator

Issue #111 is for that.

extension/chrome/content/suite.js
@@ -14,7 +27,26 @@ init: function()
nightly.variables.name=brandbundle.getString("brandShortName");
}
nightly.variables.brandname=brandbundle.getString("brandFullName");
- nightly.variables.defaulttitle=nightlyApp.storedTitle;
+
+ var tabbrowser = document.getElementById("content");

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

why don't we use this in defaultTitle() and tabTitle()?

@whimboo

whimboo Nov 8, 2012

Contributor

why don't we use this in defaultTitle() and tabTitle()?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo commented:

why don't we use this in defaultTitle() and tabTitle()?

To use what?
You would like to use the inaccessible tabbrowser variable in nightlyApp.init?
Or You would like to define a new tabBrowser property to nightlyApp?
Or?

Instead that line above, I used gBrowser not just in defaultTitle() and tabTitle() but in init() too.

@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo commented:

why don't we use this in defaultTitle() and tabTitle()?

To use what?
You would like to use the inaccessible tabbrowser variable in nightlyApp.init?
Or You would like to define a new tabBrowser property to nightlyApp?
Or?

Instead that line above, I used gBrowser not just in defaultTitle() and tabTitle() but in init() too.

extension/chrome/content/suite.js
+ tabbrowser.updateTitlebar = nightly.updateTitlebar;
+ tabbrowser.addEventListener("DOMTitleChanged", nightly.updateTitlebar, false);
+
+ var debugQABundle = document.getElementById("debugQANavigatorBundle");

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Mind adding a comment which explains what are doing here? I don't know SM that well, so it's hard for me to read that from the code.

@whimboo

whimboo Nov 8, 2012

Contributor

Mind adding a comment which explains what are doing here? I don't know SM that well, so it's hard for me to read that from the code.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo commented:

Mind adding a comment which explains what are doing here? ...

SeaMonkey Debug and QA UI addon is a default distributed addon on nightly and aurora channel:

All nightly builds currently include the Debug and QA UI extension. For release builds you can install extension from AMO.

It have a lot of feature as You can see on the wiki, one of them is modifying the title. See my comment above for pictures!

I thought titlebar-customization should provide the same result regardless how this addon is installed or not.
DebugQA modifies the original titlemodifier (==SeaMonkey or empty on Mac) to SeaMonkey {Build ID: 20121108013004} or like, and messes defaultTitle up.

These lines do checks like: debugQA addon is installed/enabled?, Current titlemodifier seems like debugQA's titlemodifier? and set a helper wariable (debugQATitleModifierWorkaround), which is used in title calculation (getWindowTitleForNavigator()).

@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo commented:

Mind adding a comment which explains what are doing here? ...

SeaMonkey Debug and QA UI addon is a default distributed addon on nightly and aurora channel:

All nightly builds currently include the Debug and QA UI extension. For release builds you can install extension from AMO.

It have a lot of feature as You can see on the wiki, one of them is modifying the title. See my comment above for pictures!

I thought titlebar-customization should provide the same result regardless how this addon is installed or not.
DebugQA modifies the original titlemodifier (==SeaMonkey or empty on Mac) to SeaMonkey {Build ID: 20121108013004} or like, and messes defaultTitle up.

These lines do checks like: debugQA addon is installed/enabled?, Current titlemodifier seems like debugQA's titlemodifier? and set a helper wariable (debugQATitleModifierWorkaround), which is used in title calculation (getWindowTitleForNavigator()).

{
+ if (nightlyApp.oldUpdateTitlebar) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Looks like browser.js would also benefit from such a check. Could be done in a follow-up bug.

@whimboo

whimboo Nov 8, 2012

Contributor

Looks like browser.js would also benefit from such a check. Could be done in a follow-up bug.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

Not just browser.js but other functions in ...

@xabolcs

xabolcs Nov 8, 2012

Collaborator

Not just browser.js but other functions in ...

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

... in suite.js too! :)

@xabolcs

xabolcs Nov 8, 2012

Collaborator

... in suite.js too! :)

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 16, 2012

Collaborator

What about filing a bug to let nightlyApp.oldUpdateTitlebar a getter / setter, like here in suite.js (and also with the other checks)?

@xabolcs

xabolcs Nov 16, 2012

Collaborator

What about filing a bug to let nightlyApp.oldUpdateTitlebar a getter / setter, like here in suite.js (and also with the other checks)?

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

Sure. Go for clarity.

@whimboo

whimboo Apr 16, 2013

Contributor

Sure. Go for clarity.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 28, 2013

Collaborator

Filed #136.

@xabolcs

xabolcs Apr 28, 2013

Collaborator

Filed #136.

extension/chrome/content/suite.js
+},
+
+/**
+ * Calculates the title like tabbrowser's updateTitlebar(), but doesn't set.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Doesn't set what?

@whimboo

whimboo Nov 8, 2012

Contributor

Doesn't set what?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

Doesn't set document.title and

window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
      .getInterface(Components.interfaces.nsIWebNavigation)
      .QueryInterface(Components.interfaces.nsIBaseWindow).title

See updateTitlebar() in suite/browser/tabbrowser.xml!

@xabolcs

xabolcs Nov 8, 2012

Collaborator

Doesn't set document.title and

window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
      .getInterface(Components.interfaces.nsIWebNavigation)
      .QueryInterface(Components.interfaces.nsIBaseWindow).title

See updateTitlebar() in suite/browser/tabbrowser.xml!

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

What I meant was that you should update the comment.

@whimboo

whimboo Nov 14, 2012

Contributor

What I meant was that you should update the comment.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 14, 2012

Collaborator

Ohh, sorry! I meant leaving a comment on GitHub is fine, but You meant creating a jsdoc comment would be fine.

@xabolcs

xabolcs Nov 14, 2012

Collaborator

Ohh, sorry! I meant leaving a comment on GitHub is fine, but You meant creating a jsdoc comment would be fine.

extension/chrome/content/suite.js
+ docTitle = aBrowser.contentTitle;
+
+ if (!docTitle && !modifier) {
+ docTitle = aBrowser.getTitleForURI(aBrowser.mCurrentBrowser.currentURI);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Shouldn't this be aBrowser.currentURI? You are calling this method with tabbrowser.mCurrentBrowser as parameter.

@whimboo

whimboo Nov 8, 2012

Contributor

Shouldn't this be aBrowser.currentURI? You are calling this method with tabbrowser.mCurrentBrowser as parameter.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

I wanted to keep close the implementation to original sources (tabbrowser's updateTitlebar() and getWindowTitleForBrowser()), therefore I did only the necessary modifications: this -> aBrowser, stripping the comments and the debugQA workaround stuff.

Let this nightlyApp.getWindowTitleForNavigator() along with nightlyApp.getWindowTitleForMessenger() keep close to original implementations, please.

@xabolcs

xabolcs Nov 8, 2012

Collaborator

I wanted to keep close the implementation to original sources (tabbrowser's updateTitlebar() and getWindowTitleForBrowser()), therefore I did only the necessary modifications: this -> aBrowser, stripping the comments and the debugQA workaround stuff.

Let this nightlyApp.getWindowTitleForNavigator() along with nightlyApp.getWindowTitleForMessenger() keep close to original implementations, please.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

All that is right, but aBrowser.mCurrentBrowser.currentURI is clearly wrong. aBrowser is already the current browser and shouldn't have an additionally mCurrentBrowser property attached.

@whimboo

whimboo Nov 14, 2012

Contributor

All that is right, but aBrowser.mCurrentBrowser.currentURI is clearly wrong. aBrowser is already the current browser and shouldn't have an additionally mCurrentBrowser property attached.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 15, 2012

Collaborator

Regarding to your comment and that the original updateTitlebar() doesn't have any parameter, I'm going to remove this aBrowser thing and use gBrowser instead .

@xabolcs

xabolcs Nov 15, 2012

Collaborator

Regarding to your comment and that the original updateTitlebar() doesn't have any parameter, I'm going to remove this aBrowser thing and use gBrowser instead .

extension/chrome/content/suite.js
+ docElement.getAttribute("titlemodifier");
+ }
+
+ if (aBrowser.docShell.contentViewer)

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Mind explaining this check in a comment?

@whimboo

whimboo Nov 8, 2012

Contributor

Mind explaining this check in a comment?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

I don't really know, and I doesn't care about it really. ;)
Because it was grabbed from comm-central as I above said, I simply trust in this piece of code. :)

@xabolcs

xabolcs Nov 8, 2012

Collaborator

I don't really know, and I doesn't care about it really. ;)
Because it was grabbed from comm-central as I above said, I simply trust in this piece of code. :)

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

It's not something I can find in the getWindowTitleForBrowser() implementation of the tabbrowser.xml file, so my request still stands.

@whimboo

whimboo Nov 14, 2012

Contributor

It's not something I can find in the getWindowTitleForBrowser() implementation of the tabbrowser.xml file, so my request still stands.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 14, 2012

Collaborator

Note to self: jsdoc please!

@xabolcs

xabolcs Nov 14, 2012

Collaborator

Note to self: jsdoc please!

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 15, 2012

Collaborator

whimboo commented:

... my request still stands.

Sorry, I don't know this code. MDN doesn't say anything for me.
I could leave a TODO here, and ask some SeaMonkey coder to say something here as a followup.
I'm really sorry.

@xabolcs

xabolcs Nov 15, 2012

Collaborator

whimboo commented:

... my request still stands.

Sorry, I don't know this code. MDN doesn't say anything for me.
I could leave a TODO here, and ask some SeaMonkey coder to say something here as a followup.
I'm really sorry.

extension/chrome/content/suite.js
+ try {
+ if (docElement.getAttribute("chromehidden").indexOf("location") != -1) {
+ var uri = aBrowser.mURIFixup.createExposableURI(
+ aBrowser.mCurrentBrowser.currentURI);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

nit: extra space before =. Also same mCurrentBrowser question as above.

@whimboo

whimboo Nov 8, 2012

Contributor

nit: extra space before =. Also same mCurrentBrowser question as above.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

Good catch! :)
comm-central has this nit too! :)

@xabolcs

xabolcs Nov 8, 2012

Collaborator

Good catch! :)
comm-central has this nit too! :)

extension/chrome/content/suite.js
+ }
+ newTitle += modifier;
+
+ try {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Mind adding documentation for this section?

@whimboo

whimboo Nov 8, 2012

Contributor

Mind adding documentation for this section?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

Of course. :)
There were comment originally. Just stripped.

@xabolcs

xabolcs Nov 8, 2012

Collaborator

Of course. :)
There were comment originally. Just stripped.

extension/chrome/content/suite.js
+ if (uri.schemeIs("about"))
+ newTitle = uri.spec + sep + newTitle;
+ else if (uri.host)
+ newTitle = uri.prePath + sep + newTitle;

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

Can we move up all this code right under the local variable declarations of this method?

@whimboo

whimboo Nov 8, 2012

Contributor

Can we move up all this code right under the local variable declarations of this method?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo commented:

Can we move up all this code ... ?

Because of the above detailed reasons, I won't like to make implementation changes in nightlyApp.getWindowTitleForNavigator().

IMHO this can't be moved up as You requested:

  • newTitle depends on: this try ... catch URI stuff, "titlepreface", docTitle
  • docTitle depends on: modifier, mCurrentBrowser.currentURI, contentTitle
  • modifier depends on: "titlemodifier" and the debugQA workaround stuff

I think it's too complex.
Of course I could be wrong.

@xabolcs

xabolcs Nov 8, 2012

Collaborator

whimboo commented:

Can we move up all this code ... ?

Because of the above detailed reasons, I won't like to make implementation changes in nightlyApp.getWindowTitleForNavigator().

IMHO this can't be moved up as You requested:

  • newTitle depends on: this try ... catch URI stuff, "titlepreface", docTitle
  • docTitle depends on: modifier, mCurrentBrowser.currentURI, contentTitle
  • modifier depends on: "titlemodifier" and the debugQA workaround stuff

I think it's too complex.
Of course I could be wrong.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

Lets leave it as is given that is mostly a copy of the tabbrowser.xml file's content. Therefor please note that in the jsdoc comment of the function. It should be clear that you copied that over.

@whimboo

whimboo Nov 14, 2012

Contributor

Lets leave it as is given that is mostly a copy of the tabbrowser.xml file's content. Therefor please note that in the jsdoc comment of the function. It should be clear that you copied that over.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 8, 2012

Contributor

I can't test this new feature in SM. So I have to trust your judgement and testing across platforms.

Contributor

whimboo commented Nov 8, 2012

I can't test this new feature in SM. So I have to trust your judgement and testing across platforms.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 9, 2012

Collaborator

Commit range updated (22afc5a inclusive) - addressed comments.

It's OK for me to do the needed tests.

Collaborator

xabolcs commented Nov 9, 2012

Commit range updated (22afc5a inclusive) - addressed comments.

It's OK for me to do the needed tests.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 9, 2012

Owner

This change isn't really needed.

This change isn't really needed.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 9, 2012

Owner

If nightlyApp.oldUpdateTitlebar is null (or something) then this would fail.

If nightlyApp.oldUpdateTitlebar is null (or something) then this would fail.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 9, 2012

Owner

I'd like to convert nightly.oldUpdateTitlebar into a getter/setter: it would work as now, but the getter would log a warning if it doesn't have a value yet.

I'd like to convert nightly.oldUpdateTitlebar into a getter/setter: it would work as now, but the getter would log a warning if it doesn't have a value yet.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 9, 2012

Collaborator

Commit 6b147ac added: nightly.oldUpdateTitlebar is now a getter / setter.

@whimboo I'm done with addressing Your comments. Feel free to start the second round of review. :)

Collaborator

xabolcs commented Nov 9, 2012

Commit 6b147ac added: nightly.oldUpdateTitlebar is now a getter / setter.

@whimboo I'm done with addressing Your comments. Feel free to start the second round of review. :)

extension/chrome/content/suite.js
+get oldUpdateTitlebar() {
+ if (!nightlyApp._oldUpdateTitlebar) {
+ Components.utils.import("resource://nightly/Logging.jsm");
+ WARN("No suitable titlebar function was found! Titlebar customization is incomplete! Please file a bug!");

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

Please file a new issue for the logging module that we do not pollute the global namespace but export a Logger object.

@whimboo

whimboo Nov 14, 2012

Contributor

Please file a new issue for the logging module that we do not pollute the global namespace but export a Logger object.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 15, 2012

Collaborator

Thanks for noting this!

If You would like to use Logger.log, Logger.warn, etc. then I propose to use (a forked/copied) log4moz.
But I'm really OK with var { what, you, want, to, use } = Components.utils.import("resource://myExt/myModule.jsm", {});

@xabolcs

xabolcs Nov 15, 2012

Collaborator

Thanks for noting this!

If You would like to use Logger.log, Logger.warn, etc. then I propose to use (a forked/copied) log4moz.
But I'm really OK with var { what, you, want, to, use } = Components.utils.import("resource://myExt/myModule.jsm", {});

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 16, 2012

Collaborator

So ... if You would like to separate Nightly's (non-existent ;) ) logging output (one prefix for screenshooting, one for titlebar customizing, other prefixes for other NTT features, etc.) then file that new issue.

IMHO in other case it's not needed to restructure Logging.jsm to export only one object. The pollution was my fault.

@xabolcs

xabolcs Nov 16, 2012

Collaborator

So ... if You would like to separate Nightly's (non-existent ;) ) logging output (one prefix for screenshooting, one for titlebar customizing, other prefixes for other NTT features, etc.) then file that new issue.

IMHO in other case it's not needed to restructure Logging.jsm to export only one object. The pollution was my fault.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Nov 14, 2012

Contributor

Due to an outtage on github I can't currenlty comment on the code. So one remaining question from me:

Why are you using typeof(gBrowser) !== "undefined" now instead of checking that we have a gBrowser object? I don't see a benefit in this change.

Contributor

whimboo commented Nov 14, 2012

Due to an outtage on github I can't currenlty comment on the code. So one remaining question from me:

Why are you using typeof(gBrowser) !== "undefined" now instead of checking that we have a gBrowser object? I don't see a benefit in this change.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 15, 2012

Collaborator

whimboo commented:

... I don't see a benefit in this change.

if (isThereSomething) check is safe for null, but will throw for undefined.
Try the code below in Error Console:

try {
  if (gBrowser && typeof(gBrowser.updateTitlebar) === "function") { "something" } 
  else { "nothing" } 
} catch (e) {"oops!"}

It will drop an oops! to the output instead nothing! (Error Console doesn't have a window or an gBrowser variable).

Collaborator

xabolcs commented Nov 15, 2012

whimboo commented:

... I don't see a benefit in this change.

if (isThereSomething) check is safe for null, but will throw for undefined.
Try the code below in Error Console:

try {
  if (gBrowser && typeof(gBrowser.updateTitlebar) === "function") { "something" } 
  else { "nothing" } 
} catch (e) {"oops!"}

It will drop an oops! to the output instead nothing! (Error Console doesn't have a window or an gBrowser variable).

Issue #31 - addressing comments part 2:
- aBroswer -> call(gBrowser) + this
- getWindowTitleForNavigator jsdoc description
- docShell.contentViewer jsdoc TODO
- not polluting with Logger.jsm
@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Nov 16, 2012

Collaborator

@whimboo I'm done with addressing comments. Feel free to review again.
Third round. :)

Collaborator

xabolcs commented Nov 16, 2012

@whimboo I'm done with addressing comments. Feel free to review again.
Third round. :)

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Apr 15, 2013

Contributor

I suppose this merge is waiting on issue #129 ? Or could this be merged now and "enhanced" later? Or is there something else?

Sorry for the bugspam but at times I get lost in these "github issues".

Contributor

tonymec commented Apr 15, 2013

I suppose this merge is waiting on issue #129 ? Or could this be merged now and "enhanced" later? Or is there something else?

Sorry for the bugspam but at times I get lost in these "github issues".

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 15, 2013

Collaborator

I suppose this merge is waiting on issue #129 ? Or could this be merged now and "enhanced" later? Or is there something else?

It's waiting for r+ from @whimboo - at least on code styling.
It's not blocked by #129, that issue is about adding a new element to the customization list.

Collaborator

xabolcs commented Apr 15, 2013

I suppose this merge is waiting on issue #129 ? Or could this be merged now and "enhanced" later? Or is there something else?

It's waiting for r+ from @whimboo - at least on code styling.
It's not blocked by #129, that issue is about adding a new element to the customization list.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 15, 2013

Collaborator

duplicated.

Collaborator

xabolcs commented Apr 15, 2013

duplicated.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

if (isThereSomething) check is safe for null, but will throw for undefined.

In this case it's not an undefined value but the variable doesn't exist at all. That's why it throws an exception. A check like if ("gBrowser" in window && ...) would do the right thing.

Contributor

whimboo commented Apr 16, 2013

if (isThereSomething) check is safe for null, but will throw for undefined.

In this case it's not an undefined value but the variable doesn't exist at all. That's why it throws an exception. A check like if ("gBrowser" in window && ...) would do the right thing.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 16, 2013

Collaborator

In this case it's not an undefined value but the variable doesn't exist at all. That's why it throws an exception. A check like if ("gBrowser" in window && ...) would do the right thing.

Ooops, you are right. :)
I missed window: if (window.gBrowser) {...} would do the right thing too.

Collaborator

xabolcs commented Apr 16, 2013

In this case it's not an undefined value but the variable doesn't exist at all. That's why it throws an exception. A check like if ("gBrowser" in window && ...) would do the right thing.

Ooops, you are right. :)
I missed window: if (window.gBrowser) {...} would do the right thing too.

extension/chrome/content/suite.js
+ * TODO Explain this check
+ */
+ if (this.docShell.contentViewer)
+ docTitle = this.contentTitle;

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

Still not sure about this check. I don't see why we have to check for docShell.contentViewer before accessing contentTitle. The latter seems to be always present as property on the browser object: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2559. Or what is the this object here? It's hard to get it from this piece of code.

@whimboo

whimboo Apr 16, 2013

Contributor

Still not sure about this check. I don't see why we have to check for docShell.contentViewer before accessing contentTitle. The latter seems to be always present as property on the browser object: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2559. Or what is the this object here? It's hard to get it from this piece of code.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 16, 2013

Collaborator

This getWindowTitleForNavigator() is based on Suite's updateTitlebar implementation: https://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#879.
Blame shows that check is really old. I'd like to forward your question to the SM maintainer guys.

@xabolcs

xabolcs Apr 16, 2013

Collaborator

This getWindowTitleForNavigator() is based on Suite's updateTitlebar implementation: https://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#879.
Blame shows that check is really old. I'd like to forward your question to the SM maintainer guys.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Apr 17, 2013

Contributor

In the current Suite source, your TODO comment and the following two lines of codes are as follows:

888             // Strip out any null bytes in the content title, since the
889             // underlying widget implementations of nsWindow::SetTitle pass
890             // null-terminated strings to system APIs.
891             if (this.docShell.contentViewer)
892               docTitle = this.contentTitle.replace(/\0+/g, "");
@tonymec

tonymec Apr 17, 2013

Contributor

In the current Suite source, your TODO comment and the following two lines of codes are as follows:

888             // Strip out any null bytes in the content title, since the
889             // underlying widget implementations of nsWindow::SetTitle pass
890             // null-terminated strings to system APIs.
891             if (this.docShell.contentViewer)
892               docTitle = this.contentTitle.replace(/\0+/g, "");

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 18, 2013

Contributor

Ok, so lets leave it as is.

@whimboo

whimboo Apr 18, 2013

Contributor

Ok, so lets leave it as is.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 28, 2013

Collaborator

@tonymec
The "new" TODO comment doesn't explains me why this check is for.
(And I moved the "new" comment in the if case).

@whimboo
I still can't answer your original question, therefore I'd like to keep as close the implementation to the SM source as possible.

@xabolcs

xabolcs Apr 28, 2013

Collaborator

@tonymec
The "new" TODO comment doesn't explains me why this check is for.
(And I moved the "new" comment in the if case).

@whimboo
I still can't answer your original question, therefore I'd like to keep as close the implementation to the SM source as possible.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 28, 2013

Collaborator

Btw I also added the null-byte strip change.

@xabolcs

xabolcs Apr 28, 2013

Collaborator

Btw I also added the null-byte strip change.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 29, 2013

Contributor

As said earlier. Leave it as is. It's fine.

@whimboo

whimboo Apr 29, 2013

Contributor

As said earlier. Leave it as is. It's fine.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

I missed window: if (window.gBrowser) {...} would do the right thing too.

Not really. This would still be a failure in strict js mode.

Contributor

whimboo commented Apr 16, 2013

I missed window: if (window.gBrowser) {...} would do the right thing too.

Not really. This would still be a failure in strict js mode.

+debugQATitleModifierWorkaround: null,
+
+get oldUpdateTitlebar() {
+ if (!nightlyApp._oldUpdateTitlebar) {

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

Can't we rename nightlyApp to this here? That adds more dependencies when it comes to variable name changes. Other places are affected too.

@whimboo

whimboo Apr 16, 2013

Contributor

Can't we rename nightlyApp to this here? That adds more dependencies when it comes to variable name changes. Other places are affected too.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 27, 2013

Collaborator

(Almost) all the code use the "absolute reference" method in Nightly Tester Tools.
If it makes sense, you could file an issue about moving away from absolute referencing, and I could do the first step in this pull.

But I don't see the benefits for now.
Of course, it could be doable when we modularize NTT.

@xabolcs

xabolcs Apr 27, 2013

Collaborator

(Almost) all the code use the "absolute reference" method in Nightly Tester Tools.
If it makes sense, you could file an issue about moving away from absolute referencing, and I could do the first step in this pull.

But I don't see the benefits for now.
Of course, it could be doable when we modularize NTT.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 29, 2013

Contributor

Ok, so lets leave it then. We might want to tackle it when working on the restartless version, which would require a rewrite in any way.

@whimboo

whimboo Apr 29, 2013

Contributor

Ok, so lets leave it then. We might want to tackle it when working on the restartless version, which would require a rewrite in any way.

extension/chrome/content/suite.js
+set oldUpdateTitlebar(aParam) {
+ if (typeof(aParam) === "function") {
+ nightlyApp._oldUpdateTitlebar = aParam;
+ }

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

Given that this method is only used from our code, I'm not sure that the extra check here is necessary.

@whimboo

whimboo Apr 16, 2013

Contributor

Given that this method is only used from our code, I'm not sure that the extra check here is necessary.

extension/chrome/content/suite.js
+
+get tabTitle() {
+ if (nightlyApp.oldUpdateTitlebar) {
+ return gBrowser.mCurrentBrowser.contentTitle;

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

See my other link to contentTitle. A gBrowser.contentTitle should be enough.

@whimboo

whimboo Apr 16, 2013

Contributor

See my other link to contentTitle. A gBrowser.contentTitle should be enough.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 27, 2013

Collaborator

Sure.

@xabolcs

xabolcs Apr 27, 2013

Collaborator

Sure.

extension/chrome/content/suite.js
+ docTitle = this.contentTitle;
+
+ if (!docTitle && !modifier) {
+ docTitle = this.getTitleForURI(this.mCurrentBrowser.currentURI);

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 16, 2013

Contributor

this.currentURI should be enough, or? At least when I check http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2500

@whimboo

whimboo Apr 16, 2013

Contributor

this.currentURI should be enough, or? At least when I check http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2500

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 27, 2013

Collaborator
@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 29, 2013

Hm, this only affects SeaMonkey?

Hm, this only affects SeaMonkey?

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 30, 2013

Owner

It only affects SeaMonkey in the context of NTT.
But it affects Firefox too, see bug 818014:

  • vulnerable Firefox + NTT == vulnerable Firefox
  • fixed Firefox + NTT == secure Firefox

We are able to secure Users with old Firefoxes, if we land an improvement to NTT (for example near nightly.variables.defaulttitle).
If we land It would be nice if we release the fix as 3.5.1 hotfix, because it's a minor change.
(I could do the branch work.)

Owner

xabolcs replied Apr 30, 2013

It only affects SeaMonkey in the context of NTT.
But it affects Firefox too, see bug 818014:

  • vulnerable Firefox + NTT == vulnerable Firefox
  • fixed Firefox + NTT == secure Firefox

We are able to secure Users with old Firefoxes, if we land an improvement to NTT (for example near nightly.variables.defaulttitle).
If we land It would be nice if we release the fix as 3.5.1 hotfix, because it's a minor change.
(I could do the branch work.)

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo May 3, 2013

Please use replace("\0", "") here which is faster than a regex and has also been used in the patch for the referenced bug.

I'm happy to get this out as a hotfix too for Firefox and other apps which are affected when running NTT.

Please use replace("\0", "") here which is faster than a regex and has also been used in the patch for the referenced bug.

I'm happy to get this out as a hotfix too for Firefox and other apps which are affected when running NTT.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs May 3, 2013

Owner

I'm happy to get this out as a hotfix too for Firefox and other apps which are affected when running NTT.

To be clear: they are affected all the time!
That hotfix would be a protection for users who have NTT enabled and use custom title.
Other users will have protection only if they update their Application.

Owner

xabolcs replied May 3, 2013

I'm happy to get this out as a hotfix too for Firefox and other apps which are affected when running NTT.

To be clear: they are affected all the time!
That hotfix would be a protection for users who have NTT enabled and use custom title.
Other users will have protection only if they update their Application.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs May 7, 2013

Owner

@whimboo
Ping.

Owner

xabolcs replied May 7, 2013

@whimboo
Ping.

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo May 8, 2013

First lets get all files for this issue updated on master. If we want to take it as hotfix we can decide it once it has been landed. Please spun another issue for it.

First lets get all files for this issue updated on master. If we want to take it as hotfix we can decide it once it has been landed. Please spun another issue for it.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs May 9, 2013

Owner

First lets get all files for this issue updated on master. ...

As I wrote this is not an issue in the context of NTT (as #31 - SM support got landed).

Owner

xabolcs replied May 9, 2013

First lets get all files for this issue updated on master. ...

As I wrote this is not an issue in the context of NTT (as #31 - SM support got landed).

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo May 10, 2013

That's also not what I said. You most likely got confused by the usage of 'this'. I was not talking about this pull request / issue but the underlying problem. So please work on it in a separate issue.

That's also not what I said. You most likely got confused by the usage of 'this'. I was not talking about this pull request / issue but the underlying problem. So please work on it in a separate issue.

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs May 10, 2013

Owner

That's also not what I said. ....

Yeah, there is a misunderstanding near us. :)
I think that not striping out null bytes for Firefox is not a regression of NTT:

  • vulnerable Firefox + NTT == vulnerable Firefox
  • fixed Firefox + NTT == secure Firefox

But if we add those striping work to NTT then we could deliver the protection to some Firefox < 22 User.
Filing a follow-up issue and working on it wasn't a question here. :)

...for Firefox and other apps which are affected when running NTT.

Based on that comment I assume, you think this is an NTT regression.
In my opinion this is (would be) an enhancement to NTT.
In short I'm talking about the label of the new isse - it's an enhancement.

Owner

xabolcs replied May 10, 2013

That's also not what I said. ....

Yeah, there is a misunderstanding near us. :)
I think that not striping out null bytes for Firefox is not a regression of NTT:

  • vulnerable Firefox + NTT == vulnerable Firefox
  • fixed Firefox + NTT == secure Firefox

But if we add those striping work to NTT then we could deliver the protection to some Firefox < 22 User.
Filing a follow-up issue and working on it wasn't a question here. :)

...for Firefox and other apps which are affected when running NTT.

Based on that comment I assume, you think this is an NTT regression.
In my opinion this is (would be) an enhancement to NTT.
In short I'm talking about the label of the new isse - it's an enhancement.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo Apr 29, 2013

Contributor

Can we get this feature tested by Tony? @xabolcs, could you upload the patches XPI? We could make use of the download section on github.

Contributor

whimboo commented Apr 29, 2013

Can we get this feature tested by Tony? @xabolcs, could you upload the patches XPI? We could make use of the download section on github.

@xabolcs

This comment has been minimized.

Show comment Hide comment
@xabolcs

xabolcs Apr 30, 2013

Collaborator

We could make use of the download section on github.

It won't work! At least easily.
I see the notification below on the `mozilla/nightly/downloads site!

We’ve recently deprecated this downloads section (manually uploaded files).
Please read the blog post for more information.

Collaborator

xabolcs commented Apr 30, 2013

We could make use of the download section on github.

It won't work! At least easily.
I see the notification below on the `mozilla/nightly/downloads site!

We’ve recently deprecated this downloads section (manually uploaded files).
Please read the blog post for more information.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Apr 30, 2013

Contributor

Any publicly accessible places where each of us can upload will do. My Internet account includes a "home site" http://users.skynet.be/antoine.mechelynck/ where I can read and write by FTP (using a different URL with a username and password) and anyone can read by HTTP. I have there some files "for anyone" which are linked from the top-level index.htm, and files "for special purposes" which can be read by anyone who has the URL but aren't linked from the index.htm.

Similarly, any place where you can conveniently upload can be used for files to share with the others.

Or you could put up a "pull request" for a branch other than "master" on tonymec/nightlytt, and I could (by exception) pull from that into my HD clone to make the XPI.

Contributor

tonymec commented Apr 30, 2013

Any publicly accessible places where each of us can upload will do. My Internet account includes a "home site" http://users.skynet.be/antoine.mechelynck/ where I can read and write by FTP (using a different URL with a username and password) and anyone can read by HTTP. I have there some files "for anyone" which are linked from the top-level index.htm, and files "for special purposes" which can be read by anyone who has the URL but aren't linked from the index.htm.

Similarly, any place where you can conveniently upload can be used for files to share with the others.

Or you could put up a "pull request" for a branch other than "master" on tonymec/nightlytt, and I could (by exception) pull from that into my HD clone to make the XPI.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec Apr 30, 2013

Contributor

OK, I just pulled xabolcs:branch-issue-31-titlebar-basic-seamonkey to tonymec:issue-31-basic-sm-titlebar — the rest will come later, I have to go to town.

Contributor

tonymec commented Apr 30, 2013

OK, I just pulled xabolcs:branch-issue-31-titlebar-basic-seamonkey to tonymec:issue-31-basic-sm-titlebar — the rest will come later, I have to go to town.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec May 1, 2013

Contributor

After

git pull https://github.com/tonymec/nightlytt issue-31-basic-sm-titlebar
ant
cp -v xpi/*.xpi ~/.download/mozilla/ext_sm

then install into SeaMonkey and restart, there is a ToolsNightly Tester ToolsCustomize Titlebar menuitem. I notice that my nightly.templates.buildid (which I had kept since a previous test of this bugfix) works. I try the new menuitem to change the title (by manually erasing unneeded parts in the input box, then clicking desired additions) and I notice that the title of the current window changes at every click but not on manual changes in the input box. I have browser.preferences.instantApply set to false (not the default) and there are two buttons, Cancel OK near the bottom right corner of the dialog, but clicking Cancel keeps the current titlebar value — it does not go back to what the title was when opening the submenu.

I see three possible ways out of this situation:

  1. Disregard browser.preferences.instantApply, which will remove one AMO validation warning unless that pref is used elsewhere in the extension. Always have Cancel and OK buttons; clicking an item changes the contents of the input box but not (yet) the current title, Cancel closes the dialog but otherwise does nothing; OK applies the changes and closes the dialog.
  2. Disregard browser.preferences.instantApply. Always have only a Close button. All changes (including manual changes in the input box) are applied immediately (or at least, no later than the next mouse click, even on the Close button or on the × closing widget at the end of the dialog titlebar). Close simply closes the dialog (after applying pending changes if any).
  3. (which I prefer but it is the most complex): Don't disregard browser.preferences.instantApply, but otherwise act as in (a) when it is false, as in (b) when it is true.

Edit: Known issue #108

Now I'll try merging this with the fix for issue #129 (pull request #137) in the hope of getting a ${PlatformChangeset} entity without losing the customization dialog. :-)

Contributor

tonymec commented May 1, 2013

After

git pull https://github.com/tonymec/nightlytt issue-31-basic-sm-titlebar
ant
cp -v xpi/*.xpi ~/.download/mozilla/ext_sm

then install into SeaMonkey and restart, there is a ToolsNightly Tester ToolsCustomize Titlebar menuitem. I notice that my nightly.templates.buildid (which I had kept since a previous test of this bugfix) works. I try the new menuitem to change the title (by manually erasing unneeded parts in the input box, then clicking desired additions) and I notice that the title of the current window changes at every click but not on manual changes in the input box. I have browser.preferences.instantApply set to false (not the default) and there are two buttons, Cancel OK near the bottom right corner of the dialog, but clicking Cancel keeps the current titlebar value — it does not go back to what the title was when opening the submenu.

I see three possible ways out of this situation:

  1. Disregard browser.preferences.instantApply, which will remove one AMO validation warning unless that pref is used elsewhere in the extension. Always have Cancel and OK buttons; clicking an item changes the contents of the input box but not (yet) the current title, Cancel closes the dialog but otherwise does nothing; OK applies the changes and closes the dialog.
  2. Disregard browser.preferences.instantApply. Always have only a Close button. All changes (including manual changes in the input box) are applied immediately (or at least, no later than the next mouse click, even on the Close button or on the × closing widget at the end of the dialog titlebar). Close simply closes the dialog (after applying pending changes if any).
  3. (which I prefer but it is the most complex): Don't disregard browser.preferences.instantApply, but otherwise act as in (a) when it is false, as in (b) when it is true.

Edit: Known issue #108

Now I'll try merging this with the fix for issue #129 (pull request #137) in the hope of getting a ${PlatformChangeset} entity without losing the customization dialog. :-)

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec May 1, 2013

Contributor

Oh, and I forgot: in (a) above (and (c) when acting like (a)) an Apply button would be nice. Maybe also an Append button to separate it from just "selecting a line" which is the usual function of a plain click.

I have created branch "issues-31-129" in tonymec/nightlytt and built from it an XPI for you to test — you might want to check how it works with Thunderbird.

Contributor

tonymec commented May 1, 2013

Oh, and I forgot: in (a) above (and (c) when acting like (a)) an Apply button would be nice. Maybe also an Append button to separate it from just "selecting a line" which is the usual function of a plain click.

I have created branch "issues-31-129" in tonymec/nightlytt and built from it an XPI for you to test — you might want to check how it works with Thunderbird.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo May 6, 2013

Contributor

Code-wise we are fine now. Whenever Tony signs off from testing we can get this pull merged.

Contributor

whimboo commented May 6, 2013

Code-wise we are fine now. Whenever Tony signs off from testing we can get this pull merged.

@tonymec

This comment has been minimized.

Show comment Hide comment
@tonymec

tonymec May 7, 2013

Contributor

:-) Go ahead. After you merge I'll build anew.

Contributor

tonymec commented May 7, 2013

:-) Go ahead. After you merge I'll build anew.

@whimboo

This comment has been minimized.

Show comment Hide comment
@whimboo

whimboo May 7, 2013

Contributor

Landed as:
fb8b465

Contributor

whimboo commented May 7, 2013

Landed as:
fb8b465

@whimboo whimboo closed this May 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment