Support changing the Accept header via a preference

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
ImageLib
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 381505 [details] [diff] [review]
Patch (v1)

Patch + tests.
Attachment #381505 - Flags: superreview?(bzbarsky)
Attachment #381505 - Flags: review?(pavlov)
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.
Attachment #381505 - Flags: review?(pavlov) → review?(joe)
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.
(Assignee)

Comment 4

9 years ago
(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.
image.http.accept sounds fine to me.
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.
Attachment #381505 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 7

9 years ago
Created attachment 381927 [details] [diff] [review]
Patch (v2)

Comments addressed.
Attachment #381505 - Attachment is obsolete: true
Attachment #381927 - Flags: superreview?(bzbarsky)
Attachment #381927 - Flags: review?(joe)
Attachment #381505 - Flags: review?(joe)
(Assignee)

Comment 8

9 years ago
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?
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.
(Assignee)

Comment 10

9 years ago
Created attachment 381972 [details] [diff] [review]
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.
Attachment #381927 - Attachment is obsolete: true
Attachment #381972 - Flags: superreview?(bzbarsky)
Attachment #381972 - Flags: review?(joe)
Attachment #381927 - Flags: superreview?(bzbarsky)
Attachment #381927 - Flags: review?(joe)
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?
(Assignee)

Comment 12

9 years ago
(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.
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?
(Assignee)

Comment 14

9 years ago
Created attachment 382058 [details] [diff] [review]
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.
Attachment #381972 - Attachment is obsolete: true
Attachment #382058 - Flags: superreview?(bzbarsky)
Attachment #382058 - Flags: review?(joe)
Attachment #381972 - Flags: superreview?(bzbarsky)
Attachment #381972 - Flags: review?(joe)
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.
Attachment #382058 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 16

9 years ago
(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.
There's no guarantee that an embeddor will ship the all.js we ship.
Comment on attachment 382058 [details] [diff] [review]
Patch (v3)

Good except for Boris' problems.
Attachment #382058 - Flags: review?(joe) → review+
(Assignee)

Comment 19

9 years ago
Created attachment 383088 [details] [diff] [review]
Patch (v3.1)

Updated version to fall back on the default header each time the pref is read.
Attachment #382058 - Attachment is obsolete: true
Attachment #383088 - Flags: superreview?(bzbarsky)
Attachment #383088 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/7c7deab50b10
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.