Last Comment Bug 620066 - Port |Bug 613477 - Make the primary Star UI (bookmarked indicator) asynchronous| to SeaMonkey
: Port |Bug 613477 - Make the primary Star UI (bookmarked indicator) asynchrono...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b2
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
Depends on: 613477 620683 624734
Blocks: 620068
  Show dependency treegraph
 
Reported: 2010-12-17 19:13 PST by Serge Gautherie (:sgautherie)
Modified: 2013-01-01 06:32 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: just port the FF patch (7.11 KB, patch)
2010-12-19 12:14 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v1.1: include two additional fixes (7.29 KB, patch)
2010-12-20 05:14 PST, Robert Kaiser (not working on stability any more)
neil: review-
Details | Diff | Review
v2: address comments, incorporate bug 620683 (5.83 KB, patch)
2011-01-11 07:25 PST, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
v2.1: incorporated bug 624734, removed about:blank special handling (5.55 KB, patch)
2011-01-14 13:19 PST, Robert Kaiser (not working on stability any more)
neil: review+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2010-12-17 19:13:53 PST

    
Comment 1 Robert Kaiser (not working on stability any more) 2010-12-19 12:14:09 PST
Created attachment 498633 [details] [diff] [review]
v1: just port the FF patch

This is a straight port of the applying parts of the FF patch to our code.
Comment 2 Robert Kaiser (not working on stability any more) 2010-12-20 05:03:06 PST
Actually, I should incorporate http://hg.mozilla.org/mozilla-central/rev/080549b4c0f8 (bustage fix) and http://hg.mozilla.org/mozilla-central/rev/23c8f19e51f1 (bug 618998) into this patch, I guess.
Comment 3 Robert Kaiser (not working on stability any more) 2010-12-20 05:14:12 PST
Created attachment 498721 [details] [diff] [review]
v1.1: include two additional fixes

This patch includes the two fixes mentioned in the last comment.
Comment 4 neil@parkwaycc.co.uk 2010-12-20 06:59:19 PST
Comment on attachment 498721 [details] [diff] [review]
v1.1: include two additional fixes

>+  get _starredTooltip()
>+  {
>+    delete this._starredTooltip;
>+    return this._starredTooltip =
>+      gNavigatorBundle.getString("starButtonOn.tooltip");
>+  },
>+  get _unstarredTooltip()
>+  {
>+    delete this._unstarredTooltip;
>+    return this._unstarredTooltip =
>+      gNavigatorBundle.getString("starButtonOff.tooltip");
>+  },
> 
>+  updateState: function PSB_updateState()
>+  {
>+    this._starIcon = document.getElementById("star-button");
I thought this was a confusing mixture of lazy getter, but then I realised that Firefox probably allows you to customise the star icon away, making it necessary to test for it all over the place. Our superior toolbar customisation code does not have that limitation of course, so you can retrieve the star icon once and for all in the init method, or convert that to a lazy getter too.

>+    if (!this._starIcon || (this._uri && gBrowser.currentURI.equals(this._uri))) {
Bah. Filed bug 620366 as you know.

>+    // We can load about:blank before the actual page, but there is no point in handling that page.
>+    if (this._uri.spec == "about:blank") {
>+      return;
It turns out that I have three about:blank bookmarks in my testing profile ;-)

>+  _updateStateInternal: function PSB__updateStateInternal()
>+  {
>+    if (!this._starIcon) {
>+      return;
I was originally going to complain that all the callers check this already :-)

>+    let starred = this._starIcon.hasAttribute("starred");
>+    if (this._itemIds.length > 0 && !starred) {
>+      this._starIcon.setAttribute("starred", "true");
>+      this._starIcon.setAttribute("tooltiptext", this._starredTooltip);
>+    }
>+    else if (this._itemIds.length == 0 && starred) {
>+      this._starIcon.removeAttribute("starred");
>+      this._starIcon.setAttribute("tooltiptext", this._unstarredTooltip);
Oops. Until you visit a bookmarked page, the star icon defaults to unstarred, but that means we never end up setting the unstarred tooltip.
[I wonder, given that you're now caching the tooltip text, whether we need to bother checking for the existing star state. In particular setAttribute early returns if the current value is the same as the one you're already setting.]

>-    // don't bubble to the textbox so that the address won't be selected
>+    // Don't bubble to the textbox, to avoid unwanted selection of the address.
Please don't change this comment, I want to remove it in bug 619309 anyway.
Comment 5 Robert Kaiser (not working on stability any more) 2010-12-20 07:13:52 PST
(In reply to comment #4)
> Our superior toolbar customisation
> code does not have that limitation of course, so you can retrieve the star icon
> once and for all in the init method, or convert that to a lazy getter too.

Ah, right, our bloated implementation does that ton of extra work to process things for all possible hidden elements for the benefit of slightly nicer-looking code. There's no init left after bug 620068, but I'll find something.

> >+  _updateStateInternal: function PSB__updateStateInternal()
> >+  {
> >+    if (!this._starIcon) {
> >+      return;
> I was originally going to complain that all the callers check this already :-)

Might be interesting for Marco, though.

> >+    let starred = this._starIcon.hasAttribute("starred");
> >+    if (this._itemIds.length > 0 && !starred) {
> >+      this._starIcon.setAttribute("starred", "true");
> >+      this._starIcon.setAttribute("tooltiptext", this._starredTooltip);
> >+    }
> >+    else if (this._itemIds.length == 0 && starred) {
> >+      this._starIcon.removeAttribute("starred");
> >+      this._starIcon.setAttribute("tooltiptext", this._unstarredTooltip);
> Oops. Until you visit a bookmarked page, the star icon defaults to unstarred,
> but that means we never end up setting the unstarred tooltip.
> [I wonder, given that you're now caching the tooltip text, whether we need to
> bother checking for the existing star state. In particular setAttribute early
> returns if the current value is the same as the one you're already setting.]

Marco, does Firefox run into the same problem?

> >-    // don't bubble to the textbox so that the address won't be selected
> >+    // Don't bubble to the textbox, to avoid unwanted selection of the address.
> Please don't change this comment, I want to remove it in bug 619309 anyway.

Ah, how I love solutions where SeaMonkey does the same thing differently just because it can. Whatever, I won't fight it any more.
Comment 6 Marco Bonardo [::mak] 2010-12-20 07:33:43 PST
> > >+  _updateStateInternal: function PSB__updateStateInternal()
> > >+  {
> > >+    if (!this._starIcon) {
> > >+      return;
> > I was originally going to complain that all the callers check this already 

true, but also since we are async we cannot be 100% sure that the check before the async work is still valid when the async work finishes. Sure, it's an edge case, and most likely we won't see it in real life.

> > >+    let starred = this._starIcon.hasAttribute("starred");
> > >+    if (this._itemIds.length > 0 && !starred) {
> > >+      this._starIcon.setAttribute("starred", "true");
> > >+      this._starIcon.setAttribute("tooltiptext", this._starredTooltip);
> > >+    }
> > >+    else if (this._itemIds.length == 0 && starred) {
> > >+      this._starIcon.removeAttribute("starred");
> > >+      this._starIcon.setAttribute("tooltiptext", this._unstarredTooltip);
> > Oops. Until you visit a bookmarked page, the star icon defaults to unstarred,
> > but that means we never end up setting the unstarred tooltip.
> > [I wonder, given that you're now caching the tooltip text, whether we need to
> > bother checking for the existing star state. In particular setAttribute early
> > returns if the current value is the same as the one you're already setting.]
> 
> Marco, does Firefox run into the same problem?

Yes, good catch, please file a bug against us. If setAttribute early returns we can remove the starred check and that would fix the tooltip issue too, that doesn't look like being documented though.
Comment 7 neil@parkwaycc.co.uk 2010-12-28 03:59:16 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Our superior toolbar customisation
> > code does not have that limitation of course, so you can retrieve the star icon
> > once and for all in the init method, or convert that to a lazy getter too.
> Ah, right, our bloated implementation does that ton of extra work to process
> things for all possible hidden elements for the benefit of slightly
> nicer-looking code.
It's much nicer code, since there are hardly any edge cases to think about.

> > >-    // don't bubble to the textbox so that the address won't be selected
> > >+    // Don't bubble to the textbox, to avoid unwanted selection of the address.
> > Please don't change this comment, I want to remove it in bug 619309 anyway.
> Ah, how I love solutions where SeaMonkey does the same thing differently just
> because it can.
Perhaps you think I should duplicate code for each element in the URLbar?

(In reply to comment #6)
> > Marco, does Firefox run into the same problem?
> Yes, good catch, please file a bug against us. If setAttribute early returns we
> can remove the starred check and that would fix the tooltip issue too, that
> doesn't look like being documented though.
It's in nsGenericElement::SetAttr - if (valueMatches && ...) return NS_OK;
[aNotify is always true for calls via SetAttribute.]
Comment 8 neil@parkwaycc.co.uk 2010-12-28 05:58:01 PST
Well, the good news is that you get to port bug 620683 too ;-)
Comment 9 Robert Kaiser (not working on stability any more) 2011-01-11 07:15:33 PST
(In reply to comment #6)
> Yes, good catch, please file a bug against us.

Neil, can you please file that one against Firefox places? I have too little clue to actually tell what to write in there.
Comment 10 Robert Kaiser (not working on stability any more) 2011-01-11 07:25:16 PST
Created attachment 502797 [details] [diff] [review]
v2: address comments, incorporate bug 620683

This incorporates bug 620683 and should address the comments - with the exception of that tooltip one - I either don't know how to fix it or it might be in core code, not sure how to interpret your comments on that one.
Comment 11 neil@parkwaycc.co.uk 2011-01-11 08:00:29 PST
(In reply to comment #9)
> (In reply to comment #6)
> > Yes, good catch, please file a bug against us.
> Neil, can you please file that one against Firefox places? I have too little
> clue to actually tell what to write in there.
Filed bug 624734. Apart from the issues mentioned there, your patch looks good. I particularly like the starIcon simplification.
Comment 12 neil@parkwaycc.co.uk 2011-01-13 05:42:34 PST
Comment on attachment 502797 [details] [diff] [review]
v2: address comments, incorporate bug 620683

>+    // We can load about:blank before the actual page, but there is no point in handling that page.
OK, so the point of this is that a) Firefox about:blank always counts as an invalid proxy state (as if you were typing) and b) Firefox always hides the star icon in an invalid proxy state.
Comment 13 neil@parkwaycc.co.uk 2011-01-13 05:57:34 PST
Whereas we force about:blank to be a "valid" page if either a) it's a popup window or b) it's not the first page in session history (bug 370555).

So I guess we need to remove the about:blank early return.
Comment 14 Robert Kaiser (not working on stability any more) 2011-01-14 13:19:54 PST
Created attachment 503961 [details] [diff] [review]
v2.1: incorporated bug 624734, removed about:blank special handling

Here's the update for the tooltip issue and the about:blank handling removal.
Comment 15 neil@parkwaycc.co.uk 2011-01-14 13:51:57 PST
Comment on attachment 503961 [details] [diff] [review]
v2.1: incorporated bug 624734, removed about:blank special handling

Bah, I found one last edge case which existed before but wasn't so noticable.

If you change the new window pref to open a blank page, and open a new window, then it hasn't initialised its star button. With the old code, that just meant that the star button wasn't starred when you had an about:blank bookmark, which normally would mean that it should be starred. With the new code though, this means that the checks for starring and unstarring now generate JS warnings or possibly even errors or crashes due to uninitialised variables.
Comment 16 Robert Kaiser (not working on stability any more) 2011-01-14 14:39:07 PST
Hmm, not sure I understand how to fix this. It it really a problem with bug 620068 applied as well (I'd push both that one and this one together anyhow)?
Comment 17 neil@parkwaycc.co.uk 2011-01-14 15:40:33 PST
(In reply to comment #16)
> Hmm, not sure I understand how to fix this. It it really a problem with bug
> 620068 applied as well (I'd push both that one and this one together anyhow)?
Ah, so bug 620068 reduces the bug to be the same as it was previously.
Comment 18 neil@parkwaycc.co.uk 2011-01-14 15:56:10 PST
Comment on attachment 503961 [details] [diff] [review]
v2.1: incorporated bug 624734, removed about:blank special handling

r=me if bug 620068 lands first.
Comment 19 Robert Kaiser (not working on stability any more) 2011-01-18 10:01:20 PST
(In reply to comment #18)
> r=me if bug 620068 lands first.

Well, I pushed both in one go, that  should be just as good. ;-)

This one is http://hg.mozilla.org/comm-central/rev/80f4b50e9398
Comment 20 neil@parkwaycc.co.uk 2011-01-20 02:59:00 PST
(In reply to comment #19)
> (In reply to comment #18)
> > r=me if bug 620068 lands first.
> 
> Well, I pushed both in one go, that  should be just as good. ;-)

By first I meant "someone shouldn't be able to accidentally bisect between these patches and get a non-working star" ...

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