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

Status: RESOLVED WONTFIX

(Core :: XUL, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

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).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Darin?  Thoughts?

Comment 3

[[e-mail]]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: the hack fix

Comment 5

[Boris Zbarsky [: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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hack fix checked in to trunk, 2004-07-27 16:31 -0700.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

verified - component bar and bookmark icons change correctly now. linux, current
cvs.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 154421 [details] [diff] [review]
the hack fix

a=[e-mail] for aviary branch

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to AVIARY_1_0_20040515_BRANCH, 2004-07-29 13:26 -0700.

Comment 10

[Darin Fisher]

> 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 :-/

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: untested nsChromeURL patch

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: untested nsChromeURL patch

Comment 13

[Steffen Wilberg]

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: WIP nsChromeURL patch

With this one, at least the browser starts.

Comment 15

[Steffen Wilberg]

The browser starts indeed. But the back and forward buttons are now affected as
well, and opening a new window doesn't help anymore.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I do know that.  That's why it's "work in progress".

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 23

[Brendan Eich [:brendan]]

NS_NOTREACHED is great, thanks.

/be

Comment 24

[Darin Fisher]

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?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Reopening so I remember to land the new patch.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChomeURL patch

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?)

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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)

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

(with some comments addressed)

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChomeURL patch

This addresses all of the review comments I haven't commented on (and I think
some of the ones I have).

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

This fixes one additional comment after discussion with darin.

Comment 33

[Darin Fisher]

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

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

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.

Comment 36

[Boris Zbarsky [:bzbarsky]]

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.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.
> 

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and nsRefPtr and nsCOMPtr don't have .forget(), and swap() doesn't work with
a different type.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch

updated to bz's comments

Comment 41

[[e-mail]]

(In reply to comment #39)
>Oh, and nsRefPtr and nsCOMPtr don't have .forget()
Should they?

Comment 42

[Boris Zbarsky [:bzbarsky]]

Ah, yes... I knew swap() didn't work, but didn't realize forget() was only on
nsAutoPtr...

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 44

[Asa Dotzler [:asa]]

Comment on attachment 154421 [details] [diff] [review]
the hack fix

a=asa for 1.7.5. We need this to match Aviary.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsChromeURL patch backed out

Comment 46

[Mike Kaply [:mkaply]]

dbaron - the hack fix doesn't apply cleanly to 1.7 - can you get a new patch
together?

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It does after merging 226791.

Comment 48

[Mike Kaply [:mkaply]]

Is any of this patch still checked in? (the second patch, not the hack fix)

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the xpcom, netwerk, and modules/libjar parts are checked in.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: nsChromeURL patch (unlanded parts, merged to trunk, not compiled or tested)

Comment 51

[Gábor Stefanik]

Shouldn't that patch get review? (At least, I think it should be requested.

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, since it was backed out due to performance regressions.

Comment 55

[Mike Kaply [:mkaply]]

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