496292 - Support changing the Accept header via a preference

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[(no longer active)]

A user requested that libpr0n should support a pref for specifying the Accept header it sends out instead of hardcoding it inside the source code.

I created a patch to do that, and I'll attach it here once the tests are finished.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Patch + tests.

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 381505 [details] [diff] [review]
Patch (v1)

Is EnsureAcceptHeaders() actually necessary? Couldn't you register for the observer service at imgLoader construction time?

Also, I'd probably drop the "default" from the pref name - that implies that there is some non-default behaviour too.

Comment 3

[Boris Zbarsky [:bzbarsky]]

If GetCharPref throws, you want to use the default value.  The in param should be a const string.  In Observe(), it might make more sense to assert the values, since that's the only notification you should ever get.

And fully agreed with comment 2.

Comment 4

[(no longer active)]

(In reply to comment #2)
> (From update of attachment 381505 [details] [diff] [review])
> Is EnsureAcceptHeaders() actually necessary? Couldn't you register for the
> observer service at imgLoader construction time?

I thought lazy initialization of the accept header value would be preferred, but I'd happily change it back to construction time.

> Also, I'd probably drop the "default" from the pref name - that implies that
> there is some non-default behaviour too.

I was just following the netwerk counterpart pref name.  Would |image.http.accept| be a better name?  Or do you have another suggestion?

I'll try to post an updated patch tomorrow with these issues and also comment 3 addressed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

image.http.accept sounds fine to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, getting eagerly would also make it possible to set the pref to "" usefully, right?  Right now that causes us to hit the pref service on every image.

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Comments addressed.

Comment 8

[(no longer active)]

Hmm, it seems that this patch introduces a leak of exactly one nsStringBuffer which is leaked even by simply opening and closing the browser:

TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 8 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsStringBuffer with size 8 bytes

The leak value does not change no matter how much I work with the patched build.  I'm suspecting that it's the sAcceptHeader nsCString which is leaking, but I find that very odd because it's a static member, which means that the compiler should take care of freeing it.  Or maybe our leak report runs before that happens?  In that case, should I resort to dynamic allocation of the string?

Comment 9

[Boris Zbarsky [:bzbarsky]]

See our code rules: no static objects with constructors and destructors.  And yes, in this case the destructor runs way after we finish leak logging and shut down everything; it runs at module unload.  So yes, you should allocate it dynamically, or make it a member of some object.

Comment 10

[(no longer active)]

Attachment: Patch (v2.1)

Sorry for my mistake, and thanks for the explanation.  Here's a new patch with dynamic allocation and with no leaks reported.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm confused.  Why do we need the separate observer object?  Why not just keep the setup in the first patch but move where we init the string?

Comment 12

[(no longer active)]

(In reply to comment #11)
> I'm confused.  Why do we need the separate observer object?  Why not just keep
> the setup in the first patch but move where we init the string?

Because imgLoader::InitCache is a static member so I can't use the this pointer there.  I see two ways to circumvent this:

1. Merge the new observer into the existing imgCacheObserver (and probably rename it because it would no longer be a cache observer.

2. Assuming that imgLoader is a singleton class and its only instance is initialized before imgLoader::InitCache is called, create a static pointer to that instance and use that pointer to setup an observer implemented as part of imgLoader itself.

I'm not sure if the assumptions in the second solution are correct (and will be correct in the future) though.

Comment 13

[Boris Zbarsky [:bzbarsky]]

You could also just add an Init method to imgLoader, change the factory registration to call it, and put the getting of the pref value and the observer addition into that method, right?

Comment 14

[(no longer active)]

Attachment: Patch (v3)

(In reply to comment #13)
> You could also just add an Init method to imgLoader, change the factory
> registration to call it, and put the getting of the pref value and the observer
> addition into that method, right?

You're right.  Here is the patch following this approach.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 382058 [details] [diff] [review]
Patch (v3)

This doesn't seem to do the right thing if the pref is unset: the GetCharPref will fail, and we won't go back to the default value in that case...  It might make sense to have a function to reget the pref value, used from both Init() and Observe() and make sure it sets the default value as needed.

Comment 16

[(no longer active)]

(In reply to comment #15)
> (From update of attachment 382058 [details] [diff] [review])
> This doesn't seem to do the right thing if the pref is unset: the GetCharPref
> will fail, and we won't go back to the default value in that case...  It might
> make sense to have a function to reget the pref value, used from both Init()
> and Observe() and make sure it sets the default value as needed.

Unless I'm missing something, GetCharPref won't fail because when the pref is cleared, it should revert to the default value in modules/libpref/src/init/all.js.  And this seems to be the case because the test makes sure that clearing the user pref works (the part starting inside checkSecond and ending in checkThird), and the test passes on all three platforms.

Comment 17

[Boris Zbarsky [:bzbarsky]]

There's no guarantee that an embeddor will ship the all.js we ship.

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 382058 [details] [diff] [review]
Patch (v3)

Good except for Boris' problems.

Comment 19

[(no longer active)]

Attachment: Patch (v3.1)

Updated version to fall back on the default header each time the pref is read.

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7c7deab50b10