Closed Bug 252703 Opened 20 years ago Closed 3 years ago

Live theme switch: Component bar icons & bookmark icons not changed

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: Biesinger, Unassigned)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, Whiteboard: [patch])

Attachments

(3 files, 12 obsolete files)

the hack fix
8.19 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
bugs
: approval-aviary+
asa
: approval1.7.5+
Details | Diff | Splinter Review
nsChromeURL patch
112.03 KB, patch
Details | Diff | Splinter Review
nsChromeURL patch (unlanded parts, merged to trunk, not compiled or tested)
43.13 KB, patch
Details | Diff | Splinter Review 
You need attachment (id=154009) to test this.

After doing View|Apply theme|Modern, the component bar icons remain to be
classic ones in currently open windows. New windows show the right icons.

The same applies to the icons in the personal toolbar, both the Home icon, and
the icon for individual bookmarks. (Haven't tested bookmark groups or folders).
I've known about this bug since fixing bug 245327, and I've been thinking about
it a bit.

This is because the chrome URL for the image doesn't change (although sometimes
-moz-image-region does) while the underlying URL that it resolves to does
change, and flushing the image cache isn't good enough, since both the style
system and nsImageBoxFrame itself suppress the change since the URL is the same.

Possible fixes include:

 1. Some way to make nsIURI::Equals return false for chrome URLs when the
underlying resolved URL changes, but I haven't thought of a good way to do this.
 The possibilities that remain (i.e., not completely ruled out) are:
   a. adding some comparator mechanism to nsStandardURL (in which chrome URLs
would need to store data),
   b. adding a necko library against which other libraries could link, and
putting nsStandardURL (and probably the stuff in nsNetUtil.h) in it, or
   c. Implementing nsChromeURL that knows all of the interfaces implemented by
nsStandardURL and forwards all the calls to it, and reimplements Equals *and*
all the other member functions that use Equals.
(b) and (c) both involve an nsChromeURL object that stores the resolved URL as a
member variable and overrides nsIURI::Equals so that when it's about to return
true, the resolved URLs are compared too.

 2. Resolving chrome URIs completely in nsChromeProtocolHandler::NewURI to have
a different scheme.  This means we need to fix everything that tests
nsIURI::SchemeIs("chrome") to do something else.  A bunch of these things are
hacks to work around the current brokenness of chrome URL equality, but many of
them aren't.

 3. Some sort of hack to rumage through all image box frames and reframe all the
ones that have chrome://*/skin/* URLs when we do a theme change.

Probably the most realistic solution is the last.
Darin?  Thoughts?
No longer blocks: 134260
Blocks: 134260
I'm also seeing other oddities when switching:
Classic -> Modern: Primary toolbargrippy, Go & Search buttons
Modern -> Classic: Dual toolbarbuttons, toolbar background colours
Both ways: Menu background colours, autocomplete dropmarker
Attachment #154421 - Flags: superreview?(bzbarsky)
Attachment #154421 - Flags: review?(bzbarsky)
Comment on attachment 154421 [details] [diff] [review]
the hack fix

Hrm.  r+sr=bzbarsky, I guess... It's not great, but I don't have any better
easy ideas that deal well with src attributes too.
Attachment #154421 - Flags: superreview?(bzbarsky)
Attachment #154421 - Flags: superreview+
Attachment #154421 - Flags: review?(bzbarsky)
Attachment #154421 - Flags: review+
Hack fix checked in to trunk, 2004-07-27 16:31 -0700.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
verified - component bar and bookmark icons change correctly now. linux, current
cvs.
Status: RESOLVED → VERIFIED
Comment on attachment 154421 [details] [diff] [review]
the hack fix

a=ben@mozilla.org for aviary branch
Attachment #154421 - Flags: approval-aviary? → approval-aviary+
Checked in to AVIARY_1_0_20040515_BRANCH, 2004-07-29 13:26 -0700.
Keywords: fixed-aviary1.0
> Darin?  Thoughts?

David and I spoke about these options (under #1) over IRC.  We didn't come up
with anything great, so I'm sort of glad to see the "hack" in place instead :-/
The "untested" patch doesn't work. My Firefox aviary gtk2 build with the patch
applied in mozilla/chrome/src/ doesn't start up. There's no error; I have to
kill the process.
Attached patch WIP nsChromeURL patch (obsolete) — — Splinter Review 
With this one, at least the browser starts.
Attachment #155743 - Attachment is obsolete: true
The browser starts indeed. But the back and forward buttons are now affected as
well, and opening a new window doesn't help anymore.
FWIW, I do know that.  That's why it's "work in progress".
Attached patch nsChromeURL patch (obsolete) — — Splinter Review 
This is a pretty much working nsChromeURL patch.  I'm not sure that it helps
all that much, though, and there are still some FastLoad issues (assertions)
and probably some other problems.
Attachment #157447 - Attachment description: patch → nsChromeURL patch
Attachment #157762 - Flags: superreview?(brendan)
Attachment #157762 - Flags: review?(darin)
Comment on attachment 157762 [details] [diff] [review]
nsChromeURL patch

dbaron: I detest NS_ENSURE*, could you please revert those changes in
nsFastLoadFile.cpp?  r=me on the other changes in that file.

I'll wait for darin's r= to sr=.

/be
I do want assertions there, though, since it's something a bug in any
nsISerializable could trigger.  Do you prefer NS_NOTREACHED?
NS_NOTREACHED is great, thanks.

/be
Comment on attachment 157762 [details] [diff] [review]
nsChromeURL patch

>Index: chrome/src/nsChromeURL.cpp

>+nsChromeURL::nsChromeURL()
>+{
>+}

perhaps this should just be defined inline in the header.


>+    // Try the global cache first.
>+    nsCOMPtr<nsIChromeRegistry> reg = gChromeRegistry;
>+
>+    // If that fails, the service has not been instantiated yet; let's
>+    // do that now.
>+    if (!reg) {
>+        reg = do_GetService(NS_CHROMEREGISTRY_CONTRACTID, &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+
>+    NS_ASSERTION(reg, "Must have a chrome registry by now");
>+    return reg->Canonify(mChromeURL);

how about:

  if (!gChromeRegistry) {
      nsCOMPtr<nsIChromeRegistry> reg = do_GetService(...
      ...
  }
  ...
  return gChromeRegistry->Canonify(mChromeURL);


also, at first glance i found mChromeURL a bit confusing as a name
for this member.  the class is nsChromeURL, so i first assumed that
mChromeURL is an instance of nsChromeURL, but of course it is not.
perhaps that's just life, but is there another name that could be
used?


>+NS_INTERFACE_MAP_END_AGGREGATED(mChromeURL) // for the |kThisImplCID| hack

this bugs me a bit.  i understand that you are using this in
your implementation of Equals, but since nsStandardURL might
implement some other interface in the future, and that would
then cause this code to incorrectly aggregate that interface,
how about adding a method on nsIChromeURL to expose other's
mChromeURL?  then from Equals, you could fetch that, and
compare it to your mChromeURL.	that would seem to be a more
direct way of dealing with things.  i can appreciate that the
current solution is a shortcut and maybe you don't want to 
expose implementation details in nsIChromeURL, so hmm... add
your own kThisImpl hack?


>+// XXX Do reinitialization after mutation?

this sounds significant.  do you believe that no one actually
modifies a chrome URL?	if so, what about setting nsIStandardURL::
mutable to false after initialization?	you could always toggle
back to true if you need to do some mutation in here somewhere.

otherwise, maybe it would be good to just return a failure code
if someone calls one of the mutators?


>+    nsresult rv = mChromeURL->Clone(NS_REINTERPRET_CAST(nsIURI**,
>+                                      NS_STATIC_CAST(nsIURL**,
>+                                        getter_AddRefs(clone->mChromeURL))));

or maybe this?

    nsIURI *temp = nsnull;
    nsresult rv = mChromeURL->Clone(&temp);
    clone->mChromeURL.swap(NS_STATIC_CAST(nsIURL *, temp));


>+NS_IMETHODIMP nsChromeURL::GetConvertedURI(nsIURI **aConvertedURI)
>+{
>+
>+    if (!mConvertedURI) {

nit: kill extra newline


>+        nsCOMPtr<nsIIOService> ioServ = do_GetService(kIOServiceCID, &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);

you could also use do_GetIOService from nsNetUtil.h.  that way you avoid
having to define kIOServiceCID in this module.	for what you are doing here
you could also just call NS_NewURI ;-)


>+NS_IMETHODIMP nsChromeURL::ReConvert(nsIChromeURL **aResult)

looks like the code in here could be shared with your Clone impl.


>+    // XXX Should this be optional, or should we resolve before writing?

or how about never serializing the converted URI?  you can just convert
it again after deserializing, right?  performance concern?  correctness
concern?


>Index: chrome/src/nsChromeURL.h

>+ * nsChromeURL is a wrapper for nsStandardURL that implements all the
>+ * interfaces that nsStandardURL does just to override the Equals method
>+ * and provide a mechanism for getting at the resolved URL.

seems like we should provide a better way to do this, huh?  perhaps if
instead of supporting full aggregation we supported aggregation of new
interfaces, and then provided other hooks to extend ::Clone and ::Equals.
*ugh*


>+    // interface nsISupports
>+    NS_DECL_ISUPPORTS
>+
>+    // interface nsIURI
>+    NS_DECL_NSIURI
>+
>+    // interface nsIURL
>+    NS_FORWARD_NSIURL(mChromeURL->)

nitty nit: aren't the macro names self documenting?  aren't the
comments redundant?


>Index: content/base/public/nsIChromeURL.idl

>+interface nsIChromeURL : nsIURL {
>+    readonly attribute nsIURI convertedURI;

document this attribute?


>Index: netwerk/protocol/jar/src/nsJARURI.cpp

>+    *aFlags = nsIClassInfo::THREADSAFE;

that's actually a lie.	i don't think this file means to use
NS_IMPL_THREADSAFE_XXX.
left over cruft from days past when people thought the way to silence
threadsafety
assertions was to implement threadsafe addref and release!!!  now-a-days i
believe
we have taken care to only use protocol handlers and uri objects on the main
thread.


>+nsJARURI::GetClassIDNoAlloc(nsCID *aClassIDNoAlloc)

it'd be nice if we had an easy way to share the code for these nsIClassInfo
impls.


>Index: netwerk/protocol/res/src/nsResProtocolHandler.cpp

i wonder... would it be better to use GetClassIDNoAlloc with
CreateInstance to achieve something similar to StartClone?
Attachment #157762 - Flags: review?(darin) → review+
Reopening so I remember to land the new patch.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Whiteboard: [patch]
Blocks: 127373
Attached patch nsChomeURL patch (obsolete) — — Splinter Review 
This is merged to the trunk (previous patches were aviary branch), and
addresses some of the review comments, but nowhere near all of them.  Don't
know if it compiles, and I'm not even sure how I'd test if it works.  (Are
there themes compatible with the trunk?)
Attachment #157762 - Attachment is obsolete: true
(In reply to comment #24)
> how about:
> 
>   if (!gChromeRegistry) {
>       nsCOMPtr<nsIChromeRegistry> reg = do_GetService(...
>       ...
>   }
>   ...
>   return gChromeRegistry->Canonify(mChromeURL);

Well, that would pretty much guarantee we'll crash if an extension implements a
revised chrome registry, but perhaps that's a good thing.

> also, at first glance i found mChromeURL a bit confusing as a name
> for this member.  the class is nsChromeURL, so i first assumed that
> mChromeURL is an instance of nsChromeURL, but of course it is not.
> perhaps that's just life, but is there another name that could be
> used?

I haven't thought of one.

> >+NS_INTERFACE_MAP_END_AGGREGATED(mChromeURL) // for the |kThisImplCID| hack
> 
> this bugs me a bit.  i understand that you are using this in
> your implementation of Equals, but since nsStandardURL might

Well, it's more because there are a whole bunch of methods on nsStandardURL that
depend on it, and I'd like them all to work.

> implement some other interface in the future, and that would
> then cause this code to incorrectly aggregate that interface,
> how about adding a method on nsIChromeURL to expose other's
> mChromeURL?  then from Equals, you could fetch that, and
> compare it to your mChromeURL.	that would seem to be a more
> direct way of dealing with things.  i can appreciate that the
> current solution is a shortcut and maybe you don't want to 
> expose implementation details in nsIChromeURL, so hmm... add
> your own kThisImpl hack?

What about the other functions?

> >Index: netwerk/protocol/res/src/nsResProtocolHandler.cpp
> 
> i wonder... would it be better to use GetClassIDNoAlloc with
> CreateInstance to achieve something similar to StartClone?

CreateInstance is expensive, no?

(More comments later, and a bunch of other things already corrected in my tree)
Attached patch nsChromeURL patch (obsolete) — — Splinter Review 
(with some comments addressed)
Attachment #164630 - Attachment is obsolete: true
(In reply to comment #24)
> or how about never serializing the converted URI?  you can just convert
> it again after deserializing, right?  performance concern?  correctness
> concern?

The whole point of fastload is performance, so I figure we may as well serialize
what we have.  It's supposed to be faster that way, and it would surprise me if
it weren't.
(In reply to comment #24)
>>+    nsresult rv = mChromeURL->Clone(NS_REINTERPRET_CAST(nsIURI**,
>>+                                      NS_STATIC_CAST(nsIURL**,
>>+                                        getter_AddRefs(clone->mChromeURL))));
> 
> or maybe this?
> 
>     nsIURI *temp = nsnull;
>     nsresult rv = mChromeURL->Clone(&temp);
>     clone->mChromeURL.swap(NS_STATIC_CAST(nsIURL *, temp));

That last line would actually need to be 
      clone->mChromeURL.swap(*NS_REINTERPRET_CAST(nsIURL**, &temp));
so I decided I preferred it as it is now.
Attached patch nsChomeURL patch (obsolete) — — Splinter Review 
This addresses all of the review comments I haven't commented on (and I think
some of the ones I have).
Attachment #165484 - Attachment is obsolete: true
Attached patch nsChromeURL patch (obsolete) — — Splinter Review 
This fixes one additional comment after discussion with darin.
Comment on attachment 166392 [details] [diff] [review]
nsChromeURL patch

>Index: chrome/src/nsChromeURL.cpp

>+NS_IMETHODIMP nsChromeURL::Equals(nsIURI *other, PRBool *_retval)
...
>+    nsRefPtr<nsChromeURL> otherChrome;
>+    CallQueryInterface(other,
>+                   NS_STATIC_CAST(nsChromeURL**, getter_AddRefs(otherChrome)));

Should this be NS_STATIC_CAST(nsIChromeURL**,...) instead?  I noticed
that the QueryInterface implementation casts |this| to |nsIChromeURL*|.

BTW, you suggested using NS_REINTERPRET_CAST to nsISupports* inside
QueryInterface.


r=darin
Attachment #166392 - Flags: review?(darin) → review+
That wouldn't be valid as a static cast.  (The NS_STATIC_CAST there is just to
force something that can happen as an implicit conversion to actually happen,
for the benefit of a few compilers that would otherwise choke.)
Attached patch nsChromeURL patch (obsolete) — — Splinter Review 
So the problem with Mozilla was that I had patched the unused file
nsJARProtocolModule.cpp instead of the real file nsJARFactory.cpp.  This was a
mistake made when I merged to the trunk, since the JAR protocol handler only
recently moved to modules/libjar/.  This file seems never to have been used.
Attachment #166392 - Attachment is obsolete: true
Comment on attachment 166620 [details] [diff] [review]
nsChromeURL patch

>Index: chrome/src/nsChromeFactory.cpp
>+    { "Chrome URL",
>+      NS_CHROMEURL_CID,
>+      nsnull,
>+      nsChromeURLConstructor

Document that this is only needed for fastload to work, and that just having a
CID is enough for that?

>Index: chrome/src/nsChromeProtocolHandler.cpp
> nsChromeProtocolHandler::NewURI(const nsACString &aSpec,

>+    NS_ADDREF(*result = url);

Could also do:

  *result = url.forget();

(saving an addref/release pair), but either way....

>Index: chrome/src/nsChromeURL.cpp

>+NS_IMETHODIMP nsChromeURL::Equals(nsIURI *other, PRBool *_retval)

>+    if (!mConvertedURI) {

At this point if !otherChrome->mConvertedURI, may as well set *_retval to true
and return.  They'll convert to the same thing, so no poing in wasting cycles
on it (and since neither has been converted before, them having the same
mChromeURL is sufficient).

>+NS_IMETHODIMP nsChromeURL::GetCommonBaseSpec(nsIURI *aURIToCompare, nsACString & _retval)

I'm not sure I follow the code in this method.	We're calling GetCommonBaseSpec
on mChromeURL in both branches of the if, and passing it the same object,
really...  Why would the two branches do anything different?

If there's something I'm missing, please document it here?

>+NS_IMETHODIMP nsChromeURL::GetRelativeSpec(nsIURI *aURIToCompare, nsACString & _retval)

Same comments as for GetCommonBaseSpec.

>+NS_IMETHODIMP nsChromeURL::ReConvert(nsIChromeURL **aResult)

>+    NS_ADDREF(*aResult = result);

Again, maybe use forget().

>Index: content/xul/document/src/nsXULDocument.cpp
>@@ -3495,55 +3496,51 @@ nsXULDocument::AddPrototypeSheets()
>         nsISupports* isupports = sheets->ElementAt(i);
>+        nsCOMPtr<nsIChromeURL> uri = do_QueryInterface(isupports);
>+        NS_RELEASE(isupports);

How about replacing those three lines with:

  nsCOMPtr<nsIChromeURL> uri = do_QueryElementAt(sheets, i);

If it's feasible to move the proto document to nsCOMArray<nsIChromeURL>, that
would be great; file a followup bug on that and cc me?

>Index: modules/libjar/nsJARFactory.cpp
>+    { NS_JARURI_CLASSNAME,
>+      NS_JARURI_CID,
>+      nsnull,
>+      nsJARURIConstructor
>     }

Again, comment that this is needed for fastload.

>Index: netwerk/build/nsNetModule.cpp
>+    { NS_RESURL_CLASSNAME,
>+      NS_RESURL_CID,
>+      nsnull,
>+      nsResURLConstructor
>+    },

And here.

>Index: rdf/chrome/build/nsChromeFactory.cpp
>+    { "Chrome URL",
>+      NS_CHROMEURL_CID,
>+      nsnull,
>+      nsChromeURLConstructor
>     }

And here.

>Index: rdf/chrome/src/nsChromeProtocolHandler.cpp
> nsChromeProtocolHandler::NewURI(const nsACString &aSpec,
>+    NS_ADDREF(*result = url);

Again, maybe forget().

>Index: rdf/chrome/src/nsChromeURL.cpp

Same comments apply as to the other copy.

sr=bzbarsky with those comments addressed.
Attachment #166620 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #36)
> >+NS_IMETHODIMP nsChromeURL::GetCommonBaseSpec(nsIURI *aURIToCompare,
nsACString & _retval)
> 
> I'm not sure I follow the code in this method.	We're calling GetCommonBaseSpec
> on mChromeURL in both branches of the if, and passing it the same object,
> really...  Why would the two branches do anything different?

Oops, the first branch in both was supposed to use otherChromeURL->mChromeURL,
not just otherChromeURL.

> 
> If there's something I'm missing, please document it here?
> 
> >+NS_IMETHODIMP nsChromeURL::GetRelativeSpec(nsIURI *aURIToCompare, nsACString
& _retval)
> 
> Same comments as for GetCommonBaseSpec.
> 
> >+NS_IMETHODIMP nsChromeURL::ReConvert(nsIChromeURL **aResult)
> 
> >+    NS_ADDREF(*aResult = result);
> 
> Again, maybe use forget().
> 
> >Index: content/xul/document/src/nsXULDocument.cpp
> >@@ -3495,55 +3496,51 @@ nsXULDocument::AddPrototypeSheets()
> >         nsISupports* isupports = sheets->ElementAt(i);
> >+        nsCOMPtr<nsIChromeURL> uri = do_QueryInterface(isupports);
> >+        NS_RELEASE(isupports);
> 
> How about replacing those three lines with:
> 
>   nsCOMPtr<nsIChromeURL> uri = do_QueryElementAt(sheets, i);
> 
> If it's feasible to move the proto document to nsCOMArray<nsIChromeURL>, that
> would be great; file a followup bug on that and cc me?
> 
> >Index: modules/libjar/nsJARFactory.cpp
> >+    { NS_JARURI_CLASSNAME,
> >+      NS_JARURI_CID,
> >+      nsnull,
> >+      nsJARURIConstructor
> >     }
> 
> Again, comment that this is needed for fastload.
> 
> >Index: netwerk/build/nsNetModule.cpp
> >+    { NS_RESURL_CLASSNAME,
> >+      NS_RESURL_CID,
> >+      nsnull,
> >+      nsResURLConstructor
> >+    },
> 
> And here.
> 
> >Index: rdf/chrome/build/nsChromeFactory.cpp
> >+    { "Chrome URL",
> >+      NS_CHROMEURL_CID,
> >+      nsnull,
> >+      nsChromeURLConstructor
> >     }
> 
> And here.
> 
> >Index: rdf/chrome/src/nsChromeProtocolHandler.cpp
> > nsChromeProtocolHandler::NewURI(const nsACString &aSpec,
> >+    NS_ADDREF(*result = url);
> 
> Again, maybe forget().
> 
> >Index: rdf/chrome/src/nsChromeURL.cpp
> 
> Same comments apply as to the other copy.
> 
> sr=bzbarsky with those comments addressed.
> 


(In reply to comment #36)
> >+NS_IMETHODIMP nsChromeURL::Equals(nsIURI *other, PRBool *_retval)
> 
> >+    if (!mConvertedURI) {
> 
> At this point if !otherChrome->mConvertedURI, may as well set *_retval to true
> and return.  They'll convert to the same thing, so no poing in wasting cycles
> on it (and since neither has been converted before, them having the same
> mChromeURL is sufficient).

I'm a little hesitant about this one, since we may want to resolve permanently
due to an equality test.  Consider the case of seeing that two URLs are equal,
resolving one of them and loading the resource, and then keeping both objects
around.  It's unusual, but I think I prefer it as-is.
Oh, and nsRefPtr and nsCOMPtr don't have .forget(), and swap() doesn't work with
a different type.
updated to bz's comments
Attachment #166620 - Attachment is obsolete: true
(In reply to comment #39)
>Oh, and nsRefPtr and nsCOMPtr don't have .forget()
Should they?
Ah, yes... I knew swap() didn't work, but didn't realize forget() was only on
nsAutoPtr...
nsChromeURL patch checked in to trunk, 2004-12-01 14:35/39 -0800.

Still need to:
 * land patch to fix up excess entry points
 * change FF so theme switching is enabled
Comment on attachment 154421 [details] [diff] [review]
the hack fix

a=asa for 1.7.5. We need this to match Aviary.
Attachment #154421 - Flags: approval1.7.5+
dbaron - the hack fix doesn't apply cleanly to 1.7 - can you get a new patch
together?
It does after merging 226791.
Keywords: fixed1.7.5
Is any of this patch still checked in? (the second patch, not the hack fix)
I think the xpcom, netwerk, and modules/libjar parts are checked in.
Shouldn't that patch get review? (At least, I think it should be requested.
No, since it was backed out due to performance regressions.
Status: REOPENED → NEW
QA Contact: jrgmorrison → xptoolkit.widgets

This corresponds to old style themes which aren't a thing anymore.

Status: NEW → RESOLVED
Closed: 20 years ago → 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: