Open Bug 583890 Opened 14 years ago Updated 1 year ago

When the full page title is not shown remove redundant text in tabs

Categories

(Firefox :: Tabbed Browser, defect)

defect

Tracking

()

REOPENED
Firefox 18

People

(Reporter: faaborg, Unassigned)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: parity-safari, ux-discovery, ux-error-prevention, Whiteboard: [Advo])

Attachments

(6 files, 70 obsolete files)

36.75 KB, image/png
Details
76.04 KB, image/png
Details
100.53 KB, image/png
Details
18.48 KB, image/png
Details
9.85 KB, patch
Details | Diff | Splinter Review
33.02 KB, patch
Details | Diff | Splinter Review
If the full page title is not displayed, we need to to do a better job of stripping proceeding redundant text in tab's title in the event of overflow.

I know Safari has looked into this, so we should check out what they are currently doing as well.  But in general we shouldn't get the user into a state where a large number of their tabs all start with "NYTime.." or "Ebay -..."

Please dupe if there is an existing bug.
Safari does this quite well, even calculating on-the-fly when scripts change the titles.
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [parity-safari]
Blocks: cuts-tabs
Assignee: nobody → fyan
Status: NEW → ASSIGNED
I was thinking maybe expose the full title in the status bar (when enabled) instead of 'Done'?
In general it may be OK. 
But, still i feel we should not edit title content before displaying it.
Since I started writing HTML in 1996, always the author expect every thing 
(or at least till window title length)in <TITLE> be displayed  

So I feel better way is Bug 588262
The problem with automatic removal is that it doesn't work with only 1 tab open from a particular site! The automation might also sometimes get it wrong, and causes websites to be displayed differently as to how they were designed.

Therefore I think a useful solution is to always display the full title in the Location Bar as follows:
https://wiki.mozilla.org/Talk:Firefox/4.0_Windows_Theme_Mockups#Page_Title_in_the_Location_Bar.3F

Combine this with this proposal to auto-trim the title in the tab and you get around the oddity that I described of it "being weird seeing the title twice".

You also get a Fitts' Law benefit of increasing the size of the most used elements of the browser interface (Location Bar, Back button, Search box and anything on the toolbar).
(In reply to comment #4)
> The problem with automatic removal is that it doesn't work with only 1 tab open
> from a particular site! The automation might also sometimes get it wrong, and
> causes websites to be displayed differently as to how they were designed.
> 
> Therefore I think a useful solution is to always display the full title in the
> Location Bar as follows:
> https://wiki.mozilla.org/Talk:Firefox/4.0_Windows_Theme_Mockups#Page_Title_in_the_Location_Bar.3F
> 
> Combine this with this proposal to auto-trim the title in the tab and you get
> around the oddity that I described of it "being weird seeing the title twice".
> 
> You also get a Fitts' Law benefit of increasing the size of the most used
> elements of the browser interface (Location Bar, Back button, Search box and
> anything on the toolbar).

You're right, it's not much use if there is only a single tab. But at some point webmasters have to take responsibility for userability. Also, the idea of a double-height location bar and thus buttons negates all the other space saving measures.
> But at some
> point webmasters have to take responsibility for userability.
 
I don't think you can place the responsibility on webmasters for bad browser design. For example, the title of this page! (:P) : "Bug 583890 - When the full page title is not shown remove redundant text in tabs" won't exactly be helped in any way by this proposal. A lot of page titles are auto-generated based upon database content. Another point is that with a lot of tabs open even short titles will be obscured.

Biju's proposal (to have a minimum tab width for the active tab) solves this problem wonderfully. As does displaying the full title somewhere else in the browser. 

> Also, the idea of
> a double-height location bar and thus buttons negates all the other space
> saving measures.

The mock-up that I created isn't double-height!! Currently there is a lot of wasted space in padding above and below the location bar (due to the large Back button). If you read the detail below the mock-up it points out that the toolbar + tab bar height is actually exactly the same height as the toolbar + tab bar height in earlier FF4 betas!
> > Also, the idea of
> > a double-height location bar and thus buttons negates all the other space
> > saving measures.
> 
> The mock-up that I created isn't double-height!!

I just measured it to double-check:
Tab Bar + Toolbar height in beta 6 = 61px height.
Tab Bar + Toolbar height in my mock-up = 61px height.

Exactly the same!

My only concern is that a creative way would need to be found to merge in the new "identity block" as well.
The combined height in the mock-up is equal only because you made the text (font) so much smaller. If you had such a small text in every place, the beta 6 design would be much smaller.

A design should consider taking equal space only if text size are equal.

How about getting rid of google search box and using awesome bar to do awesome things (such as allowing both URL input and search query input, Google chrome works this way already)?
(In reply to comment #8)
> The combined height in the mock-up is equal only because you made the text
> (font) so much smaller. If you had such a small text in every place, the beta 6
> design would be much smaller.

For convenience I made that mock-up by copying and pasting one of the awesomebar history items - the size of which was 9pt Segoe UI. I do apologize for making the URL text 9pt instead of the necessary 9.15pt :/ - not exactly what I'd call "so much smaller". Anyway, attached is the correction and still 61px!
Can we request blocking on this? It's been assigned to someone after all, so should definitely be on the list of things to make 4.0 final. I've also filed bug 610543 and bug 610547 to help deal with UX and Accessibility issues regarding the lack of full page title.
Whiteboard: [parity-safari] → [parity-safari][target-betaN]
Unassigning self until Firefox 4 is shipped, unless it becomes a blocker. Community contributors can of course pick this up.
Assignee: fryn → nobody
Status: ASSIGNED → NEW
Doing this simple fix would make Panorama a lot more useful (since there's a lot of title cropping there), is this something that you'd be willing to take a quick look at, Ian?
(In reply to comment #12)
> Doing this simple fix would make Panorama a lot more useful (since there's a
> lot of title cropping there), is this something that you'd be willing to take a
> quick look at, Ian?

This doesn't seem trivial to me... the idea is you would go through all open tabs and remove any substrings that are shared between two tabs? Seems like there would be all sorts of unintended consequences we'd have to hunt down. Also, we'd need to make sure to re-run the check every time a tab is added/removed/retitled and make sure that all works properly.
(In reply to comment #12)
> Doing this simple fix would make Panorama a lot more useful (since there's a
> lot of title cropping there).

Yeah, I must also say that this isn't a simple fix. We'd be repeatedly parsing the tab titles and building a tree or some structure to tokenize words within each title.
This should only concern visible tabs. Which is less than 20 strings.
This wouldn't be such an issue if sites would all title "page - site name", however they don't and as such, this is incredibly important. are the titles of all pages in a window not cached already, given that they're both in aero peek and the tab list?
Quick overview:
1. Partition visibleTabs by domain, host, or URI scheme (whichever is best).  Excludes pinned tabs and those that have no title.  
2. Sort bins and match for parts of the labels to be chopped.  
3. Apply chops. Zero chops are included, as some tabs may have lost collisions (due to tab removal, neighbor tabs having their titles changed, etc.).  

Possible issues with the patch as I see it, feedback appreciated:

1. This patch filters the labels all of visibleTabs, not just the ones that are actually visible.  
2. This patch filters all labels, even if they aren't currently cropped.  I wasn't sure how to determine if cropping was actually occurring.  
3. This patch treats the selected tab as the others; I think this is preferable, as changing labels when tabs are selected might confuse the user.  
4. I've not done work with RTL UI directly, so I'm not sure if I'm handling it correctly.  I just reverse the strings and treat them like LTR until the very end, when I reverse them back.  
5. Haven't done anything for weird cases like a site that uses the document title as a marquee (continuously updating it).  
6. Haven't done anything for labels that aren't space-delimited (eg, file paths).  
7. Haven't done anything to prevent a label from being chopped before a non-space delimiter (eg, "- Happy Fun Ball - Toy Store").  The issue here is simply which delimiters to include (-,|,: would seem to be a start, but not sure whether there are some for other locales, etc. that need to be considered too).  
8. Whether a pre-firing companion event ("TabsWillRelabel"?) is needed for the "TabsRelabeled" event (which fires on completion).
9. If more URI schemes need to be covered (currently only about, chrome, and file).

Also, I'm still new to mochitests.  There are some tests written, but I'm doubtful they give the coverage desired.  More will likely be needed as the details are finalized.
Attachment #512937 - Flags: feedback?(dao)
In bug 645274, Brian mentioned Google Chrome has a method of doing it. Chromium is open source. Why can't Mozilla just copy their code?
(In reply to comment #19)
> In bug 645274, Brian mentioned Google Chrome has a method of doing it. Chromium
> is open source. Why can't Mozilla just copy their code?

Patches are welcome.
(In reply to comment #19)
> In bug 645274, Brian mentioned Google Chrome has a method of doing it. Chromium
> is open source. Why can't Mozilla just copy their code?

Have you ever aspired to do better than someone else?
Comment on attachment 512937 [details] [diff] [review]
WIP: Initial attempt at a patch, with a few basic test cases.

Thanks for making a patch! :)

> +          for (domain in domainSets) {

This should be the following to avoid clobbering global variables:
for (let domain in domainSets) {

I'm surprised that RTL requires reversing everything. I thought that even though RTL appears right-to-left, the characters in the string are still indexed just like LTR strings, i.e. str[0] always returns the first character  (the leftmost character if the string is LTR or the rightmost if it's RTL). Therefore, there should be no need to reverse the strings before comparing/sorting, but I could be wrong. Ehsan might want to take a look at that part.

I only skimmed the patch briefly. I'll try it out and take a closer look when I have time. Again, thanks for working on this!
(In reply to comment #22)
> I'm surprised that RTL requires reversing everything. I thought that even
> though RTL appears right-to-left, the characters in the string are still
> indexed just like LTR strings, i.e. str[0] always returns the first character 
> (the leftmost character if the string is LTR or the rightmost if it's RTL).
> Therefore, there should be no need to reverse the strings before
> comparing/sorting, but I could be wrong. Ehsan might want to take a look at
> that part.

The RTL handling done in this patch doesn't seem right for two reasons:

1. A tab's direction property isn't representative of the directionality of the text displayed in the tab title.  It maps to the direction set on the tab element, which is only RTL in RTL builds (I think).

2. RTL text is stored in logical order, which means that the first logcial character is at offset 0 of the string, so on.  Therefore, I don't think that you need any special RTL handling here at all.  That said, feel free to ask me for review for RTL compatibility when you're done, and I'll take a look at your code and will also test it myself too.
Blocks: 651822
No longer blocks: 651822
Comment on attachment 512937 [details] [diff] [review]
WIP: Initial attempt at a patch, with a few basic test cases.

feedback- for RTL issues.
Attachment #512937 - Flags: feedback-
This version of the patch adds integration with Panorama, keeping the titles consistent with the regular browser view.

It adds a preference, browser.tabs.cropRedundancyInTitles, to allow for turning the feature off.

It removes the RTL handling, as it wasn't needed and wouldn't work correctly.

It also adds a rudimentary mechanism for throttling retitling to prevent heavy load scenarios (large number of tabs with frequently changing titles).

Finally, it splits up the code into several methods for improved readability/maintenance as well as to make it easier to integrate with Panorama.
Attachment #512937 - Attachment is obsolete: true
Attachment #512937 - Flags: feedback?(dao)
Attached patch WIP v2 (unrotted) (obsolete) — Splinter Review
Attachment #531122 - Attachment is obsolete: true
Assignee: nobody → unusualtears
Comment on attachment 545428 [details] [diff] [review]
WIP v2 (unrotted)

I noticed that if you set browser.sessionstore.max_concurrent_tabs to 0, titles aren't cropped. This is because contentTitle is empty for tabs that aren't loaded yet. To solve this, each tab needs a copy of the original uncropped label that you'd use instead of contentTitle. We should also use that uncropped label instead of the label attribute in createTooltip.

By the way, some of your methods expect a "aTabbrowser" attribute; they should just get it via this.tabbrowser.
Thanks, Dão, I've included your suggestions in the patch. 

It keeps a copy of the URL (in addition to the original title) from the restore data.  This is needed because switch-to-tab restores the URL before the tab has been restored, but reverts it to about:blank when the restore is occurring.  That could cause a tab to be titled incorrectly (it would be matched within the "about" scheme/domain set).

Removes the RTL tests, as they were based on the earlier, erroneous RTL handling.  

Also, changes setTabTitle() to directly invoke the retitling system (if it's enabled).  This should reduce title flickering — otherwise, a tab could receive a new title and immediately have it replaced with the shortened version.
Attachment #545428 - Attachment is obsolete: true
I pushed this patch to the try server. Builds should appear here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dgottwald@mozilla.com-e7b198169bd3
I noticed that this patch makes app tabs glow when they shouldn't.
Thanks.  This should fix that.

When a pinned tab would refresh, it's possible that its label is not in a state consistent with its DOM title, and it would appear the title had been changed.  This updates the check to use _originalTitle, so that won't happen.

This also makes the tab list menu (#alltabs-popup) use the _originalTitle for app tabs; they will continue using the possibly-chopped label for non-app tabs, for consistent names between the tabs and the tab list menu.
Attachment #546667 - Attachment is obsolete: true
(In reply to comment #31)
> When a pinned tab would refresh, it's possible that its label is not in a
> state consistent with its DOM title, and it would appear the title had been
> changed.  This updates the check to use _originalTitle, so that won't happen.

Seems like pinned tabs should be excluded completely from the start, since their title isn't displayed.
This version of the patch strengthens their exclusion a bit, but that change was necessary for non-app tabs as well.

The code path that was causing app tabs to highlight: 

1. DOMTitleChanged event handler invoked.
2. It called setTabTitle() for the tab to determine if the title really changed.
3. setTabTitle() would return true if the title changed (based on the tab label).
4. If the title changed, the event handler would set the 'titlechanged' attribute to true, and a CSS rule would make the app tab glow.

The previous version of the patch changed [3] to use the _originalTitle instead of the label.  That change ensures correct behavior for app tabs and regular ones alike.

This patch refines the app tab exclusion in setTabTitle().  They won't cause adjustTabTitles() to be invoked.  They will have their label and _originalTitle updated and return whether or not their title changed.  They need to keep the label set, so if they are unpinned and _cropRedundancyInTitles is false, they will have the correct label.

That also allows removing the change from the previous patch for setting the menuitem labels differently, since both app tabs and non-app tabs will now have the label as needed for the menuitems.

Additionally, it balks on adjustTabTitles() if invoked as an event handler for TabAttrModified event with the target being a app tab.  This prevents the other tabs from being retitled if the change was to an app tab.
Attachment #547487 - Attachment is obsolete: true
Tabs that aren't loaded cannot be determined to be directory listings, meaning that when viewed only in Tab View they will use space as their word break.

Detection of directory listings itself is clunky, relying on checking whether a text/html tab has the right style applied to it.

This also fixes a problem where, if cropping was turned off, tabs could get stuck with the 'Connecting...' label.
Attachment #547793 - Attachment is obsolete: true
Does this new patch need to have a reviewer added? :)
Whiteboard: [parity-safari][target-betaN] → [parity-safari]
(and feel free to land this on the UX branch if it needs testing!)
This version does away with relying on the style to determine a page is a directory listing.  

If the title could be a URI, it crops by '/'.  Otherwise, it crops on spaces.  If it determines the remainder of title could be a URI it will crop on '/'.  

For example, the W3C Validator output has titles of the form "[Valid] Markup Validation of http://www.example.com/ - W3C Markup Validator".  In that case, it finds a crop at the URI, and then will continue from there using '/'.
Attachment #565683 - Attachment is obsolete: true
Attachment #566421 - Flags: review?(dao)
Here's instructions on how to land it on the UX branch in the meantime, if you want testing on a wider audience: https://wiki.mozilla.org/UX_Branch
I'll push the patch to try and if all tests pass then I'll push it to the UX branch.
Status: NEW → ASSIGNED
A couple tests failed when I pushed to try. I'll hold off on pushing to the UX branch until the tests are fixed.

Here is a sampling of some of the tests that failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly two tokens - Got Foo - Bar - Baz, expected Bar - Baz
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly two tokens - Got New Tab, expected Baz - Baz
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly two tokens - Got New Tab, expected Baz - Baz
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly four tokens - Got New Tab, expected Baz
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly two tokens - Got New Tab, expected Baz - Baz
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_search.js | Match something when the whole title exists - Got 0, expected 1
Attached patch Fixes broken tests. (obsolete) — Splinter Review
Thanks for getting broader test results.  

Not clear why the tests for this bug failed on OS X64 debug.  Most (including the first "oth" set for OS X64 debug) had two failures:  

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug627288.js | tab label is up-to-date - Got mochitest index /, expected mochitest index /browser/
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_search.js | Match something when the whole title exists - Got 0, expected 1

The first was caused because the TabAttrModified event was hitting it too early.  Switching that particular test to use the TabsRelabeled event fixed it.  

The second was caused by trying to search the full title against a cropped version.  The route I went was to keep a copy of the full title around, so that searches happen against the full title rather than the chopped version.  

Also, a minor adjustment to the tests for this bug, but if they still fail for OS X64 debug, I'll revisit them in the next revision of this patch.
Attachment #566421 - Attachment is obsolete: true
Attachment #566421 - Flags: review?(dao)
Pushed to try again:
https://tbpl.mozilla.org/?tree=Try&rev=c96a2f5a6d27

Did you cancel review intentionally?
Comment on attachment 567943 [details] [diff] [review]
Fixes broken tests.

Sorry, didn't mean to cancel the review request.
Attachment #567943 - Flags: review?(dao)
Hey Adam, thanks for fixing those tests so quickly. There still appears to be quite a bit of failures in the Win Debug, XP Win Debug, and other platforms. Can you take a look at those test failures and see if they are caused by your patch?

Here is a link to the relevant try server build:
https://tbpl.mozilla.org/?tree=Try&rev=c96a2f5a6d27
Attached patch Fixup tests, again. (obsolete) — Splinter Review
Most of the failures were cascading from the initial test's failure and subsequent timeout.  

This version of the patch reverts to using the TabAttrModified event in the test for bug 627288 and adds a timeout to make that work for both regular and debug builds.  This bug's tests have been restructured a bit and a timeout is added for them that should fix them for OS X64 debug.
Attachment #567943 - Attachment is obsolete: true
Attachment #568251 - Flags: review?(dao)
Attachment #567943 - Flags: review?(dao)
The main change here was to unbitrot, but I took another look at the two tests that used setTimeout() in the last patch, and I was able to find better ways to get them to pass locally on debug builds.
Attachment #568251 - Attachment is obsolete: true
Attachment #570881 - Flags: review?(dao)
Attachment #568251 - Flags: review?(dao)
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=659f815bb24e

If the tests pass then I will push this to the UX branch.
Thanks, almost there! I see these two test failures on the WinXP debug build. Can you reproduce them locally?

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly two tokens - Got Foo - Bar - Baz, expected Bar - Baz
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Should remove exactly two tokens - Got New Tab, expected Baz - Baz
Try run for 659f815bb24e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=659f815bb24e
Results (out of 131 total builds):
    success: 125
    warnings: 6
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-659f815bb24e
Thanks Jared.

I can't recreate those failures locally with that version of the test (though the earlier version I could).  I've reworked the test again, as segregating the label events completely should completely rule out those failures.
Attachment #570881 - Attachment is obsolete: true
Attachment #571182 - Flags: review?(dao)
Attachment #570881 - Flags: review?(dao)
Try run for 534cf54d1f2e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=534cf54d1f2e
Results (out of 49 total builds):
    exception: 3
    success: 43
    warnings: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jwein@mozilla.com-534cf54d1f2e
This has been pushed to the UX branch and should appear in tomorrows nightly build:
https://hg.mozilla.org/projects/ux/rev/71eb8ce0979e
Attached image Screenshot of titles
I've been implementing multitouch. Today I opened:

https://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html
and
http://www.w3.org/TR/touch-events/

which results in two tabs labeled 1 and 2.
Yeah, this is definitely not working correctly. I think it should also be based on domain/subdomain. "Google" was removed from Google Reader and Google Voice. Google+ does not have a title on the tab at all.
Hate to double post, but Google doesn't use subdomains--my apologies. I guess there would have to be another workaround for issues like this.
Comment on attachment 571182 [details] [diff] [review]
Test-fixing: Segregate the naming events completely to avoid failures

Need to iron out the behavior before doing a code review here.
Attachment #571182 - Flags: review?(dao)
Keywords: uiwanted
(In reply to Jacob Saxon (J.Sax) from comment #55)
> Created attachment 572206 [details]
> "Google" removed from each tab on different sites
> 
> Yeah, this is definitely not working correctly. I think it should also be
> based on domain/subdomain. "Google" was removed from Google Reader and
> Google Voice. Google+ does not have a title on the tab at all.

The right thing is being done by the patch. However it appears to be over-active. It's performing it's task on each and every tab rather than only on tabs where the title is truncated.
This patch fixes the Google+ tab not having a label (Comment 55).  This will make it not get chopped at all in that case (so it would have "Google+" for the tab title).

It also adds a constraint on the minimum remaining length (at least 4 chars) to prevent short tab titles (like the w3.org example (Comment 54)).

In the case of only removing redundancy when a label already crops, the hard part will probably be exposing that information from the layout (seems to be nsTextBoxFrame).  I'm not very familiar with the layout code, so not sure how cleanly it can be done.  It will probably also require a mock label which can be tested with to see if a given string crops (to determine how much to leave).

Note that going that route means the labels could change based on tab width (window width) and font (both size and face).
Attachment #571182 - Attachment is obsolete: true
This should not hide parts of words from titles. I had a Bug xyz open in one tab and in the other tab where i was searching, it showed "illa is ponder..." , hiding "Bug" from "Bugzilla".
(In reply to Adam [:hobophobe] from comment #59)
> Created attachment 573920 [details] [diff] [review] [diff] [details] [review]
> Fix error that would allow similar words to be treated the same (eg, 'Google
> ' and 'Google+'); add constraint for minimum remaining after chopping.

I have pushed this new version of the patch to the UX branch. It should show up in tomorrow's nightly build.
After using this for a few weeks, I have a few observations:

1. After redundant text is removed, sometimes a tab title is left to start with a delimiter. For example, open these two links:

http://www.metalstorm.net/bands/biography.php?band_id=87
http://www.metalstorm.net/pub/interview.php?interview_id=620

The tabs are titled:

Devin Townsend - Biography - Metal Storm
Devin Townsend interview (11/2011) - Metal Storm

These are reduced to:

- Biography - Metal Storm
interview (11/2011) - Metal Storm

2. If a website adds its name to the end of a title, removal of redundant beginning doesn't seem to always help. If I have the same two Metal Storm pages (from #1) open for more than one artist, I would get identical tab titles. Maybe the removal of redundant text should not be applied for tabs that also have redundant text in the end of the title (after a delimiter)?
Thanks Vykintas.  This patch fixes the "- Biography[...]" bug.  

The second case, where tabs with different titles before can be the same after chopping, it seems like a fairly even tradeoff.  Not sure which is best.

This patch also adds a direct test on _getChopsForSet(), which most of the other tests will be converted to (a few that will still test the full behavior).
Attachment #573920 - Attachment is obsolete: true
(In reply to Adam [:hobophobe] from comment #63)
> Created attachment 580558 [details] [diff] [review]
> Fixes bug, add direct test on core method.

Pushed to the UX branch: https://hg.mozilla.org/projects/ux/rev/71cac9c8d4e8

This new version should show up in tomorrow's nightly build. I can't wait! :)
This caused a11y (accessibility) test failures.
See here: https://tbpl.mozilla.org/?tree=UX&rev=71cac9c8d4e8

I backed it out due to those failures, since the last time that we became uncertain about the cause of a11y test failures, I had to revert to mozilla-central's state and re-land everything, and I want to avoid that.

Backout changeset:
https://hg.mozilla.org/projects/ux/rev/b244d6276f25
Fixes the problem the patch was causing for accessibility tests.

Also converted more of this bug's tests to use direct tests.
Attachment #580558 - Attachment is obsolete: true
(In reply to Adam [:hobophobe] from comment #66)
> Created attachment 581414 [details] [diff] [review]
> Change to not break a11y tests, convert more tests to the direct form.

Pushed to the UX branch: https://hg.mozilla.org/projects/ux/rev/70645995784e
(In reply to Adam [:hobophobe] from comment #66)
> Created attachment 581414 [details] [diff] [review]

This is working really great and feels so close. I'm not sure how hard it would be, but I'd like to request one small tweak.

I have two tabs open with the following titles:
Bug 583890 - When the full page title is not shown remove redundant...
Bug 419231 - Floating scrollbar in the customize toolbar dialog...
Bug List

In this scenario, the two bug pages have the "Bug" prefix removed which is great, but this also removes the "Bug" prefix from the "Bug List" title, which didn't need to be removed.

Adam: Based on your experience thus far, would a fix for that be very complex?
(In reply to Jared Wein [:jwein and :jaws] from comment #68)
> I have two tabs open with the following titles:
> Bug 583890 - When the full page title is not shown remove redundant...
> Bug 419231 - Floating scrollbar in the customize toolbar dialog...
> Bug List
> 
> In this scenario, the two bug pages have the "Bug" prefix removed which is
> great, but this also removes the "Bug" prefix from the "Bug List" title,
> which didn't need to be removed.

Could you describe algorithmically what you would like?
Is it because Bug List fits without requiring an ellipsis or because Bug List is dissimilar?

(If it's the former, unfortunately <xul:label/> doesn't expose any property or event that indicates whether the text is overflowing or being cropped/clipped/truncated, which has made fixing other bugs like bug 653670 more difficult.)
(In reply to Frank Yan (:fryn) from comment #69)
> (In reply to Jared Wein [:jwein and :jaws] from comment #68)
> > I have two tabs open with the following titles:
> > Bug 583890 - When the full page title is not shown remove redundant...
> > Bug 419231 - Floating scrollbar in the customize toolbar dialog...
> > Bug List
> > 
> > In this scenario, the two bug pages have the "Bug" prefix removed which is
> > great, but this also removes the "Bug" prefix from the "Bug List" title,
> > which didn't need to be removed.
> 
> Could you describe algorithmically what you would like?
> Is it because Bug List fits without requiring an ellipsis or because Bug
> List is dissimilar?

Because "Bug List" fits without requiring an ellipsis.
 
> (If it's the former, unfortunately <xul:label/> doesn't expose any property
> or event that indicates whether the text is overflowing or being
> cropped/clipped/truncated, which has made fixing other bugs like bug 653670
> more difficult.)

Yeah, I figured that this was probably impossible with our current toolkit code. At this point, it's probably just good to make not of it as a known-issue/follow-up bug.
Adam: Do you think you could implement a patch that only removes redundant text if it is succeeded by a phrase separator?

For example, given these tab titles:
CNN.com - Headline news story #1
CNN.com - Headline news story #2
Lord of the Rings
Lord of the Flies

Results:
Headline news story #1
Headline news story #2
Lord of the Rings
Lord of the Flies

We would need to build up a list of these separators, and I'm not sure how to do it for all locales. We may be able to look for non-alphanumeric characters (again, a localization question).

In English, we could start with this set: {-, :, >, |}
There should be a pref to disable this feature. When you have two Facebook profiles open with the same first name, the name is hidden. This feels weird. Also I don't see why should the first name be hidden because the entire name would easily fit into the 100px minimum tab width.

Another issue I see is when the tab is dragged to the bookmarks toolbar, only the visible part of the name appears as the bookmark label. Please fix this.
This fixes DND bookmark creation.  It will now use the full title for bookmarks when dragging a chopped tab to create a bookmark (thanks, sdrocking).  It also fixes tabitem titles in tabview/Panorama, and will now use the full titles there too (shows up as tooltips upon hover).

The main focus of this revision is using separators (current regex is /\s[-:>\|]+\s/) for finding chop spots for non-URIs.  This will currently match separators like " - ", " :: ", and even " -|- " and " >:||->: ".  If this is overbroad, a more constrained matching can be devised.

Having Unicode character classes in RegExp (bug 258974) would allow use of \p{P} (short for \p{Punctuation}), which would separate the concern for selecting punctuation that can be separators.  

Also note that depending on the punctuation considered separators, things like mathematical expressions can be shortened (eg, "3 - 4 = -1" >> "4 = -1").

sdrocking: There is a preference for disabling this: browser.tabs.cropRedundancyInTitles.
Attachment #581414 - Attachment is obsolete: true
(In reply to Adam [:hobophobe] from comment #73)
> sdrocking: There is a preference for disabling this:
> browser.tabs.cropRedundancyInTitles.

Thanks for this, but a UI pref would be useful if this feature would be enabled by default.
(In reply to Adam [:hobophobe] from comment #73)
> Created attachment 584841 [details] [diff] [review]
> Switch to separator-based chops, fix a few bugs.

Pushed to UX branch: https://hg.mozilla.org/projects/ux/rev/3ff3854190ff
When the end of a title is shortened we add an ellipsis (“…”) to indicate that some of the title has been removed. We should also add an ellipsis here, in place of the removed text (and retain a separating space).
I noticed a little error caused by this patch. While the tab is connecting the tooltip shows "undefined". This is pretty easy to reproduce, just Strg/Cmd+Click on the link to open it in a new tab:

http://15.15.15.15/
Fixes the issue with undefined tooltips (thanks Tim).  Now this falls back on the tab label for setting the tooltip, as the _originalTitle isn't always set for connecting tabs.

Should a leading ellipsis be added?  This patch adds it, but turned off by default.  It can be turned on through the preference "browser.tabs.addEllipsisToCropped" in about:config.

Finally, this patch fixed a small bug where data: URIs wouldn't get titled when cropping was active.
Attachment #584841 - Attachment is obsolete: true
(In reply to Adam [:hobophobe] from comment #79)
> Created attachment 589017 [details] [diff] [review]

My apologies for not doing this sooner. I have just pushed this new version to the UX branch: https://hg.mozilla.org/projects/ux/rev/b76ea2923998
Comment on attachment 589017 [details] [diff] [review]
Fix undefined tooltip when connecting, adds optional leading ellipsis, fixes data: titles.

Can we get a UI review on this to see if we should get this patch reviewed? It has been on the UX branch now for quite a while.
Attachment #589017 - Flags: ui-review?(ux-review)
I tried getting this to kick in on the UX branch, but couldn't make it work. Are we sure it's still active on the UX branch?
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #82)
> I tried getting this to kick in on the UX branch, but couldn't make it work.
> Are we sure it's still active on the UX branch?

Yes, yesterday it was. Here are some URLs, where it really makes a difference for me (so much, that I wish this were available in extension form!):
http://www.heise.de/
http://www.heise.de/newsticker/meldung/Flache-Notebooks-von-Acer-von-11-6-bis-15-6-Zoll-1464498.html
http://www.heise.de/newsticker/meldung/Thunderbolt-auf-der-CeBIT-1465032.html
Comment on attachment 589017 [details] [diff] [review]
Fix undefined tooltip when connecting, adds optional leading ellipsis, fixes data: titles.

Looks good to me.
Attachment #589017 - Flags: ui-review?(ux-review) → ui-review+
Attachment #589017 - Flags: review?(dietrich)
Comment on attachment 589017 [details] [diff] [review]
Fix undefined tooltip when connecting, adds optional leading ellipsis, fixes data: titles.

adding tim for tabview code review.
Attachment #589017 - Flags: review?(ttaubert)
Comment on attachment 589017 [details] [diff] [review]
Fix undefined tooltip when connecting, adds optional leading ellipsis, fixes data: titles.

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

::: browser/base/content/tabbrowser.xml
@@ +2944,5 @@
> +          if (aEvent && (aEvent.target.pinned ||
> +              tabbrowser.visibleTabs.indexOf(aEvent.target) < 0)) {
> +            return;
> +          }
> +          else if (!this._cropRedundancyInTitles) {

} else if (...) {
Thanks Jared.
Attachment #589017 - Attachment is obsolete: true
Attachment #609950 - Flags: review?(ttaubert)
Attachment #609950 - Flags: review?(dietrich)
Attachment #589017 - Flags: review?(ttaubert)
Attachment #589017 - Flags: review?(dietrich)
Comment on attachment 609950 [details] [diff] [review]
Unbitrot patch and switch to butterfly-style elses for the files that use them.

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

Adam, can you please un-bitrot the patch? I tried to apply it but there were recent changes to tabitems.js which prevented that.

Additionally, this code seems to clutter tabbrowser.xml even more. I think it might make sense to move certain parts of this patch to a JSM that would take care of removing common prefixes and maybe even setting those labels.

::: browser/base/content/tabbrowser.xml
@@ +3021,5 @@
> +          }
> +          this._lastTitleAdjustment = Date.now();
> +          event = document.createEvent("Events");
> +          event.initEvent("TabsRelabeled", true, false);
> +          this.dispatchEvent(event);

Looks like this event's sole purpose is to write a test for this feature. Do we really need this event? We should rather have a kind of hacky test than including test code in the product, even if it's just some events that are fired.

::: browser/components/tabview/test/browser_tabview_bug595560.js
@@ +47,5 @@
>      let hasFocus = doc.hasFocus() && doc.activeElement == searchBox[0];
>      ok(hasFocus, "The search box has focus");
>  
>      let tab = win.gBrowser.tabs[1];
> +    searchBox.val(tab._tabViewTabItem._originalTitle || tab._tabViewTabItem.$tabTitle[0].textContent);

"tab" is the <xul:tab> in this context. Shouldn't _originalTitle be a property of it?

::: browser/components/tabview/test/browser_tabview_bug627288.js
@@ +80,5 @@
>  }
>  
>  // ----------
> +function whenTabsWillRelabel(tab, callback) {
> +  let onModified = function (event) {

Nit: function onModified(event) {

@@ +88,5 @@
> +    // is executed before the test logic.
> +    executeSoon(callback);
> +  }
> +
> +  tab.parentNode.addEventListener("TabsWillRelabel", onModified, false);

Btw, did you introduce this whole event just for this test? I didn't see it used anywhere else... Can't we just use onTabAttrModified?
Attachment #609950 - Flags: review?(ttaubert)
the event may be use by extensions
Thanks Tim.  This follows your suggestion with a JavaScript module (browser/modules/TabTitleAdjuster.jsm).  

It also removes the feature-specific events in favor of "TabAttrModified" and updates the tests.  They were originally added speculatively, and the only place they had immediate use was in the tests, but the reasoning was:

1. They could be used by extensions
2. They were tab-wide, instead of per-tab (only important for the "TabsRelabeled"), so a listener would know all tab labels were in their new state
3. At the time I wasn't sure how the patch would develop (the "TabsRelabeled" event was in the very first patch) and thought it might be needed later (YAGNI wins again)

Finally, cleans up the changes to tabview a bit, to always rely on the tab._originalTitle and not try to keep a copy on tabitems.
Attachment #609950 - Attachment is obsolete: true
Attachment #610789 - Flags: review?(ttaubert)
Attachment #610789 - Flags: review?(dietrich)
Attachment #609950 - Flags: review?(dietrich)
Comment on attachment 610789 [details] [diff] [review]
Unbitrot, move bulk of feature out of tabbrowser.xml and into separate JS module.

Looks like you forgot to 'hg add' the new JSM file.
Attached patch Include the new JS module (obsolete) — Splinter Review
Attachment #610789 - Attachment is obsolete: true
Attachment #610789 - Flags: review?(ttaubert)
Attachment #610789 - Flags: review?(dietrich)
Attachment #611521 - Flags: review?(ttaubert)
Attachment #611521 - Flags: review?(dietrich)
Comment on attachment 611521 [details] [diff] [review]
Include the new JS module

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

Thank you for moving all the remove-common-prefixes functionality into a JSM. Looks much cleaner now! I didn't review all of the code yet but there are some bigger issues that can be addressed now. So I won't overwhelm you with feedback and we can make this feature happen iteratively.

::: browser/base/content/browser.js
@@ +196,5 @@
>    Cu.import("resource:///modules/devtools/Tilt.jsm", tmp);
>    return new tmp.Tilt(window);
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "TabTitleAdjuster", function() {

XPCOMUtils.defineLazyModuleGetter(this, "TabTitleAdjuster",
  "resource:///modules/TabTitleAdjuster.jsm");

::: browser/base/content/tabbrowser.xml
@@ +66,5 @@
>      </resources>
>  
>      <content>
>        <xul:stringbundle anonid="tbstringbundle" src="chrome://browser/locale/tabbrowser.properties"/>
> +      <xul:stringbundle anonid="intlstringbundle" src="chrome://global-platform/locale/intl.properties"/>

You're using this in the JSM, only so there's a better way to do it. In the JSM just define:

XPCOMUtils.defineLazyGetter(this, "intlEllipsis", function() {
  let bundle = Services.strings.
    createBundle("chrome://global-platform/locale/intl.properties");
  return bundle.getString("intl.ellipsis");
});

This way have a lazy getter that caches the value and you don't need to mess around with the document.

@@ +1106,5 @@
>                return false;
>  
> +            aTab._originalTitle = title;
> +            if (aTab.pinned || !this.tabContainer._cropRedundancyInTitles ||
> +                this.visibleTabs.indexOf(aTab) == -1) {

You can just use tab.hidden here.

@@ +3008,5 @@
> +      <method name="adjustTheseTabTitles">
> +        <parameter name="aTabsSets"/>
> +        <body><![CDATA[
> +          TabTitleAdjuster.container = this;
> +          TabTitleAdjuster.document = document;

That's definitely a no-go. You shouldn't inject values like this into a JSM. This creates leaks and there's a better way to do it.

If you want to get a tab's container just do: tab.ownerDocument.defaultView.gBrowser.tabContainer

If you want a tab's document do: tab.ownerDocument

::: browser/components/places/content/controller.js
@@ +1526,5 @@
>        else if (data instanceof XULElement && data.localName == "tab" &&
>                 data.ownerDocument.defaultView instanceof ChromeWindow) {
>          let uri = data.linkedBrowser.currentURI;
>          let spec = uri ? uri.spec : "about:blank";
> +        let title = data._originalTitle ? data._originalTitle : data.label;

let title = data._originalTitle || data.label;

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2990,5 @@
>  
>        // If the page has a title, set it.
>        if (activePageData) {
>          if (activePageData.title) {
>            tab.label = activePageData.title;

This looks to me as if unrestored tabs would always show the full tab label. We should remove common prefixes after all tabs have been created and initialized.

@@ +3000,3 @@
>            tab.crop = "center";
>          }
> +        tab._originalURL = activePageData.url;

Do we really need this? Line 2989 above sets the docShell's currentURI which should be sufficient for our needs, right?

::: browser/components/tabview/search.js
@@ +74,5 @@
>      // because we have to deal with both tabs represented inside
>      // of active Panoramas as well as for windows in which
>      // Panorama has yet to be activated. We uses object sniffing to
>      // determine the type of tab and then returns its name.     
> +    if (tab._originalTitle != undefined) {

if ("_originalTitle" in tab) {

@@ +76,5 @@
>      // Panorama has yet to be activated. We uses object sniffing to
>      // determine the type of tab and then returns its name.     
> +    if (tab._originalTitle != undefined) {
> +      return tab._originalTitle;
> +    } else if (tab.tab) {

} else if ("tab" in tab) {

@@ +77,5 @@
>      // determine the type of tab and then returns its name.     
> +    if (tab._originalTitle != undefined) {
> +      return tab._originalTitle;
> +    } else if (tab.tab) {
> +      if (tab.tab._originalTitle != undefined)

if ("_originalTitle" in tab.tab) {

@@ +82,5 @@
> +        return tab.tab._originalTitle;
> +      else
> +        return tab.$tabTitle[0].textContent;
> +    } else {
> +      return tab.linkedBrowser.contentTitle;

Can't we use tab.label here?

::: browser/components/tabview/tabitems.js
@@ +209,5 @@
> +
> +    let tipTitle = "";
> +
> +    if ("_originalTitle" in this.tab)
> +      tipTitle = this.tab._originalTitle + "\n" + url;

Shouldn't ss.getTabState() return the full title? Looks like we don't need this here?

::: browser/components/tabview/test/browser_tabview_bug627288.js
@@ +38,5 @@
>      });
>    }
>  
>    let testChangeUrlAfterReconnect = function () {
> +    // Wait for the third TabAttrModified event

Instead of waiting for the third TabAttrModified event - which seems rather flaky to me - we should check if $label is what we expect it to be, and if so remove the listener and call testReconnectWithNewUrl().

::: browser/modules/TabTitleAdjuster.jsm
@@ +8,5 @@
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +
> +let TabTitleAdjuster = {

I'm not really happy about the JSM's name. TabTitleAdjuster is a bit too generic and doesn't really tell what it's supposed to do.

@@ +9,5 @@
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +
> +let TabTitleAdjuster = {
> +  adjustTheseTabs: function TabTitleAdjuster_adjustTheseTabs(aTabSets) {

The name of the function is a bit misleading as well. It doesn't adjust tabs, it removes common prefixes from tab labels.

@@ +46,5 @@
> +
> +  _getDomainSets: function TabTitleAdjuster_getDomainSets(aTabs) {
> +    let domainSets = {};
> +    let eTLDSVC = Cc["@mozilla.org/network/effective-tld-service;1"].getService(
> +      Ci.nsIEffectiveTLDService);

You should use XPCOM.defineLazyServiceGetter() at the top of the file and call it something like "gETLDService".

@@ +62,5 @@
> +          browserURI = this.makeURI(tab._originalURL);
> +        }
> +        catch (ex) {}
> +      }
> +      let tabDomain = "unknown";

Using an arbitrary string to check whether we have a domain name or not is rather uncommon. You could just leave it undefined and check with |if (domain)| afterwards.

@@ +96,5 @@
> +    let separators = /\s[-:>\|]+\s/;
> +    let pathMode = false;
> +    function titleIsURI(aTitle) {
> +      try {
> +        TabTitleAdjuster.makeURI(aTitle);

There should be a better way to check if the given title is a URI. Creating a nsURI instance and using try/catch is a very expensive way of doing that.

@@ +103,5 @@
> +      catch (e) {}
> +      return false;
> +    }
> +    // indexOfSep returns the end index of the match
> +    function indexOfSep(aStr, aStart) {

All these separate functions should be defined as ._indexOfSep(), ._findFrontChop(), etc.

@@ +267,5 @@
> +    let caUtils = {};
> +    let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +                       getService(Ci.mozIJSSubScriptLoader);
> +    scriptLoader.loadSubScript("chrome://global/content/contentAreaUtils.js",
> +                               caUtils);

You should make this a lazy getter or you'll load and parse contentAreaUtils.js every time you call TabTitleAdjuster.makeURI(). Actually, you don't need this at all because you could do:

tab.ownerDocument.defaultView.makeURI() or browser.ownerDocument.defaultView.makeURI()
Attachment #611521 - Flags: review?(ttaubert)
Attached patch Updates based on feedback (obsolete) — Splinter Review
Thanks Tim.  This should have everything as suggested, except for the following which may need further changes:

> TabTitleAdjuster is a bit too generic and doesn't really tell what it's
> supposed to do.

I went with TabTitleAbridger for this patch, but I'm open to suggestions.  I believe the Chromium implementation of this feature calls itself an elider?

> ::: browser/components/sessionstore/src/nsSessionStore.js
> @@ +2990,5 @@
> >  
> >        // If the page has a title, set it.
> >        if (activePageData) {
> >          if (activePageData.title) {
> >            tab.label = activePageData.title;
>
> This looks to me as if unrestored tabs would always show the full tab label.
> We should remove common prefixes after all tabs have been created and
> initialized.

The unrestored tabs do get put up for abridgment when the first tab loads, and any other time the abridgment occurs.

> ::: browser/components/tabview/search.js
> @@ +82,5 @@
> > +        return tab.tab._originalTitle;
> > +      else
> > +        return tab.$tabTitle[0].textContent;
> > +    } else {
> > +      return tab.linkedBrowser.contentTitle;

> Can't we use tab.label here?

The issue here is whether the search function should search only the label or the whole title.  My surmise was the user would want to see all possible matches, so I went with the full title rather than the abridged.  It can go the other way, but users may already get matches in the end of the title beyond what's visible, so we may want to stay with the full title?

> ::: browser/modules/TabTitleAdjuster.jsm
> @@ +96,5 @@
> > +    let separators = /\s[-:>\|]+\s/;
> > +    let pathMode = false;
> > +    function titleIsURI(aTitle) {
> > +      try {
> > +        TabTitleAdjuster.makeURI(aTitle);
>
> There should be a better way to check if the given title is a URI. Creating a
> nsURI instance and using try/catch is a very expensive way of doing that.

Alternatives are welcome.  This patch switches to using a regular expression, which simply identifies any path-like (including URI-like), space-free string.  I'm not sure if there's already a better way elsewhere (didn't find one so far).  The current expression can likely be improved upon if there's not a better way.
Attachment #611521 - Attachment is obsolete: true
Attachment #612081 - Flags: review?(ttaubert)
Attachment #612081 - Flags: review?(dietrich)
Attachment #611521 - Flags: review?(dietrich)
Comment on attachment 612081 [details] [diff] [review]
Updates based on feedback

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

Thanks Adam. Distributing review to qualified people for the browser and places bits.
Attachment #612081 - Flags: review?(mak77)
Attachment #612081 - Flags: review?(dietrich)
Attachment #612081 - Flags: review?(dao)
Comment on attachment 612081 [details] [diff] [review]
Updates based on feedback

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

the Places part is trivial, I will trust whoever reviews this finally.  The concern is that we are using a private property and fallback every time, so maybe we could extend the tab with a setter/getter to simplify that access.
some drive-by comments

::: browser/app/profile/firefox.js
@@ +412,5 @@
>  pref("browser.tabs.animate", true);
>  pref("browser.tabs.onTop", true);
>  pref("browser.tabs.drawInTitlebar", true);
> +pref("browser.tabs.cropRedundancyInTitles", true);
> +pref("browser.tabs.addEllipsisToCropped", false);

I'd suggest shorter names like: cropTitleRedundancy and cropWithEllipsis

::: browser/base/content/tabbrowser.xml
@@ +1099,5 @@
>                  title = this.mStringBundle.getString("tabs.emptyTabTitle");
>              }
>  
> +            if (aTab.label != this.mStringBundle.getString("tabs.connecting") &&
> +                "_originalTitle" in aTab && aTab._originalTitle == title &&

the most expensive check should always be the last one

@@ +2809,5 @@
>            tab.setAttribute("validate", "never");
>            tab.setAttribute("onerror", "this.removeAttribute('image');");
>            this.adjustTabstrip();
> +          this._lastTitleAbridgment = -1;
> +          this.addEventListener("TabAttrModified", this.abridgeTabTitles, false);

would be nice to remove this in the destructor

@@ +2965,5 @@
>              tabstripClosebutton.collapsed = this.mCloseButtons != 3;
>          ]]></body>
>        </method>
>  
> +      <method name="abridgeTabTitles">

all of this method could get some more comments explaining non-obvious things. Lots of conditions missing an explain on why they matter.

@@ +2971,5 @@
> +        <body><![CDATA[
> +          let tabbrowser = this.tabbrowser;
> +
> +          if (aEvent && (aEvent.target.pinned ||
> +              tabbrowser.visibleTabs.indexOf(aEvent.target) < 0)) {

I think tim suggested using .hidden

@@ +2978,5 @@
> +            if (this._lastTitleAbridgment != -1) {
> +              // Resets all tabs, not just visible ones.
> +              let tabs = tabbrowser.tabs
> +              for (let i = 0; i < tabs.length; i++) {
> +                let tab = tabs[i];

for (let tab of tabbrowser.tabs)

@@ +2991,5 @@
> +              // try again in 100ms unless the abridgment's happened since then.
> +              let oldAbridgmentTime = this._lastTitleAbridgment;
> +              let tabContainer = this;
> +              setTimeout(function() {
> +                // Protects from multiple timeouts cascading.

could you instead store the timeout id and clearTimeout it when a new one is generated?

::: browser/base/content/test/browser_bug583890.js
@@ +3,5 @@
> + */
> +
> +function getTabLabel(aTab) {
> +  return aTab.label;
> +}

this helper doesn't seem to bring any gain to code size, abstraction or clarity

@@ +5,5 @@
> +function getTabLabel(aTab) {
> +  return aTab.label;
> +}
> +
> +function waitForTabLabel(aTab, aCallback, aWaitForLabel) {

some javadoc above functions would allow understanding what they expect and what they'll do based on input.
for example aWaitForLabel sounds like a bool to me, maybe should be aExpectedLabel

@@ +20,5 @@
> +    aTab.addEventListener("TabAttrModified", callback, true);
> +}
> +
> +function addTabsAndWaitForRelabeled(
> +  aBaseTab, aCount, aCallback, aWaitForLabel, aAddedTabs) {

indent params like
function a(param, param
           param, param) {

@@ +141,5 @@
> +  ];
> +  for (let i = 0; i < testGroups.length; i++) {
> +    let rawInputSet = testGroups[i][0];
> +    let inputSet = [];
> +    for (let j = 0; j < rawInputSet.length; j++)

always brace loops, even onelines

::: browser/components/places/content/controller.js
@@ +1526,5 @@
>        else if (data instanceof XULElement && data.localName == "tab" &&
>                 data.ownerDocument.defaultView instanceof ChromeWindow) {
>          let uri = data.linkedBrowser.currentURI;
>          let spec = uri ? uri.spec : "about:blank";
> +        let title = data._originalTitle || data.label;

hm, it's a bit cumbersome that we have to access a private property... but I'll leave that to some more experienced tabbrowser peer to evaluate.  Maybe the tab could grow a getter/setter that will do the right thing.

::: browser/modules/TabTitleAbridger.jsm
@@ +23,5 @@
> +  "@mozilla.org/network/effective-tld-service;1",
> +  "nsIEffectiveTLDService");
> +
> +let TabTitleAbridger = {
> +  abridgeTitlesForTabs: function TabTitleAbridger_abridgeTitlesForTabs(aTabSets) {

javadoc the methods please.

@@ +28,5 @@
> +    if (!aTabSets)
> +      return;
> +
> +    for (let index = 0; index < aTabSets.length; index++) {
> +      let tabs = aTabSets[index];

for (let tab of aTabSets)

@@ +51,5 @@
> +
> +  _getDomainSets: function TabTitleAbridger_getDomainSets(aTabs) {
> +    let domainSets = {};
> +    for (let i = 0; i < aTabs.length; i++) {
> +      let tab = aTabs[i];

for...of

@@ +75,5 @@
> +            try {
> +              tabDomain = browserURI.host;
> +            }
> +            catch (ex) {}
> +          }

comment on what this is doing

@@ +91,5 @@
> +  _pathMode: false,
> +
> +  _titleIsURI: function TabTitleAbridger_titleIsURI(aTitle) {
> +    return /^\s?[^\s\/]*(:\/)?\/[^\s\/]*([^\s\/]+\/?)*$/.test(aTitle);
> +  },

most of the private stuff (constants and helpers), not requiring "this", could be moved to the global scope of the module, or to a separate object, hiding it properly from module users.
Attachment #612081 - Flags: review?(mak77)
Thanks mak.

Added an originalTitle getter/setter to tabbrowser-tab and updated consumers to use it.

Added comments and documentation.

Moved everything except the public function out of the exported object of the module.  The test now uses the JSSubScript Loader to load the module directly in order to access to the function it needs for direct tests.
Attachment #612081 - Attachment is obsolete: true
Attachment #613054 - Flags: review?(ttaubert)
Attachment #613054 - Flags: review?(dao)
Attachment #612081 - Flags: review?(ttaubert)
Attachment #612081 - Flags: review?(dao)
Tim, Dao: Can you give an ETA on the review for this?
Attachment #613054 - Attachment is obsolete: true
Attachment #621147 - Flags: review?(ttaubert)
Attachment #621147 - Flags: review?(dao)
Attachment #613054 - Flags: review?(ttaubert)
Attachment #613054 - Flags: review?(dao)
Attachment #621149 - Flags: review?(ttaubert)
Attachment #621149 - Flags: review?(dao)
Attachment #621151 - Flags: review?(ttaubert)
Attachment #621151 - Flags: review?(dao)
Comment on attachment 621149 [details] [diff] [review]
[browser] Unbitrot and split into multiple patches

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

::: browser/base/content/tabbrowser.xml
@@ +2810,5 @@
>            this.mTabClipWidth = Services.prefs.getIntPref("browser.tabs.tabClipWidth");
>            this.mCloseButtons = Services.prefs.getIntPref("browser.tabs.closeButtons");
>            this._closeWindowWithLastTab = Services.prefs.getBoolPref("browser.tabs.closeWindowWithLastTab");
> +          this._cropTitleRedundancy = Services.prefs.getBoolPref("browser.tabs.cropTitleRedundancy");
> +          this._cropWithEllipsis = Services.prefs.getBoolPref("browser.tabs.cropWithEllipsis");

Can we remove the cropWithEllipsis pref? I'd say the leading ellipsis is unnecessary, and we shouldn't include the pref and extra functionality with this feature. Would removing it simplify the code much?
This and the next patch remove the leading-ellipsis feature and preference.

> Would removing it simplify the code much?

Not much:

Seven lines to handle the pref itself
Five lines to get the ellipsis character
One line to add the ellipsis to cropped labels

But at least that's six fewer lines being added to tabbrowser.xml.
Attachment #621147 - Attachment is obsolete: true
Attachment #621147 - Flags: review?(ttaubert)
Attachment #621147 - Flags: review?(dao)
Attachment #625851 - Flags: review?(ttaubert)
Attachment #625851 - Flags: review?(dao)
Comment on attachment 625851 [details] [diff] [review]
[Module and feature tests] Remove leading ellipsis code.

Whoops, I decided not to move the test, but meant to ask if it should be moved into browser/modules/test.
Attachment #625851 - Attachment description: [Module and feature tests] Remove leading ellipsis code, move test to module test directory. → [Module and feature tests] Remove leading ellipsis code.
Attachment #621149 - Attachment is obsolete: true
Attachment #621149 - Flags: review?(ttaubert)
Attachment #621149 - Flags: review?(dao)
Attachment #625853 - Flags: review?(ttaubert)
Attachment #625853 - Flags: review?(dao)
Comment on attachment 621151 [details] [diff] [review]
[components] Unbitrot and split into multiple patches

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

Jared asked me to review, but I'm going to leave that to people who are more familiar (and I don't know enough about panorama to look at those parts). I'll f+ though.

One point: the changes to nsSessionStore.js is going to be bitrotted, you'll want to make those changes to SessionStore.jsm

::: browser/components/places/content/controller.js
@@ +1526,5 @@
>        else if (data instanceof XULElement && data.localName == "tab" &&
>                 data.ownerDocument.defaultView instanceof ChromeWindow) {
>          let uri = data.linkedBrowser.currentURI;
>          let spec = uri ? uri.spec : "about:blank";
> +        let title = data.originalTitle;

we can get rid of this line (it wasn't being used anyway)

@@ +1531,2 @@
>          unwrapped = { uri: spec,
> +                      title: title,

just use data.originalTitle

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +3066,5 @@
>        // If the page has a title, set it.
>        if (activePageData) {
>          if (activePageData.title) {
>            tab.label = activePageData.title;
> +          tab.originalTitle = activePageData.title;

I haven't looked through the rest of the patches so I have a question - should anything be setting tab.label anymore? Will setting tab.originalTitle update the label? If so, then we probably don't need to set tab.label in these blocks.
Attachment #621151 - Flags: feedback+
Comment on attachment 621151 [details] [diff] [review]
[components] Unbitrot and split into multiple patches

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

I don't have a ton of experience with these files, but I tried to give the code a look-over. Nothing strange jumped out at me, so feedback+.

::: browser/components/tabview/search.js
@@ +72,5 @@
>    nameOf: function TabUtils_nameOf(tab) {
>      // We can have two types of tabs: A <TabItem> or a <xul:tab>
>      // because we have to deal with both tabs represented inside
>      // of active Panoramas as well as for windows in which
>      // Panorama has yet to be activated. We uses object sniffing to

s/uses/use

::: browser/components/tabview/test/browser_tabview_bug595560.js
@@ +47,5 @@
>      let hasFocus = doc.hasFocus() && doc.activeElement == searchBox[0];
>      ok(hasFocus, "The search box has focus");
>  
>      let tab = win.gBrowser.tabs[1];
> +    searchBox.val(tab.originalTitle || tab._tabViewTabItem.$tabTitle[0].textContent);

This got switched from innerHTML to textContent, which seems like a move in the right direction, but how will that change the functioning of the test?

::: browser/components/tabview/ui.js
@@ +573,5 @@
> +        function buildSet (aEvent) {
> +          gBrowser.tabContainer.abridgeTheseTabTitles([aTabSet]);
> +        }
> +        aTabSet.forEach(function (aTab) {
> +          if ("_tabViewTabItem" in aTab)

side thought: would using hasOwnProperty here work? it should be faster than walking the whole prototype chain.
Attachment #621151 - Flags: feedback+
Comment on attachment 625851 [details] [diff] [review]
[Module and feature tests] Remove leading ellipsis code.

Flagging Benjamin and Sean for feedback on the algorithm approach here. Thanks for any and all feedback you can give here :)
Attachment #625851 - Flags: feedback?(sstangl)
Attachment #625851 - Flags: feedback?(bpeterson)
Comment on attachment 625851 [details] [diff] [review]
[Module and feature tests] Remove leading ellipsis code.

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

::: browser/modules/TabTitleAbridger.jsm
@@ +16,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "gETLDService",
> +  "@mozilla.org/network/effective-tld-service;1",
> +  "nsIEffectiveTLDService");
> +
> +let _pathMode = false;

Does this have to be a global? Couldn't it be passed around?

@@ +34,5 @@
> +      return aStr.indexOf('/', aStart);
> +    let first = aStr.indexOf(' ', aStart);
> +    let second = aStr.indexOf(' ', first + 1);
> +    while (first != -1) {
> +      if (second > first &&

Isn't this more nicely expressed as "second != -1"?

@@ +35,5 @@
> +    let first = aStr.indexOf(' ', aStart);
> +    let second = aStr.indexOf(' ', first + 1);
> +    while (first != -1) {
> +      if (second > first &&
> +          this._separators.test(aStr.substring(first, second + 1)))

This will succeed for strings like " foo\t<\tbar ", which I'm not sure is intended.

@@ +98,5 @@
> +   * @return an index indicating the divergence between the strings
> +   */
> +  _findSuffix: function RedundancyFinder_findSuffix(aStr, aNextStr) {
> +    let last = this._lastIndexOfSep(aStr)[0]
> +    let oldLast = last;

The assignment is redundant. I think the let could also be moved closer to the loop.

@@ +182,5 @@
> +  },
> +
> +  /**
> +   * Finds the proper abridgment indexes for the given tabs.
> +   * @param aTabSet the tabs to find abridgments for

Should mention it requires them to be sorted by title.

@@ +201,5 @@
> +      if (!chopList[i])
> +        chopList[i] = 0;
> +      let nextStr = aTabSet[next]._originalTitle;
> +
> +      // Is the title one word, empty, or completely different than next?

The possibility of empty was eliminated in _getDomainSets.

Also, it seems it only has to be different not "completely different" from next.

@@ +208,5 @@
> +        chopList[next] = 0;
> +        continue;
> +      }
> +
> +      // Treat neighbors with same title as ourselves

This could be done before the previous block.

@@ +216,5 @@
> +          nextStr = aTabSet[next]._originalTitle;
> +      }
> +
> +      // Bail on these strings early, using the first as the basis
> +      if (next >= aTabSet.length ||

next can't be greater than aTabSet.length, can it?

@@ +224,5 @@
> +        chop = RedundancyFinder._findFrontChop(tabStr, nextStr, chop);
> +        // Does a URI remain?
> +        if (!_pathMode) {
> +          _pathMode = this._titleIsURI(tabStr.substr(chop));
> +          chop = RedundancyFinder._findFrontChop(tabStr, nextStr, chop);

You only need to recompute this if you're if pathMode changes.

@@ +232,5 @@
> +        let sufPos = tabStr.length - suffix;
> +        let nextSufPos = nextStr.length - suffix;
> +
> +        chop += 1;
> +        chopList[next] = chop;

Some lines could be saved by doing this assignment after the following if blocks.

@@ +267,5 @@
> +   * @param aTabSet   the tabs to be abridged
> +   * @param aChopList the corresponding abridgment indexes
> +   * @return an array of abridgment indexes corresponding to the tabs
> +   */
> +  _applyChops: function AbridgmentTools_applyChops(aTabSet, aChopList) {

This function seems rather out of place for this file. Maybe it should be back in tabbrowser.xml?

@@ +304,5 @@
> +
> +      // Process each domain
> +      for (let domain in domainSets) {
> +        let tabSet = domainSets[domain];
> +        tabSet.sort(function(aTab, bTab) {

Maybe get chops for set should sort it, since it requires that?
Attachment #625851 - Flags: feedback?(bpeterson)
Minor cleanup (mostly replacing "in" operator with hasOwnProperty where needed).
Attachment #625851 - Attachment is obsolete: true
Attachment #625851 - Flags: review?(ttaubert)
Attachment #625851 - Flags: review?(dao)
Attachment #625851 - Flags: feedback?(sstangl)
Attachment #628558 - Flags: review?(ttaubert)
Attachment #628558 - Flags: review?(dao)
Attachment #628558 - Flags: feedback?(sstangl)
Attachment #628558 - Flags: feedback?(bpeterson)
> [tab.label vs. tab.originalTitle]

The main difference is that originalTitle isn't set until there's a real title (eg, when loading is complete).  This is to keep the tab from being used in abridgment decisions or being abridged while loading.

Most abridgments won't be caused by title changes, but by tabs being added/removed, so title changes do trigger abridgment, but not by setting the originalTitle.

When abridgment is completely disabled, the label still gets updated directly.

> ::: browser/components/tabview/test/browser_tabview_bug595560.js
> @@ +47,5 @@
> >      let hasFocus = doc.hasFocus() && doc.activeElement == searchBox[0];
> >      ok(hasFocus, "The search box has focus");
> >  
> >      let tab = win.gBrowser.tabs[1];
> > +    searchBox.val(tab.originalTitle || tab._tabViewTabItem.$tabTitle[0].textContent);
> This got switched from innerHTML to textContent, which seems like a move in the right direction, but how will that change the functioning of the test?

That should have just been originalTitle.  It was always falling back because of a bug in the getter for originalTitle, but that's now fixed (in the updated browser patch), so this removes the fallback.

The test just needs to have that search value match the tab (it's testing that the search automatically hides tabview and then selects the appropriate tab).

This also swaps in hasOwnProperty where it had used "in" operator.
Attachment #621151 - Attachment is obsolete: true
Attachment #621151 - Flags: review?(ttaubert)
Attachment #621151 - Flags: review?(dao)
Attachment #628562 - Flags: review?(ttaubert)
Attachment #628562 - Flags: review?(dao)
Attachment #625853 - Attachment is obsolete: true
Attachment #625853 - Flags: review?(ttaubert)
Attachment #625853 - Flags: review?(dao)
Attachment #628563 - Flags: review?(ttaubert)
Attachment #628563 - Flags: review?(dao)
Thanks Benjamin!  I've addressed all your feedback.

> > @@ +232,5 @@
> > [...]
> Some lines could be saved by doing this assignment after the following if blocks.

This needed a little more work.  It should have checked against MIN_CHOP separately for nextStr.  This fixes that, and adds tests for those cases.
Attachment #628558 - Attachment is obsolete: true
Attachment #628558 - Flags: review?(ttaubert)
Attachment #628558 - Flags: review?(dao)
Attachment #628558 - Flags: feedback?(sstangl)
Attachment #628558 - Flags: feedback?(bpeterson)
Attachment #628987 - Flags: review?(ttaubert)
Attachment #628987 - Flags: review?(dao)
Attachment #628987 - Flags: feedback?(sstangl)
Attachment #628563 - Attachment is obsolete: true
Attachment #628563 - Flags: review?(ttaubert)
Attachment #628563 - Flags: review?(dao)
Attachment #628992 - Flags: review?(ttaubert)
Attachment #628992 - Flags: review?(dao)
Adam, thank you for your patience so far. I tried to review these patches several times and couldn't really wrap my head around them. While I'd really like to see this feature implemented I'm not sure we're currently taking the right approach.

All this feature is supposed to do is remove redundant text from tab titles but still it is so invasive that we need to adapt session store, Panorama, tests and will probably break a couple of add-ons, too. We're changing the meaning of one of the core attributes of tabs (the label) which doesn't sound like a really good idea to me.

I should've said this earlier but I'd really like this patch to be about changing the view (the rendering of tab labels) but not the attribute itself and/or its meaning. I don't think there's a sane way to do this yet but then we should think about the things we'd like to have and talk to the platform guys that can implement this or will us help to do that. I'm not familiar with how XUL tabs are implemented/rendered but as a quick idea maybe there's a way to write our own renderer for them that accesses your JSM, passes a tab and gets a label to render in return?
Thanks, Tim!

This update drops the components patch entirely (no longer needed) and makes this feature simpler.

This adds a |visibleLabel| attribute to the tab, which gets the cropped version of the label.  The |xul:label| of the tab inherits its value from |visibleLabel| instead of |label|.  Updates to |visibleLabel| do not trigger a |TabAttrModified| event on the tab; if desired, they could.

The |_originalTitle| business goes away entirely.  The only bit from the components patch that we may want back was for tabview/ui.js.  That bit caused redundancy removal on tab groups.

That was so the user would see the same labels in panorama as in the browser view.  The concern is that otherwise the user skips a beat when switching between the views, to reconcile the differences in labels.  If desired, that can be reworked for this new approach.
Attachment #628562 - Attachment is obsolete: true
Attachment #628992 - Attachment is obsolete: true
Attachment #628562 - Flags: review?(ttaubert)
Attachment #628562 - Flags: review?(dao)
Attachment #628992 - Flags: review?(ttaubert)
Attachment #628992 - Flags: review?(dao)
Attachment #636124 - Flags: review?(ttaubert)
Attachment #636124 - Flags: review?(dao)
Attachment #628987 - Attachment is obsolete: true
Attachment #628987 - Flags: review?(ttaubert)
Attachment #628987 - Flags: review?(dao)
Attachment #628987 - Flags: feedback?(sstangl)
Attachment #636125 - Flags: review?(ttaubert)
Attachment #636125 - Flags: review?(dao)
Comment on attachment 636124 [details] [diff] [review]
[browser] Tab labels render from new visibleLabel attribute; leave the label attribute alone.

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

Thank you, Adam. The new approach and patch looks much cleaner and is a little easier to review.

I think, ideally, we shouldn't have to touch tabbrowser.xml and the tabbrowser shouldn't need to know about TabTitleAbridger.jsm. The module should just add event listeners for TabPinned/Unpinned/Show/Hide/Close/Move/AttrModified on the tabContainer and act accordingly when it receives an event. In browser.js/delayedStartup you could create an instance for the window - like new TabTitleAbridger(browserWindow) - that takes care of all visible tabs and their titles. Maybe we could also create some kind of cache with a WeakMap so that we don't need to re-calculate everything in case one of 30 tab labels changes - not sure if that's possible.

PS: Please don't forget to listen for TabMove. If if move a tab out of a group of tabs we might need to adjust their tab labels.


The few tabbrowser.xml changes that we really need to make should go into separate patches and/or bugs:

1) In order to only react to TabAttrModified events that are fired when the tab label was changed we should extend the event itself to actually tell the name of the changed attribute.

2) I think the visibleLabel attribute/property is a good solution. We should define a 'label' property that always returns the originalLabel and always sets the visibleLabel as well. Another property would then be 'visibleLabel' which just sets the attribute itself accordingly. This could be implemented like this: http://pastebin.mozilla.org/1677912

These two should be independent patches that provide/extend APIs for all consumers, not just this feature (please don't forget tests).
Attachment #636124 - Flags: review?(ttaubert)
Comment on attachment 636125 [details] [diff] [review]
[module and feature tests] Use label as basis for finding redundancy, rework feature tests to follow the changes.

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

::: browser/base/content/test/browser_bug583890.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */

Nit:

> /* Any copyright is dedicated to the Public Domain.
>  * http://creativecommons.org/publicdomain/zero/1.0/ */

::: browser/modules/Makefile.in
@@ +19,5 @@
>  	NewTabUtils.jsm \
>  	offlineAppCache.jsm \
>  	TelemetryTimestamps.jsm \
>  	webappsUI.jsm \
> +	TabTitleAbridger.jsm \

Nit: please put this above TelemetryTimestamps.jsm to keep it ordered (mostly) alphabetically.

::: browser/modules/TabTitleAbridger.jsm
@@ +6,5 @@
> +
> +let EXPORTED_SYMBOLS = ["TabTitleAbridger"];
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;

You're not using Ci and Cc, please remove.

@@ +10,5 @@
> +const Cc = Components.classes;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

You're not using Services anywhere.

@@ +16,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "gETLDService",
> +  "@mozilla.org/network/effective-tld-service;1",
> +  "nsIEffectiveTLDService");
> +
> +let RedundancyFinder = {

All methods in this object are used by others, please remove the leading underscores.

@@ +129,5 @@
> +   * This allows only tabs within the same family to be compared.
> +   * @param aTabs an array of tabs needing categorization
> +   * @return the categorized tabs in arrays keyed by scheme or host
> +   */
> +  _getDomainSets: function AbridgmentTools_getDomainSets(aTabs) {

This method is used outside of this object, please remove the underscore here.

@@ +143,5 @@
> +        case "chrome":
> +        case "file":
> +        case "resource":
> +        case "data":
> +        case "about":

Please put all these in a hash like:

> let AbridgmentTools = {
>   _noHostSchemes: {
>     chrome: true,
>     file: true,
>     /* ... */
>   }
> };

So you can write:

> if (browserURI.scheme in this._noHostSchemes) {
>   tabDomain = browserURI.scheme;
> } else  {
>   /* getBaseDomain, host, etc. */
> }

@@ +189,5 @@
> +   * Finds the proper abridgment indexes for the given tabs.
> +   * @param aTabSet the array of tabs to find abridgments for
> +   * @return        an array of abridgment indexes corresponding to the tabs
> +   */
> +  _getChopsForSet: function AbridgmentTools_getChopsForSet(aTabSet) {

Please remove the leading underscore here as well.

@@ +279,5 @@
> +    return chopList;
> +  }
> +};
> +
> +let TabTitleAbridger = {

Please put this at the top of the file. It's the only exposed object and it's much easier to read then.

@@ +290,5 @@
> +                                                                       aContainer) {
> +    if (!aTabSets)
> +      return;
> +
> +    for (let tabs of aTabSets) {

This doesn't really seem necessary anymore since it's always just [gBrowser.visibleTabs] and not multiple tab sets.
Attachment #636125 - Flags: review?(ttaubert)
The two properties/attributes are |label| and |visibleLabel|.  When the |label| is updated, |visibleLabel| is also set.  One concern with this approach is that the rendered label of the tab will get the new value briefly before the |TabTitleAbridger| replaces it, causing a sort of flicker.
Attachment #636124 - Attachment is obsolete: true
Attachment #636124 - Flags: review?(dao)
Attachment #637352 - Flags: review?(ttaubert)
There are a few places where label and crop are updated together, so the detail portion is an array containing the names of the attributes.  Should those cases only report "label," and change it to a string?

The test here isn't great.  It adds a new tab (with favicon, to get the "image" flavored event) and waits to see all the TabAttrModified events for each of the attributes that occurs during tab creation.  Suggestions for a better test are welcome, or I might have a jolt of inspiration.
Attachment #637355 - Flags: review?(ttaubert)
Because the changes to existing files are rather small (adds the pref in firefox.js; includes the module/init/destroy it in browser.js), I included them here.  If desired, I can split them back out.

Believe this gets to all the feedback except the bits about Services, Cc, and Ci, as those ended up being needed for the changes to the module.

Also, have not looked at caching possibilities yet.
Attachment #636125 - Attachment is obsolete: true
Attachment #636125 - Flags: review?(dao)
Attachment #637369 - Flags: review?(ttaubert)
Comment on attachment 637352 [details] [diff] [review]
[visibleLabel] Adds the visibleLabel property/attribute, adds tests.

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

(In reply to Adam [:hobophobe] from comment #120)
> One concern with this
> approach is that the rendered label of the tab will get the new value
> briefly before the |TabTitleAbridger| replaces it, causing a sort of flicker.

Yeah... I didn't think of this. Ok, I just came up with an IMO better solution. We should make the label-setter look like this:

> <setter>
>   this.setAttribute("label", val);
>
>   let event = new CustomEvent("TabLabelModified", {
>     bubbles: true,
>     cancelable: true
>   });
>   this.dispatchEvent(event);
>
>   if (!event.defaultPrevented)
>     this.visibleLabel = val;
> </setter>

This is how to use the new API:

> let tabContainer = window.gBrowser.tabContainer;
> tabContainer.addEventListener("TabLabelModified", function (e) {
>   e.preventDefault();
>   e.target.visibleLabel = "[foobar] " + e.target.label;
> });

Calling .preventDefault() prevents the default action (i.e. setting the visible label) and we can set our own. What do you think about that solution? We don't even need to modify the TabAttrModified event then.

::: browser/base/content/tabbrowser.xml
@@ +3792,5 @@
> +        </getter>
> +        <setter>
> +          this.visibleLabel = val;
> +          this.setAttribute("label", val);
> +          return val;

No need to return here.

@@ +3797,5 @@
> +        </setter>
> +      </property>
> +      <property name="visibleLabel"
> +        onset="this.setAttribute('visibleLabel',val); return val;"
> +        onget="return this.getAttribute('visibleLabel');"/>

Please use <getter> and <setter> here as well. No need to return in the setter.
Attachment #637352 - Flags: review?(ttaubert)
Comment on attachment 637355 [details] [diff] [review]
[TabAttrModified] Add detail to the TabAttrModified events, add test.

I think we don't need this anymore.
Attachment #637355 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #123) 
> ::: browser/base/content/tabbrowser.xml
> @@ +3792,5 @@
> > +        </getter>
> > +        <setter>
> > +          this.visibleLabel = val;
> > +          this.setAttribute("label", val);
> > +          return val;
> 
> No need to return here.

In my understanding, we *should* be returning here so that we don't break the ability to do: |foo = bar = true;|
(In reply to Jared Wein [:jaws] from comment #125)
> In my understanding, we *should* be returning here so that we don't break
> the ability to do: |foo = bar = true;|

I thought about this too but isn't this automatically possible? We should check, I guess.
(In reply to Tim Taubert [:ttaubert] from comment #126)
> (In reply to Jared Wein [:jaws] from comment #125)
> > In my understanding, we *should* be returning here so that we don't break
> > the ability to do: |foo = bar = true;|
> 
> I thought about this too but isn't this automatically possible? We should
> check, I guess.

We don't need to return anything from setters to be able to do this. I just checked with a little custom property and the scratchpad.
Comment on attachment 637352 [details] [diff] [review]
[visibleLabel] Adds the visibleLabel property/attribute, adds tests.

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

(In reply to Tim Taubert [:ttaubert] from comment #127)
> (In reply to Tim Taubert [:ttaubert] from comment #126)
> > (In reply to Jared Wein [:jaws] from comment #125)
> > > In my understanding, we *should* be returning here so that we don't break
> > > the ability to do: |foo = bar = true;|
> > 
> > I thought about this too but isn't this automatically possible? We should
> > check, I guess.
> 
> We don't need to return anything from setters to be able to do this. I just
> checked with a little custom property and the scratchpad.

According to bug 337885, we should return the new value, so that things like what Jared wrote would work.
MDN mentions this on a few pages too, but its code examples then fail to do this several times. :/
But then Waldo and Gavin just told me that, in practice, `a = b = true` work regardless and that the return value of the setter is completely ignored, so Tim is correct.
from IRC:

12:22 < Waldo> gavin, fryn: the value returned by an XBL setter is ignored
12:23 < Waldo> there's no reason to return anything from a setter
12:23 < Waldo> this was kind of a JS engine change, a few years ago
12:23 < Waldo> technically what we were doing was sort of kind of a spec 
               violation, of sorts
12:24 < Waldo> now a = b = c = d = 5 assigns 5 to every variable
12:24 < Waldo> and of course evaluates 5 exactly once

In other words, having `d = 5` not return 5 was a spec violation.
Also, the right-hand expression is also evaluated once.
Updated the docs on all the 12 MDN pages where I could find wrong <setter>/onset documentation (http://pastebin.mozilla.org/1689702).
Comment on attachment 638844 [details] [diff] [review]
[visibleLabel] Update setters and getters based on feedback.

Thanks, Tim!

This approach works well.  The only issue is for the feature where the |TabLabelModified| event is received, we have to cancel the event before we know if we'd set |visibleLabel| via abridgment, so we have to clean up for cases where we won't.

Good to know that we don't need to return the value in setters.
Attachment #638844 - Attachment description: [visib → [visibleLabel] Update setters and getters based on feedback.
Attachment #638844 - Flags: review?(ttaubert)
Attachment #637352 - Attachment is obsolete: true
Attachment #637355 - Attachment is obsolete: true
(In reply to Adam [:hobophobe] from comment #132)
> This approach works well.  The only issue is for the feature where the
> |TabLabelModified| event is received, we have to cancel the event before we
> know if we'd set |visibleLabel| via abridgment, so we have to clean up for
> cases where we won't.

That's ok I think. If you cancel the event then you should be responsible to take care of the visibleLabel yourself.
Comment on attachment 638844 [details] [diff] [review]
[visibleLabel] Update setters and getters based on feedback.

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

This looks really good, thank you also for writing a test!

Can you please include a test for the new API as well? I mean, listening for the TabLabelModified event, setting .label, cancelling the event and setting a custom .visibleLabel?

::: browser/base/content/tabbrowser.xml
@@ +3797,5 @@
> +            cancelable: true
> +          });
> +          this.dispatchEvent(event);
> +
> +          if(!event.defaultPrevented)

Nit: if (!event.defaultPrevented)

::: browser/base/content/test/browser_bug583890_label.js
@@ +7,5 @@
> +  waitForExplicitFinish();
> +  let tab = gBrowser.addTab("data:text/html,<title>New Tab</title>",
> +                            {skipAnimation: true});
> +  tab.linkedBrowser.addEventListener("load", function (event) {
> +    event.currentTarget.removeEventListener("load", arguments.callee, true);

Please don't use arguments.callee, that's deprecated. You can just give the anonymous function a name like 'onLoad' and then do:

> tab.linkedBrowser.removeEventListener("load", onLoad, true);

@@ +15,5 @@
> +}
> +
> +function afterLoad() {
> +  let tab = gBrowser.selectedTab;
> +  let xulLabel = tab.boxObject.firstChild.lastChild.lastChild.previousSibling;

You should assign an 'anonid="tab-label"' attribute to the <xul:label> so that we can access it like this:

> let xulLabel = document.getAnonymousElementByAttribute(tab, "anonid", "tab-label");

@@ +20,5 @@
> +  // Verify we're starting out on the right foot
> +  is(tab.label, "New Tab", "Fresh tab has bogus label!");
> +  is(xulLabel.value, "New Tab", "Label element has wrong value!");
> +  is(tab.visibleLabel, "New Tab",
> +     "visibleLabel differs before a direct change!");

As a general comment, those aren't exactly error messages. They are always shown, no matter if the test succeeds or not. It's better to describe what you're testing here.

@@ +45,5 @@
> +  is(tab.visibleLabel, "One more label",
> +     "visibleLabel not set from label property!");
> +
> +  // Check that setting the document title updates the labels
> +  tab.linkedBrowser.contentDocument.title = "And one title";

I don't think this belongs to the scope of this test.
Attachment #638844 - Flags: review?(ttaubert) → feedback+
Attachment #638844 - Attachment is obsolete: true
Attachment #639164 - Flags: review?(ttaubert)
Comment on attachment 639164 [details] [diff] [review]
[visibleLabel] Add TabLabelModified test, address feedback

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

Yay, first patch finally got r+. Thanks for so much endurance, Adam :)

I'd like to get the opinion of a second reviewer about the approach we're taking with the TabLabelModified event just to make sure that's alright.
Attachment #639164 - Flags: review?(ttaubert)
Attachment #639164 - Flags: review?(jaws)
Attachment #639164 - Flags: review+
Comment on attachment 639164 [details] [diff] [review]
[visibleLabel] Add TabLabelModified test, address feedback

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

I like this :)
Attachment #639164 - Flags: review?(jaws) → review+
Comment on attachment 638845 [details] [diff] [review]
[module, feature tests, browser] Swap listening for TabAttrModified with listening for TabLabelModified; prevent default for that event and clean up if we don't abridge.

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

I think the biggest change we need to make is to not re-calculate all tab labels when only a single tab changes. We have events for (almost) everything and should use them accordingly to determine the smallest set of changes we need to do. I outlined how this could look like below, please don't see it as a requirement to implement it that way, it's rather a specific suggestion ;) If you feel like this wouldn't work for some reason then please let me know.

::: browser/modules/TabTitleAbridger.jsm
@@ +45,5 @@
> +    this._cropTitleRedundancy = Services.prefs.getBoolPref(data);
> +    this.abridgeTabTitles();
> +  },
> +
> +  _eventNames: [

Please put this at the top of the object, feels more like a constant to me.

@@ +48,5 @@
> +
> +  _eventNames: [
> +    "TabPinned",
> +    "TabUnpinned",
> +    "TabMove",

I think we don't actually need to listen for TabMove. I was mistaken that tab labels depend on their position on the tab strip. They don't, do they?

@@ +53,5 @@
> +    "TabShow",
> +    "TabHide",
> +    "TabClose",
> +    "TabLabelModified"
> +  ],

Please add a comment that we're intentionally not listening for TabCreate as we will be notified of those by TabLabelModified.

@@ +66,5 @@
> +    }, this);
> +  },
> +
> +  /**
> +   * Adds all the necessary event listeners for the instance.

That's not the right description :)

@@ +79,5 @@
> +  handleEvent: function TabTitleAbridger_handler(aEvent) {
> +    if (aEvent.type == "TabLabelModified")
> +      aEvent.preventDefault();
> +
> +    this.abridgeTabTitles();

We currently take the easy way, every time we receive an event we're just re-calculating everything from scratch. I think we shouldn't do this for performance reasons. We could for example have a DomainSets object that has a WeakMap with DOMWindows as keys and domain sets as values. These are the actions we should do on specific events:

* TabPinned/TabClose/TabHide:
  DomainSets.removeTab(event.target);
  reset the tab label (tab.visibleLabel = tab.label)

* TabShow/TabUnpinned:
  DomainSets.addTab(event.target);

* TabLabelModified:
  if (!event.target.hidden && !event.target.pinned)
    DomainSets.updateTab(event.target);


The DomainSets object should lazily initialize domain sets for a window (which can be determined by accessing aTab.ownerDocument.defaultView). It should do the following tasks / provide the following methods:

* addTab:
  Determine the given tab's domain and add it to its corresponding domain set. If there's none, create one. Update all tab labels contained in the domain set.

* removeTab:
  Determine the given tab's domain and remove it from its corresponding domain set. Update all tab labels contained in the domain set.

* updateTab:
  Determine the given tab's domain and see if it has changed. This can be done by also keeping a WeakMap of tabs with their domain sets. Move it to a different domain set if needed and update the changed domain sets.
Attachment #638845 - Flags: review?(ttaubert)
Thanks, all!

The abridgment timer means that some labels don't get set immediately (when changes occur in rapid succession), so this updates the test for |visibleLabel| to simply add an event listener which stops the event from reaching the abridger.
Attachment #639164 - Attachment is obsolete: true
Thanks, Tim.

I didn't quite follow the part about mapping windows to domain sets, but I can add that if it's still needed.  As it stands, each Window has an instance of TabTitleAbridger, and each of those (when enabled) has a DomainSets instance.

The DomainSets object has an object |domainSets| mapping domain/scheme names to arrays of tabs and a WeakMap of tabs to domain/scheme names.

In order to allow deferment of abridgment, the domains may get thrown into an array, |_deferredSet|, while the timer spins.  When actually running, the union of |_deferredSet| and |aDomains| is taken.

Also, |domainSets| gets dropped/added when disabling/enabling the feature, so I added a |DomainSets.bootstrap| to rebuild off of |visibleTabs|.  That's used for enabling the feature after a window is up and running.
Attachment #638845 - Attachment is obsolete: true
Attachment #639823 - Flags: review?(ttaubert)
Comment on attachment 639823 [details] [diff] [review]
[module, feature tests, browser] Add DomainSets object and rework to use that

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

I really like the direction of this now. The domain sets should be good as is and will certainly help reducing the feature's performance impact. There still are some minor/medium things to fix/ figure out, though.

::: browser/modules/TabTitleAbridger.jsm
@@ +58,5 @@
> +
> +  /**
> +   * Preference observer
> +   */
> +  observe: function TabTitleAbridger_PrefObserver(subject, topic, data) {

Nit: Please name the arguments aSubject, aTopic and aData for consistency.

@@ +61,5 @@
> +   */
> +  observe: function TabTitleAbridger_PrefObserver(subject, topic, data) {
> +    let val = Services.prefs.getBoolPref(data);
> +    if (this._cropTitleRedundancy && !val) {
> +      this._cropTitleRedundancy = val;

You could move this assignment out of the branches to the bottom of the function, we have to do it either way.

@@ +83,5 @@
> +   * the instance.
> +   */
> +  _addListeners: function TabTitleAbridger_addListeners() {
> +    let tabContainer = this.browserWin.gBrowser.tabContainer;
> +    this._eventNames.forEach(function (aEventName) {

Nit: for (let eventName of this._eventNames) {

@@ +93,5 @@
> +   * Removes event listeners and listener-supporting objects for the instance.
> +   */
> +  _dropListeners: function TabTitleAbridger_dropListeners() {
> +    let tabContainer = this.browserWin.gBrowser.tabContainer;
> +    this._eventNames.forEach(function (aEventName) {

Nit: for (let eventName of this._eventNames) {

@@ +119,5 @@
> +          aEvent.preventDefault();
> +          updateSets = this.domainSets.updateTab(tab);
> +        }
> +        break;
> +      default:

We don't really need the 'default:' here as it's a no-op anyway.

@@ +135,5 @@
> +      // We're freshly disabled, so resets all tabs, including hidden ones
> +      for (let tab of this.browserWin.gBrowser.mTabs) {
> +        if (tab.visibleLabel != tab.label) {
> +          tab.visibleLabel = tab.label;
> +        }

The only caller of this is line 69, right? Why not create a separate function for this like 'resetTabTitles'?

@@ +143,5 @@
> +    } else if (Date.now() - this._lastTitleAbridgment < 80) {
> +      // Rate-limit abridgments to handle situations where one or more
> +      // tabs updates their title constantly
> +      // try again in 100ms unless the abridgment's happened since then.
> +      if (aDomains && Array.isArray(aDomains)) {

I wondered if we really need all this rate-limiting magic. Sites might change the title quite a lot when loading for the first time and especially when starting up the browser with lots of tabs that might delay updating tab labels quite a bit.

After all, this is just in case some tab updates its label a lot, which should rather be an exception. To handle this well we should make sure to speed up handling 'TabLabelModified' events and I think we don't really need the throttling unless you have a compelling reason :)

@@ +177,5 @@
> +    }
> +
> +    // Process each domain
> +    for each (let domain in domains) {
> +      let tabSet = this.domainSets.domainSets[domain];

Couldn't DomainSets.{update,add,remove}Tab just return an array of arrays with tabs? This way we wouldn't need to fetch them here again.

@@ +191,5 @@
> +          let chop = chopList[i] || 0;
> +          title = title.substr(chop);
> +          if (title != tab.visibleLabel) {
> +            tab.visibleLabel = title;
> +          }

I think this should rather be:

> let chop = chopList[i] || 0;
> if (chop > 0) {
>   title = title.substr(chop);
>   tab.visibleLabel = title;
> }

@@ +198,5 @@
> +    }
> +  }
> +};
> +
> +function DomainSets() {

Please add a comment here describing the purpose of this class.

@@ +266,5 @@
> +    let oldTabDomain = aTabDomain || this._tabsMappedToSets.get(aTab);
> +    if (!oldTabDomain) {
> +      return [];
> +    }
> +    let index = this.domainSets[oldTabDomain].indexOf(aTab);

We should check if (index > -1). Actually we could assert that (index > -1) but this is JS, so maybe just return [] in this case?

@@ +298,5 @@
> +      // No change was needed
> +      return [tabDomain];
> +    }
> +    // Tab wasn't a member yet
> +    return this.addTab(aTab, tabDomain);

We can include this in the if (oldTabDomain != tabDomain) branch if we conditionally call this.removeTab if (oldTabDomain).

@@ +350,5 @@
> +   * Finds the proper abridgment indexes for the given tabs.
> +   * @param aTabSet the array of tabs to find abridgments for
> +   * @return        an array of abridgment indexes corresponding to the tabs
> +   */
> +  getChopsForSet: function AbridgmentTools_getChopsForSet(aTabSet) {

This is one *big* function. Can you please refactor this into a couple of smaller ones? That would make it definitely more readable.

@@ +351,5 @@
> +   * @param aTabSet the array of tabs to find abridgments for
> +   * @return        an array of abridgment indexes corresponding to the tabs
> +   */
> +  getChopsForSet: function AbridgmentTools_getChopsForSet(aTabSet) {
> +    const MIN_CHOP = 3;

Please define it so that it's a 'constant' for the object, like AbridgmentTools.MIN_CHOP.
Attachment #639823 - Flags: review?(ttaubert)
Thanks, Tim.

> I think we don't really need the throttling unless you have a compelling reason :)  

The reason behind it was for sites that use the title as a marquee[1], a game board[2], or something similar.  If they don't pound the title too hard, it shouldn't be an issue.

> @@ +191,5 @@
> > +          let chop = chopList[i] || 0;
> > +          title = title.substr(chop);
> > +          if (title != tab.visibleLabel) {
> > +            tab.visibleLabel = title;
> > +          }
> 
> I think this should rather be:
> 
> > let chop = chopList[i] || 0;
> > if (chop > 0) {
> >   title = title.substr(chop);
> >   tab.visibleLabel = title;
> > }

We must set the |visibleLabel| (unless it's already correct), because of the cases where we |preventDefault| for a |TabLabelModified| event.

I've updated that code so that the only time we don't update the |visibleLabel| is if the |label| is undefined, and added the |chop > 0| condition as you suggested.

> > +  getChopsForSet: function AbridgmentTools_getChopsForSet(aTabSet) {
> 
> This is one *big* function.

This is how I've split it up:

a. |getChopsForSet| becomes mostly a shell, with the loop calling:
b. |_abridgePair| which is the biggest remnant of the original.  It returns the "next" index, as updated by [c].  [c-e] are split out from this.
c. |_nextUnproxied| finds the next tab in the array with a different label from the current index.  This is used by [a] to update the loop indices.
d. |_getChopPoint| finds the actual chop point, returning |[pathMode, chop]| because the path mode may change in finding the chop point.
e. |_adjustChops| handles backing down due to a suffix and due to being less than the |MIN_CHOP|.  It also returns a pair |[chop, nextChop]|, because they can (but don't always) differ.

1. http://www.htmlmarquee.com/title.html
2. http://www.geek.com/articles/games/first-ever-browser-address-bar-game-created-in-html-5-2011038/ (Talks about a game using the address bar for the game display (the link there to the game seems dead; http://www.thegillowfamily.co.uk/ has a similar game), but someone could do something similar with the page title.)
Attachment #639823 - Attachment is obsolete: true
Attachment #643689 - Flags: review?(ttaubert)
Comment on attachment 643689 [details] [diff] [review]
[module, feature tests, browser] Remove throttling, break up getChopsForSet, improve DomainSets

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

(In reply to Adam [:hobophobe] from comment #144)
> > I think we don't really need the throttling unless you have a compelling reason :)  
> 
> The reason behind it was for sites that use the title as a marquee[1], a
> game board[2], or something similar.  If they don't pound the title too
> hard, it shouldn't be an issue.

Hmm, this could potentially destroy these demos. That's not too easy to solve. I think ignoring a single tab that constantly changes its title is better than not abridging titles at all for every tab out there. Also, we shouldn't ignore a whole tab but rather a website loaded in a tab that frequently changes its title. There might be websites that change their titles more frequently (like Facebook and Twitter) to signal changes but we still might want to abridge for those.

Do we know how Safari handles those sites? After all, it's quite an edge case and this is a feature that the user can disable manually or needs to enable manually so I'm not sure we should care too much about this.

::: browser/modules/TabTitleAbridger.jsm
@@ +137,5 @@
> +                                                              aChopList) {
> +    for (let i = 0; i < aTabSet.length; i++) {
> +      let tab = aTabSet[i];
> +      let label = tab.label;
> +      if (label != undefined) {

Does this ever occur? Anyway, we could probably just do:

> let label = tab.label || "";

and omit the condition.
(In reply to Tim Taubert [:ttaubert] from comment #145)

> Also, we
> shouldn't ignore a whole tab but rather a website loaded in a tab that
> frequently changes its title.

I would say "a Web page" instead of "a website". Let's just ignore a particular URL in a particular tab.

But, more generally, I'm rather for not having a special handling for these "games". Usability first. After all, these "games" play on the way the browser displays titles. We (well, you) improve this display. Firefox evolves. The "games" will adapt to the improved display.

Nicolas
Thinking about how this feature has changed, I think the risk posed by those demos is low enough that we shouldn't worry about them.  It was a greater risk for the earlier implementation: one hyperactive tab would have caused all of the titles to be checked.

That risk is much smaller now.  It's reduced to where the user has a lot of tabs from the same domain, some of that domain's tabs are hyperactive, and there's a lot of redundancy.
Attachment #643689 - Attachment is obsolete: true
Attachment #643689 - Flags: review?(ttaubert)
Attachment #645443 - Flags: review?(ttaubert)
Comment on attachment 645443 [details] [diff] [review]
[module, feature tests, browser] Replace undefined label conditional

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

The TabTitleAbridger and DomainSets objects look good to me with the fixes below. They're mostly nits with the exception of the updateTab() fix.

I still need to take a look at AbridgmentTools, the RedundancyFinder and the test but feel free to update the patch in the meantime :)

::: browser/modules/TabTitleAbridger.jsm
@@ +15,5 @@
> +  "@mozilla.org/network/effective-tld-service;1",
> +  "nsIEffectiveTLDService");
> +
> +function TabTitleAbridger(aBrowserWin) {
> +  this.browserWin = aBrowserWin;

We should prefix this property with an underscore. Also, if we're always accessing this.browserWin.gBrowser we might want to store only this?

@@ +35,5 @@
> +
> +  init: function TabTitleAbridger_Initialize() {
> +    this._cropTitleRedundancy = Services.prefs.getBoolPref(
> +      "browser.tabs.cropTitleRedundancy");
> +    Services.prefs.addObserver("browser.tabs.cropTitleRedundancy", this, false);

Please define the preference name as a constant at the top of the file.

@@ +58,5 @@
> +    if (this._cropTitleRedundancy && !val) {
> +      this._dropListeners();
> +      this.domainSets.destroy();
> +      delete this.domainSets;
> +      this._resetTabTitles(); // Cleans up abridgments

Nit: please remove this comment or put it on its own line.

@@ +62,5 @@
> +      this._resetTabTitles(); // Cleans up abridgments
> +    } else if (!this._cropTitleRedundancy && val) {
> +      this._addListeners();
> +      // We're just turned on, so we want to abridge everything
> +      this.domainSets = new DomainSets();

Let's prefix this property with an underscore as well.

@@ +110,5 @@
> +      case "TabLabelModified":
> +        if (!tab.hidden && !tab.pinned) {
> +          aEvent.preventDefault();
> +          updateSets = this.domainSets.updateTab(tab);
> +        }

Nit: Please add a "break" here at the end. If someone adds a new case to the switch here they might be tripping over this.

@@ +119,5 @@
> +  /**
> +   * Make all tabs have their visibleLabels be their labels.
> +   */
> +  _resetTabTitles: function TabTitleAbridger_resetTabTitles() {
> +    // We're freshly disabled, so resets all tabs, including hidden ones

Nit: ... so reset all tabs, ...

@@ +120,5 @@
> +   * Make all tabs have their visibleLabels be their labels.
> +   */
> +  _resetTabTitles: function TabTitleAbridger_resetTabTitles() {
> +    // We're freshly disabled, so resets all tabs, including hidden ones
> +    for (let tab of this.browserWin.gBrowser.mTabs) {

That should be "..gBrowser.tabs". And I think we only need to fix visibleTabs, other tabs will still have their original label. Also we don't need to process pinned tabs.

@@ +155,5 @@
> +   * @param aTabSets Array of tab sets needing abridgment
> +   */
> +  _abridgeTabTitles: function TabTitleAbridger_abridgeTabtitles(aTabSets) {
> +    // Process each set
> +    for each (let tabSet in aTabSets) {

We should better use "for ... of" here because one shouldn't use "for each" to iterate over arrays (although it's all over the code base).

@@ +198,5 @@
> +   * an empty array if none of the tabs belonged to domains.
> +   */
> +  bootstrap: function DomainSets_bootstrap(aVisibleTabs) {
> +    let needAbridgment = [];
> +    for each (let tab in aVisibleTabs) {

Nit: for ... of

@@ +199,5 @@
> +   */
> +  bootstrap: function DomainSets_bootstrap(aVisibleTabs) {
> +    let needAbridgment = [];
> +    for each (let tab in aVisibleTabs) {
> +      let tabsInDomain = this.addTab(aTab)[0];

This throws a warning if [0] is undefined. This would work, though:

> let tabsInDomain = this.addTab(aTab)[0] || null;

You might also rename the variable to "domainSet" as that's a better description, I think.

@@ +203,5 @@
> +      let tabsInDomain = this.addTab(aTab)[0];
> +      if (!tabsInDomain) {
> +        continue;
> +      } else if (needAbridgment.indexOf(tabsInDomain) == -1) {
> +        needAbridgment.push(tabsInDomain);

This all would be a little more readable without the "continue", like:

> if (domainSet && needAbridgment.indexOf(domainSet) == -1) {
>   needAbridgment.push(tabsInDomain);
> }

@@ +248,5 @@
> +    }
> +    this._domainSets[oldTabDomain].splice(index, 1);
> +    if (!this._domainSets[oldTabDomain].length) {
> +      // Keep the sets clean of empty domains
> +      delete this._domainSets[oldTabDomain];

If you move |this._tabsMappedToSets.delete()| above this branch then you could just do

> this._tabsMappedToSets.delete(aTab);
> if (!this._domainSets[oldTabDomain].length) {
>   // Keep the sets clean of empty domains
>   delete this._domainSets[oldTabDomain];
>   return [];
> }
> return [this._domainSets[oldTabDomain]];

which is a little easier to read, I think.

@@ +267,5 @@
> +  updateTab: function DomainSets_updateTab(aTab) {
> +    let tabDomain = this._getDomainForTab(aTab);
> +    let oldTabDomain = this._tabsMappedToSets.get(aTab);
> +    if (oldTabDomain != tabDomain) {
> +      let needAbridgment = this.addTab(aTab, tabDomain);

addTab() must be called *after* removeTab() because the latter calls |this._tabsMappedToSets.delete(aTab)| and we're left with an existing untracked tab.

@@ +271,5 @@
> +      let needAbridgment = this.addTab(aTab, tabDomain);
> +      // Probably swapping domain sets out; we pass the domains along to avoid
> +      // re-getting them in addTab/removeTab
> +      if (oldTabDomain) {
> +        needAbridgment.concat(this.removeTab(aTab, oldTabDomain));

This doesn't modify the array in place. Needs to be:

> needAbridgment = needAbridgment.concat(this.removeTab(aTab, oldTabDomain));
Thanks Tim!

I also simplified |_abridgeTabTitles|, as |aTabSets| won't contain non-arrays nor empty arrays.
Attachment #645443 - Attachment is obsolete: true
Attachment #645443 - Flags: review?(ttaubert)
Attachment #646312 - Flags: review?(ttaubert)
Comment on attachment 646312 [details] [diff] [review]
[module, feature tests, browser] Address feedback

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

Took a deeper look at the test and there are some things we should fix. Next up are AbridgmentTools and RedundancyFinder.

Regarding "test_direct_tests()":

I'm generally ok with using mozIJSSubScriptLoader to load JSMs and test stuff that isn't easily accessible but needs to be for unit tests. In this case I'd say that it actually isn't necessary. All we want is to test if we see the expected tab labels when progressively adding tabs with specific labels. It would be better to progressively add tabs with URLs like "data:text/html,<title>foobar</title>" and check if all tabs have the expected labels. This way can easily refactor/replace/optimize the internals of the TabTitleAbridger in the future without having to adapt the tests.

::: browser/base/content/test/browser_bug583890.js
@@ +9,5 @@
> + * @param aTab           the tab whose label is being tested
> + * @param aCallback      the callback for use upon success
> + * @param aExpectedLabel the value the tab's label must match
> + */
> +function waitForTabLabel(aTab, aCallback, aExpectedLabel) {

Nit: I think the callback should be the last argument here.

@@ +23,5 @@
> + * @param aBaseTab       the tab whose label is being tested
> + * @param aCount         the number of tabs to add
> + * @param aCallback      the callback for use upon success
> + */
> +function addTabs(aBaseTab, aCount, aCallback) {

What's the 'aBaseTab' argument for? Seems unused.

@@ +38,5 @@
> + * @param aBaseTab       the tab whose label is being tested
> + * @param aTabs          the array of tabs to remove
> + * @param aCallback      the callback for use upon success
> + */
> +function removeTabs(aBaseTab, aTabs, aCallback) {

What's the 'aBaseTab' argument for? Seems unused.

Also, looks like we wouldn't need to explicitly call removeTabs() every time before runNextTest() if runNextTest() would do something like this before running the next test:

> while (gBrowser.tabs.length > 1) {
>   gBrowser.removeTab(gBrowser.tabs[1]);
> }

@@ +55,5 @@
> + * @param aTitle         the value to give the tab title
> + * @param aCallback      the callback for use upon success
> + * @param aExpectedLabel the value the tab's label must match
> + */
> +function setTitleForTab(aTab, aTitle, aCallback, aExpectedLabel) {

Nit: The callback should be the last argument here as well.

@@ +60,5 @@
> +  aTab.linkedBrowser.contentDocument.title = aTitle;
> +  waitForTabLabel(aTab, aCallback, aExpectedLabel);
> +}
> +
> +var TESTS = [

Nit: let TESTS = [

@@ +64,5 @@
> +var TESTS = [
> +function test_about_blank() {
> +  let tab1 = gBrowser.selectedTab;
> +  addTabs(tab1, 2, test_blank);
> +  function test_blank(aTabs) {

Nit: this could just be:

> addTabs(tab1, 2, function () {
>   ...
> });

@@ +86,5 @@
> +    setTitleForTab(tab2, "Foo - Baz - Baz", setupComplete, "Baz - Baz");
> +  }
> +  function setupComplete() {
> +    is(tab1.visibleLabel, "Bar - Baz", "Removed exactly two tokens");
> +    is(tab2.visibleLabel, "Baz - Baz", "Removed exactly two tokens");

How about checking here that removing the second tab resets the first tab's title again?

@@ +97,5 @@
> +  let jsLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +                 getService(Ci.mozIJSSubScriptLoader);
> +  jsLoader.loadSubScript("resource:///modules/TabTitleAbridger.jsm", tempScope);
> +  let useScope = tempScope.AbridgmentTools;
> +  let useFunc = tempScope.AbridgmentTools.getChopsForSet;

These variable names are a little unfortunate, I think you could just do:

> let abridgmentTools = tempScope.AbridgmentTools;

Then the code below could become:

> let output = abridgmentTools.getChopsForSet(useScope, inputSet.slice(0, j + 1));

That should be rather expressive and I don't need to look up what's assigned to "useFunc".

@@ +102,5 @@
> +  let sortFunc = function (aTab, bTab) {
> +    let aLabel = aTab.label;
> +    let bLabel = bTab.label;
> +    return (aLabel < bLabel) ? -1 : (aLabel > bLabel) ? 1 : 0;
> +  };

We don't need to duplicate this sorting function if you bring all test group labels in the right order. If your plan was to test the sorting function itself you probably should do so explicitly - duplicating the code doesn't really test that :)

@@ +161,5 @@
> +      ]
> +    ]
> +  ];
> +  for (let i = 0; i < testGroups.length; i++) {
> +    let inputSet = testGroups[i][0].map(function(aElement) {

If testGroups were an array of objects you could rather do:

> let inputSet = testGroups[i].labels.map(function(aElement) {

@@ +166,5 @@
> +      return {label: aElement};
> +    });
> +    inputSet.sort(sortFunc);
> +    for (let j = 1; j < inputSet.length; j++) {
> +      let correct = testGroups[i][1][j - 1];

Same here:

> let correct = testGroups[i].chops[j - 1];

Also it's not necessarily "correct" but rather the "expected" value ;)

@@ +176,5 @@
> +  runNextTest();
> +}
> +];
> +
> +var gTestStart = null;

Nit: let gTestStart = null;

@@ +180,5 @@
> +var gTestStart = null;
> +
> +function runNextTest() {
> +  if (gTestStart)
> +    info("Test part took " + (Date.now() - gTestStart) + "ms");

Is it really useful to measure the test's running time? We're waiting for events to be processed and use executeSoon() multiple times so this measurement doesn't really say anything about the performance of our feature but includes a number of side effects as well.

::: browser/modules/TabTitleAbridger.jsm
@@ +16,5 @@
> +  "@mozilla.org/network/effective-tld-service;1",
> +  "nsIEffectiveTLDService");
> +
> +function TabTitleAbridger(aBrowserWin) {
> +  this._gBrowser = aBrowserWin.gBrowser;

"gBrowser" is just the variable name in browser.js. It's actually a <tabbrowser> so maybe this._tabbrowser?

@@ +169,5 @@
> + * in a TabLabelModified event need to be modified by the TabTitleAbridger.
> + */
> +function DomainSets() {
> +  this._domainSets = {};
> +  this._tabsMappedToSets = new WeakMap();

Nit: shouldn't this rather be "_tabsMappedToDomains"?

@@ +268,5 @@
> +        needAbridgment = needAbridgment.concat(
> +          this.removeTab(aTab, oldTabDomain));
> +      }
> +      needAbridgment = needAbridgment.concat(this.addTab(aTab, tabDomain));
> +      return needAbridgment;

Nit: We can just do |return needAbridgment.concat(this.addTab(aTab, tabDomain))| here.
Thanks Tim.

> Regarding "test_direct_tests()" [...]

I've removed that and replaced it with a |GroupTest| object that handles those tests (in a similar array format).

Also removed the test-timing bits.
Attachment #646312 - Attachment is obsolete: true
Attachment #646312 - Flags: review?(ttaubert)
Attachment #647396 - Flags: review?(ttaubert)
(In reply to Adam [:hobophobe] from comment #151)
> I've removed that and replaced it with a |GroupTest| object that handles
> those tests (in a similar array format).

Great work, that looks good to me.
Comment on attachment 647396 [details] [diff] [review]
[module, feature tests, browser] Rework testing

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

Thanks Adam, that looks good to me. I added some suggestions that IMO clarify the code a bit but I didn't find any errors (well, except that we don't account for multiple white spaces as I wrote below). It took me quite some time to get behind the algorithms but I think I understand most of it now :)

Also it would be great if you could add some test cases to the GroupTest that covers separators surrounded by multiple white spaces.

I feel good about landing this with some/all of the suggestions applied, I just want to take a last quick look at the whole thing before I'll give a r+.

::: browser/modules/TabTitleAbridger.jsm
@@ +376,5 @@
> +    }
> +
> +    // Bail on these strings early, using the first as the basis
> +    if (chop == -1 || aNext == aTabSet.length ||
> +        tabStr.substr(0, chop + 1) != nextStr.substr(0, chop + 1)) {

With a method like "Utils.startsWith(string, prefix)" we can make this look a little nicer, I think.

@@ +427,5 @@
> +   * first separator)
> +   * @return An array containing the resulting path mode (in case it changes)
> +   * and the diverence index for the labels.
> +   */
> +  _getChopPoint: function AbridgmentTools_getChopPoint(aPathMode, aTabStr,

How about "_getCommonChopPoint"?

@@ +468,5 @@
> +      nextChop = RedundancyFinder.lastIndexOfSep(aPathMode, aNextStr,
> +                                                 nextSufPos - 1)[1] + 1;
> +    }
> +
> +    if (aTabStr.substr(aChop).length < this.MIN_CHOP) {

Nit: we don't need to actually get the substring here. We can do:

> if (aTabStr.length - aChop < this.MIN_CHOP) {

@@ +472,5 @@
> +    if (aTabStr.substr(aChop).length < this.MIN_CHOP) {
> +      aChop = RedundancyFinder.lastIndexOfSep(aPathMode, aTabStr,
> +                                              aChop - 2)[1] + 1;
> +    }
> +    if (aNextStr.substr(nextChop).length < this.MIN_CHOP) {

Nit: Also no need to get the substring:

> if (aNextStr.length - nextChop < this.MIN_CHOP) {

@@ +481,5 @@
> +  }
> +};
> +
> +let RedundancyFinder = {
> +  _isSeparator: function RedundancyFinder_isSeparator(aStr) {

We could actually remove this function if we use the regexes below...

@@ +493,5 @@
> +   * @param aStr      the string to look for a separator in
> +   * @param aStart    (optional) an index to start the search from
> +   * @return the next index of a separator or -1 for none
> +   */
> +  indexOfSep: function RedundancyFinder_indexOfSep(aPathMode, aStr, aStart) {

This function looks overly complex (but actually isn't). Also we don't account for multiple spaces surrounding separators. We could achieve this with a quite simple regex:

> function RedundancyFinder_indexOfSep(aPathMode, aStr, aStart) {
>   if (aPathMode) {
>     return aStr.indexOf('/', aStart);
>   }
>   
>   let match = aStr.slice(aStart).match(/^.+?\s+[-:>\|]+\s+/);
>   if (match) {
>     return (aStart || 0) + match[0].length - 1;
>   }
>   
>   return -1;
> }

@@ +518,5 @@
> +   * @param aNextStr  the lexicographically next string to compare with
> +   * @param aChop     the basis index, a best-known index to begin comparison
> +   * @return the index at which aStr's abridged title should begin
> +   */
> +  findFrontChop: function RedundancyFinder_findFrontChop(aPathMode, aStr,

We should maybe name this "findCommonFrontChop" or "findCommonPrefix", that's a little clearer about what it does.

@@ +524,5 @@
> +    // Advance until the end of the title or the pair diverges
> +    do {
> +      aChop = this.indexOfSep(aPathMode, aStr, aChop + 1);
> +    } while (aChop != -1 &&
> +             aStr.substr(0, aChop + 1) == aNextStr.substr(0, aChop + 1));

You could use Utils.startsWith() here as well.

@@ +545,5 @@
> +   * @param aEnd      (optional) an index to start the backwards search from
> +   * @return an array containing the endpoints of a separator (-1, -1 for none)
> +   */
> +  lastIndexOfSep: function RedundancyFinder_lastIndexOfSep(aPathMode, aStr,
> +                                                            aEnd) {

This function is also quite complex and doesn't account for multiple spaces. How about this?

> function RedundancyFinder_lastIndexOfSep(aPathMode, aStr, aEnd) {
>   if (aPathMode) {
>     let path = aStr.lastIndexOf('/', aEnd);
>     return [path, path];
>   }
> 
>   let string = aStr.slice(0, aEnd);
>   let match = string.match(/.+((\s+[-:>\|]+\s+).*?)$/);
>   if (match) {
>     let index = string.length - match[1].length;
>     return [index, index + match[2].length - 1];
>   }
>   
>   return [-1, -1];
> }

@@ +569,5 @@
> +   * @param aStr      a base string to look for a suffix in
> +   * @param aNextStr  a string that may share a common suffix with aStr
> +   * @return an index indicating the divergence between the strings
> +   */
> +  findSuffix: function RedundancyFinder_findSuffix(aPathMode, aStr, aNextStr) {

This should also rather be named "findCommonSuffix".

@@ +570,5 @@
> +   * @param aNextStr  a string that may share a common suffix with aStr
> +   * @return an index indicating the divergence between the strings
> +   */
> +  findSuffix: function RedundancyFinder_findSuffix(aPathMode, aStr, aNextStr) {
> +    let last = this.lastIndexOfSep(aPathMode, aStr)[0]

Nit: missing semicolon.

@@ +577,5 @@
> +
> +    // Is there any suffix match?
> +    if (aStr.substr(last) != aNextStr.substr(nextLast)) {
> +      return 0;
> +    }

If you'd introduce a method like "Utils.endsWith(string, suffix)" we can make this look a little nicer:

> // Is there any suffix match?
> if (!Utils.endsWith(aNextStr, aStr.slice(last))) {
>   return 0;
> }

That means we don't need the "size" and "nextLast" variables and can also simplify the do-while() condition at the bottom a lot.
Attachment #647396 - Flags: review?(ttaubert) → feedback+
Attached patch [visibleLabel] Fix New Tab label (obsolete) — Splinter Review
Missed a bug on this patch, where the New Tab label wasn't being set.  A couple of places set the |label| attribute directly.  

Unfortunately, using the property doesn't work there for unknown reasons (too early for the property to be available?).  This sets both attributes (|label| and |visibleLabel|) for those cases, but an alternative would be preferred.
Attachment #639801 - Attachment is obsolete: true
|test_about_blank| is updated to properly test against |visibleLabel| instead of |label|.

The other issue is whitespace.  The test as written expects normalization: multiple spaces (eg, setting |document.title|) are collapsed because HTML normalizes whitespace.  Directly setting the tab label (eg, by an extension) doesn't normalize, though, so another test will probably be needed.
Attachment #647396 - Attachment is obsolete: true
Attachment #647823 - Flags: review?(ttaubert)
Comment on attachment 647821 [details] [diff] [review]
[visibleLabel] Fix New Tab label

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

Good catch but I think we shouldn't do it that way. The real fix to make |tab.label = "foo"| work in gBrowser.addTab() is to do this *after* the tab has been inserted into the DOM. That happens here:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1263
Attachment #647821 - Flags: review-
Using the property as soon as the |tab| is added to the DOM didn't work.  It doesn't work until the |browser| is inserted in the DOM.  There's a note there that seems to warn this is the wrong order, though (the |notificationbox| is ancestor of the |browser|):

> // NB: this appendChild call causes us to run constructors for the
> // browser element, which fires off a bunch of notifications. Some
> // of those notifications can cause code to run that inspects our
> // state, so it is important that the tab element is fully
> // initialized by this point.
> this.mPanelContainer.appendChild(notificationbox);
Attachment #647821 - Attachment is obsolete: true
Attachment #648146 - Flags: review?(ttaubert)
Comment on attachment 648146 [details] [diff] [review]
[visibleLabel] Use label property in addTab

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

r=me with the small additions below.

::: browser/base/content/tabbrowser.xml
@@ +1324,5 @@
>  
> +            if (uriIsBlankPage)
> +              t.label = this.mStringBundle.getString("tabs.emptyTabTitle");
> +            else
> +              t.label = aURI;

Yeah... I didn't try my approach with your second patch applied. When setting .label the TabTitleAbridger receives the TabLabelModified event and tries to get the tab's currentURI which obviously doesn't succeed at that point, yet.

Please add braces for both branches here. Please also add a comment that says why we're setting the label *after* the tab has been initialized and inserted into the DOM.
Attachment #648146 - Flags: review?(ttaubert) → review+
Attached patch [visibleLabel] Address feedback (obsolete) — Splinter Review
Attachment #648146 - Attachment is obsolete: true
For directly setting the label, whitespace is significant.

Another problem we may need address is tabs that throw in the inner |try| in |DomainSets::_getDomainForTab| (if the |linkedBrowser.currentURI| is not a |nsStandardURL|).  If they can occur, the |visibleLabel| won't get updated for that case (and an error will occur).

The current behavior looks like:

1. event(TabLabelModified).preventDefault
2. domainSets.updateTab(tab)
3. tabDomain = _getDomainForTab(tab) [= undefined]
4. oldTabDomain = _tabsMappedToDomains.get(tab) [= undefined]
5. return _domainSets[undefined] [= undefined]
6. _abridgeTabTitles(undefined)
7. for (let tabSet of undefined) [TypeError]

I'm guessing that this can occur in practice, albeit rarely.  It would require an extension with its own |nsIURI| implementation, though.  The non-|nsStandardURL| |nsIURI|s that we already have are covered by |DomainSets._noHostSchemes|?
Attachment #647823 - Attachment is obsolete: true
Attachment #647823 - Flags: review?(ttaubert)
Attachment #648583 - Flags: review?(ttaubert)
To deal with the aforementioned problem with strange |nsIURI|s:

1. |DomainSets| will pad the returned array with |null| where it would use an empty array before.  We want the padding to distinguish on |updateTab| which set is |null|, rather than just seeing a single-member array.
2. The event listener will check for those cases and directly set the |visibleLabel| for the tab when needed.
3. |_abridgeTabTitles| will check for existence before processing a |tabSet|.

We could do this for the early tabs from |gBrowser.addTab|, moving the |label| setting back up to just after the tab is inserted in the DOM.  Not sure there's a gain there though.
Attachment #648583 - Attachment is obsolete: true
Attachment #648583 - Flags: review?(ttaubert)
Attachment #648845 - Flags: review?(ttaubert)
Comment on attachment 648845 [details] [diff] [review]
[module, feature tests, browser] Have DomainSets methods deal out nulls as placeholders

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

I don't really like returning [null] and checking the return values of addTab() and updateTab() likes this. There are some more options:

1) We could just return browser.currentURI.spec if we're not able to determine a real hostname. So we don't have to care about those special cases at all as they should happen rarely anyway.

2) We could make add/update/removeTab() accept the array of sets to update as a second argument. The function itself could then return true or false based on whether we want to set .visibleLabel = .label

3) We could somehow make add/updateTab() throw if we need to set .visibleLabel = .label

------------

[2] + [3] do not sound very compelling to me. How about [1]? That's certainly an easy way to deal with non-standard URLs that most of the users will probably never hit.

::: browser/modules/TabTitleAbridger.jsm
@@ +286,5 @@
> +   * Given a tab, determine the URI scheme or host to categorize it.
> +   * @param aTab The tab to get the domain for
> +   * @return The domain or scheme for the tab
> +   */
> +  _getDomainForTab: function DomainSets_getDomainForTab(aTab) {

We could make this function look a little less complicated if we don't nest the try-catch statements:

> function DomainSets_getDomainForTab(aTab) {
>   let browserURI = aTab.linkedBrowser.currentURI;
>   if (browserURI.scheme in this._noHostSchemes) {
>     return browserURI.scheme;
>   }
>   
>   try {
>     return gETLDService.getBaseDomain(browserURI);
>   } catch (e) { /* ... */ }
>   
>   try {
>     return browserURI.host;
>   } catch (e) { /* ... */ }
>   
>   return null; // or return browserURI.spec;
> }

@@ +617,5 @@
> +   * @param aStr    string to check the prefix of
> +   * @param aPrefix prefix to check against
> +   * @return boolean value, true if prefix matches, false otherwise
> +   */
> +  startsWith: function RedundancyFinder_startsWith(aStr, aPrefix) {

Some days ago, String.startsWith() and .endsWith() landed... Let's use that!

@@ +627,5 @@
> +   * @param aStr    string to check the prefix of
> +   * @param aSuffix suffix to check against
> +   * @return a boolean value, true if suffix matches, false otherwise
> +   */
> +  endsWith: function RedundancyFinder_endsWith(aStr, aSuffix) {

(see above)
Attachment #648845 - Flags: review?(ttaubert)
Thank you, Tim!

Went with your first suggestion for failure case for |_getDomainForTab|, and flattened it too.

Awesome news about the |String| methods!
Attachment #648845 - Attachment is obsolete: true
Attachment #653927 - Flags: review?(ttaubert)
Comment on attachment 653927 [details] [diff] [review]
[module, feature tests, browser] Rework DomainSets._getDomainForTab, move to new String methods

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

Awesome, thank you! We should push it to try (with all mochitests and the whole talos suite, I think) before checking it in.
Attachment #653927 - Flags: review?(ttaubert) → review+
Comment on attachment 653927 [details] [diff] [review]
[module, feature tests, browser] Rework DomainSets._getDomainForTab, move to new String methods

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

::: browser/modules/TabTitleAbridger.jsm
@@ +229,5 @@
> +   * from, or an empty array if the tab was not removed from a domain set.
> +   */
> +  removeTab: function DomainSets_removeTab(aTab, aTabDomain) {
> +    let oldTabDomain = aTabDomain || this._tabsMappedToDomains.get(aTab);
> +    let index = this._domainSets[oldTabDomain].indexOf(aTab);

Looks like |this._domainSets[oldTabDomain]| can be undefined in some cases, as the try run revealed.
Got a bit overzealous in removing unneeded checks on the result of |_getDomainForTab| and hit a needed one.  Fixed.
Attachment #653927 - Attachment is obsolete: true
Attachment #654343 - Flags: review?(ttaubert)
Comment on attachment 654343 [details] [diff] [review]
[module, feature tests, browser] Restore removeTab's check of membership in _domainSets

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

Let's push it to try again, please.
Attachment #654343 - Flags: review?(ttaubert) → review+
(In reply to Adam [:hobophobe] from comment #169)
> https://tbpl.mozilla.org/?tree=Try&rev=42113653d601

Try wait times are terrible nowadays... Looks all green to me. Wanna prepare the patches for checkin? :)
Attachment #648573 - Attachment is obsolete: true
Sorry, backed out for adding a new intermittent failure:
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js | an unexpected uncaught JS exception reported through window.onerror - ReferenceError: executeSoon is not defined at chrome://mochitests/content/browser/browser/base/content/test/browser_bug583890.js:17 
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=14661365&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5c7b2f7d61
The screenshot included in the timeout failure shows the second tab having the default |visibleLabel|.  That means the title was never changed.  My hope is that by delaying setting the title using |executeSoon| will give enough time this won't occur.  

I've also added |skipAnimation| to the |addTab| calls, which should help a little.

Trying on the failing platforms:
https://tbpl.mozilla.org/?tree=Try&rev=e8435218078b
Attachment #654883 - Attachment is obsolete: true
So opt-only doesn't hit PGO, but for now will just see how opt does.
Trying to strengthen this a bit more.  This changes the test's |setTitleForTab| to:

1. Only begin polling for the expected label (via |waitForTabLabel|) after receiving the |TabLabelModified| event, which is what will kick off the abridgment.
2. Wait until |aTab.linkedBrowser.contentDocument| is exposed, on the off chance that we could have tried to set the title before it was possible.

Pushed it to try:
https://tbpl.mozilla.org/?tree=Try&rev=4e91228ee462
Attachment #655160 - Attachment is obsolete: true
(Purely as an aside - thanks for picking up this bug, Adam! I'm looking forward to it, and I appreciate your work on it.)
(In reply to Adam [:hobophobe] from comment #178)
> https://tbpl.mozilla.org/?tree=Try&rev=4e91228ee462

Looking "green"
Comment on attachment 656576 [details] [diff] [review]
[module, feature tests, browser] Only poll for expected label after TabLabelModified

Thanks, guys!  

Hopefully Tim is busy relaxing and won't review this until Wednesday at the earliest, but I'll go ahead and flag it anyway.
Attachment #656576 - Flags: review?(ttaubert)
Attachment #656576 - Flags: review?(ttaubert) → review+
Thanks Tim!
Attachment #656576 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8c3df98a9f06
https://hg.mozilla.org/mozilla-central/rev/62ef2365014e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Sorry had to backout for a couple of different intermittent failures in browser_bug583890.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15265623&tree=Mozilla-Inbound
and
https://tbpl.mozilla.org/php/getParsedLog.php?id=15267642&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f81207015b5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 791796
This fixes bug 791698, where tabview tests would show uncaught errors.  Individual tabview tests wouldn't manifest the behavior, but it looked to be due to short-lived tabs that weren't ripe enough to have a label added.

Haven't been able to reproduce bug 791672 yet.  Asking there for more details.
Attachment #660919 - Attachment is obsolete: true
(In reply to Adam [:hobophobe] from comment #188)
> This fixes bug 791698, where tabview tests would show uncaught errors. 
> Individual tabview tests wouldn't manifest the behavior, but it looked to be
> due to short-lived tabs that weren't ripe enough to have a label added.

Yeah, that fixes the problem for me as well. Can't we just do

> let tabStr = aTabSet[aIndex].label || "";

here? That seems a little easier for a fallback. Also we probably don't need the early return if that mostly fixes test runs and not real user interactions.
Depends on: 792016
This regresses the tab titles for me, see bug 792016.
This also regressed the Talos MaxHeap number by about half a percent; in other words, it was about a half-percent increase in the maximum amount of memory allocated by malloc over the course of a relatively simple Firefox run.  (The regression was fixed by the backout; see the various Talos notification emails on mozilla.dev.tree-management.)
Thanks, Tim; this incorporates your fix for |_abridgePair|.

While looking at the other bug, I found a different problem in |handleEvent|, that |updateSets| could be undefined; this would happen when a tab was hidden when the |TabLabelModified| event was processed.

Fix is to initialize |updateSets| to an empty array literal.

I looked a little at memory consumption, but not sure how lean this feature should be?  If I read the numbers correctly, it was ~1/3 MiB increase in the heaviest case (which is approximately the figure I see for the module in about:memory).
Attachment #661964 - Attachment is obsolete: true
This is a debug patch to be applied atop the other two, in case anyone who can reproduce the bugs wants to build it themselves.  Will add links and instructions for the try builds to the other bugs.
See bug 773845 for an investigation into a maxheap regression for Social modules. The conclusion there was that there was some unavoidable overhead from adding additional JS modules. How do the numbers here compare to those? Is the TabTitleAbridger's compartment size comparable to other simple modules?
Thanks Gavin, that was a good example.

A valid comparison here seems to be NewTabUtils.jsm, which has similar module file size, and similar compartment footprint:

0.30 MB (00.18%) -- compartment([System Principal], resource:///modules/NewTabUtils.jsm)
├──0.13 MB (00.08%) ++ gc-heap
├──0.13 MB (00.08%) ── analysis-temporary
├──0.02 MB (00.01%) ── shapes-extra/compartment-tables
├──0.01 MB (00.01%) ── objects/slots
├──0.01 MB (00.01%) ── other-sundries
└──0.01 MB (00.01%) ── script-data

0.27 MB (00.17%) -- compartment([System Principal], resource:///modules/TabTitleAbridger.jsm)
├──0.13 MB (00.08%) ── analysis-temporary
├──0.12 MB (00.07%) ++ gc-heap
├──0.01 MB (00.01%) ── other-sundries
├──0.01 MB (00.01%) ── script-data
└──0.01 MB (00.01%) ── shapes-extra/compartment-tables

It seems reasonable.  Analysis-temporary may stick around more than some modules, because this module gets used during the browsing activity.
FWIW people is misconstruing the Safari feature as "censorship"[1].

I stumbled into it as it somehow made the 25th entry[2] on http://reddit.com/r/technology/

1| http://safari-censor.blogspot.co.uk/2012/09/safari-censors-google-in-tab-titles.html
2| http://reddit.com/r/technology/comments/1097fn/safari_censors_google_in_tab_titles/
So the real problem behind bug 791698 was that we'd occasionally get a |TabLabelModified| after a tab had been shown (so |tab.hidden| was false), but directly before the |TabShown| event.  

This caused the tab to be added to a domainset *twice*, and when it was eventually removed, it would be only removed once leaving a second copy to muck things up.

The change here is to check for membership in |addTab|.  An alternative would convert the individual domainsets into |Set| objects, but would require an array comprehension on each one for the |AbridgmentTools| to use them properly.  Another alternative would make |addTab| into |_addTab| and have consumers always call |updateTab| instead, which already proxies |addTab| and would prevent duplicates.

I've left the earlier fix intact.
Attachment #662754 - Attachment is obsolete: true
Adam, did you intend to request review/feedback?
The logic needed to be |if/if| rather than |if/else if|.

Not sure of the best move here, as I'm not certain that the blank title/Connecting bug is fixed.

In bug 792016, Ehsan did not see the problem in a build that included the earlier fix for bug 791698, but that could mean either:

1. fixed the problem
2. papered over it (unapparent in previously common cases, but still there and might appear in more exotic circumstances)
3. the problem just didn't manifest itself
Attachment #668234 - Attachment is obsolete: true
Attachment #668691 - Flags: review?(ttaubert)
FWIW, I wasn't seeing that problem too frequently, iirc.
Whiteboard: [parity-safari] → [parity-safari] [Advo]
Comment on attachment 668691 [details] [diff] [review]
[module, feature tests, browser] Fix logic of previous patch

Hey Adam, sorry, this completely slipped off my plate... The patch does unfortunately not apply anymore.

Serious question: is this still something we want to have in the product by default? With some distance and time passed I think this should probably be an add-on.
Attachment #668691 - Flags: review?(ttaubert)
I'd argue "yes", so many sites clutter their title tags with the site name and category hierarchies, making tabs indistinguishable.
(and: Safari for iPad does it, so "parity-safari"?)
(In reply to Tim Taubert [:ttaubert] from comment #201)
> Comment on attachment 668691 [details] [diff] [review]
> [module, feature tests, browser] Fix logic of previous patch
> 
> Hey Adam, sorry, this completely slipped off my plate... The patch does
> unfortunately not apply anymore.
> 
> Serious question: is this still something we want to have in the product by
> default? With some distance and time passed I think this should probably be
> an add-on.

Yeah, I think we should keep pushing forward with it. It solves a problem that faces over 80% of our users and the web hasn't been moving away from long and redundant title text.
Attached patch [visibleLabel] unbitrot (obsolete) — Splinter Review
Attachment #654881 - Attachment is obsolete: true
I had turned an early version into an add-on.  This version could become one without much trouble.  

That said, I think it should be a built-in.  Add-ons seem to fit two cases best (though values like privacy can overrule them): 

a. A niche audience (Firebug for developers, or a site-specific add-on)
b. A broad audience that will seek it out (Adblock Plus)

TabView, which will move to an add-on, fits [a], targeting power users.  A few years ago Sync might have been an add-on for users with multiple computers.  There were add-ons for that.  Today it fits as a built-in, due to the proliferation of mobile computing.

This feature has a broad audience, but few will feel compelled to seek a fix.  Compare to the new tab page: there were add-ons in that space first, but as a built-in there are many more users than ever installed the add-ons.  

As a built-in, this would hit all but the few who disable it.  An add-on would hit very few users.  Most users have at least one add-on [1] (2011 data), but the curve centers around a few total.

Almost nothing hits the low-count users.  The high-count users?  Few will recognize the problem enough to look for an add-on.  It is a pebble in your shoe, not a paper cut.  A small dull pain or distraction, until you step on it directly (close the wrong tab, or type in the wrong bug or chat).

1. https://blog.mozilla.org/addons/2011/06/21/firefox-4-add-on-users/
Attachment #668691 - Attachment is obsolete: true
Attachment #726457 - Flags: review?(ttaubert)
This should definitely be a built-in function. There will always be edge cases, but in general this does more good than not having it.
Sad faces, it looks like this bug got forgotten about... What is the next step here?
Flags: needinfo?(unusualtears)
Flags: needinfo?(ttaubert)
Flags: needinfo?(shorlander)
Attached patch Unbitrot visible label patch (obsolete) — Splinter Review
Attachment #726395 - Attachment is obsolete: true
Attached patch Unbitrot module patch (obsolete) — Splinter Review
I feel like the bug has been idle long enough to warrant a re-review on both patches. 

I'll give them another look over (they apply/all tests pass now) and think about how to move forward here.

I'm not sure if Tim wants/has time to pick back up on such an old review, or if a new reviewer is best. I'll leave that up to him.
Attachment #726457 - Attachment is obsolete: true
Attachment #726457 - Flags: review?(ttaubert)
Flags: needinfo?(unusualtears)
Thanks for picking this up again, Adam. I think we should start with landing the "visible label" patch first. Maybe we could even move this to a separate bug blocking this one just for clarity? I can promise a fast review. After we got this dependency cleared we can then take another look at the TabTitleAbridger and its algorithm. Sound good?
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #210)
> I think we should start with landing
> the "visible label" patch first. Maybe we could even move this to a separate
> bug blocking this one just for clarity?

Filed bug 943820.
Adds test/fixes bug to avoid an intermediate tab (one whose lexical neighbors both had some amount of prefix in common with it) losing a longer chop due to having a shorter common prefix with its next neighbor than previous neighbor (the "hotel" test set in the test).

Also adds tests to make sure that tabs get reset and abridged when disabling/enabling the feature via the preference.
Attachment #8338998 - Attachment is obsolete: true
Attachment #8339009 - Attachment is obsolete: true
Attachment #8341425 - Flags: review?(ttaubert)
Flags: needinfo?(shorlander)
Comment on attachment 8341425 [details] [diff] [review]
Update visibleLabel test to disable abridger, add new feature tests

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

Looks good in general but there's quite some parts to clean up. I will still need to take a closer look at |AbridgmentTools| and |RedundancyFinder| again but I figured you could get started on fixing the other stuff in the meanwhile :)

::: browser/base/content/test/general/browser.ini
@@ +184,5 @@
>  [browser_bug580956.js]
>  [browser_bug581242.js]
>  [browser_bug581253.js]
>  [browser_bug581947.js]
> +[browser_bug583890.js]

We should give the test a better name. browser_cropTitleRedundancy.js or the like.

::: browser/base/content/test/general/browser_bug583890.js
@@ +15,5 @@
> +function waitForTabLabel(aTab, aExpectedLabel, aCallback) {
> +  if (aTab.visibleLabel == aExpectedLabel) {
> +    executeSoon(aCallback);
> +  } else {
> +    executeSoon(() => waitForTabLabel(aTab, aExpectedLabel, aCallback));

Shouldn't the tab label change immediately? Why don't we wait for a specific event instead of polling?

@@ +30,5 @@
> +  let addedTabs = [];
> +  for (let i = aCount; i > 0; i--) {
> +    addedTabs.push(gBrowser.addTab(null, {skipAnimation: true}));
> +  }
> +  executeSoon(() => aCallback(addedTabs));

No need to use executeSoon() here. In combination with the comment at the bottom of the file (regarding Tasks and Promises) this should be a Task that finishes after all these tabs have finished loading?

@@ +54,5 @@
> +  }
> +  // On the off chance we're trying to set the title on a too-young tab,
> +  // we wait until it's mature
> +  function doSetTitle() {
> +    if (aTab.linkedBrowser && aTab.linkedBrowser.contentDocument) {

.linkedBrowser should always be set except for dying tabs. Instead of using executeSoon() to check for .contentDocument you should probably check document.readyState and wait for "load" if that's not equal to "complete". OTOH, shouldn't we wait for tab to finish loading after creating them instead of shifting responsibility to setTitleForTab()?

@@ +60,5 @@
> +    } else {
> +      executeSoon(doSetTitle);
> +    }
> +  }
> +  executeSoon(doSetTitle);

Why do we start off a timeout?

@@ +76,5 @@
> +function setLabelForTab(aTab, aTitle, aExpectedLabel, aCallback) {
> +  executeSoon(function () {
> +    aTab.label = aTitle;
> +    waitForTabLabel(aTab, aExpectedLabel, aCallback);
> +  });

Why do we use executeSoon() here?

@@ +296,5 @@
> +      }
> +    }
> +    if (this.labels.length) {
> +      this.tabs.push(gBrowser.addTab(
> +        "data:text/html,<title>" + this.labels.shift() + "</title>",

Nit: "data:text/html;charset=utf-8,..." to avoid some console warnings

@@ +416,5 @@
> +
> +function test() {
> +  waitForExplicitFinish();
> +  runNextTest();
> +}

Instead of implementing your own test runner, can you please use add_task() and thus use Tasks and Promises?

::: browser/modules/TabTitleAbridger.jsm
@@ +17,5 @@
> +  "nsIEffectiveTLDService");
> +
> +function TabTitleAbridger(aBrowserWin) {
> +  this._tabbrowser = aBrowserWin.gBrowser;
> +}

We should export an instance that has .init() and .destroy() as the only methods. We could do it like:

function TabTitleAbridger(aBrowserWin) {
  let internal = new TabTitleAbridgerInternal(aBrowserWin);
  let external = {};

  for (let method of ["init", "destroy"]) {
    external[method] = internal[method].bind(internal);
  }

  return Object.freeze(external);
}

function TabTitleAbridgerInternal() { ... }
TabTitleAbridgerInternal.prototype = { ... }

@@ +33,5 @@
> +    "TabClose",
> +    "TabLabelModified"
> +  ],
> +
> +  init: function TabTitleAbridger_Initialize() {

|init: function () {|

We don't need to name anonymous functions (that are methods) anymore. Stack traces will be properly annotated by the JS Engine. Can you please fix this for the whole file and remove them all?

@@ +42,5 @@
> +      this._addListeners();
> +    }
> +  },
> +
> +  destroy: function TabTitleAbridger_Destroy() {

Can you please drop all the redundant function names for "methods"? We don't really do that anymore now that we have better JS engine support for readable stack traces.

@@ +168,5 @@
> + * Maintains a mapping between tabs and domains, so that only the tabs involved
> + * in a TabLabelModified event need to be modified by the TabTitleAbridger.
> + */
> +function DomainSets() {
> +  this._domainSets = {};

This should be a Map. |this._domainSets = new Map();|

@@ +169,5 @@
> + * in a TabLabelModified event need to be modified by the TabTitleAbridger.
> + */
> +function DomainSets() {
> +  this._domainSets = {};
> +  this._tabsMappedToDomains = new WeakMap();

Maybe "_tabsPerDomain"?

@@ +175,5 @@
> +
> +DomainSets.prototype = {
> +  _noHostSchemes: {
> +    chrome: true,
> +    file: true,

This should be a Set, I think. |_noHostSchemes: new Set(["chrome", "file", ...])|

@@ +184,5 @@
> +
> +  destroy: function DomainSets_destroy() {
> +    delete this._domainSets;
> +    delete this._tabsMappedToDomains;
> +  },

This method doesn't seem useful. If all references to the DomainSet itself have been removed we shouldn't need to delete those properties.

@@ +197,5 @@
> +  bootstrap: function DomainSets_bootstrap(aVisibleTabs) {
> +    let needAbridgment = [];
> +    for (let tab of aVisibleTabs) {
> +      let domainSet = this.addTab(tab)[0] || null;
> +      if (domainSet && needAbridgment.indexOf(domainSet) == -1) {

needAbridgment should be a Set and thus bootstrap() should return a Set.

@@ +216,5 @@
> +    if (!this._domainSets.hasOwnProperty(tabDomain)) {
> +      this._domainSets[tabDomain] = [];
> +    }
> +    if (this._domainSets[tabDomain].indexOf(aTab) == -1) {
> +      this._domainSets[tabDomain].push(aTab);

I think this should be a WeakMap to not hold strong refs to all tabs in DomainSets.

let domainSet = this._domainSets.get(tabDomain);
if (!domainSet.has(aTab)) {
  domainSet.set(aTab, true);
}

@@ +245,5 @@
> +      // Keep the sets clean of empty domains
> +      delete this._domainSets[oldTabDomain];
> +      return [];
> +    }
> +    return [this._domainSets[oldTabDomain]];

Should return a Set?

@@ +257,5 @@
> +   */
> +  updateTab: function DomainSets_updateTab(aTab) {
> +    let tabDomain = this._getDomainForTab(aTab);
> +    let oldTabDomain = this._tabsMappedToDomains.get(aTab);
> +    if (oldTabDomain != tabDomain) {

Maybe:

if (oldTabDomain == tabDomain) {
  // No change was needed
  return [this._domainSets[tabDomain]];
}

To have less indentation and make the code a tad more readable.
Attachment #8341425 - Flags: review?(ttaubert) → feedback+
Thanks, Tim.

Only real snag I hit was with converting domain sets to |WeakMaps|: this means we cannot directly determine when a domain set is empty, so I added a new |Map| (|_DomainCounts|) to track that and remove empty ones.

Also note |_weakMapToTabArray| which is needed to determine the tabs being abridged from a given domain set (runs over |visibleTabs| and asks the |WeakMap| if it |has| each one, pushing them into an |Array|).

Good to know that naming the functions isn't needed anymore. The names were eating a lot of columns/making thing less readable.

> ::: browser/base/content/test/general/browser_bug583890.js
> @@ +15,5 @@
> > +function waitForTabLabel(aTab, aExpectedLabel, aCallback) {
> > +  if (aTab.visibleLabel == aExpectedLabel) {
> > +    executeSoon(aCallback);
> > +  } else {
> > +    executeSoon(() => waitForTabLabel(aTab, aExpectedLabel, aCallback));
> 
> Shouldn't the tab label change immediately? Why don't we wait for a specific event instead of polling?

This is the only remaining use of |executeSoon| (the function is now called |promiseVisibleLabel|). We could add an event when the abridger actually changes the |visibleLabel|, but not sure what other use it would have besides easing this part of the test or use by extensions?

I will give this another pass tomorrow.
Attachment #8341425 - Attachment is obsolete: true
Attached patch Minor cleanups (obsolete) — Splinter Review
Attachment #8349200 - Attachment is obsolete: true
Attachment #8349811 - Flags: review?(ttaubert)
Comment on attachment 8349811 [details] [diff] [review]
Minor cleanups

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

::: browser/base/content/test/general/browser_cropTitleRedundancy.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const ABRIDGMENT_PREF = "browser.tabs.cropTitleRedundancy";

Please ensure that this pref is correctly reset by adding this at the top:

add_task(function setup() {
  registerCleanupFunction(() => {
    Services.prefs.clearUserPref("browser.tabs.cropTitleRedundancy");
  });
});

Additionally this should also save gBrowser.visibleLabel on setup and restore that value when the test has finished to clean up properly.

@@ +34,5 @@
> +  let tab = gBrowser.addTab(aURL, {skipAnimation: true});
> +  let browser = tab.linkedBrowser;
> +  if (browser.contentDocument.readyState == "complete") {
> +    deferred.resolve(tab);
> +  } else {

This looks a little complicated. As long we're not passing "about:newtab" to promiseLoadedTab (which we're not) we can just remove the readyState check.

@@ +38,5 @@
> +  } else {
> +    browser.addEventListener("load", function onLoad() {
> +      browser.removeEventListener("load", onLoad, true);
> +      deferred.resolve(tab);
> +    }, true);

This could just be:

whenTabLoaded(tab, () => deferred.resolve(tab));

@@ +51,5 @@
> + * @param aExpectedLabel the value the tab's label must match
> + */
> +function setTitleForTab(aTab, aTitle, aExpectedLabel) {
> +  aTab.linkedBrowser.contentDocument.title = aTitle;
> +  yield promiseVisibleLabel(aTab, aExpectedLabel);

This needs to be |return promiseVisibleLabel(...|.

@@ +62,5 @@
> + * @param aExpectedLabel the value the tab's label must match
> + */
> +function setLabelForTab(aTab, aTitle, aExpectedLabel) {
> +  aTab.label = aTitle;
> +  yield promiseVisibleLabel(aTab, aExpectedLabel);

return promiseVisibleLabel(...

@@ +251,5 @@
> + * Run tests on one part of the above array of tests.
> + * @param aGroup       The subarray to run tests with
> + * @param aGroupNumber The index of the subarray (for logging purposes)
> + */
> +function groupTest(aGroup, aGroupNumber) {

function* groupTest(... as it seems to be a generator.

How about also going backwards to test whether tab label adjustment on tab removal works?

@@ +278,5 @@
> +    gBrowser.removeTab(tabs.pop());
> +  }
> +}
> +
> +add_task(function test_about_blank() {

add_task(function* test_about_blank() {

@@ +290,5 @@
> +  gBrowser.removeTab(tab2);
> +  gBrowser.removeTab(tab3);
> +});
> +
> +add_task(function test_two_tabs() {

add_task(function* test_two_tabs() {

@@ +302,5 @@
> +  yield promiseVisibleLabel(tab1, "Foo - Bar - Baz");
> +  is (tab1.visibleLabel, "Foo - Bar - Baz", "Single tab has full title");
> +});
> +
> +add_task(function test_direct_label() {

add_task(function* test_direct_label() {

@@ +326,5 @@
> +  is (tab1.visibleLabel, "Foo  - Bar -  Baz", "Single tab has full title");
> +  gBrowser.removeTab(tab2);
> +});
> +
> +add_task(function test_groups() {

add_task(function* test_groups() {

@@ +328,5 @@
> +});
> +
> +add_task(function test_groups() {
> +  for (let i = 0; i < groups.length; i++) {
> +    yield groupTest(groups[i], i);

You can do:

  yield groupTest(...groups[i], i);

to change groupTest's function signature to:

  function groupTest(aLabels, aExpectedLabels, aGroupNumber) {

Also... if groupTest() is just another iterator, wouldn't we need to do |yield* groupTest(...| to delegate? Maybe Task.jsm handles iterables somehow because I just assume you made sure the test actually runs...

::: browser/modules/TabTitleAbridger.jsm
@@ +61,5 @@
> +
> +  /**
> +   * Preference observer
> +   */
> +  observe: function(aSubject, aTopic, aData) {

TBH, I don't think it's worth having and maintaining all the code to be able to switch this on and off while the browser is running. We can have this simpler with just requiring a restart and having .init() bail out if cropping titles is disabled. (It would actually apply to all newly opened windows.)

@@ +115,5 @@
> +        tab.visibleLabel = tab.label;
> +        break;
> +      case "TabLabelModified":
> +        if (!tab.hidden && !tab.pinned) {
> +          aEvent.preventDefault();

Please mention why we're calling .preventDefault() here and what effect that has.

@@ +119,5 @@
> +          aEvent.preventDefault();
> +          updateSets = this._domainSets.updateTab(tab);
> +        } else {
> +          updateSets = new Set();
> +        }

We could get rid of this else branch and make the method a little clearer by initializing |updateSets| with |new Set()|, no?

@@ +130,5 @@
> +   * Make all tabs have their visibleLabels be their labels.
> +   */
> +  _resetTabTitles: function() {
> +    // We're freshly disabled, so reset unpinned, visible tabs (see handleEvent)
> +    for (let tab of this._tabbrowser.visibleTabs) {

Instead of iterating over all tabs here it would be better to use a for() loop and start with i=gBrowser._numPinnedTabs to skip pinned tabs. Without the pref observer we wouldn't need to have that function all, wouldn't we?

@@ +168,5 @@
> +    let tabArray = [];
> +    if (!aDomainSet) {
> +      return tabArray;
> +    }
> +    for (let tab of this._tabbrowser.visibleTabs) {

Same comment about skipping pinned tabs here. Maybe introduce some helper function/generator that does that. OTOH, we could probably let a DomainSet be a normal Map and remove some awkward code below. removeTab() keeps track of everything anyway so there probably isn't much need for using a WeakMap. Sorry, I think I suggested using WeakMaps before.

@@ +180,5 @@
> +  /**
> +   * Abridges the tabs in the domain sets in the abridgment Set.
> +   * @param aAbridgmentSet Set of domain sets (WeakMaps) needing abridgment
> +   */
> +  _abridgeTabTitles: function(aAbridgmentSet) {

Nit: could you please reverse the code order of _abridgeTabTitles(), _domainSetToTabArray(), and _applyAbridgment()? It would make the code a little easier to read.

@@ +240,5 @@
> +      domainSet.set(aTab, true);
> +      this._tabsToDomains.set(aTab, tabDomain);
> +      let domainCount = this._domainCounts.get(tabDomain) || 0;
> +      this._domainCounts.set(tabDomain, domainCount + 1);
> +    }

Using a Map() for a DomainSet we can probably get rid of _domainCounts again and not use .set(tab, true).

@@ +409,5 @@
> +    // Bail on these strings early, using the first as the basis
> +    if (chop == -1 || aNext == aTabSet.length || nextStr === undefined ||
> +        !nextStr.startsWith(tabStr.substr(0, chop + 1))) {
> +      chop = aChopList[aIndex];
> +      if (aNext != aTabSet.length) {

You mean (aNext < aTabSet.length), right?

@@ +436,5 @@
> +   * @param aStr    Label string to check against
> +   * @param aStart  First item to check for proxying
> +   * @return The index of the next different tab.
> +   */
> +  _nextUnproxied: function(aTabSet, aTabStr, aStart) {

Unproxied is a little to fancy as a word, it doesn't clearly convey what we're doing here. How about something that says "skip tabs with labels that match the given string"?

@@ +444,5 @@
> +      if (aStart < aTabSet.length) {
> +        nextStr = aTabSet[aStart].label;
> +      }
> +    }
> +    return aStart;

How about:

while (aStart < aTabSet.length && aTabStr == aTabSet[aStart].label) {
  aStart++;
}

return aStart;
Attachment #8349811 - Flags: review?(ttaubert) → feedback+
Thanks, Tim!

(In reply to Tim Taubert [:ttaubert] from comment #216)
> ::: browser/base/content/test/general/browser_cropTitleRedundancy.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +const ABRIDGMENT_PREF = "browser.tabs.cropTitleRedundancy";
> 
> Please ensure that this pref is correctly reset by adding this at the top:

I would do that, but: 

> ::: browser/modules/TabTitleAbridger.jsm
> @@ +61,5 @@
> > +
> > +  /**
> > +   * Preference observer
> > +   */
> > +  observe: function(aSubject, aTopic, aData) {
> 
> TBH, I don't think it's worth having and maintaining all the code to be able
> to switch this on and off while the browser is running. We can have this
> simpler with just requiring a restart and having .init() bail out if
> cropping titles is disabled. (It would actually apply to all newly opened
> windows.)

I removed that, so the test doesn't need to touch the pref at all. I'll have it reset if I add a test that disables the pref and opens a new window to test that it actually disables. Not sure if that would be a useful test (I made a run at it, but the window/module wasn't getting ready before the test began (even with, e.g., |whenNewWindowLoaded|), so I'd need to dig at it a bit to get it working).

I also removed the modification to the |browser/base/content/test/general/browser_visibleLabel.js| that turned this feature off while running that test, as it won't do anything (the tests pass even when the abridger is turned on). If a test *needs* to disable the abridger, I guess it will need to also run the tests in a new window.

> @@ +436,5 @@
> > +   * @param aStr    Label string to check against
> > +   * @param aStart  First item to check for proxying
> > +   * @return The index of the next different tab.
> > +   */
> > +  _nextUnproxied: function(aTabSet, aTabStr, aStart) {
> 
> Unproxied is a little to fancy as a word, it doesn't clearly convey what
> we're doing here. How about something that says "skip tabs with labels that
> match the given string"?

Went with |_skipUntilDifferent|, but suggestions welcome (|_skipToDifferent|? |_skipIdenticals|?).

> [...]
> > +    for (let tab of this._tabbrowser.visibleTabs) {
> [...]
> Instead of iterating over all tabs here it would be better to use a for()
> loop and start with i=gBrowser._numPinnedTabs to skip pinned tabs.

The only place that sort of loop was still used (the other places already removed with the removal of the pref observer) was in |_domainSetToTabArray|, which went away as it made more sense to do an array comprehension over the domain set (Set) directly in |_abridgeTabTitles|.

> @@ +240,5 @@
> > +      domainSet.set(aTab, true);
> > +      this._tabsToDomains.set(aTab, tabDomain);
> > +      let domainCount = this._domainCounts.get(tabDomain) || 0;
> > +      this._domainCounts.set(tabDomain, domainCount + 1);
> > +    }
> 
> Using a Map() for a DomainSet we can probably get rid of _domainCounts again
> and not use .set(tab, true).

I'm guessing you meant Set() based on that comment about not using |.set(tab, true)|. I made it a Set().

Pretty sure I hit everything else.
Attachment #8349811 - Attachment is obsolete: true
Attachment #8377361 - Flags: review?(ttaubert)
Attachment #8377361 - Flags: review?(ttaubert)
Assignee: unusualtears → nobody
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-safari
Whiteboard: [parity-safari] [Advo] → [Advo]
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 36 votes and 68 CCs.
:dao, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dao+bmo)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.