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.

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.

Unassigning self until Firefox 4 is shipped, unless it becomes a blocker. Community contributors can of course pick this up.

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.

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.

Comment on attachment 512937 [details] [diff] [review] WIP: Initial attempt at a patch, with a few basic test cases. feedback- for RTL issues.

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.

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.

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.

(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.

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.

Does this new patch need to have a reviewer added? :)

(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 '/'.

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.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9673f64f54cc

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

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.

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.

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

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.

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.

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.

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

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.

(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).

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).

(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.

(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.

(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.

(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.

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.

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.

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.

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?

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.

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.

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()

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.

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.

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.

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.

Tim, Dao: Can you give an ETA on the review for this?

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.

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.

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.

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.

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 :)

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?

Minor cleanup (mostly replacing "in" operator with hasOwnProperty where needed).

> [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.

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.

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.

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).

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.

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.

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.

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.

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.

Comment on attachment 637355 [details] [diff] [review] [TabAttrModified] Add detail to the TabAttrModified events, add test. I think we don't need this anymore.

(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.

(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.

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.

Comment on attachment 639164 [details] [diff] [review] [visibleLabel] Add TabLabelModified test, address feedback Review of attachment 639164 [details] [diff] [review]: ----------------------------------------------------------------- I like this :)

\o/

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.

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.

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.

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.

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.)

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.

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.

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.

(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.

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.

|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.

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

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);

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.

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|?