Search plugins with diacritics in their name cannot be hidden (only removed)

RESOLVED FIXED in Firefox 37

Status

()

Firefox
Search
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: MikkCZ, Assigned: Chris, Mentored)

Tracking

35 Branch
Firefox 39
Points:
2
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 wontfix, firefox36- wontfix, firefox37+ fixed, firefox38+ fixed, firefox39 fixed, firefox-esr31 unaffected)

Details

Attachments

(2 attachments, 14 obsolete attachments)

15.51 KB, patch
Details | Diff | Splinter Review
15.52 KB, patch
Details | Diff | Splinter Review
Happens in Czech Firefox 35 with plugins Heuréka and Slunečnice (default ones), or with any plugin I add myself (e.g. on Seznam.cz website).

How to reproduce it:
1. Go to the search options.
2. Uncheck a search plugin with diacritics in its name.
3. Now the plugin shouldn't be shown in the search tooltip.
4. But it is even after restarting Firefox. The options stays unchecked.

I tried to remove the diacritics from the name to be without diacritics, and after that the hiding works OK.

Btw. I also wonder, why its necessary to run Firefox once without the plugin .xml file, just changing the file contents makes no change in Firefox even after restart - maybe another standalone bug?
browser.search.hiddenOneOffs is read/set using getCharPref/setCharPref, which likely loses data here.

We should just use Preferences.jsm for this, which uses getComplexValue(..., nsISupportsString) automatically.
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Duplicate of this bug: 1122528
[Tracking Requested - why for this release]: hiding search providers doesn't work reliably on localized builds.
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox36: --- → ?

Comment 4

4 years ago
If I can get a DXR link up in here I bet this'd be a good mentored or first bug.
Here is where the preference is defined for the preference UIs, they probably both need to be switched to unichar types:

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.xul#9
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/search.xul#29

We also get the pref in the main UI here and there we need to use Preferences.jsm:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1144
(In reply to Mike Hoye [:mhoye] from comment #4)
> If I can get a DXR link up in here I bet this'd be a good mentored or first
> bug.

It would certainly be a good mentored bug. My only concern is it might bitrot with the patch I'm currently mentoring in bug 1106926.
(Assignee)

Comment 7

4 years ago
I'll take a look into this.
Assignee: nobody → chrishood
Mentor: florian
Chris, any news on this? Thanks
Flags: needinfo?(chrishood)
(Assignee)

Comment 9

4 years ago
Hey Sylv, I've been waiting on bug 1106926 to resolve (It looks like it just has) since I'll need to switch the code added for it from using the browser.search.hiddenOneOffs pref to using Preferences.jsm.

I've already done what should be most of the work for this bug and should have a patch posted sometime later this week or this weekend when time permits.
Flags: needinfo?(chrishood)
(Assignee)

Comment 10

4 years ago
Created attachment 8561143 [details] [diff] [review]
bug-1121417.patch

I'm almost finished, but I'm having some issues with getting the test case to work properly.  I've varified that my changes work manually but on the test case, when I check the value of shownOneOffs in browser_hiddenOneOffs_diacritics.js it doesn't contain the test engine that I added regardless of whether or not I add it to the hiddenOneOffs pref.

I also added another test case to browser_hiddenOneOffs_cleanup.js to make sure that the observer is handling diacritics correctly (Using Preferences.jsm instead of Service.prefs).
Flags: needinfo?(florian)
Comment on attachment 8561143 [details] [diff] [review]
bug-1121417.patch

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

Thanks for the patch and the tests!

::: browser/components/nsBrowserGlue.js
@@ +424,3 @@
>            let hiddenEngines = hiddenPref ? hiddenPref.split(",") : [];
>            hiddenEngines = hiddenEngines.filter(x => x !== engineName);
>            Services.prefs.setCharPref("browser.search.hiddenOneOffs",

This should use Preferences.jsm too.

::: browser/components/search/test/browser_hiddenOneOffs_cleanup.js
@@ +71,5 @@
> +  const diacritic_engine = "Foo \u2661";
> +  let ns = {};
> +  Cu.import("resource://gre/modules/Preferences.jsm", ns);
> +
> +  Services.prefs.setCharPref("browser.search.hiddenOneOffs", diacritic_engine);

This needs to use Preferences.jsm

@@ +79,5 @@
> +    ns.Preferences.get("browser.search.hiddenOneOffs").split(",");
> +  is(hiddenOneOffs.includes(diacritic_engine), false,
> +     "Observer cleans up added hidden engines that include a diacritic.");
> +
> +  Services.prefs.setCharPref("browser.search.hiddenOneOffs", diacritic_engine);

This too.

::: browser/components/search/test/browser_hiddenOneOffs_diacritics.js
@@ +38,5 @@
> +
> +add_task(function* test_hidden() {
> +  let ns = {};
> +  Cu.import("resource://gre/modules/Preferences.jsm", ns);
> +  Services.prefs.setCharPref("browser.search.hiddenOneOffs", diacritic_engine);

You are importing Preferences.jsm here, but not using it; that's probably the reason why your test doesn't pass.

@@ +45,5 @@
> +  info("Opening search panel");
> +  searchbar.focus();
> +  yield promise;
> +
> +  let shownOneOffs = Array.from(getOneOffs(), x => x.getAttribute("label"));

getOneOffs() already returns an array, so you don't need to use Array.from, Array.map would be enough.

@@ +46,5 @@
> +  searchbar.focus();
> +  yield promise;
> +
> +  let shownOneOffs = Array.from(getOneOffs(), x => x.getAttribute("label"));
> +  is(shownOneOffs.includes(diacritic_engine), false,

comparing the value with |false| isn't really useful, so you could just do ok(!..., 

This could probably be simplified even more with Array.some:
ok(!getOneOffs().some(x => x.getAttribute("label") == diacritic_engine),
   "shown oneOffs...
Attachment #8561143 - Flags: feedback+
Flags: needinfo?(florian)
Status: NEW → ASSIGNED
We are late in the 36 cycle. I don't think we should rush to fix it. We can however accept a patch in 37.
status-firefox36: affected → wontfix
tracking-firefox36: ? → -
tracking-firefox37: --- → +
(Assignee)

Comment 13

4 years ago
Created attachment 8564785 [details] [diff] [review]
bug-1121417.patch

I corrected the errors that were present in the test case, swapped out Services.prefs for Preferences.jsm, and added a new test to ensure that engines with Diacritics are shown after being removed from hiddenOneOffs.

I applied this patch on top of the patch for Bug 1132028 so that will need to land before this patch does.
Attachment #8561143 - Attachment is obsolete: true
Comment on attachment 8564785 [details] [diff] [review]
bug-1121417.patch

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

Did you mean to request review on this updated patch?

::: browser/components/nsBrowserGlue.js
@@ +427,1 @@
>                                       hiddenEngines.join(","));

nit: Please fix the indent on this line.

::: browser/components/search/test/browser_hiddenOneOffs_diacritics.js
@@ +11,5 @@
> +
> +let ns = {};
> +Cu.import("resource://gre/modules/Preferences.jsm", ns);
> +
> +function promiseNewEngine(basename) {

I wonder if it would be possible to use the promiseNewEngine function from head.js (http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/head.js#139) instead of duplicating it.
Attachment #8564785 - Flags: feedback+
(Assignee)

Comment 15

4 years ago
I agree that using the promiseNewEngine in head.js would be the preferred option, but the cleanup functions that it registered weren't working properly after adding two engines (nsSearchService complained about an engine not being in store).  The only real difference between the two functions is that my version isn't doing anything with the current engine.  Instead I took care of that in the init function since that seems to work better and nsSearchService doesn't complain.
(In reply to Chris from comment #15)

> The only real difference between the two
> functions is that my version isn't doing anything with the current engine. 

Should we add a parameter to disable that behavior?
(Assignee)

Comment 17

4 years ago
Created attachment 8567324 [details] [diff] [review]
bug-1121417.patch

Made the fixes suggested, I added an optional parameter to promiseNewEngine in head.js and removed the function from the diacritic test case.

Push to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e245510855

There's an extra try job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a2b5f4fe299 that I still can't seem to cancel.

I thought that it might be related to me giving the system wrong credentials when I cancelled it last time but I gave it the same username/password that I have set up for pushing to try server and it still won't allow me to cancel.  So I'll have to look into what Gavin was saying for that.
Attachment #8564785 - Attachment is obsolete: true
Attachment #8567324 - Flags: review?(florian)
(Assignee)

Comment 18

4 years ago
Created attachment 8567551 [details] [diff] [review]
bug-1121417.patch

Fixed indent.
Attachment #8567324 - Attachment is obsolete: true
Attachment #8567324 - Flags: review?(florian)
Attachment #8567551 - Flags: review?(florian)
Comment on attachment 8567551 [details] [diff] [review]
bug-1121417.patch

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

Thanks for removing the promiseNewEngine code duplication! :-)

::: browser/components/nsBrowserGlue.js
@@ +437,1 @@
>                                       hiddenEngines.join(","));

Please fix the indent of this line. Already requested in comment 14.

::: browser/components/search/test/head.js
@@ +142,5 @@
>      Services.search.init({
>        onInitComplete: function() {
>          let url = getRootDirectory(gTestPath) + basename;
> +        if (swapCurrent) {
> +          var current = Services.search.currentEngine;

While this (using 'var') works, I think there's a risk that it would confuse someone in the future, so I would prefer if you instead either just added "let current;" before the if, or just kept doing "let current = Services.search.currentEngine;" unconditionally (this code is only running in tests; not in the product itself, so a tiny amount of extra unneeded code execution isn't really a problem).

@@ +167,5 @@
>          });
>        }
>      });
>    });
> +}
\ No newline at end of file

Please keep a line break at the end of the file.
Attachment #8567551 - Flags: review?(florian) → feedback+
(Assignee)

Comment 20

4 years ago
Created attachment 8568053 [details] [diff] [review]
bug-1121417.patch

I apologize about the indent.  I changed promiseNewEngine to just instantiate current and set it to current engine regardless of whether or not it's used.
Attachment #8567551 - Attachment is obsolete: true
Attachment #8568053 - Flags: review?(florian)
Comment on attachment 8568053 [details] [diff] [review]
bug-1121417.patch

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

Thanks!
Attachment #8568053 - Flags: review?(florian) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 8568087 [details] [diff] [review]
bug-1121417.patch

checkin-patch
Attachment #8568053 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a7923665bfa0
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Backed out in https://hg.mozilla.org/integration/fx-team/rev/3554805703a7 since e10s bc1 was unhappy about it, https://treeherder.mozilla.org/logviewer.html#?job_id=2081232&repo=fx-team
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 25

4 years ago
I've narrowed the problem down to the tests in browser_diacritic_engine.js.  It looks like something in those tests is causing issues when run with e10s.  I'll have to take another look and see why it's not cleaning up properly when run with e10s.
(Assignee)

Comment 26

3 years ago
The issue is being caused by this snippet of code.

>  let promise = promiseEvent(searchPopup, "popupshown");
>  searchbar.focus();
>  yield promise;

Keyboard navigation tests seem to be the only tests affected by this.  I'm not entirely sure about the best way to change this to get keyboard navigation tests and diacritic one offs test to play nice together.  The only thing I can think of is to remove the diacritic tests from the e10s test suite.
Flags: needinfo?(florian)
(Assignee)

Comment 27

3 years ago
Asking Gavin since Florian is unavailable.
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 28

3 years ago
Created attachment 8569438 [details] [diff] [review]
bug-1121417.patch

Uploading an updated patch since the old one had bitrotted.  There aren't any changes in here to fix the e10s testing issue.
Attachment #8568087 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
I think I found out what the devious problem was.  I've pushed the updated patch to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f46fd900d2d

It looks like opening the search popup by changing the textbox's value caused an issue with keyboard navigation tests, for some reason it interfered with that test's ability to alter the search history when run under e10s.
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(florian)
(Assignee)

Comment 30

3 years ago
Created attachment 8570521 [details] [diff] [review]
bug-1121417.patch

The e10s try run looks good, this patch should good to go.  I had to make some pretty substantial changes to the way to test case worked in order to pass e10s.  Gavin would you mind taking a look at the test case since Florian is away?
Attachment #8569438 - Attachment is obsolete: true
Attachment #8570521 - Flags: review?(gavin.sharp)
Comment on attachment 8570521 [details] [diff] [review]
bug-1121417.patch

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>+        let ns = {};
>+        Cu.import("resource://gre/modules/Preferences.jsm", ns);
>+        let pref =
>+          ns.Preferences.get("browser.search.hiddenOneOffs");

You should be able to re-write this a bit more clearly as:

let Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
let pref = Preferences.get("browser.search.hiddenOneOffs");

(and then use this pattern more consistently throughout the patch)

>diff --git a/browser/components/search/test/browser_hiddenOneOffs_diacritics.js b/browser/components/search/test/browser_hiddenOneOffs_diacritics.js

>+add_task(function* init() {
>+  const cachedPref = Services.prefs.getCharPref("browser.search.hiddenOneOffs");

Shouldn't you avoid using getCharPref here? Though actually, I don't think you need to store the old value - you can just clearUserPref at the end of the test?

>diff --git a/browser/components/search/test/head.js b/browser/components/search/test/head.js

>+function promiseNewEngine(basename, swapCurrent = true) {

Boolean parameters are a little bit evil. To make the callers clearer, you could make this an optional "options" parameter, defined as an object that optionally has the "setAsCurrent" property. E.g.:

function promiseNewEngine(basename, options = {}) {
  let setAsCurrent = options.setAsCurrent || false;
  // ...
}

>diff --git a/browser/components/search/test/testEngine_diacritics.xml b/browser/components/search/test/testEngine_diacritics.xml

>+  <ShortName>Foo &#9825;</ShortName>
>+  <Description>Diacritics Test Engine</Description>

I might adjust this just to explain what's unique about this engine in more detail: "Engine whose ShortName contains non-BMP Unicode characters"

It looks like there's no coverage for the preferences window getting/setting of this pref, but that's probably not worth worrying about too much.

Good job chasing down the e10s issue!

r=me with those comments addressed.
Attachment #8570521 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 32

3 years ago
Created attachment 8570834 [details] [diff] [review]
bug-1121417.patch

Thanks Gavin.

> function promiseNewEngine(basename, options = {}) {
>  let setAsCurrent = options.setAsCurrent || false;
>  // ...
> }

I changed the optional parameter as suggested but I decided to use

let setAsCurrent = 
  options.setAsCurrent == undefined ? true : options.setAsCurrent;

I want to default setAsCurrent to true to match the most likely use case without having to alter other parts of the code.

I made the other changes that you suggested.
Attachment #8570521 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0bc001f799df
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0bc001f799df
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39

Updated

3 years ago
Iteration: --- → 39.1 - 9 Mar
Setting as qe-verify- since this seems to be well covered by automated tests, so I don't think additional manual testing is needed. Feel free to set it back if you think otherwise.
Flags: qe-verify+ → qe-verify-
Comment on attachment 8570834 [details] [diff] [review]
bug-1121417.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1088660 (new search bar)
[User impact if declined]: impossible to hide the one-off buttons of some default engines in some localized builds (eg. cz).
[Describe test coverage new/current, TreeHerder]: tests included in the patch.
[Risks and why]: low risk; most of the patch is test additions.
[String/UUID change made/needed]: none.
Attachment #8570834 - Flags: approval-mozilla-beta?
Attachment #8570834 - Flags: approval-mozilla-aurora?
status-firefox-esr31: --- → unaffected
tracking-firefox38: --- → +
Comment on attachment 8570834 [details] [diff] [review]
bug-1121417.patch

Given the test coverage in this patch and that it is QE-, let's take this in Beta 3. Note that this is a fix for an issue introduced in 34. Beta+ Aurora+
Attachment #8570834 - Flags: approval-mozilla-beta?
Attachment #8570834 - Flags: approval-mozilla-beta+
Attachment #8570834 - Flags: approval-mozilla-aurora?
Attachment #8570834 - Flags: approval-mozilla-aurora+
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #37)

> this is a fix for an issue introduced in 34.

While the problem in the code was indeed introduced in 34, we only started localizing the new search UI in 35, so this problem was unlikely to impact significant numbers of users in 34.
Needs rebasing for beta uplift.
Flags: needinfo?(chrishood)
Keywords: branch-patch-needed
(Assignee)

Comment 41

3 years ago
Created attachment 8572788 [details] [diff] [review]
bug-1121417_r.patch

I rebased this on top of the changeset with the tag FIREFOX_AURORA_38_BASE using the command hg update -r FIREFOX_AURORA_38_BASE.  This is the first time that I have rebased a patch to match the beta channel of firefox so please feel free to correct me if I've made a mistake.
Attachment #8570834 - Attachment is obsolete: true
Flags: needinfo?(chrishood)
Comment on attachment 8570834 [details] [diff] [review]
bug-1121417.patch

You need to rebase on top of Gecko 37, not 38. Also, please don't obsolete the original patch as in this case, the branch patch is something entirely different.
Attachment #8570834 - Attachment is obsolete: false
(Assignee)

Comment 43

3 years ago
Created attachment 8572877 [details] [diff] [review]
bug-1121417_r.patch

I based this patch off of FIREFOX_AURORA_37_BASE as requested.  I ran the tests on my box for safety and they all passed.  I'll push to try if necassary as soon as I know that this is the right patch.

This patch does include stuff from the patch for Bug 1106926 which is necassary since the this patch changes code that was made in that patch (I've also included the test cases for Bug 1106926 to make sure we don't lose coverage).
Attachment #8572788 - Attachment is obsolete: true
Gavin, I'm not sure if this needs re-review or not given the inclusion of changes from another bug. Likewise on approval. What are your thoughts?
Flags: needinfo?(gavin.sharp)
Keywords: branch-patch-needed
If this is built on bug 1106926, we should just also uplift that to 37. Does the patch in that bug apply cleanly?
Flags: needinfo?(gavin.sharp) → needinfo?(chrishood)
(Assignee)

Comment 46

3 years ago
I included the code changes for bug 1106926 in bug-1121417_r.patch since the patch for this bug was written on top of the patch for bug 1106926.

I could split bug-1121417_r.patch up into two patches, one that makes changes to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml and one that makes changes to http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js and other code files affected by bug 1106926.  Just let me know which you would prefer.
Flags: needinfo?(chrishood)
(Assignee)

Comment 47

3 years ago
It does look like the patch for bug 1106926 will apply cleanly to 37.
It'd probably be better for blame and consistency if we just uplift bug 1106926 separately in that case. How risky would that be? If you don't see that as a big deal, go ahead and nominate it.
(Assignee)

Comment 49

3 years ago
Created attachment 8574094 [details] [diff] [review]
bug-1121417_r37.patch

I went ahead and made a patch for 37 that should apply cleanly as long as the patch for bug 1106926 is uplifted first, from what I tested the patch for bug 1106926 should just apply clean to 37 without any issues (Tested using hg import).

I added the promiseNewEngine function to http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/head.js since the tests in http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_hiddenOneOffs_diacritics.js rely on it and it didn't exist when 37 was cut.
Attachment #8572877 - Attachment is obsolete: true
I don't see a big concern in uplifting bug 1106926 to Beta. Let's get a nom on that bug and another nom on the branch patch in this bug. If the branch patch in this bug is close enough to the original, I think we can carry forward the r+. If not, let's get another review on the patch before landing it on Beta.
Flags: needinfo?(chrishood)
(Assignee)

Comment 51

3 years ago
Created attachment 8574212 [details] [diff] [review]
bug-1121417_r37.patch

Fixed missing newline at end of file.
Attachment #8574094 - Attachment is obsolete: true
(Assignee)

Comment 52

3 years ago
The patch that I uploaded earlier today is pretty much identical to the original patch except for the changes mentioned in my previous post.  We may want to go ahead and do a review for that function but everything else should be fine.
Flags: needinfo?(chrishood)
(Assignee)

Updated

3 years ago
Attachment #8574212 - Flags: review?(gavin.sharp)
Comment on attachment 8574212 [details] [diff] [review]
bug-1121417_r37.patch

This will need to be adjusted to include a proper commit message (include the a=lmandel).
Attachment #8574212 - Flags: review?(gavin.sharp)
Attachment #8574212 - Flags: review+
Attachment #8574212 - Flags: approval-mozilla-beta?
Comment on attachment 8574212 [details] [diff] [review]
bug-1121417_r37.patch

I have already approved bug 1106926 for uplift. Approving this patch as well. Please note comment 53 about the required commit message. Beta+
Attachment #8574212 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 55

3 years ago
Created attachment 8574780 [details] [diff] [review]
bug-1121417_r37.patch

The patch for bug 1106926 will need to be uplifted to 37 first otherwise this patch will fail to apply cleanly.  The patch for bug 1106926 was approved to be uplifted in bug 1106926 comment 43.  Once it lands this patch should apply cleanly.
Attachment #8574212 - Attachment is obsolete: true
I had to back this out for hitting mochitest-bc failures. Please rebase your patch on top of mozilla-beta tip.
https://hg.mozilla.org/releases/mozilla-beta/rev/89b593b91e5e

https://treeherder.mozilla.org/logviewer.html#?job_id=293807&repo=mozilla-beta
status-firefox37: fixed → affected
ni Chris to fix this up for potential relanding for Beta 5 by Thu, Mar 12.
Flags: needinfo?(chrishood)
(Assignee)

Comment 59

3 years ago
Created attachment 8575052 [details] [diff] [review]
bug-1121417_r37.patch

Rebasad on mozilla-beta tip.
Attachment #8574780 - Attachment is obsolete: true
Flags: needinfo?(chrishood)
(Assignee)

Comment 60

3 years ago
Created attachment 8575055 [details] [diff] [review]
bug-1121417_r37.patch

Indenting
Attachment #8575052 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.