HTTP handler should register as a listener for user agent prefs changes.

VERIFIED FIXED in mozilla0.9.4

Status

()

defect
P1
normal
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: jud, Assigned: darin.moz)

Tracking

({topembed})

Trunk
mozilla0.9.4
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-on-trunk [PDT+])

Attachments

(2 attachments)

Reporter

Description

19 years ago
the HTTP protocol handler should listen to changes in useragent prefs.
Assignee

Comment 1

19 years ago
Futuring this...
Target Milestone: --- → Future

Comment 2

19 years ago
http bugs to "Networking::HTTP"
Assignee: gagan → darin
Component: Networking → Networking: HTTP
Target Milestone: Future → M19
Assignee

Updated

19 years ago
Target Milestone: --- → Future
Assignee

Updated

18 years ago
Target Milestone: Future → mozilla1.0

Comment 3

18 years ago
Added Topembed keyword. 
Mozilla not paying attention to the changes in useragent preferences. This
can cause wrong user agent on the Embedded app startup (on new user profiles only). 
Keywords: topembed
Assignee

Comment 4

18 years ago
-> mozilla0.9.5
Target Milestone: mozilla1.0 → mozilla0.9.5
Assignee

Updated

18 years ago
Keywords: nsbranch
Assignee

Updated

18 years ago
Priority: P3 → P1
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Assignee

Comment 5

18 years ago
why is this necessary?  we already listen for general.useragent.override.  why
would different profiles need to modify portions of the default useragent string?

Comment 6

18 years ago
There aren't any comments on this bug since the 17th of
Sept.  Can QA regess against the Netscape commercial builds and determine if
this is still a valid bug?  If so, and we can get fixes/reviews in the next day
or two, please mark as nsbranch+ which will get this on the PDT radar. Else,
mark is as nsbranch-. Also, can someone comment in the bug how serious you think
this is?  PDT is only accepting "stop ship" bugs such as data loss and loss of
major functionality.
Reporter

Comment 7

18 years ago
override was an afterthought. the need here is in individual component
modification. this bug was created long before the override stuff was put in,
and IMO, is still valid. 

caching the whole string and parsing in/out components will lead to folks
banking on the format of the string (which as history tells us, isn't good). the
format should be left up to impl to deal w/, and consumers should just have to
worry about their component.
Assignee

Comment 8

18 years ago
ok, but nsIHttpHandler allows any component the freedom to change the UA parts.
why do we want to be able to program the UA parts via prefs?
Because nsIPrefService is an API which is exposed to embeddors and
nsIHttpHandler isn't. Unless we want to freeze nsIHttpHandler and expose it but
I bet we don't.
Assignee

Comment 10

18 years ago
ok.. that definitely makes sense.  then does this need to be nsbranch?
Reporter

Comment 11

18 years ago
nsbranch and topembed are synonymous right now I think. topembed means drive it
into the 0.9.4 branch. I'll PDT+ it.
Whiteboard: PDT+
Assignee

Comment 12

18 years ago
nsbranch -> nsbranch+
Keywords: nsbranchnsbranch+
Assignee

Comment 13

18 years ago
so i understand this to mean that the http handler should simply listen to pref
changes on anything matching "general.useragent." ... correct?

attaching a patch to make it so.
Yeah - just use the nsIPrefBranchInternal::AddObserver() stuff instead of the
old C callbacks. I bet you knew that but just making sure ;-)
Assignee

Comment 15

18 years ago
yeah, i'm going to try to completely cleanup the pref code in http.
Assignee

Comment 17

18 years ago
jud: can you review this?
Reporter

Comment 18

18 years ago
cc'ing bnesse so he can see this too.

Brian, why does Darin need the "internal" interface here? IIRC, it's because we
don't expose observation publically; right?

Looks good to me. My nested #define macro know-how is rusty though, your #define
PREF_CHANGED() macro refers to "pref" but I don't see that declared anywhere.
Does your UA_PREF() "_pref" get translated to "pref" when nested?

If the answer is yes to my above question, r=valeski.
Jud, yes that's correct. The Observation stuff was placed into the internal
interface because there were questions as to whether or not the Observer
interface was going to be exposed to embedding IIRC.

The patch looks great. Thanks darin for wacking one off my list of interfaces
that need to be cleaned up.
Reporter

Updated

18 years ago
Attachment #50859 - Flags: review+
Assignee

Comment 20

18 years ago
jud: pref is the argument to PrefsChanged... _pref is the argument to the UA_PREF
macro... they are not the same thing.
Reporter

Comment 21

18 years ago
thanks for clarification.

Comment 22

18 years ago
Comment on attachment 50859 [details] [diff] [review]
v1.0 patch, cleans up and completes http pref handling

sr=mscott
Attachment #50859 - Flags: superreview+
Reporter

Comment 23

18 years ago
pulling branch '+', because this isn't needed for eMojo. but leaving topembed
because we'll need this on 0.9.4 after 6.2 is done.
Keywords: nsbranch+nsbranch-
removing PDT - The PDT
Whiteboard: PDT+
Assignee

Comment 25

18 years ago
fixed-on-trunk
Whiteboard: fixed-on-trunk
Assignee

Comment 26

18 years ago
-> mozilla0.9.4 for checkin on the branch
Target Milestone: mozilla0.9.5 → mozilla0.9.4
This checkin caused thi tinderbox leak stats to increase: see bug 102332.
Reporter

Comment 28

18 years ago
has this hit the 0.9.4 branch yet?
Assignee

Comment 29

18 years ago
nope
pls check this into the branch - PDT+, once you have good confidence.
Whiteboard: fixed-on-trunk → fixed-on-trunk [PDT+]

Comment 31

18 years ago
Comment on attachment 50859 [details] [diff] [review]
v1.0 patch, cleans up and completes http pref handling

r=rpotts@netscape.com

Looks good to me.  The only thing that i noticed was that the UA_PREF.misc was checked for twice :-)

you might want to remove the extra one...

-- rick
Comment on attachment 52810 [details] [diff] [review]
patch for the 0.9.4 branch, including leak fixes

Looks good. r=bnesse.
Attachment #52810 - Flags: review+

Updated

18 years ago
Blocks: 64833

Comment 34

18 years ago
Comment on attachment 52810 [details] [diff] [review]
patch for the 0.9.4 branch, including leak fixes

sr=rpotts@netscape.com

this patch makes my eyes cross :-)
Attachment #52810 - Flags: superreview+
Assignee

Comment 35

18 years ago
fixed-on-0.9.4-branch
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

Comment 36

18 years ago
* useragent string for version (misc) appears in console window.
* useragent string for locale didn't appear in the console, but Darren explained
this will appear in the user agent.
* Accept-Language entries are listed and in correct order.
* Accept-Charset entry is listed, also 0.66 "other" entry.
* User-Agent string incls app name, version, platform, OS: "Mozilla", "5.0",
"Windows", "WinNT4.0"
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.