Closed Bug 620066 Opened 13 years ago Closed 13 years ago

Port |Bug 613477 - Make the primary Star UI (bookmarked indicator) asynchronous| to SeaMonkey

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b2

People

(Reporter: sgautherie, Assigned: kairo)

References

Details

Attachments

(1 file, 3 obsolete files)

      No description provided.
Blocks: FF2SM
Blocks: 620068
Attached patch v1: just port the FF patch (obsolete) — Splinter Review
This is a straight port of the applying parts of the FF patch to our code.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #498633 - Flags: review?(neil)
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.
This patch includes the two fixes mentioned in the last comment.
Attachment #498633 - Attachment is obsolete: true
Attachment #498721 - Flags: review?(neil)
Attachment #498633 - Flags: review?(neil)
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.
Attachment #498721 - Flags: review?(neil) → review-
(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.
> > >+  _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.
(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.]
Well, the good news is that you get to port bug 620683 too ;-)
(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.
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.
Attachment #498721 - Attachment is obsolete: true
Attachment #502797 - Flags: review?(neil)
(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 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.
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.
Depends on: 624734, 620683
Here's the update for the tooltip issue and the about:blank handling removal.
Attachment #502797 - Attachment is obsolete: true
Attachment #503961 - Flags: review?(neil)
Attachment #502797 - Flags: review?(neil)
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.
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)?
(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 on attachment 503961 [details] [diff] [review]
v2.1: incorporated bug 624734, removed about:blank special handling

r=me if bug 620068 lands first.
Attachment #503961 - Flags: review?(neil) → review+
(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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
(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" ...
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.