Closed Bug 697679 Opened 13 years ago Closed 12 years ago

[Downloads Panel] Remove obsolete Download manager preferences from the options window

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 726444

People

(Reporter: Paolo, Assigned: javirid)

References

(
URL
)

Details

Attachments

(1 file, 7 obsolete files)

Hiding download window options from General tab v1.1
2.33 KB, patch
Details | Diff | Splinter Review 
| This bug describes changes to the version of the downloads panel and
| indicator that is currently present in the UX branch, more specifically:
| * Bug 564934, attachment 558500 [details] [diff] [review]
| * Bug 663772, attachment 558504 [details] [diff] [review]
| 
| Patches for this bug should apply on top of the two mentioned patches.

The preferences "Show the Downloads window when downloading a file" and
"Close it when all downloads are finished" in the "General" tab of the
options window are obsoleted by the new panel and should be removed.

Bug 697678 should ensure that the current value of those preferences has
no effect on the downloads panel, if possible, or reset them to their
default values during migration.
So, this bug should remove UI elements from the XUL and corresponding back-end code that updates the preference values?

And then bug 697678 will do whatever needed with the preference value itself?
Assignee: nobody → leofigueres
(In reply to Javi Rueda from comment #1)
> So, this bug should remove UI elements from the XUL and corresponding
> back-end code that updates the preference values?
> 
> And then bug 697678 will do whatever needed with the preference value itself?

Exactly. Thanks for helping out!

I also noticed that, in addition to the preferences named above, there is a
"Remember download history" checkbox, displayed in the "Privacy" pane when
"Use custom settings for history" is selected. I think this option should
disappear as well, because currently by "Download History" we mean a view in
the Library window, while the option only affects the old Download Manager
window, which we are removing.
Requesting review to Gavin Sharp as in the Wiki URL there is no specific reviewer for this bug. Could you take a look at these, please?
Attachment #574375 - Flags: review?(gavin.sharp)
(In reply to Javi Rueda from comment #3)
> Requesting review to Gavin Sharp as in the Wiki URL there is no specific
> reviewer for this bug. Could you take a look at these, please?

While review doesn't hurt on this small patch, the good news are that we can land
this on the UX branch before a full review is made. We only need a tryserver
build, in case you think there are regression tests that can be affected.

Before we land, however, please take a look at removing the unneeded prefs from
the privacy pane also.
(In reply to Paolo Amadini from comment #4)
> (In reply to Javi Rueda from comment #3)
> > Requesting review to Gavin Sharp as in the Wiki URL there is no specific
> > reviewer for this bug. Could you take a look at these, please?
> 
> While review doesn't hurt on this small patch, the good news are that we can
> land
> this on the UX branch before a full review is made. We only need a tryserver
> build, in case you think there are regression tests that can be affected.

Ok. I didn't know the procedure of this kind of new features implementation.
 
> Before we land, however, please take a look at removing the unneeded prefs
> from
> the privacy pane also.

Ok. Attached in a separate patch.
Comment on attachment 574375 [details] [diff] [review]
Removing options, back-end code and labels

Not entirely related to this bug, but there appear to be some other unused "closeWhenDone" strings in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd - we might as well get rid of these too.

Is this patch going to land on top of some other patch? Are you going to remove the relevant default prefs from firefox.js too? There also seem to be tests that refer to this.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Comment on attachment 574375 [details] [diff] [review] [diff] [details] [review]
> Removing options, back-end code and labels
> 
> Not entirely related to this bug, but there appear to be some other unused
> "closeWhenDone" strings in
> toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd - we might as
> well get rid of these too.

It seems what they are doing is testing with a try-build. I am assuming that they are using this one (attachment 574375 [details] [diff] [review]) and the other patch for the privacy options(attachment 574395 [details] [diff] [review]). If tests are ok and this bug should follow its way, then I will post a new patch with your suggestion for a new review. It has not been noted on the patches, but they are UI-only.

> Is this patch going to land on top of some other patch? Are you going to
> remove the relevant default prefs from firefox.js too? There also seem to be
> tests that refer to this.

Paolo should answer this, I believe. But, from comment 2 (to my question), bug 697678 will do whatever necessary to preference values itselves.
Following Gavin Sharp suggestion (thank you), this patch also removes the string from the toolkit/mozapps directory. No reference found on other files, except for the tests.

No requesting review as noted in previous commentaries.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> Is this patch going to land on top of some other patch?

Yes, bug 564934 and bug 663772, as per comment 0.

> Are you going to remove the relevant default prefs from firefox.js too?

I think not, they're still used by Toolkit's Download Manager if the new
Downloads Panel is preffed off manually in "about:config".

> There also seem to be tests that refer to this.

We must make sure that a tryserver build with this patch passes all tests.
Usually, we should include bug 564934 and bug 663772 in the patch queue for
the tryserver build, but in this case it might not be needed since this patch
touches a different area of the code.

By the way, I think reducing attachment 574375 [details] [diff] [review] and attachment 575263 [details] [diff] [review] to a single
patch can make landing on the UX branch slightly easier.
Following comment 10 suggestion.
Attachment #574375 - Attachment is obsolete: true
Attachment #575263 - Attachment is obsolete: true
Attachment #574375 - Flags: review?(gavin.sharp)
I pushed the General tab part to the tryserver. Results will be available here:

https://tbpl.mozilla.org/?tree=Try&rev=6adcacef3a0b

If this succeeds, this part can be already pushed to the UX branch. For the
Privacy tab part instead, there are indeed some tests that time out here:

TEST_PATH=browser/components/preferences/tests make mochitest-browser-chrome

The required test file changes should be included in the current patch.
Same as attachment 574395 [details] [diff] [review] but removing references to the UI on the tests.
Attachment #574395 - Attachment is obsolete: true
Previous patch failed some tests. This one has been tested on my Nightly trunk build and there were no failed tests.

Sorry for posting the previous patch without running the MozTests.
Attachment #575686 - Attachment is obsolete: true
Thanks for the patch! I pushed it to the tryserver also. Results will be here:

https://tbpl.mozilla.org/?tree=Try&rev=9046377c35e1

The previous patch completed successfully on tryserver, and if this does too,
then we can push both to the UX branch.
I leave to anyone else to update status of this bug and requesting the push of the patches (if needed) as I am not used to this kind of feature implementation. ^.^
There is a problem with this approach, how do we handle the case the new panel is preffed-off, for example if we decide to disable it in Aurora?
This is just doing blind removals, but that kind of thing should happen after we remove the pref-ability on an official release.
I think we should rather have a method that on window opening hides the relevant entries if the new panel is enabled, and file a follow-up to remove the code when the pref-ability will go away.

I also think it's sorta complicated for the user to understand that "browsing history" includes downloads, so we should relabel "Remember my browsing history" to "Remember my browsing and download history" (obviously changing the localization key so localizers catch-up the change). After all in Clear recent history we already have "Browsing & Download History" (I wonder who decided to use  "&" there, it looks really ugly!).
(In reply to Marco Bonardo [:mak] from comment #18)
> There is a problem with this approach, how do we handle the case the new
> panel is preffed-off, for example if we decide to disable it in Aurora?

Can't we just land these patches on mozilla-central when we enable the preference
there, and back them out on Aurora if necessary? I don't think people that go to
"about:config" to enable/disable the panel need the preferences UI to match the
manual changes they did intentionally.

> I also think it's sorta complicated for the user to understand that
> "browsing history" includes downloads, so we should relabel "Remember my
> browsing history" to "Remember my browsing and download history" (obviously
> changing the localization key so localizers catch-up the change). After all
> in Clear recent history we already have "Browsing & Download History" (I
> wonder who decided to use  "&" there, it looks really ugly!).

We're on the same page - bug 697678 comment 3 :-)
(In reply to Paolo Amadini from comment #19)
> (In reply to Marco Bonardo [:mak] from comment #18)
> > There is a problem with this approach, how do we handle the case the new
> > panel is preffed-off, for example if we decide to disable it in Aurora?
> 
> Can't we just land these patches on mozilla-central when we enable the
> preference
> there, and back them out on Aurora if necessary?

Considered this causes l10n-churn, I think conditional hiding the elements would be much simpler to handle.
Your suggestion also assumes we will immediately land preffed-on in central (that may delay initial landing) and we will never switch the pref. And that we'll have to coordinate with the Aurora uplift to ensure the backout does not causes l10n issues by reintroducing strings, should likely be done on the merge day.
I'm not trying to save users missing options, I'm trying to save ourselves complications :)

Gavin may have different thoughts though?
(In reply to Marco Bonardo [:mak] from comment #20)
> Considered this causes l10n-churn, I think conditional hiding the elements
> would be much simpler to handle.

Makes sense in this light :-)

Javi, do you want to try hiding prefs in the General pane conditionally based
on "DownloadsCommon.useWindowUI"? You can use it like we do here:

https://bugzilla.mozilla.org/attachment.cgi?id=574268&action=diff#a/browser/components/downloads/src/DownloadsStartup.js_sec1

You'd need to import the latest patches for bug 564934 and bug 663772 to your
local patch queue first.
I also suppose that, according to bug 697678 comment 4, we can also separate the
work on the Privacy pane to a separate bug, as it's an improvement that doesn't
depend on which downloads user interface is used, and can land independently
on mozilla-central before everything else.
(In reply to Paolo Amadini from comment #21)
> Javi, do you want to try hiding prefs in the General pane conditionally based
> on "DownloadsCommon.useWindowUI"? You can use it like we do here:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=574268&action=diff#a/browser/
> components/downloads/src/DownloadsStartup.js_sec1

Yes.
> 
> You'd need to import the latest patches for bug 564934 and bug 663772 to your
> local patch queue first.

Done with this now.
Although my building process was fine and testing the feature worked good also, when running the Mochitest (mochitest-chrome-browser) I got some failed tests.

In order to be sure that the failed tests were because of the patch, I tried to clone again the repo. Without any patch (the imported ones) there were some tests failed yet. Now I will try to rebuild with the imported patches (the ones with the latest date in both bugs) and run the same test to see if there are any new failed test. Then I will post the patch here.

Just updating :-)

        Attachment #575498 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Javi Rueda from comment #24)
> Although my building process was fine and testing the feature worked good
> also, when running the Mochitest (mochitest-chrome-browser) I got some
> failed tests.
> 
> In order to be sure that the failed tests were because of the patch, I tried
> to clone again the repo. Without any patch (the imported ones) there were
> some tests failed yet. Now I will try to rebuild with the imported patches
> (the ones with the latest date in both bugs) and run the same test to see if
> there are any new failed test. Then I will post the patch here.

Thanks for your patience, I know it's not nice to spend time debugging general
intermittent test failures! The recent introduction of the mozilla-inbound
branch greatly improved the situation, and the War on Orange project is expected
to cause a reduction in the number of random failures as well. Sometimes tests
fail consistently on local builds that have different environments (for example
a different operating system locale), I file bugs and fix them when I can :-)

I hope to be able to test your patch locally, and have it land on the UX branch,
together with other changes, in a week or two.

    
    

    
  
Thank you, Paolo.

    
    

    
  
Comment on attachment 576993 [details] [diff] [review]
Hiding download window options from General tab

I've tested the patch together with the queue for bug 564934, and works fine on
my machine. I'll start a tryserver build with the full queue, including this
patch, and notify on bug 564934 when I've tested the build again locally. Then,
we'll land the queue on the UX branch.

        Attachment #576993 -
        Flags: feedback+

    
    

    
  
Comment on attachment 575723 [details] [diff] [review]
Removing option to Remember Download History from Privacy panel v1.2

With regard to the Privacy pane, I've filed bug 708595 with a summary of our
current understanding of the issue. This improvement is not related to whether
we are using the separate Downloads window or the Downloads Panel, so we can
continue our work there with the usual procedures for landing directly on
mozilla-central. Feel free to attach a new patch there when you're ready.

        Attachment #575723 -
        Attachment is obsolete: true

    
    

    
  
On OS X only, browser/components/preferences/tests/browser_bug567487.js timed out
after this message (see <https://tbpl.mozilla.org/?tree=Try&rev=f728f0325671>):

[JavaScript Error: "redeclaration of var Cu" {file: "chrome://browser/content/preferences/main.js" line: 3}]

I can't find where the original Cu could be declared, maybe a global overlay?

I'll re-run the tests to see if the result is consistent.

    
    

    
  
(In reply to Paolo Amadini from comment #30)
> I can't find where the original Cu could be declared, maybe a global overlay?

Never mind, it's probably somewhere in the various other overlays included by
"macBrowserOverlay.xul" (which I didn't notice immediately as it's located in
another folder).

Maybe the simplest thing to do here here is to avoid declaring Cu, and using
Components.utils.import directly. Not sure if there's a better alternative.

    
    

    
  
The other panel scripts use Component.utils.import approach. I will try it and post the new patch later, Paolo.

    
    

    
  


  
While importing patches from auxiliar bugs, I got many rejects, maybe because files modified by those were patched between 14-11-11 and today, making them bit-roded.

So I attach the new patch untested but modified as needed. I hope it works fine and will be awaiting if any new modification is needed. Flag it as needed, if try-builds are ok, in order to get it to follow its way (whatever it was).

        Attachment #576993 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Javi Rueda from comment #33)
> So I attach the new patch untested but modified as needed. I hope it works
> fine and will be awaiting if any new modification is needed. Flag it as
> needed, if try-builds are ok, in order to get it to follow its way (whatever
> it was).

Thank you! I tried it in the latest tryserver build and it worked fine. We're
going to include it in the next UX branch push of the feature.
Status: NEW → ASSIGNED

    
    

    
  
The patch on this bug is now included as part of the patch in bug 726444. Thank
you Javi for your help here! By the way, the patch on bug 708595 is also ready
to land soon, independently from this one.

    
  
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: