Closed Bug 574514 Opened 14 years ago Closed 14 years ago

Hide personal toolbar only if user did not customize it or edited bookmarks

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0b9
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: regression, relnote)

Attachments

(1 file, 2 obsolete files)

patch v1.2
10.55 KB, patch
Details | Diff | Splinter Review 
Original patch had this functionality, but it did not make it, try to reintroduce it without ugly hacks.
relnote: beta 1 users might freak out a bit since we don't have this yet, need to add a release note and I'll also mention it in the UI video.
Keywords: relnote
blocking2.0: --- → beta2+
Marco: any ETA on this? Would have rather had it for beta2, but think it's a definite must for beta3.
Assignee: nobody → mak77
blocking2.0: beta2+ → beta3+
I'm unsure we can do this with solid code. My code was comparing currentset with defaultset to check for a customized toolbar, and using a pref to avoid doing the check more than once. Dao said this was an overkill to do. And would affect only users who moved other buttons in personal toolbar that is probably not an interesting percentage. So maybe we should just drop the idea.

Regarding bookmarks we can again just do a fragile compare with the default set of bookmarks, will work only for Firefox official builds and not for any Linux build or other build with custom default bookmarks. Doing a full compare of default bookmarks against contents would indeed be pretty slow.
I can for sure do that, but I guess I'd be unable to get a review on such fragile checks.
as a side note, currently toolbar is not hidden if user did collapse and uncollapse it explicitly.
I actually haven't seen anyone complain about us doing this in feedback yet.  Seems really strange.  Either I'm not reading enough feedback, we didn't actually turn it off, or no one uses the bookmarks toolbar.
(In reply to comment #5)
> I actually haven't seen anyone complain about us doing this in feedback yet. 
> Seems really strange.  Either I'm not reading enough feedback, we didn't
> actually turn it off, or no one uses the bookmarks toolbar.

I think nightly testers and beta users are quite able to re-enable a toolbar (especially since the new bookmarks button has a nice shortcut for that).
Some of the other users probably would easily find the show toolbar menuitem. 
People who doesn't know what's the toolbar were probably using the menu and won't notice much difference.
But yeah, I've not seen complains as well, people are pretty happy everything we add is still customize-able and not forced on them.
I for one didn't submit a feedback report because:

a) the feedback feature seems to be broken in my installation, I'll try to uninstall and reinstall the feedback plugin... but that's another matter

b) the issue is already mentioned in the release notes and it seems to be a well known error, so why submit any new feedback for it, if I don't have anything new to add?

I suspect b) is the reason why you haven't seen more complaints. And I understand you have more pressing issues for beta2. But know that of all the great new things coming, having this bug fixed is what I most avidly look forward to :)
When comparing the bookmarks to the default set, you might want to ignore deletions.  (If I've deleted the default bookmarks, but I haven't added my own, I probably don't need to see the toolbar.)
(In reply to comment #8)
> When comparing the bookmarks to the default set, you might want to ignore
> deletions.

we can't compare bookmarks with default set for all implementers, as I said, and there are too many edge and special cases. Nobody would like to review a complicated code full of edge cases.
No activity here in a while, are we still on track for the beta3 code freeze on Monday, Aug 2?
I can't make it for b3 and we still need to evaluate a good strategy.
Pushing back to b4, since there's not an overabundance of hateback about this and Marco can't make b3 anyway.


1) Mike, why is this a beta-ship-blocking issue? Apparently it's not pissing people off, and we clearly want people to try life without the toolbar. Maybe we should ride it out through the betas before deciding if it's really a blocker?

2) WRT to the approach: Yeah, the original option is not sufficient, and comparing users' bookmarks to the default set is even worse. Could we checksum the default bookmarks backup file-size or something like that? No matter what way we go, if we want to fix this, we have to accept that we can only do so for a subset of the affected users.
blocking2.0: beta3+ → beta4+
Having users need to turn the bar back on manually does have the added benefit of making them momentarily consider if they really want the bar shown all of the time.  I'm still a little scared that they might bounce out of the product entirely if they feel like we deleted their bookmarks, or they didn't quickly figure out how to control the option.
Whiteboard: [needs definition]
Changing to blocking final. Doesn't need to block B4. The course of action is not fully decided, and the core code changes for the feature are complete - this is a minor behavior change.
blocking2.0: beta4+ → final+
(In reply to comment #12)
> 1) Mike, why is this a beta-ship-blocking issue? Apparently it's not pissing
> people off, and we clearly want people to try life without the toolbar. Maybe

I actually think that it is pissing people off, and worry more about the potential for a dataloss like feeling. I'm OK with it blocking final, I guess, if you don't think we're going to need a beta to shake out corner cases.
we can try betaN eventually, I doubt this will stay open till final, but there is higher priority stuff for lock contentions and performances that is stealing time atm.
Yep, when I searched input.m.o for "bookmarks toolbar" at the time of that comment, there were very few hits for this issue. However I just did another search and there are definitely more negatives for the issue now.

Flipping to blocking betaN+.

WRT to the implementation, the previous approach of checking default set and using a pref, along with a check of the count and content of items on the toolbar should be sufficient to cover the vast majority of cases.
blocking2.0: final+ → betaN+
Whiteboard: [needs definition]
Attached patch patch v1.0 (obsolete) — — Splinter Review 
This un-collapses the toolbar if "collapsed" is not yet persisted and user customized the toolbar or changed default bookmarks by an interesting amount.

To support all distributions i'm doing so:
- import default bookmarks in a temp hidden folder
- compare current toolbar with imported folder and generate a ratio of how many of the bookmarks are defaults
- uncollapse if less than 40% are defaults

I cannot do a 1:1 comparison because of smart bookmarks that are like defaults but added later, so I have to use a ratio to avoid crazy hacks.

Doing this could be a bit slow (matter of milliseconds though) depending on number of bookmarks in the toolbar, but it's done just once in the profile life through ui migration.

Before starting writing a b-c test for this, I'd like to know if the approach is OK.
Attachment #479020 - Flags: feedback?(gavin.sharp)
Whiteboard: [needs feedback and a test]
Comment on attachment 479020 [details] [diff] [review]
patch v1.0

Sorry for the really large feedback delay :(

A couple of concerns (in increasing order of magnitude):
- not sure about the implications of the _setPersist change, or why that's necessary
- _getDefaultBookmarksRatio makes some assumptions about bookmarks.html (e.g. that it creates a subfolder at index 0, and it only checks items in that folder), so it probably shouldn't have such a generic name, and should probably use PlacesUtils.toolbarFolderId directly rather than taking aFolderID
- where does 40% come from? Seems kind of low... are the smart bookmark items etc. really that significant a portion of the default set? 
- we're only comparing current default bookmarks, so this won't produce the right results for someone who has different defaults from some previous version, right?
- this seems like it could have a pretty serious impact on startup time. We'll only run it once, granted, but forcing an import from disk + doing an O(N*M) comparison feels like something that would best be avoided, especially considering that this code runs even for new profiles (at the very least it seems like you could avoid the comparisons entirely if the child counts are very different)

Given those, I think this is feedback-, but I don't really have any suggestions for a better approach... I wonder if we could get away with just checking the number of children in the toolbarFolder, and not collapsing if > 5 or something like that - ask faaborg?
Attachment #479020 - Flags: feedback?(gavin.sharp) → feedback-
(In reply to comment #20)
> - not sure about the implications of the _setPersist change, or why that's
> necessary

it's needed because you can't persist an attribute of a toolbar if the toolbar itself is not in the persisted set. If you look at nsXULDocument.cpp you see it is doing this, because you can't tell offhand if a toolbar is already persisted (http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/nsXULDocument.cpp#1455). Indeed it does not work if you don't persist it. So I'm not sure why the browser part has not inherited that code, probably because as of today nobody tried to persist a common attribute like "collapsed".

> - _getDefaultBookmarksRatio makes some assumptions about bookmarks.html (e.g.
> that it creates a subfolder at index 0, and it only checks items in that
> folder)

it's hard to tell if a certain folder is the toolbar on a import, I'd guess there is no way today. We could annotate it in the importExportService though, so on the temp import we can recognize it. This will increasing the code  size and I'm starting feeling bad about it. I'm tempted to wontfix the bug and let users uncollapse their toolbar :(

> - where does 40% come from? Seems kind of low... are the smart bookmark items
> etc. really that significant a portion of the default set?

In our case there is 1 smart bookmark and 2 bookmarks, so it's 33% and 66%, but I'd not want to update this code for each change in default bookmarks. Thus I guessed "something less than a half", that is the 40%.
 
> - we're only comparing current default bookmarks, so this won't produce the
> right results for someone who has different defaults from some previous
> version, right?

right, but default bookmarks did not change much in the last years, probably they did not change at all (on toolbar at least, we removed Get bookmarks Add-ons from the menu).

> - this seems like it could have a pretty serious impact on startup time. We'll
> only run it once, granted, but forcing an import from disk + doing an O(N*M)
> comparison feels like something that would best be avoided

the impact is probably towards half a second, can be optimized with some count as you said and we could maybe detect new profiles better and skip it. I could maybe consider that if browser.migration pref does not exist could be a new profile. But First I need to figure out if the approach has to be dropped.

> Given those, I think this is feedback-, but I don't really have any suggestions
> for a better approach...

And that's my greatest concern, there is not a perfect solution.
This bug can be fixed 2 ways:
1. doing the proper thing is expensive and involves a lot of code.
2. doing the unproper thing will work for most users, will fail for some (few bookmarks on toolbar but not default ones) and for all third party distributions (that customized bookmarks on toolbar).

> I wonder if we could get away with just checking the
> number of children in the toolbarFolder, and not collapsing if > 5 or something
> like that - ask faaborg?

this is the second approach, works for most users but those who have a few non-default bookmarks on toolbar.

Alex, please, do you have any ideas? We can do both options, it's just matter of how complex the code becomes if we want to really count default bookmarks on toolbar.
>> I wonder if we could get away with just checking the
>> number of children in the toolbarFolder, and not collapsing if > 5 or something
>> like that - ask faaborg?
>
>this is the second approach, works for most users but those who have a few
>non-default bookmarks on toolbar.
>
>Alex, please, do you have any ideas?

We'll from my limited perspective code complexity isn't much of an issue, and we should try as hard as we can to only disable the toolbar if it hasn't been modified.  I would rather not fail for small numbers of customizations and third party distributions, so from the UX side I vote for the proper but expensive solution.

If we have to go for approach 2, that isn't ideal but would still be ok.
(In reply to comment #21)
> > - _getDefaultBookmarksRatio makes some assumptions about bookmarks.html (e.g.
> > that it creates a subfolder at index 0, and it only checks items in that
> > folder)

To be clear, I wasn't suggesting you necessarily needed to fix this. I think the assumption is fine, I just think the method should be renamed (and the parameter removed) to avoid confusion. But I think we don't want to go down this route, so the point is moot.

> I could maybe consider that if browser.migration pref does not exist could be
> a new profile.

I thought of this, but it's a harder problem than that to solve - you need to distinguishing new-users from users-upgrading-from-old-builds. If you add a default value, you'll treat users upgrading from builds from before the migration code was added (i.e. those that don't already have custom values for browser.migration.version) the same as new users, which isn't really what you want.

We should probably add some backend support that would help us with these scenarios in the future. Some kind of indication of whether a given root has been customized, or something.
(In reply to comment #23)
> We should probably add some backend support that would help us with these
> scenarios in the future. Some kind of indication of whether a given root has
> been customized, or something.

That would mix up things, the backend does not care about default bookmarks, smart bookmarks or stuff like that and it should not care. Its scope is to manage bookmarks, for it the default bookmarks are just added bookmarks like any other, doing differently would just complicate the API. All of this management is relative to the browser part. We could annotate default bookmarks, but that's not something we can do backwards without paying a price. Also changing defaults can be practically any change, so each API would become slower to check if it's acting on a default bookmark. I don't think making the backend aware of defaults will solve our problem, there will always be the need to do a costly comparison since having a flag would cost more.

So, you think we don't want to go this route, and Alex thinks we should try to go this route if possible. I think I have an headache :)
You don't need to do any costly comparisons. All you need is an annotation that a bookmark is a "default" that can later be retrieved. Or a "DB creation" timestamp along with "last bookmark addition" timestamp for each folder. Or some other similar trick. It doesn't need to work retroactively. In any case, fodder for a separate bug.

For this bug, I think we should just use the bookmark toolbar child count.
that would work for new imports, but we should pay a cost for old profiles, the same cost we want to avoid here. Btw, looks like the number comparison will be the accepted solution here so i'll go for it.
I'm not suggesting we pay a cost for old profiles. As I said, it doesn't need to be retroactive. It was merely a suggestion to think ahead now to avoid this kind of difficult situation in the future. But the odds of us needing it in the future may be low enough that it's not worth even considering, so just forget I said anything.
Whiteboard: [needs feedback and a test] → [needs new patch and a test]
Attached patch patch v1.1 (obsolete) — — Splinter Review 
uncollapses if user has >5 bookmarks on toolbar.
I had to slightly modify browserGlue profile startup method to allow the test to notify multiple times final-ui-startup. If you think this is bad, I'll just kill the test since there is no other way I can think of.
Attachment #479020 - Attachment is obsolete: true
Attachment #488167 - Flags: review?(gavin.sharp)
Whiteboard: [needs new patch and a test] → [needs review gavin]
hm the test fails the first test, looks like some previous b-c test collapses it, I'll just remove that initial test so that the first call to clean() will restore the situation.
Comment on attachment 488167 [details] [diff] [review]
patch v1.1

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>   _migrateUI: function BG__migrateUI() {

>+      // Check if the user has explicitly (un)collapsed the toolbar.
>+      if (collapsed === null) {

"If the user does not have a persisted value for the toolbar's "collapsed" attribute, try to determine whether it's customized."

Then inside the block: "We consider the toolbar customized if it has more than 5 children, or if it has a persisted currentSet value."

>+        let toolbarFolder =
>+          PlacesUtils.getFolderContents(PlacesUtils.toolbarFolderId).root;
>+        let toolbarChildCount = toolbarFolder.childCount;
>+        toolbarFolder.containerOpen = false;

It would be nice to avoid doing this work if toolbarIsCustomized is true. Could make a getToolbarFolderCount() helper and then just do if (toolbarIsCustomized || getToolbarFolderCount() > 5) so that it can short-circuit.

The places "containerOpen" thing confuses me. Why do you need to set it here? The other non-test caller of getFolderContents() in the tree doesn't seem to. Should it be set on toolbarFolder.root, given that that's what the implementation sets containerOpen=true on?

>+      // Add the entry to the persisted set for this document if it's not there.

So I guess the reason this wasn't here before was because setPersist is currently only called if there is already persisted value that needs to be modified, but now we're calling it when the collapsed state hasn't been persisted at all yet. Man, this RDF code is so gross :(

>diff --git a/browser/components/places/tests/browser/browser_toolbar_migration.js b/browser/components/places/tests/browser/browser_toolbar_migration.js

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;

Shouldn't be needed, these are defined in browser.js.

>+let gOriginalCurrentSet = localStore.getPersist("currentset");
>+let gOriginalMigrationVersion = Services.prefs.getIntPref("browser.migration.version");
>+registerCleanupFunction(clean);

These should be in test() (browser chrome tests should generally avoid running code outside of test()).

It seems to me like we lose test coverage of the child count case if gOriginalCurrentSet is not null (which I suppose may be true if any customization tests ran before this test), since toolbarIsCustomized will always be true in that case. So I think you should check that it is null as part of the test (or enforce that it is by setting it, if needed).

WebContentConverter.js's _init and Sanitizer._checkAndSanitize in sanitize.js (among other things) are triggered multiple times by the test, which is rather unfortunate. It would probably be better to add a way to trigger only UI migration specifically, without the rest of _onProfileStartup.

You should get explicit ux-feedback on the heuristic (is 5 the right number? is a number threshold acceptable?) via ui-review, I think.
Attachment #488167 - Flags: review?(gavin.sharp) → review+
(In reply to comment #30)
> You should get explicit ux-feedback on the heuristic (is 5 the right number? is
> a number threshold acceptable?) via ui-review, I think.

Consider this an informal UI review ;)

I really don't like using a number to determine this, is there no way to strip out the smart bookmarks from the menu bar and compare with the default set (sans smart bookmarks too)? I guess I don't understand why this is so hard, tried reading the comments above, but couldn't see why.

If we're going to use the simplistic approach of using a number, we should err on the low side. If the default set is 3 items ("Getting Started", "Most Visited", "Latest Headlines"), we should have the number be >3.
(In reply to comment #31)
> I really don't like using a number to determine this, is there no way to strip
> out the smart bookmarks from the menu bar and compare with the default set
> (sans smart bookmarks too)? I guess I don't understand why this is so hard,
> tried reading the comments above, but couldn't see why.

Figuring out whether the bookmarks on the toolbar are exactly "default bookmarks" is problematic for several reasons:
- default bookmarks have changed over the years - users with old profiles might have default bookmarks that no longer match our current defaults, and we can't easily detect that case
- default bookmarks can be impacted by distribution customizations, and that's done in a way that can't easily be replicated when we try to generate the default set for comparison
- getting "the default set" and comparing it to the user's bookmarks are both expensive operations (in terms of perf impact on startup time), and we'd need to do it for all new profiles and upgrades.

These are all implementation limitations related to the current way we implement default bookmarks and customizations. There are things we can do to make determining the set of default bookmarks easier in the future, but those won't help until all users we care about have profiles created with the build where we introduce those changes, and that's far too late to help 4.0.
(In reply to comment #32)
> (In reply to comment #31)
> > I really don't like using a number to determine this, is there no way to strip
> > out the smart bookmarks from the menu bar and compare with the default set
> > (sans smart bookmarks too)? I guess I don't understand why this is so hard,
> > tried reading the comments above, but couldn't see why.
> 
> Figuring out whether the bookmarks on the toolbar are exactly "default
> bookmarks" is problematic for several reasons:

Thanks for the explanation. Let's err on the conservative side and make it >3, then?
(In reply to comment #33)
> Thanks for the explanation. Let's err on the conservative side and make it >3,
> then?

yes, we can do it. I think we went for 5 to take in count the fact third party builds (linux ones mostly) could be adding a couple entries to the toolbar.
This is a hard guess to do obviously and there is no good way to tell if the user actually used these bookmarks or not. For these users we will most likely always show the toolbar. It's also to be told that vast majority of them should be more than able to hide a toolbar.
So if you think going strict by 3 is fine to proceed, I'll go for it.

In the meanwhile, looks like I have comments to address.
(In reply to comment #30)
> >+        let toolbarFolder =
> >+          PlacesUtils.getFolderContents(PlacesUtils.toolbarFolderId).root;
> >+        let toolbarChildCount = toolbarFolder.childCount;
> >+        toolbarFolder.containerOpen = false;

> The places "containerOpen" thing confuses me. Why do you need to set it here?
> The other non-test caller of getFolderContents() in the tree doesn't seem to.
> Should it be set on toolbarFolder.root, given that that's what the
> implementation sets containerOpen=true on?

notice the .root at the end of getFolderContents... also, if we have any getFolderContents call that does not call containerOpen = false, that's a bug to fix.
Attached patch patch v1.2 — — Splinter Review 

        Attachment #488167 -
        Attachment is obsolete: true
Whiteboard: [needs review gavin]

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/3f60c5c6c0b5
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9

    
    

    
  
This is working as intended as far as I can tell.  It hides on new profiles (windows) and stays open on current profiles with additional bookmarks on the personal (bookmarks) toolbar.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20101222 Firefox/4.0b9pre ID:20101222132729

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: